On Thu, Oct 10, 2019 at 5:11 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Oct 10, 2019 at 03:12:49PM -0700, Linus Torvalds wrote:
>
> > But I've not gotten around to rewriting those disgusting sequences to
> > the unsafe_get/put_user() model. I did look at it, and it requires
> > some changes exactly *because* the _ex() functions are broken and
> > continue, but also because the current code ends up also doing other
> > things inside the try/catch region that you're not supposed to do in a
> > user_access_begin/end() region .
>
> Hmm...  Which one was that?  AFAICS, we have
>         do_sys_vm86: only get_user_ex()
>         restore_sigcontext(): get_user_ex(), set_user_gs()
>         ia32_restore_sigcontext(): get_user_ex()

Try this patch.

It works fine (well, it worked fine the lastr time I tried this, I
might have screwed something up just now: I re-created the patch since
I hadn't saved it).

It's nice and clean, and does

 1 file changed, 9 insertions(+), 91 deletions(-)

by just deleting all the nasty *_ex() macros entirely, replacing them
with unsafe_get/put_user() calls.

And now those try/catch regions actually work like try/catch regions,
and a fault branches to the catch.

BUT.

It does change semantics, and you get warnings like

  arch/x86/ia32/ia32_signal.c: In function ‘ia32_restore_sigcontext’:
  arch/x86/ia32/ia32_signal.c:114:9: warning: ‘buf’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
    114 |  err |= fpu__restore_sig(buf, 1);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~
  arch/x86/ia32/ia32_signal.c:64:27: warning: ‘ds’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
     64 |  unsigned int pre = (seg) | 3;  \
        |                           ^
  arch/x86/ia32/ia32_signal.c:74:18: note: ‘ds’ was declared here
...
  arch/x86/kernel/signal.c: In function ‘restore_sigcontext’:
  arch/x86/kernel/signal.c:152:9: warning: ‘buf’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
    152 |  err |= fpu__restore_sig(buf, IS_ENABLED(CONFIG_X86_32));
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

because it's true: those things reall may not be initialized, because
the catch thing could have jumped out.

So the code actually needs to properly return the error early, or
initialize the segments that didn't get loaded to 0, or something.

And when I posted that, Luto said "just get rid of the get_user_ex()
entirely, instead of changing semantics of the existing ones to be
sane.

Which is probably right. There aren't that many.

I *thought* there were also cases of us doing some questionably things
inside the get_user_try sections, but those seem to have gotten fixed
already independently, so it's really just the "make try/catch really
try/catch" change that needs some editing of our current broken stuff
that depends on it not actually *catching* exceptions, but on just
continuing on to the next one.

                Linus