get_mm_cmdline() leaks an uninitialized byte located at
user-controlled offset in a newly-allocated kernel page in
the following scenario.
- When reading the last chunk of cmdline, access_remote_vm()
fails to copy the requested number of bytes, but still copies
enough bytes so that we get into the body of
"if (pos + got >= arg_end)" statement. This can be arranged by user,
for example, by applying mprotect(PROT_NONE) to the env block.
- strnlen() doesn't find a NUL byte. This too can be arranged
by user via suitable modifications of argument and env blocks.
- The above causes the following condition to be true despite
that no NUL byte was found:
/* Include the NUL if it existed */
if (got < size)
got++;
The resulting increment causes the subsequent copy_to_user()
to copy an extra byte from "page" to userspace. That byte might
come from previous uses of memory referred by "page" before
it was allocated by get_mm_cmdline(), potentially leaking
data belonging to other processes or kernel.
Fix this by ensuring that "size + offset" doesn't exceed the number
of bytes copied by access_remote_vm().
Fixes: f5b65348fd77 ("proc: fix missing final NUL in get_mm_cmdline() rewrite")
Cc: Alexey Dobriyan <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Signed-off-by: Alexey Izbyshev <[email protected]>
---
This patch was initially sent to <[email protected]> accompanied
with a little program that exploits the bug to dump the kernel page
used in get_mm_cmdline().
Thanks to Oleg Nesterov and Laura Abbott for their feedback!
fs/proc/base.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 255f6754c70d..6e30dd791761 100644
--- 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;
/* Don't walk past a NUL character once you hit arg_end */
if (pos + got >= arg_end) {
--
2.21.0
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 <[email protected]>
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 <[email protected]>
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
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 <[email protected]>
>
> 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
On 2019-07-12, Alexey Izbyshev wrote:
> On 7/12/19 8:46 PM, Alexey Dobriyan wrote:
>> 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.
I posted this (corner?) case ~3 months ago, unfortunately it wasn't picked
up by anyone: https://lkml.org/lkml/2019/4/5/825
You can treat it as another datapoint in this discussion.
Regards,
Jakub
--
Jakub Jankowski|[email protected]|https://toxcorp.com/
On 7/13/19 12:17 AM, Jakub Jankowski wrote:
> On 2019-07-12, Alexey Izbyshev wrote:
>
>> On 7/12/19 8:46 PM, Alexey Dobriyan wrote:
>>> 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.
>
> I posted this (corner?) case ~3 months ago, unfortunately it wasn't picked up by anyone: https://lkml.org/lkml/2019/4/5/825
> You can treat it as another datapoint in this discussion.
>
Thanks, this is interesting. Perl explicitly relies on special treatment of
non-NUL at arg_end-1 in pre-5ab8271899658042: on argv0 replace, it fills
everything after the new argv0 in the combined argv/env block with spaces [1,2].
While personally I don't approve what Perl does here, 5ab8271899658042
did change the user-visible behavior in this case.
[1] https://perl5.git.perl.org/perl.git/blob/86b50d930caa:/mg.c#l2733
[2] https://perl5.git.perl.org/perl.git/blob/86b50d930caa:/perl.c#l1698
Alexey
>
> Regards,
> Jakub
>
On Fri, Jul 12, 2019 at 09:43:03PM +0300, Alexey Izbyshev wrote:
> 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 <[email protected]>
> >
> > 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?
Of course! Do you have a reproducer?
> In the former case, will it be OK for long term branches?
For everyone.
If a rewrite causes 1 bug, 1 user visible change and a infoleak, it is
called revert.
> get_mm_cmdline() does seem easier to read for me before 5ab8271899658042.
> But it also has different semantics in corner cases, for example:
All semantics changes are recent.
> - 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.
That's because fixed-length /proc/*/cmdline did that.
> - 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.
On 7/13/19 10:26 AM, Alexey Dobriyan wrote:
> On Fri, Jul 12, 2019 at 09:43:03PM +0300, Alexey Izbyshev wrote:
>> On 7/12/19 8:46 PM, Alexey Dobriyan wrote:
>>> 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?
>
> Of course! Do you have a reproducer?
>
Attached.
Alexey