On Mon, Oct 25, 2021 at 3:25 PM Andy Lutomirski <[email protected]> wrote:
>
> I think the result would be nicer if, instead of adding an extra goto,
> you just literally moved all the cleanup under the unsafe_put_user()s
> above them. Unless I missed something, none of the put_user stuff reads
> any state that is written by the cleanup code.
Sure it does:
memcpy(®s->pt, &vm86->regs32, sizeof(struct pt_regs));
is very much part of the cleanup code, and overwrites that regs->pt thing.
Which is exactly what we're writing back to user space in that
unsafe_put_user() thing.
That said, thinking more about this, and looking at it again, I take
back my statement that we could just make it a catchable SIGSEGV
instead.
If we can't write the vm86 state to user space, we will have
fundamentally lost it, and while it's not fatal to the kernel, and
while we've recovered the original 32-bit state, it's not something
that user space can sanely recover from because the register state at
the end of the vm86 work has now been irrecoverably thrown away.
So I think Eric's patch is fine.
Except, as mentioned as part of the other patch, the "force_sigsegv()"
conversion to use "force_fatal_sig()" was broken, because that
function wasn't actually fatal at all.
Linus
On Mon, Oct 25, 2021, at 4:45 PM, Linus Torvalds wrote:
> On Mon, Oct 25, 2021 at 3:25 PM Andy Lutomirski <[email protected]> wrote:
>>
>> I think the result would be nicer if, instead of adding an extra goto,
>> you just literally moved all the cleanup under the unsafe_put_user()s
>> above them. Unless I missed something, none of the put_user stuff reads
>> any state that is written by the cleanup code.
>
> Sure it does:
>
> memcpy(®s->pt, &vm86->regs32, sizeof(struct pt_regs));
>
> is very much part of the cleanup code, and overwrites that regs->pt thing.
>
> Which is exactly what we're writing back to user space in that
> unsafe_put_user() thing.
D’oh, right.
>
> That said, thinking more about this, and looking at it again, I take
> back my statement that we could just make it a catchable SIGSEGV
> instead.
>
> If we can't write the vm86 state to user space, we will have
> fundamentally lost it, and while it's not fatal to the kernel, and
> while we've recovered the original 32-bit state, it's not something
> that user space can sanely recover from because the register state at
> the end of the vm86 work has now been irrecoverably thrown away.
There’s “recoverable” and there’s “recoverable”. Sure, the vm86 state is gone, but the process is getting a signal that doesn’t indicate that one can freely return and carry on as if nothing happened. But one can catch the signal and go on to do something else.
>
> So I think Eric's patch is fine.
Me too.
>
> Except, as mentioned as part of the other patch, the "force_sigsegv()"
> conversion to use "force_fatal_sig()" was broken, because that
> function wasn't actually fatal at all.
>
> Linus