netdev - Fwd: FW: [PATCH 17/31] nds32: Signal handling 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>] [day] [month] [year] [list]

Message-ID: <CAJsyPhzO=o4Eqc9J8SP0rEvb65tXD11wpPtxhbE4KCRvd-==mA@mail.gmail.com>
Date:   Mon, 13 Nov 2017 10:34:06 +0800
From:   Vincent Chen <deanbo422@...il.com>
To:     viro@....linux.org.uk
Cc:     greentime@...estech.com, linux-kernel@...r.kernel.org,
        arnd@...db.de, linux-arch@...r.kernel.org, tglx@...utronix.de,
        jason@...edaemon.net, marc.zyngier@....com, robh+dt@...nel.org,
        netdev@...r.kernel.org, vincentc@...estech.com
Subject: Fwd: FW: [PATCH 17/31] nds32: Signal handling support

>> +static int restore_sigframe(struct pt_regs *regs,
>> +                         struct rt_sigframe __user * sf) {
>
>[snip]
>
>> +     err |= !valid_user_regs(regs);
>
>IDGI...  Where do you modify ->ipsw at all and how can valid_user_regs() come to be false here?
>
Thanks.
This code is trivial and I will remove it in the next version patch



>> +asmlinkage int sys_rt_sigreturn(struct pt_regs *regs) {
>> +     struct rt_sigframe __user *frame;
>> +
>> +     /* Always make any pending restarted system calls return -EINTR */
>> +     current->restart_block.fn = do_no_restart_syscall;
>> +
>> +     /*
>> +      * Since we stacked the signal on a 64-bit boundary,
>> +      * then 'sp' should be two-word aligned here.  If it's
>> +      * not, then the user is trying to mess with us.
>> +      */
>> +     if (regs->sp & 7)
>> +             goto badframe;
>> +
>> +     frame = (struct rt_sigframe __user *)regs->sp;
>> +
>> +     if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>> +             goto badframe;
>> +
>> +     if (restore_sigframe(regs, frame))
>> +             goto badframe;
>> +
>> +     if (restore_altstack(&frame->uc.uc_stack))
>> +             goto badframe;
>> +
>> +     return regs->uregs[0];
>> +
>> +badframe:
>> +     force_sig(SIGSEGV, current);
>> +     return 0;
>> +}\
>
>AFAICS, you are copying arm; take a good look at sys_rt_sigreturn_wrapper there - specifically, the 'mov why, #0' part.  Consider what happens if you get an interrupt at the moment when $r0 contains -ERESTARTSYS (for
example) and signal arrives while we are processing the interrupt.  It
will be handled on the way out, without any syscall restart crap
('why'
is 0 on that codepath).  So far, so good, but think what'll happen
when you are done with the signal handler.  sigreturn() is called, the
values we had stashed in sigcontext go back into registers (OK,
pt_regs on kernel stack that will be used to reload the registers on
return to userland)... and the signals that had been blocked for the
duration of handlers (see sa_mask in sigaction(2)) get unblocked.  And
it turns out that you have one of those pending - it had arrived while
we'd been in the handler.

>Now we are fucked.  You have TIF_SIGPENDING set, so do_notify_resume() is called.  And everything looks exactly as if you had a syscall restart situation - regs->uregs[0] being one of -ERESTART... and 'syscall' flag being true.  So we go into the second signal handler (as we ought to) with saved ->ipc set 4 bytes back from where we were going to return.
>It would've been the right thing to do if it *was* a syscall restart, but we were returning to the location where the original interrupt had caught us.
>
>Result: with the right timing, an interrupt arriving when userland process has $r0 equal -512 may lead to instruction pointer jumping 4 bytes back.
>Pity the poor sod trying to debug that kind of breakage...
>
>Restart should *NOT* be triggered upon sigreturn(2).

Thanks for your detailed description.
I got it and I will fix this bug in the next version patch


>>
>> +static int
>> +setup_return(struct pt_regs *regs, struct ksignal *ksig, void __user
>> +* frame) {
>> +     unsigned long handler = (unsigned long)ksig->ka.sa.sa_handler;
>> +     unsigned long retcode;
>> +
>> +     /*
>> +      * Maybe we need to deliver a 32-bit signal to a 26-bit task.
>>
>Deliver to what, again?  That comment made sense (if rather sad one) on arm, but what is it doing here?
>
Thanks.
I will remove it in the next version patch


>> +static int do_signal(struct pt_regs *regs, int syscall) {
>> +     unsigned int retval = 0, continue_addr = 0, restart_addr = 0;
>> +     struct ksignal ksig;
>> +     int restart = 0;
>> +
>> +     /*
>> +      * We want the common case to go fast, which
>> +      * is why we may in certain cases get here from
>> +      * kernel mode. Just return without doing anything
>> +      * if so.
>> +      */
>> +     if (!user_mode(regs))
>> +             return 0;
>
>Which cases would those be?

Thanks.
This code is trivial too. I will remove it in the next version patch