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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Jun 2018 08:09:59 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexey Dobriyan <adobriyan@...il.com>
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@...e.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@...e.cz>
Tested-by: Michal Kubecek <mkubecek@...e.cz>

Michal Kubecek