linux-kernel - Re: [RFC][PATCH 13/22] x86: ia32_setup_sigcontext(): lift user_access

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:   Mon, 23 Mar 2020 21:42:48 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        the arch/x86 maintainers <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][PATCH 13/22] x86: ia32_setup_sigcontext(): lift
 user_access_{begin,end}() into the callers

On Mon, Mar 23, 2020 at 11:53:39AM -0700, Linus Torvalds wrote:
> On Mon, Mar 23, 2020 at 11:39 AM Al Viro <viro@...iv.linux.org.uk> wrote:
> >
> > -static int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
> > +static __always_inline int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
> 
> Please rename this at the same time (to "unsafe_ia32_setup_sigcontext()").
> 
> I absolutely _hate_ how we have historically split the "__get_user()"
> calls from the "access_ok()" calls, and then have had bugs when we had
> ways to reach the user access without checking it.
> 
> Yes, we have static checking for the unsafe stuff in objtool now, but
> I still want this to be explicit on the source level too: if you do
> unsafe user accesses, you make it very very explicit in the naming, so
> that you can't possibly even by mistake have a "let's call this
> function withou having done the user_access_begin()" calls.

Umm...  OK, but I wonder if unsafe_... makes the right naming conventions
for such cases.  Note that towards the end of that series we get

#define unsafe_put_sigcontext(sc, fpstate, regs, set, label)    \
do {                                                            \
        if (setup_sigcontext(sc, fpstate, regs, set->sig[0]))   \
                goto label;                                     \
} while(0);

and that's not an uncommon pattern.  We generally have unsafe_...
mean "doesn't return anything, takes a label, needs to be called
from under user_access_begin" and I suspect that it would make sense
to have another recognizable naming pattern for "it must be called
from under user_access_begin() and you need to look at return value".

In this case we could grit teeth and turn that sucker into a macro.
But what about e.g. lifting user_access_{begin,end}() out of
raw_copy_from_user()?  unsafe_copy_from_user() would imply
"all or nothing" kind of calling conventions, like e.g.
unsafe_copy_to_user() currently does.  Which is fine in some
situations, and it's a good helper to have, but we definitely
want a "how much is left to copy" variant as well.

Hmm...  raw_setup_sigcontext(), perhaps, along with the
macro above for unsafe_put_sigcontext()?