linux-kernel - Re: [PATCH 1/3] mm: Create utility functions for accessing a tasks commandline value

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52AB21F1.6030003@tycho.nsa.gov>
Date:	Fri, 13 Dec 2013 10:04:17 -0500
From:	Stephen Smalley <sds@...ho.nsa.gov>
To:	William Roberts <bill.c.roberts@...il.com>
CC:	"linux-audit@...hat.com" <linux-audit@...hat.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Richard Guy Briggs <rgb@...hat.com>,
	"viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
	William Roberts <wroberts@...sys.com>
Subject: 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@...ho.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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/