Re: [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.