linux-kernel - Re: [PATCH 4/4] mm: variable length argument support

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:	Tue, 5 Jun 2007 17:48:54 -0700
From:	"Ollie Wild" <aaw@...gle.com>
To:	"Andrew Morton" <akpm@...ux-foundation.org>
Cc:	"Peter Zijlstra" <a.p.zijlstra@...llo.nl>,
	linux-kernel@...r.kernel.org, parisc-linux@...ts.parisc-linux.org,
	linux-mm@...ck.org, linux-arch@...r.kernel.org,
	"Ingo Molnar" <mingo@...e.hu>, "Andi Kleen" <ak@...e.de>
Subject: Re: [PATCH 4/4] mm: variable length argument support

OK.  It sounds like a healthy dose of comments is in order.  I'll
clean things up and send out a new patch sometime tonight or tomorrow.

Additional comments inline below:

> > -             len = strnlen_user((void __user *)p, PAGE_SIZE*MAX_ARG_PAGES);
> > -             if (!len || len > PAGE_SIZE*MAX_ARG_PAGES)
> > +             len = strnlen_user((void __user *)p, MAX_ARG_STRLEN);
> > +             if (!len || len > MAX_ARG_STRLEN)
>
> strnlen_user() is a scary function.  Please do remember that if the memory
> we just strlen'ed is writeable by any user thread then that thread can at
> any time invalidate the number which the kernel now holds.

At this point, we've already called setup_arg_pages(), so the user
memory is our own private copy.  No other threads can access it.

> > -                     !(len = strnlen_user(compat_ptr(str), bprm->p))) {
> > +                 !(len = strnlen_user(compat_ptr(str), MAX_ARG_STRLEN))) {
> >                       ret = -EFAULT;
> >                       goto out;
> >               }
> >
> > -             if (bprm->p < len)  {
> > +             if (MAX_ARG_STRLEN < len) {
> >                       ret = -E2BIG;
> >                       goto out;
> >               }
>
> Do we have an off-by-one here?  Should it be <=?

No, strnlen_user() returns N+1 (where N==MAX_ARG_STRLEN) if the string
is too large.

> If not, then this code is relying upon the string's terminating \0 coming
> from userspace?  If so, that's buggy: userspace can overwrite the \0 after
> we ran the strnlen_user(), perhaps, and confound the kernel?

If that's the case, then we will fail to copy the null terminator, and
the string will munge into the following string.  Since we always
access this data via the various userspace access routines, we will
either return an error on a later operation, or the new process will
segfault shortly upon starting.

> > +             vma_adjust(vma, new_start, old_end,
> > +                        vma->vm_pgoff - (-shift >> PAGE_SHIFT), NULL);
>
> hm, a right-shift of a negated unsigned value.  That's pretty unusual.  I
> hope you know what you're doing ;)

This is correct.  In this case, shift is already populated with a
negative, wrapped unsigned value.  The -shift is needed to make it
positive before the bitwise shift.

> >  #define EXTRA_STACK_VM_PAGES 20      /* random */
> >
> > +/* Finalizes the stack vm_area_struct.  The flags and permissions are updated,
> > + * the stack is optionally relocated, and some extra space is added.
> > + */
>
> That's better.
>
> But what extra space is added, and why?

We add EXTRA_STACK_VM_PAGES.  To be honest, I think neither of us know
why this is done.  It's just what the old code did, so we preserved
it.

Ollie
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/