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 14:51:23 +0900
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Michal Kubecek <mkubecek@...e.cz>
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 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?

              Linus