Messages in this thread | | | Date | Wed, 20 Jun 2018 07:08:24 +0200 | From | Michal Kubecek <> | Subject | Re: [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline |
| |
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@suse.cz> 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
| |