Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
User-Agent: Cyrus-JMAP/3.5.0-alpha0-1369-gd055fb5e7c-fm-20211018.002-gd055fb5e
Mime-Version: 1.0
Message-Id: <530952e5-27d7-40b8-ac9a-debc36bb4fdf@www.fastmail.com>
In-Reply-To: <CAHk-=wioHUhqXU3_PR82VbfS8G=+zH+z8igeG-QAuCaWm5Cgqg@mail.gmail.com>
References: <87y26nmwkb.fsf@disp2133>
 <20211020174406.17889-10-ebiederm@xmission.com> <875ytkygfj.fsf_-_@disp2133>
 <4b203254-a333-77b1-0fa9-75c11fabac36@kernel.org>
 <CAHk-=wioHUhqXU3_PR82VbfS8G=+zH+z8igeG-QAuCaWm5Cgqg@mail.gmail.com>
Date:   Mon, 25 Oct 2021 17:21:36 -0700
From:   "Andy Lutomirski" <luto@kernel.org>
To:     "Linus Torvalds" <torvalds@linux-foundation.org>
Cc:     "Eric W. Biederman" <ebiederm@xmission.com>,
        "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
        linux-arch <linux-arch@vger.kernel.org>,
        "Oleg Nesterov" <oleg@redhat.com>,
        "Al Viro" <viro@zeniv.linux.org.uk>,
        "Kees Cook" <keescook@chromium.org>,
        "Thomas Gleixner" <tglx@linutronix.de>,
        "Ingo Molnar" <mingo@redhat.com>, "Borislav Petkov" <bp@alien8.de>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        "H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH v2 10/32] signal/vm86_32: Properly send SIGSEGV when the vm86 state
 cannot be saved.
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On Mon, Oct 25, 2021, at 4:45 PM, Linus Torvalds wrote:
> On Mon, Oct 25, 2021 at 3:25 PM Andy Lutomirski <luto@kernel.org> wrot=
e:
>>
>> 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 re=
ads
>> any state that is written by the cleanup code.
>
> Sure it does:
>
>         memcpy(&regs->pt, &vm86->regs32, sizeof(struct pt_regs));
>
> is very much part of the cleanup code, and overwrites that regs->pt th=
ing.
>
> Which is exactly what we're writing back to user space in that
> unsafe_put_user() thing.

D=E2=80=99oh, 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=E2=80=99s =E2=80=9Crecoverable=E2=80=9D and there=E2=80=99s =E2=80=
=9Crecoverable=E2=80=9D.  Sure, the vm86 state is gone, but the process =
is getting a signal that doesn=E2=80=99t indicate that one can freely re=
turn 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