3:10 p.m.
This patch series relates to work started on the audit mailing list.
It eventually involved touching other modules, so I am trying to
pull in those owners as well. In a nutshell I add new utility
functions for accessing a processes cmdline value as displayed
in proc/<self>/cmdline, and then refactor procfs to use the
utility functions, and then add the ability to the audit subsystem
to record this value.
Thanks for any feedback and help.
[PATCH 1/3] mm: Create utility functions for accessing a tasks
[PATCH 2/3] proc: Update get proc_pid_cmdline() to use mm.h helpers
[PATCH 3/3] audit: Audit proc cmdline value
3:10 p.m.
New subject: [PATCH 1/3] mm: Create utility functions for accessing a tasks commandline value
Add two new functions to mm.h:
* copy_cmdline()
* get_cmdline_length()
Signed-off-by: William Roberts <wroberts(a)tresys.com>
---
include/linux/mm.h | 7 +++++++
mm/util.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 55 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 1cedd00..b4d7c26 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1135,6 +1135,13 @@ int set_page_dirty(struct page *page);
int set_page_dirty_lock(struct page *page);
int clear_page_dirty_for_io(struct page *page);
+extern int copy_cmdline(struct task_struct *task, struct mm_struct *mm,
+ char *buf, unsigned int buflen);
+static inline unsigned int get_cmdline_length(struct mm_struct *mm)
+{
+ return mm->arg_end ? mm->arg_end - mm->arg_start : 0;
+}
+
/* Is the vma a continuation of the stack vma above it? */
static inline int vma_growsdown(struct vm_area_struct *vma, unsigned long addr)
{
diff --git a/mm/util.c b/mm/util.c
index f7bc209..c8cad32 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -9,6 +9,7 @@
#include <linux/swapops.h>
#include <linux/mman.h>
#include <linux/hugetlb.h>
+#include <linux/mm.h>
#include <asm/uaccess.h>
@@ -410,6 +411,53 @@ unsigned long vm_commit_limit(void)
* sysctl_overcommit_ratio / 100) + total_swap_pages;
}
+/**
+ * copy_cmdline - Copy's the tasks commandline value to a buffer
+ * @task: The task whose command line to copy
+ * @mm: The mm struct refering to task with proper semaphores held
+ * @buf: The buffer to copy the value into
+ * @buflen: The length og the buffer. It trucates the value to
+ * buflen.
+ * @return: The number of chars copied.
+ */
+int copy_cmdline(struct task_struct *task, struct mm_struct *mm,
+ char *buf, unsigned int buflen)
+{
+ int res = 0;
+ unsigned int len;
+
+ if (!task || !mm || !buf)
+ return -1;
+
+ res = access_process_vm(task, mm->arg_start, buf, buflen, 0);
+ if (res <= 0)
+ return 0;
+
+ if (res > buflen)
+ res = buflen;
+ /*
+ * If the nul at the end of args had been overwritten, then
+ * assume application is using setproctitle(3).
+ */
+ if (buf[res-1] != '\0') {
+ /* Nul between start and end of vm space?
+ If so then truncate */
+ len = strnlen(buf, res);
+ if (len < res) {
+ res = len;
+ } else {
+ /* No nul, truncate buflen if to big */
+ len = mm->env_end - mm->env_start;
+ if (len > buflen - res)
+ len = buflen - res;
+ /* Copy any remaining data */
+ res += access_process_vm(task, mm->env_start, buf+res,
+ len, 0);
+ res = strnlen(buf, res);
+ }
+ }
+ return res;
+}
/* Tracepoints definitions. */
EXPORT_TRACEPOINT_SYMBOL(kmalloc);
--
1.7.9.5
8:12 a.m.
New subject: [PATCH 1/3] mm: Create utility functions for accessing a tasks commandline value
On 12/02/2013 04:10 PM, William Roberts wrote:
Add two new functions to mm.h:
* copy_cmdline()
* get_cmdline_length()
Signed-off-by: William Roberts <wroberts(a)tresys.com>
---
include/linux/mm.h | 7 +++++++
mm/util.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 55 insertions(+)
index f7bc209..c8cad32 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -410,6 +411,53 @@ unsigned long vm_commit_limit(void)
* sysctl_overcommit_ratio / 100) + total_swap_pages;
}
+/**
+ * copy_cmdline - Copy's the tasks commandline value to a buffer
spelling: Copies, task's, command-line or command line
+ * @task: The task whose command line to copy
is to be copied?
+ * @mm: The mm struct refering to task with proper semaphores held
referring
+ * @buf: The buffer to copy the value into
+ * @buflen: The length og the buffer. It trucates the value to
of, truncates
+ * buflen.
+ * @return: The number of chars copied.
+ */
+int copy_cmdline(struct task_struct *task, struct mm_struct *mm,
+ char *buf, unsigned int buflen)
+{
+ int res = 0;
+ unsigned int len;
+
+ if (!task || !mm || !buf)
+ return -1;
Typically these kinds of tests are frowned upon in the kernel unless
there is a legal usage where NULL is valid. Otherwise you may just be
covering up a bug.
Also, why not just get_task_mm(task) within the function rather than
pass it in by the caller?
+
+ res = access_process_vm(task, mm->arg_start, buf, buflen, 0);
Unsigned int buflen passed as int len argument without a range check?
Note that in the proc_pid_cmdline() code, they first cap it at PAGE_SIZE
before passing it.
The closer you can keep your code to the original proc_pid_cmdline()
code, the better (less chance for new bugs to be introduced).
+ if (res <= 0)
+ return 0;
+
+ if (res > buflen)
+ res = buflen;
Is this a possible condition? Under what circumstances?
+ /*
+ * If the nul at the end of args had been overwritten, then
+ * assume application is using setproctitle(3).
+ */
+ if (buf[res-1] != '\0') {
Lost the len < PAGE_SIZE check from proc_pid_cmdline() here, and that
isn't the same as the check above.
+ /* Nul between start and end of vm space?
+ If so then truncate */
Not sure where these comments are coming from. Isn't the issue that
lack of NUL at the end of args indicates that the cmdline extends
further into the environ and thus they need to copy in the rest?
+ len = strnlen(buf, res);
+ if (len < res) {
+ res = len;
+ } else {
+ /* No nul, truncate buflen if to big */
It isn't truncating buflen but rather copying the remainder of the
cmdline from the environ, right?
+ len = mm->env_end - mm->env_start;
+ if (len > buflen - res)
+ len = buflen - res;
+ /* Copy any remaining data */
+ res += access_process_vm(task, mm->env_start, buf+res,
+ len, 0);
+ res = strnlen(buf, res);
+ }
+ }
+ return res;
+}
I think you are better off just copying proc_pid_cmdline() exactly as is
into a common helper function and then reusing it for audit. Far less
work, and far less potential for mistakes.
/* Tracepoints definitions. */
EXPORT_TRACEPOINT_SYMBOL(kmalloc);
8:51 a.m.
New subject: [PATCH 1/3] mm: Create utility functions for accessing a tasks commandline value
On Fri, Dec 13, 2013 at 9:12 AM, Stephen Smalley <sds(a)tycho.nsa.gov> wrote:
On 12/02/2013 04:10 PM, William Roberts wrote:
> Add two new functions to mm.h:
> * copy_cmdline()
> * get_cmdline_length()
>
> Signed-off-by: William Roberts <wroberts(a)tresys.com>
> ---
> include/linux/mm.h | 7 +++++++
> mm/util.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 55 insertions(+)
>
> index f7bc209..c8cad32 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -410,6 +411,53 @@ unsigned long vm_commit_limit(void)
> * sysctl_overcommit_ratio / 100) + total_swap_pages;
> }
>
> +/**
> + * copy_cmdline - Copy's the tasks commandline value to a buffer
spelling: Copies, task's, command-line or command line
> + * @task: The task whose command line to copy
is to be copied?
> + * @mm: The mm struct refering to task with proper semaphores held
referring
> + * @buf: The buffer to copy the value into
> + * @buflen: The length og the buffer. It trucates the value to
of, truncates
> + * buflen.
> + * @return: The number of chars copied.
> + */
> +int copy_cmdline(struct task_struct *task, struct mm_struct *mm,
> + char *buf, unsigned int buflen)
> +{
> + int res = 0;
> + unsigned int len;
> +
> + if (!task || !mm || !buf)
> + return -1;
Typically these kinds of tests are frowned upon in the kernel unless
there is a legal usage where NULL is valid. Otherwise you may just be
covering up a bug.
Also, why not just get_task_mm(task) within the function rather than
pass it in by the caller?
Yes I was debating whether or not to drop the pointer checks... np
WRT the locking and moving it into the function. You need to take the lock
to determine the size of the cmdline area. The idea on the interface is you
would take the locks, acquire the size via the inline func, alloc memory and
then call the copy function. In some cases, like proc/pid/cmdline, they just
alloc a page and truncate on that boundary. However, one may with to truncate
on an arbitrary boundry, especially when cacheing the values, as you don't want
to allocate too much. So inbetween functions calls that get the length and copy,
one can make a decision based on their allocation scheme. Moving the locks
to the functions would require multiple locks and unlocks in the common case.
> +
> + res = access_process_vm(task, mm->arg_start, buf, buflen, 0);
Unsigned int buflen passed as int len argument without a range check?
Note that in the proc_pid_cmdline() code, they first cap it at PAGE_SIZE
before passing it.
buflen is passed by the caller. So if you look in the following patch
introducing its
use in proc/fs/base.c, their is a check.
/*The caller of this allocates a page */
if (len > PAGE_SIZE)
len = PAGE_SIZE;
res = copy_cmdline(task, mm, buffer, len);
The closer you can keep your code to the original proc_pid_cmdline()
code, the better (less chance for new bugs to be introduced).
> + if (res <= 0)
> + return 0;
> +
> + if (res > buflen)
> + res = buflen;
Is this a possible condition? Under what circumstances?
for (res <= 0), in that case, the underlying call
to __access_remote_vm() returns an int. Most of the mm functions look
like they are using
ints for probably some historical reason I am not aware of. I tried to
pick the strongest invariant,
however, I don't think < 0 is possible.
For the res > buflen check, that might might be an artifact from the
PAGE_SIZE cap from the original
code. It would only be possible if a process was able to write to
their mm when the semaphores are held.
I am assuming the case of:
kernel gets size
kernel allocs buffer
kernel copys but size has differed. I guess if I broke the locking out
it could happen, you need size and copy
to be autonomous.
> + /*
> + * If the nul at the end of args had been overwritten, then
> + * assume application is using setproctitle(3).
> + */
> + if (buf[res-1] != '\0') {
Lost the len < PAGE_SIZE check from proc_pid_cmdline() here, and that
isn't the same as the check above.
> + /* Nul between start and end of vm space?
> + If so then truncate */
Not sure where these comments are coming from. Isn't the issue that
lack of NUL at the end of args indicates that the cmdline extends
further into the environ and thus they need to copy in the rest?
Their is no guarantee that their is a NULL from what I understand. So you need
to look for it, and copy from there. I have no qualms about dropping
the comments
their not very useful, as well as moving the block back to what the
original procfs
code had.
> + len = strnlen(buf, res);
> + if (len < res) {
> + res = len;
> + } else {
> + /* No nul, truncate buflen if to big */
It isn't truncating buflen but rather copying the remainder of the
cmdline from the environ, right?
> + len = mm->env_end - mm->env_start;
> + if (len > buflen - res)
> + len = buflen - res;
> + /* Copy any remaining data */
> + res += access_process_vm(task, mm->env_start, buf+res,
> + len, 0);
> + res = strnlen(buf, res);
> + }
> + }
> + return res;
> +}
I think you are better off just copying proc_pid_cmdline() exactly as is
into a common helper function and then reusing it for audit. Far less
work, and far less potential for mistakes.
I don't like caching a whole page in that audit context. So most of
the complexity relates to
determining the size of the cache. Steve Grub was in favor of limiting
the cmdline value to
PATH_MAX. So if that is an acceptable cache size, we can take the
existing code from
procfs/base.c and just add an argument indicating the size of the
buffer. procfs will be
PAGE_SIZE and audit will be PATH_MAX. Thoughts?
>
> /* Tracepoints definitions. */
> EXPORT_TRACEPOINT_SYMBOL(kmalloc);
>
--
Respectfully,
William C Roberts
9:04 a.m.
New subject: [PATCH 1/3] mm: Create utility functions for accessing a tasks commandline value
On 12/13/2013 09:51 AM, William Roberts wrote:
On Fri, Dec 13, 2013 at 9:12 AM, Stephen Smalley
<sds(a)tycho.nsa.gov> wrote:
> Also, why not just get_task_mm(task) within the function rather than
> pass it in by the caller?
>
Yes I was debating whether or not to drop the pointer checks... np
WRT the locking and moving it into the function. You need to take the lock
to determine the size of the cmdline area. The idea on the interface is you
would take the locks, acquire the size via the inline func, alloc memory and
then call the copy function. In some cases, like proc/pid/cmdline, they just
alloc a page and truncate on that boundary. However, one may with to truncate
on an arbitrary boundry, especially when cacheing the values, as you don't want
to allocate too much. So inbetween functions calls that get the length and copy,
one can make a decision based on their allocation scheme. Moving the locks
to the functions would require multiple locks and unlocks in the common case.
I don't think it is a good idea to split it up, as what happens if the
range changes between the time you compute the length and the time you
copy? And your current callers appear to always get_task_mm(), compute
len, call the helper, and mmput. So just take it all to the helper (at
which point the helper essentially becomes proc_pid_cmdline).
> Unsigned int buflen passed as int len argument without a range
check?
> Note that in the proc_pid_cmdline() code, they first cap it at PAGE_SIZE
> before passing it.
>
buflen is passed by the caller. So if you look in the following patch
introducing its
use in proc/fs/base.c, their is a check.
/*The caller of this allocates a page */
if (len > PAGE_SIZE)
len = PAGE_SIZE;
res = copy_cmdline(task, mm, buffer, len);
I understand that, but you are making correct operation of the helper
dependent on the caller already having applied such a cap to the length.
Which is unsafe practice and may not hold true for future callers.
>> + if (res <= 0)
>> + return 0;
>> +
>> + if (res > buflen)
>> + res = buflen;
>
> Is this a possible condition? Under what circumstances?
for (res <= 0), in that case, the underlying call
to __access_remote_vm() returns an int. Most of the mm functions look
like they are using
ints for probably some historical reason I am not aware of. I tried to
pick the strongest invariant,
however, I don't think < 0 is possible.
For the res > buflen check, that might might be an artifact from the
PAGE_SIZE cap from the original
code. It would only be possible if a process was able to write to
their mm when the semaphores are held.
I am assuming the case of:
kernel gets size
kernel allocs buffer
kernel copys but size has differed. I guess if I broke the locking out
it could happen, you need size and copy
to be autonomous.
Sorry, you misunderstood. The <=0 case is clearly possible; I was only
asking about the res > buflen check, which seems impossible as you
provided buflen as the max for the access_process_vm() call. That one
does not make sense to me and has no equivalent in the original
proc_pid_cmdline() code.
> I think you are better off just copying proc_pid_cmdline()
exactly as is
> into a common helper function and then reusing it for audit. Far less
> work, and far less potential for mistakes.
I don't like caching a whole page in that audit context. So most of
the complexity relates to
determining the size of the cache. Steve Grub was in favor of limiting
the cmdline value to
PATH_MAX. So if that is an acceptable cache size, we can take the
existing code from
procfs/base.c and just add an argument indicating the size of the
buffer. procfs will be
PAGE_SIZE and audit will be PATH_MAX. Thoughts?
Yes, that seems reasonable to me.
9:26 a.m.
New subject: [PATCH 1/3] mm: Create utility functions for accessing a tasks commandline value
On Fri, Dec 13, 2013 at 10:04 AM, Stephen Smalley <sds(a)tycho.nsa.gov> wrote:
On 12/13/2013 09:51 AM, William Roberts wrote:
> On Fri, Dec 13, 2013 at 9:12 AM, Stephen Smalley <sds(a)tycho.nsa.gov> wrote:
>> Also, why not just get_task_mm(task) within the function rather than
>> pass it in by the caller?
>>
>
> Yes I was debating whether or not to drop the pointer checks... np
>
> WRT the locking and moving it into the function. You need to take the lock
> to determine the size of the cmdline area. The idea on the interface is you
> would take the locks, acquire the size via the inline func, alloc memory and
> then call the copy function. In some cases, like proc/pid/cmdline, they just
> alloc a page and truncate on that boundary. However, one may with to truncate
> on an arbitrary boundry, especially when cacheing the values, as you don't want
> to allocate too much. So inbetween functions calls that get the length and copy,
> one can make a decision based on their allocation scheme. Moving the locks
> to the functions would require multiple locks and unlocks in the common case.
I don't think it is a good idea to split it up, as what happens if the
range changes between the time you compute the length and the time you
copy? And your current callers appear to always get_task_mm(), compute
len, call the helper, and mmput. So just take it all to the helper (at
which point the helper essentially becomes proc_pid_cmdline).
That's why I keep the semaphore over the whole operation. The issue arises with
the amount to allocate by the caller, which is a bit of a shot in the
dark. The caller
may not wish to over allocate a buffer. This same issue could arise in
the current
code, but they hold the sem.
>> Unsigned int buflen passed as int len argument without a
range check?
>> Note that in the proc_pid_cmdline() code, they first cap it at PAGE_SIZE
>> before passing it.
>>
>
> buflen is passed by the caller. So if you look in the following patch
> introducing its
> use in proc/fs/base.c, their is a check.
> /*The caller of this allocates a page */
> if (len > PAGE_SIZE)
> len = PAGE_SIZE;
>
> res = copy_cmdline(task, mm, buffer, len);
I understand that, but you are making correct operation of the helper
dependent on the caller already having applied such a cap to the length.
Which is unsafe practice and may not hold true for future callers.
Again, what if the caller wished to cap it as 32, 64, 128, or 2 * PAGE_SIZE?
They can just pass the value on which to cap, which is their buffer that they
allocated. Thus they know the size. Again, all of this boils down to
the allocation
scheme which you comment on below. With that said, this becomes a whole lot
simpler.
>>> + if (res <= 0)
>>> + return 0;
>>> +
>>> + if (res > buflen)
>>> + res = buflen;
>>
>> Is this a possible condition? Under what circumstances?
>
> for (res <= 0), in that case, the underlying call
> to __access_remote_vm() returns an int. Most of the mm functions look
> like they are using
> ints for probably some historical reason I am not aware of. I tried to
> pick the strongest invariant,
> however, I don't think < 0 is possible.
>
> For the res > buflen check, that might might be an artifact from the
> PAGE_SIZE cap from the original
> code. It would only be possible if a process was able to write to
> their mm when the semaphores are held.
> I am assuming the case of:
> kernel gets size
> kernel allocs buffer
> kernel copys but size has differed. I guess if I broke the locking out
> it could happen, you need size and copy
> to be autonomous.
Sorry, you misunderstood. The <=0 case is clearly possible; I was only
asking about the res > buflen check, which seems impossible as you
provided buflen as the max for the access_process_vm() call. That one
does not make sense to me and has no equivalent in the original
proc_pid_cmdline() code.
>> I think you are better off just copying proc_pid_cmdline() exactly as is
>> into a common helper function and then reusing it for audit. Far less
>> work, and far less potential for mistakes.
>
> I don't like caching a whole page in that audit context. So most of
> the complexity relates to
> determining the size of the cache. Steve Grub was in favor of limiting
> the cmdline value to
> PATH_MAX. So if that is an acceptable cache size, we can take the
> existing code from
> procfs/base.c and just add an argument indicating the size of the
> buffer. procfs will be
> PAGE_SIZE and audit will be PATH_MAX. Thoughts?
Yes, that seems reasonable to me.
SGTM, Ill go that route with these... much simpler and avoids
the caller having to know how to lock the memory region properly.
--
Respectfully,
William C Roberts
9:27 a.m.
New subject: [PATCH 1/3] mm: Create utility functions for accessing a tasks commandline value
On Fri, Dec 13, 2013 at 10:26 AM, William Roberts
<bill.c.roberts(a)gmail.com> wrote:
On Fri, Dec 13, 2013 at 10:04 AM, Stephen Smalley
<sds(a)tycho.nsa.gov> wrote:
> On 12/13/2013 09:51 AM, William Roberts wrote:
>> On Fri, Dec 13, 2013 at 9:12 AM, Stephen Smalley <sds(a)tycho.nsa.gov>
wrote:
>>> Also, why not just get_task_mm(task) within the function rather than
>>> pass it in by the caller?
>>>
>>
>> Yes I was debating whether or not to drop the pointer checks... np
>>
>> WRT the locking and moving it into the function. You need to take the lock
>> to determine the size of the cmdline area. The idea on the interface is you
>> would take the locks, acquire the size via the inline func, alloc memory and
>> then call the copy function. In some cases, like proc/pid/cmdline, they just
>> alloc a page and truncate on that boundary. However, one may with to truncate
>> on an arbitrary boundry, especially when cacheing the values, as you don't
want
>> to allocate too much. So inbetween functions calls that get the length and copy,
>> one can make a decision based on their allocation scheme. Moving the locks
>> to the functions would require multiple locks and unlocks in the common case.
>
> I don't think it is a good idea to split it up, as what happens if the
> range changes between the time you compute the length and the time you
> copy? And your current callers appear to always get_task_mm(), compute
> len, call the helper, and mmput. So just take it all to the helper (at
> which point the helper essentially becomes proc_pid_cmdline).
>
That's why I keep the semaphore over the whole operation. The issue arises with
the amount to allocate by the caller, which is a bit of a shot in the
dark. The caller
may not wish to over allocate a buffer. This same issue could arise in
the current
code, but they hold the sem.
>>> Unsigned int buflen passed as int len argument without a range check?
>>> Note that in the proc_pid_cmdline() code, they first cap it at PAGE_SIZE
>>> before passing it.
>>>
>>
>> buflen is passed by the caller. So if you look in the following patch
>> introducing its
>> use in proc/fs/base.c, their is a check.
>> /*The caller of this allocates a page */
>> if (len > PAGE_SIZE)
>> len = PAGE_SIZE;
>>
>> res = copy_cmdline(task, mm, buffer, len);
>
> I understand that, but you are making correct operation of the helper
> dependent on the caller already having applied such a cap to the length.
> Which is unsafe practice and may not hold true for future callers.
>
Again, what if the caller wished to cap it as 32, 64, 128, or 2 * PAGE_SIZE?
They can just pass the value on which to cap, which is their buffer that they
allocated. Thus they know the size. Again, all of this boils down to
the allocation
scheme which you comment on below. With that said, this becomes a whole lot
simpler.
>>>> + if (res <= 0)
>>>> + return 0;
>>>> +
>>>> + if (res > buflen)
>>>> + res = buflen;
>>>
>>> Is this a possible condition? Under what circumstances?
>>
>> for (res <= 0), in that case, the underlying call
>> to __access_remote_vm() returns an int. Most of the mm functions look
>> like they are using
>> ints for probably some historical reason I am not aware of. I tried to
>> pick the strongest invariant,
>> however, I don't think < 0 is possible.
>>
>> For the res > buflen check, that might might be an artifact from the
>> PAGE_SIZE cap from the original
>> code. It would only be possible if a process was able to write to
>> their mm when the semaphores are held.
>> I am assuming the case of:
>> kernel gets size
>> kernel allocs buffer
>> kernel copys but size has differed. I guess if I broke the locking out
>> it could happen, you need size and copy
>> to be autonomous.
>
> Sorry, you misunderstood. The <=0 case is clearly possible; I was only
> asking about the res > buflen check, which seems impossible as you
> provided buflen as the max for the access_process_vm() call. That one
> does not make sense to me and has no equivalent in the original
> proc_pid_cmdline() code.
>
>>> I think you are better off just copying proc_pid_cmdline() exactly as is
>>> into a common helper function and then reusing it for audit. Far less
>>> work, and far less potential for mistakes.
>>
>> I don't like caching a whole page in that audit context. So most of
>> the complexity relates to
>> determining the size of the cache. Steve Grub was in favor of limiting
>> the cmdline value to
>> PATH_MAX. So if that is an acceptable cache size, we can take the
>> existing code from
>> procfs/base.c and just add an argument indicating the size of the
>> buffer. procfs will be
>> PAGE_SIZE and audit will be PATH_MAX. Thoughts?
>
> Yes, that seems reasonable to me.
>
>
SGTM, Ill go that route with these... much simpler and avoids
the caller having to know how to lock the memory region properly.
FYI, i am away from my dev machine. ill get these out in a couple of weeks.
--
Respectfully,
William C Roberts
3:10 p.m.
New subject: [PATCH 2/3] proc: Update get proc_pid_cmdline() to use mm.h helpers
Re-factor proc_pid_cmdline() to use get_cmdline_length() and
copy_cmdline() helpers from mm.h
Signed-off-by: William Roberts <wroberts(a)tresys.com>
---
fs/proc/base.c | 35 ++++++++++-------------------------
1 file changed, 10 insertions(+), 25 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 03c8d74..fb4eda5 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -203,37 +203,22 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
static int proc_pid_cmdline(struct task_struct *task, char * buffer)
{
int res = 0;
- unsigned int len;
+ unsigned int len = 0;
struct mm_struct *mm = get_task_mm(task);
if (!mm)
- goto out;
- if (!mm->arg_end)
- goto out_mm; /* Shh! No looking before we're done */
+ return 0;
- len = mm->arg_end - mm->arg_start;
-
+ len = get_cmdline_length(mm);
+ if (!len)
+ goto mm_out;
+
+ /*The caller of this allocates a page */
if (len > PAGE_SIZE)
len = PAGE_SIZE;
-
- res = access_process_vm(task, mm->arg_start, buffer, len, 0);
-
- // If the nul at the end of args has been overwritten, then
- // assume application is using setproctitle(3).
- if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE)
{
- len = strnlen(buffer, res);
- if (len < res) {
- res = len;
- } else {
- len = mm->env_end - mm->env_start;
- if (len > PAGE_SIZE - res)
- len = PAGE_SIZE - res;
- res += access_process_vm(task, mm->env_start, buffer+res, len, 0);
- res = strnlen(buffer, res);
- }
- }
-out_mm:
+
+ res = copy_cmdline(task, mm, buffer, len);
+mm_out:
mmput(mm);
-out:
return res;
}
--
1.7.9.5
8:23 a.m.
New subject: [PATCH 2/3] proc: Update get proc_pid_cmdline() to use mm.h helpers
On 12/02/2013 04:10 PM, William Roberts wrote:
Re-factor proc_pid_cmdline() to use get_cmdline_length() and
copy_cmdline() helpers from mm.h
Signed-off-by: William Roberts <wroberts(a)tresys.com>
---
fs/proc/base.c | 35 ++++++++++-------------------------
1 file changed, 10 insertions(+), 25 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 03c8d74..fb4eda5 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -203,37 +203,22 @@ static int proc_root_link(struct dentry *dentry, struct path
*path)
static int proc_pid_cmdline(struct task_struct *task, char * buffer)
{
int res = 0;
- unsigned int len;
+ unsigned int len = 0;
Why? You set len below before first use, so this is redundant.
struct mm_struct *mm = get_task_mm(task);
if (!mm)
- goto out;
- if (!mm->arg_end)
- goto out_mm; /* Shh! No looking before we're done */
+ return 0;
Equivalent to goto out in the original code, so why change it? Don't
make unnecessary changes.
Also, I think the get_task_mm() ought to move into the helper (or all of
proc_pid_cmdline() should just become the helper). In what situation
will you not be calling get_task_mm() and mmput() around every call to
the helper?
- len = mm->arg_end - mm->arg_start;
-
+ len = get_cmdline_length(mm);
+ if (!len)
+ goto mm_out;
Could be moved into the helper. Not sure how the inline function helps
readability or maintainability.
+
+ /*The caller of this allocates a page */
if (len > PAGE_SIZE)
len = PAGE_SIZE;
If the capping of len is handled by the caller, then pass an int to your
helper rather than an unsigned int to avoid problems later with
access_process_vm().
-out_mm:
+
+ res = copy_cmdline(task, mm, buffer, len);
+mm_out:
mmput(mm);
Odd style. If there is only one exit path, just call it out; if there
are two, keep them as out_mm and out.
-out:
return res;
}
8:57 a.m.
New subject: [PATCH 2/3] proc: Update get proc_pid_cmdline() to use mm.h helpers
On Fri, Dec 13, 2013 at 9:23 AM, Stephen Smalley <sds(a)tycho.nsa.gov> wrote:
On 12/02/2013 04:10 PM, William Roberts wrote:
> Re-factor proc_pid_cmdline() to use get_cmdline_length() and
> copy_cmdline() helpers from mm.h
>
> Signed-off-by: William Roberts <wroberts(a)tresys.com>
> ---
> fs/proc/base.c | 35 ++++++++++-------------------------
> 1 file changed, 10 insertions(+), 25 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 03c8d74..fb4eda5 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -203,37 +203,22 @@ static int proc_root_link(struct dentry *dentry, struct path
*path)
> static int proc_pid_cmdline(struct task_struct *task, char * buffer)
> {
> int res = 0;
> - unsigned int len;
> + unsigned int len = 0;
Why? You set len below before first use, so this is redundant.
Yep you're right.
> struct mm_struct *mm = get_task_mm(task);
> if (!mm)
> - goto out;
> - if (!mm->arg_end)
> - goto out_mm; /* Shh! No looking before we're done */
> + return 0;
Equivalent to goto out in the original code, so why change it? Don't
make unnecessary changes.
Also, I think the get_task_mm() ought to move into the helper (or all of
proc_pid_cmdline() should just become the helper). In what situation
will you not be calling get_task_mm() and mmput() around every call to
the helper?
Again my thought on this is to reduce get_task_mm() and mmput() calls.
How expensive are they, etc. However, just to recap the other email. If we
move to saying the audit cache of this can be capped at PATH_MAX even
if it results in some wasted memory, we can just take the original procfs code
and add a length argument.
>
> - len = mm->arg_end - mm->arg_start;
> -
> + len = get_cmdline_length(mm);
> + if (!len)
> + goto mm_out;
Could be moved into the helper. Not sure how the inline function helps
readability or maintainability.
Sure... mostly for readability.
> +
> + /*The caller of this allocates a page */
> if (len > PAGE_SIZE)
> len = PAGE_SIZE;
If the capping of len is handled by the caller, then pass an int to your
helper rather than an unsigned int to avoid problems later with
access_process_vm().
Ok... just weird that lengths are signed to me. when will you ever have negative
space?
> -out_mm:
> +
> + res = copy_cmdline(task, mm, buffer, len);
> +mm_out:
> mmput(mm);
Odd style. If there is only one exit path, just call it out; if there
are two, keep them as out_mm and out.
Yes their is only 1 jmp label. Your right, this is odd.
> -out:
> return res;
> }
>
>
--
Respectfully,
William C Roberts
3:10 p.m.
New subject: [PATCH 3/3] audit: Audit proc cmdline value
During an audit event, cache and print the value of the process's
cmdline value (proc/<pid>/cmdline). This is useful in situations
where processes are started via fork'd virtual machines where the
comm field is incorrect. Often times, setting the comm field still
is insufficient as the comm width is not very wide and most
virtual machine "package names" do not fit. Also, during execution,
many threads have thier comm field set as well. By tying it back to
the global cmdline value for the process, audit records will be more
complete in systems with these properties. An example of where this
is useful and applicable is in the realm of Android.
The cached cmdline is tied to the lifecycle of the audit_context
structure and is built on demand.
Signed-off-by: William Roberts <wroberts(a)tresys.com>
---
kernel/audit.h | 1 +
kernel/auditsc.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+)
diff --git a/kernel/audit.h b/kernel/audit.h
index b779642..bd6211f 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -202,6 +202,7 @@ struct audit_context {
} execve;
};
int fds[2];
+ char *cmdline;
#if AUDIT_DEBUG
int put_count;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 90594c9..bfb1698 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -842,6 +842,14 @@ static inline struct audit_context *audit_get_context(struct
task_struct *tsk,
return context;
}
+static inline void audit_cmdline_free(struct audit_context *context)
+{
+ if (!context->cmdline)
+ return;
+ kfree(context->cmdline);
+ context->cmdline = NULL;
+}
+
static inline void audit_free_names(struct audit_context *context)
{
struct audit_names *n, *next;
@@ -955,6 +963,7 @@ static inline void audit_free_context(struct audit_context *context)
audit_free_aux(context);
kfree(context->filterkey);
kfree(context->sockaddr);
+ audit_cmdline_free(context);
kfree(context);
}
@@ -1271,6 +1280,78 @@ static void show_special(struct audit_context *context, int
*call_panic)
audit_log_end(ab);
}
+static char *audit_cmdline_get(struct audit_buffer *ab,
+ struct task_struct *task)
+{
+ int len;
+ int res;
+ char *buf;
+ struct mm_struct *mm;
+
+ if (!ab || !task)
+ return NULL;
+
+ mm = get_task_mm(task);
+ if (!mm)
+ return NULL;
+
+ len = get_cmdline_length(mm);
+ if (!len)
+ goto mm_err;
+
+ if (len > PATH_MAX)
+ len = PATH_MAX;
+
+ buf = kmalloc(len, GFP_KERNEL);
+ if (!buf)
+ goto mm_err;
+
+ res = copy_cmdline(task, mm, buf, len);
+ if (res <= 0)
+ goto alloc_err;
+
+ mmput(mm);
+ /*
+ * res is guarenteed not to be longer than
+ * than the buf as it was truncated to len
+ * in copy_cmdline()
+ */
+ len = res;
+
+ /*
+ * Ensure NULL terminated as application
+ * could be using setproctitle(3)
+ */
+ buf[len-1] = '\0';
+ return buf;
+
+alloc_err:
+ kfree(buf);
+mm_err:
+ mmput(mm);
+ return NULL;
+}
+
+static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
+ struct audit_context *context)
+{
+ char *msg = "(null)";
+ audit_log_format(ab, " cmdline=");
+
+ /* Already cached */
+ if (context->cmdline) {
+ msg = context->cmdline;
+ goto out;
+ }
+ /* Not cached */
+ context->cmdline = audit_cmdline_get(ab, tsk);
+ if (!context->cmdline)
+ goto out;
+ msg = context->cmdline;
+out:
+ audit_log_untrustedstring(ab, msg);
+}
+
static void audit_log_exit(struct audit_context *context, struct task_struct *tsk)
{
int i, call_panic = 0;
@@ -1302,6 +1383,7 @@ static void audit_log_exit(struct audit_context *context, struct
task_struct *ts
context->name_count);
audit_log_task_info(ab, tsk);
+ audit_log_cmdline(ab, tsk, context);
audit_log_key(ab, context->filterkey);
audit_log_end(ab);
--
1.7.9.5
9:33 a.m.
New subject: [PATCH 3/3] audit: Audit proc cmdline value
On Mon, Dec 02, 2013 at 01:10:39PM -0800, William Roberts wrote:
During an audit event, cache and print the value of the
process's
cmdline value (proc/<pid>/cmdline). This is useful in situations
where processes are started via fork'd virtual machines where the
comm field is incorrect. Often times, setting the comm field still
is insufficient as the comm width is not very wide and most
virtual machine "package names" do not fit. Also, during execution,
many threads have thier comm field set as well. By tying it back to
the global cmdline value for the process, audit records will be more
complete in systems with these properties. An example of where this
is useful and applicable is in the realm of Android.
The cached cmdline is tied to the lifecycle of the audit_context
structure and is built on demand.
Signed-off-by: William Roberts <wroberts(a)tresys.com>
Acked-by: Richard Guy Briggs <rgb(a)redhat.com>
I'll Signed-off-by: when I add it to my for-next tree.
---
kernel/audit.h | 1 +
kernel/auditsc.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+)
diff --git a/kernel/audit.h b/kernel/audit.h
index b779642..bd6211f 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -202,6 +202,7 @@ struct audit_context {
} execve;
};
int fds[2];
+ char *cmdline;
#if AUDIT_DEBUG
int put_count;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 90594c9..bfb1698 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -842,6 +842,14 @@ static inline struct audit_context *audit_get_context(struct
task_struct *tsk,
return context;
}
+static inline void audit_cmdline_free(struct audit_context *context)
+{
+ if (!context->cmdline)
+ return;
+ kfree(context->cmdline);
+ context->cmdline = NULL;
+}
+
static inline void audit_free_names(struct audit_context *context)
{
struct audit_names *n, *next;
@@ -955,6 +963,7 @@ static inline void audit_free_context(struct audit_context *context)
audit_free_aux(context);
kfree(context->filterkey);
kfree(context->sockaddr);
+ audit_cmdline_free(context);
kfree(context);
}
@@ -1271,6 +1280,78 @@ static void show_special(struct audit_context *context, int
*call_panic)
audit_log_end(ab);
}
+static char *audit_cmdline_get(struct audit_buffer *ab,
+ struct task_struct *task)
+{
+ int len;
+ int res;
+ char *buf;
+ struct mm_struct *mm;
+
+ if (!ab || !task)
+ return NULL;
+
+ mm = get_task_mm(task);
+ if (!mm)
+ return NULL;
+
+ len = get_cmdline_length(mm);
+ if (!len)
+ goto mm_err;
+
+ if (len > PATH_MAX)
+ len = PATH_MAX;
+
+ buf = kmalloc(len, GFP_KERNEL);
+ if (!buf)
+ goto mm_err;
+
+ res = copy_cmdline(task, mm, buf, len);
+ if (res <= 0)
+ goto alloc_err;
+
+ mmput(mm);
+ /*
+ * res is guarenteed not to be longer than
+ * than the buf as it was truncated to len
+ * in copy_cmdline()
+ */
+ len = res;
+
+ /*
+ * Ensure NULL terminated as application
+ * could be using setproctitle(3)
+ */
+ buf[len-1] = '\0';
+ return buf;
+
+alloc_err:
+ kfree(buf);
+mm_err:
+ mmput(mm);
+ return NULL;
+}
+
+static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
+ struct audit_context *context)
+{
+ char *msg = "(null)";
+ audit_log_format(ab, " cmdline=");
+
+ /* Already cached */
+ if (context->cmdline) {
+ msg = context->cmdline;
+ goto out;
+ }
+ /* Not cached */
+ context->cmdline = audit_cmdline_get(ab, tsk);
+ if (!context->cmdline)
+ goto out;
+ msg = context->cmdline;
+out:
+ audit_log_untrustedstring(ab, msg);
+}
+
static void audit_log_exit(struct audit_context *context, struct task_struct *tsk)
{
int i, call_panic = 0;
@@ -1302,6 +1383,7 @@ static void audit_log_exit(struct audit_context *context, struct
task_struct *ts
context->name_count);
audit_log_task_info(ab, tsk);
+ audit_log_cmdline(ab, tsk, context);
audit_log_key(ab, context->filterkey);
audit_log_end(ab);
--
1.7.9.5
--
Linux-audit mailing list
Linux-audit(a)redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
4026
days inactive
4037
days old
linux-audit@lists.linux-audit.osci.io
11 comments
3 participants
participants (3)
-
Richard Guy Briggs -
Stephen Smalley -
William Roberts