linux-kernel - Re: [PATCH] proc: Fix uninitialized byte read in get_mm

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: <3de2d71b-37be-6238-7fd8-0a40c9b94a98@ispras.ru>
Date:   Fri, 12 Jul 2019 21:43:03 +0300
From:   Alexey Izbyshev <izbyshev@...ras.ru>
To:     Alexey Dobriyan <adobriyan@...il.com>,
        Oleg Nesterov <oleg@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        security@...nel.org
Subject: Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()

On 7/12/19 8:46 PM, Alexey Dobriyan wrote:
> On Fri, Jul 12, 2019 at 06:36:26PM +0200, Oleg Nesterov wrote:
>> On 07/12, Alexey Izbyshev wrote:
>>>
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -275,6 +275,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
>>>  		if (got <= offset)
>>>  			break;
>>>  		got -= offset;
>>> +		if (got < size)
>>> +			size = got;
>>
>> Acked-by: Oleg Nesterov <oleg@...hat.com>
> 
> The proper fix to all /proc/*/cmdline problems is to revert
> 
> 	f5b65348fd77839b50e79bc0a5e536832ea52d8d
> 	proc: fix missing final NUL in get_mm_cmdline() rewrite
> 
> 	5ab8271899658042fabc5ae7e6a99066a210bc0e
> 	fs/proc: simplify and clarify get_mm_cmdline() function
> 
Should this be interpreted as an actual suggestion to revert the patches,
fix the conflicts, test and submit them, or is this more like thinking out
loud? In the former case, will it be OK for long term branches?

get_mm_cmdline() does seem easier to read for me before 5ab8271899658042.
But it also has different semantics in corner cases, for example:

- If there is no NUL at arg_end-1, it reads only the first string in
the combined arg/env block, and doesn't terminate it with NUL.

- If there is any problem with access_remote_vm() or copy_to_user(),
it returns -EFAULT even if some data were copied to userspace.

On the other hand, 5ab8271899658042 was merged not too long ago (about a year),
so it's possible that the current semantics isn't heavily relied upon.

Alexey