On Wed, Jun 20, 2018 at 06:56:04AM +0900, Linus Torvalds wrote:
> On Wed, Jun 20, 2018 at 1:24 AM Michal Kubecek <[email protected]> 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
