Messages in this thread | | | Date | Wed, 20 Jun 2018 08:09:59 +0200 | From | Michal Kubecek <> | Subject | Re: [PATCH] proc: add missing '\0' back to /proc/$pid/cmdline |
| |
On Wed, Jun 20, 2018 at 02:51:23PM +0900, Linus Torvalds wrote: > On Wed, Jun 20, 2018 at 2:08 PM Michal Kubecek <mkubecek@suse.cz> wrote: > > > > > > > @@ -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. > > Actually, no. We limit size not just to PAGE_SIZE, but to count as well. > > And there's *another* limit on 'count' that you missed, namely this part: > > /* .. and limit it to a maximum of one page of slop */ > if (env_end >= arg_end + PAGE_SIZE) > env_end = arg_end + PAGE_SIZE - 1; > > coupled with > > /* .. and we never go past env_end */ > if (env_end - pos < count) > count = env_end - pos; > > so we know that "pos + size" can never be larger than "arg_end + PAGE_SIZE - 1" > > And then: > > offset = (pos >= arg_end) ? pos - arg_end + 1 : 0; > > means that "offset" will be bigger than zero only if "pos > arg_end-1". > > So let's ignore all other cases, and just say that we care about that > case where 'offset' can be non-zero. > > So we have > > offset = pos - arg_end +1 > > (from the above initialization of offset), but we also know that > > pos + count <= end_end > > and since we've limited end_end to "arg_end + PAGE_SIZE -1" we have > > pos + count <= arg_end + PAGE_SIZE -1 > > agreed? > > Now, we can do some math on the above. Re-write that "offset = .." equation as > > pos = arg_end + offset - 1 > > And the same time we can take that "pos + count" inequality, and > replacing "pos", we get > > arg_end + offset - 1 + count <= arg_end + PAGE_SIZE -1 > > and then we can remove "arg_end - 1" from both sides, and get > > offset+count <= PAGE_SIZE > > agreed?
Right... I missed the trick where env_end is no longer real env_end when it's used to limit count.
And the tests look good too so
Reviewed-by: Michal Kubecek <mkubecek@suse.cz> Tested-by: Michal Kubecek <mkubecek@suse.cz>
Michal Kubecek
| |