by Al Viro

[permalink] [raw]

2020-03-27 02:35:06

by Al Viro

[permalink] [raw]

Subject: [RFC][PATCH v2 05/22] vm86: get rid of get_user_ex() use

2020-03-27 02:35:11

by Al Viro

[permalink] [raw]

Subject: [RFC][PATCH v2 04/22] x86: get rid of small constant size cases in raw_copy_{to,from}_user()

2020-03-27 02:35:15

by Al Viro

[permalink] [raw]

Subject: [RFC][PATCH v2 06/22] x86: get rid of get_user_ex() in ia32_restore_sigcontext()

From: Al Viro <[email protected]>

Just do copyin into a local struct and be done with that - we are
on a shallow stack here.

[reworked by tglx, removing the macro horrors while we are touching that]

Signed-off-by: Al Viro <[email protected]>
---
arch/x86/ia32/ia32_signal.c | 106 ++++++++++++++++++--------------------------
1 file changed, 44 insertions(+), 62 deletions(-)

diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index c72025d615f8..23e2c55d8a59 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -36,70 +36,56 @@
#include <asm/sighandling.h>
#include <asm/smap.h>

+static inline void reload_segments(struct sigcontext_32 *sc)
+{
+ unsigned int cur;
+
+ savesegment(gs, cur);
+ if ((sc->gs | 0x03) != cur)
+ load_gs_index(sc->gs | 0x03);
+ savesegment(fs, cur);
+ if ((sc->fs | 0x03) != cur)
+ loadsegment(fs, sc->fs | 0x03);
+ savesegment(ds, cur);
+ if ((sc->ds | 0x03) != cur)
+ loadsegment(ds, sc->ds | 0x03);
+ savesegment(es, cur);
+ if ((sc->es | 0x03) != cur)
+ loadsegment(es, sc->es | 0x03);
+}
+
/*
* Do a signal return; undo the signal stack.
*/
-#define loadsegment_gs(v) load_gs_index(v)
-#define loadsegment_fs(v) loadsegment(fs, v)
-#define loadsegment_ds(v) loadsegment(ds, v)
-#define loadsegment_es(v) loadsegment(es, v)
-
-#define get_user_seg(seg) ({ unsigned int v; savesegment(seg, v); v; })
-#define set_user_seg(seg, v) loadsegment_##seg(v)
-
-#define COPY(x) { \
- get_user_ex(regs->x, &sc->x); \
-}
-
-#define GET_SEG(seg) ({ \
- unsigned short tmp; \
- get_user_ex(tmp, &sc->seg); \
- tmp; \
-})
-
-#define COPY_SEG_CPL3(seg) do { \
- regs->seg = GET_SEG(seg) | 3; \
-} while (0)
-
-#define RELOAD_SEG(seg) { \
- unsigned int pre = (seg) | 3; \
- unsigned int cur = get_user_seg(seg); \
- if (pre != cur) \
- set_user_seg(seg, pre); \
-}
-
static int ia32_restore_sigcontext(struct pt_regs *regs,
- struct sigcontext_32 __user *sc)
+ struct sigcontext_32 __user *usc)
{
- unsigned int tmpflags, err = 0;
- u16 gs, fs, es, ds;
- void __user *buf;
- u32 tmp;
+ struct sigcontext_32 sc;

/* Always make any pending restarted system calls return -EINTR */
current->restart_block.fn = do_no_restart_syscall;

- get_user_try {
- gs = GET_SEG(gs);
- fs = GET_SEG(fs);
- ds = GET_SEG(ds);
- es = GET_SEG(es);
-
- COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
- COPY(dx); COPY(cx); COPY(ip); COPY(ax);
- /* Don't touch extended registers */
-
- COPY_SEG_CPL3(cs);
- COPY_SEG_CPL3(ss);
-
- get_user_ex(tmpflags, &sc->flags);
- regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);
- /* disable syscall checks */
- regs->orig_ax = -1;
+ if (unlikely(copy_from_user(&sc, usc, sizeof(sc))))
+ return -EFAULT;

- get_user_ex(tmp, &sc->fpstate);
- buf = compat_ptr(tmp);
- } get_user_catch(err);
+ /* Get only the ia32 registers. */
+ regs->bx = sc.bx;
+ regs->cx = sc.cx;
+ regs->dx = sc.dx;
+ regs->si = sc.si;
+ regs->di = sc.di;
+ regs->bp = sc.bp;
+ regs->ax = sc.ax;
+ regs->sp = sc.sp;
+ regs->ip = sc.ip;
+
+ /* Get CS/SS and force CPL3 */
+ regs->cs = sc.cs | 0x03;
+ regs->ss = sc.ss | 0x03;
+
+ regs->flags = (regs->flags & ~FIX_EFLAGS) | (sc.flags & FIX_EFLAGS);
+ /* disable syscall checks */
+ regs->orig_ax = -1;

/*
* Reload fs and gs if they have changed in the signal
@@ -107,14 +93,8 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
* the handler, but does not clobber them at least in the
* normal case.
*/
- RELOAD_SEG(gs);
- RELOAD_SEG(fs);
- RELOAD_SEG(ds);
- RELOAD_SEG(es);
-
- err |= fpu__restore_sig(buf, 1);
-
- return err;
+ reload_segments(&sc);
+ return fpu__restore_sig(compat_ptr(sc.fpstate), 1);
}

COMPAT_SYSCALL_DEFINE0(sigreturn)
@@ -172,6 +152,8 @@ COMPAT_SYSCALL_DEFINE0(rt_sigreturn)
* Set up a signal frame.
*/

+#define get_user_seg(seg) ({ unsigned int v; savesegment(seg, v); v; })
+
static int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
void __user *fpstate,
struct pt_regs *regs, unsigned int mask)
--
2.11.0

2020-03-27 02:39:03

by Al Viro

[permalink] [raw]

On Sun, Mar 29, 2020 at 2:22 PM Andy Lutomirski <[email protected]> wrote:
>
> On Sun, Mar 29, 2020 at 11:16 AM Linus Torvalds
> <[email protected]> wrote:
> > But that magical asm-goto-with-outputs patch probably won't land
> > upstream for a couple of years.
>
> I'm not that familiar with gcc politics, but what's the delay?

No, I mean that _my_ magical patch for the kernel won't land upstream,
simply because even if both gcc and clang supported it today, nobody
would effectively have those compilers for a couple of years..

> ISTM
> having an actual upstream Linux asm-goto-with-outputs that works on
> clang might help light a fire under some butts and/or convince someone
> to pay a gcc developer to implement it on gcc.

Yes, but even for clang, it needs a version that isn't even released yet.

And right now my patch is unconditional. It started out that way
because the whole x86 uaccess.h files were such a mess, and I couldn't
be bothered to fix all the small cases to then have *two* cases (one
for asm goto with outputs, one without).

These days my patch is much simpler (thanks to Al's simplifications),
and I think making it handle both cases would likely not be too
painful.

And in that case I might just commit it, even if effectively nobody
has the clang version installed to make use of it.

Anyway, just in case people want to see it, I'm attaching my current
unconditional patch.

Note that it requires Al's uaccess cleanups, but I do want to point
out how it actually makes for simpler code:

arch/x86/include/asm/uaccess.h | 72 +++++++++++++++++++-----------------------
1 file changed, 32 insertions(+), 40 deletions(-)

and not only does it delete more lines than it adds, the lines it adds
are shorter and simpler than the ones it deletes.

But that "deletes more lines than it adds" is only because it doesn't
even try to build without that "asm goto with outputs" support..

Linus

Attachments:

patch.diff (4.38 kB)

2020-03-29 22:55:11

by Linus Torvalds

[permalink] [raw]

Subject: Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()

On Sun, Mar 29, 2020 at 3:06 PM Linus Torvalds
<[email protected]> wrote:
>
> Anyway, just in case people want to see it, I'm attaching my current
> unconditional patch.

Side note: I've actually been running variations of this patch for a
couple of months now, so it's even tested, but I debugged one of the
problems with the clang asm goto support it exposed only this last
week. I thought it was a problem with my patch for the longest time,
and couldn't figure it out.

So to work it needs tip-of-tree clang *and* an additional llvm bugfix
from Nick Desaulniers for that problem I reported that hasn't landed
yet.

So I included that patch just for people to look at. If you actually
want to test it, I can send you Nick's llvm fix too, and you need to
have the ability to build your own clang.

Linus

2020-03-30 15:55:55

by David Laight

[permalink] [raw]

Subject: RE: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()

From: Al Viro
> Sent: 29 March 2020 18:58
...
> [*] IMO compat_alloc_user_space() should die; this "grab some space on
> user stack, copy the 32bit data structure into 64bit equivalent there,
> complete with pointer chasing and creating 64bit equivalents of everything
> that's referenced from that struct, then call native ioctl, then do the
> reverse conversion" is just plain wrong. That native ioctl is going to
> bring the structures we'd constructed back into the kernel space and
> work with them there; we might as well separate the function that work
> with the copied struct (usually we do have those anyway) and call those
> instead the native ioctl. And skip the damn "copy the structures we'd
> built into temp allocation on user stack, then have it copied back"
> part. We have relatively few callers, thankfully.

I helped rip the same 'stackgap' code out of netbsd many years ago.
No only was it being used for system call compatibility, but
also for security checks and rewriting filenames.
Completely hopeless in a threaded program.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)