Re: [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline

From: Michal Kubecek
Date: Wed Jun 20 2018 - 01:08:31 EST


On Wed, Jun 20, 2018 at 06:56:04AM +0900, Linus Torvalds wrote:
> On Wed, Jun 20, 2018 at 1:24 AM Michal Kubecek <mkubecek@xxxxxxx> wrote:
> >
> > Recent rewrite introduced a regression, /proc/$pid/cmdline is missing the
> > trailing null character:
> >
> > mike@lion:/tmp> cat /proc/self/cmdline | od -t c
> > 0000000 c a t \0 / p r o c / s e l f / c
> > 0000020 m d l i n e
> > 0000026
>
> Thanks, and obviously right you are.
>
> That said, I'm not a fan of your patch. I'd much rather just tweak the
> "strnlen()" logic a bit instead, and make the rule be that when we go
> into the "slop" area, we always include the last byte of the "real"
> argv area.
>
> That limits the slop to a page (well, one byte less, since we want the
> one byte of non-slop), but honestly, a page for *everything* was what
> we used to do originally, so..

Yes, that should be enough for real life applications.

> How does the attached patch work for you?

I haven't tested it yet but it looks good except this:

> @@ -254,10 +258,19 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
> while (count) {
> int got;
> size_t size = min_t(size_t, PAGE_SIZE, count);

We limit size to be at most PAGE_SIZE here.

> + long offset;
>
> - got = access_remote_vm(mm, pos, page, size, FOLL_ANON);
> - if (got <= 0)
> + /*
> + * Are we already starting past the official end?
> + * We always include the last byte that is *supposed*
> + * to be NUL
> + */
> + offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
> +
> + got = access_remote_vm(mm, pos - offset, page, size + offset, FOLL_ANON);

But here we read (size + offset) bytes which may be more than PAGE_SIZE.
I guess it should rather be

size_t size;
...
offset = (pos >= arg_end) ? pos - arg_end + 1 : 0;
size = min_t(size_t, PAGE_SIZE - offset, count);

We already made sure that offset < PAGE_SIZE so that size will be at
least 1.

Michal Kubecek