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;
IronPort-SDR: b0bpm16eB0ZsV71xFQXAs/A77KcUfaMDiEiQEDBBQ7QYdZrciFdnAXlNePXRtNXtB16e8ZuSRr
 BjF5EQGyLWQg==
IronPort-SDR: hP6DLBZjceLWCEY2NyHaKjVYmUnp2KwsgKSM5EpC34kDiWmii6s8n8qGvSHyNYlYzgEj65Hwui
 fOXJjTMKholw==
From:   "Bae, Chang Seok" <chang.seok.bae@intel.com>
To:     David Laight <David.Laight@ACULAB.COM>
CC:     "bp@suse.de" <bp@suse.de>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "mingo@kernel.org" <mingo@kernel.org>,
        "luto@kernel.org" <luto@kernel.org>,
        "x86@kernel.org" <x86@kernel.org>,
        "Brown, Len" <len.brown@intel.com>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        "hjl.tools@gmail.com" <hjl.tools@gmail.com>,
        "Dave.Martin@arm.com" <Dave.Martin@arm.com>,
        "jannh@google.com" <jannh@google.com>,
        "mpe@ellerman.id.au" <mpe@ellerman.id.au>,
        "carlos@redhat.com" <carlos@redhat.com>,
        "Luck, Tony" <tony.luck@intel.com>,
        "Shankar, Ravi V" <ravi.v.shankar@intel.com>,
        "libc-alpha@sourceware.org" <libc-alpha@sourceware.org>,
        "linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
        "linux-api@vger.kernel.org" <linux-api@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v8 5/6] x86/signal: Detect and prevent an alternate signal
 stack overflow
Thread-Topic: [PATCH v8 5/6] x86/signal: Detect and prevent an alternate
 signal stack overflow
Thread-Index: AQHXNzOQXxUM6Mub30elxTKCBjQG3arAOYAAgACB+IA=
Date:   Thu, 22 Apr 2021 16:31:26 +0000
Message-ID: <9C452E66-0C41-462B-9971-56825444AD65@intel.com>
References: <20210422044856.27250-1-chang.seok.bae@intel.com>
 <20210422044856.27250-6-chang.seok.bae@intel.com>
 <854d6aefdf604b559e37e82669b5e67f@AcuMS.aculab.com>
In-Reply-To: <854d6aefdf604b559e37e82669b5e67f@AcuMS.aculab.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <D98A9DC7F23BCD4BB8AF09CC91F17005@namprd11.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 73d1824b-59a5-4d8b-8915-08d905ac1320
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Apr 2021 16:31:26.0998
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Becq6QPv7PxhFES/3IqJm6F2CwN33EvicbxqjR+KgSdHzQ0vthJui5+AzA4qmsl52LZRIunvYCGtEyM3qMPayWhVYi5xN9ox6bl7Nf1Kurk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5159
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Apr 22, 2021, at 01:46, David Laight <David.Laight@ACULAB.COM> wrote:
> From: Chang S. Bae
>> Sent: 22 April 2021 05:49
>>=20
>> The kernel pushes context on to the userspace stack to prepare for the
>> user's signal handler. When the user has supplied an alternate signal
>> stack, via sigaltstack(2), it is easy for the kernel to verify that the
>> stack size is sufficient for the current hardware context.
>>=20
>> Check if writing the hardware context to the alternate stack will exceed
>> it's size. If yes, then instead of corrupting user-data and proceeding w=
ith
>> the original signal handler, an immediate SIGSEGV signal is delivered.
>=20
> What happens if SIGSEGV is caught?

Boris pointed out the relevant notes before [1]. I think "unpredictable
results" is a somewhat vague statement but process termination is unavoidab=
le
in this situation.

In the thread [1], a new signal number was discussed for the signal deliver=
y
failure, but my takeaway is this SIGSEGV is still recognizable.

FWIW, Len summarized other possible approaches as well [2].

>> Refactor the stack pointer check code from on_sig_stack() and use the ne=
w
>> helper.
>>=20
>> While the kernel allows new source code to discover and use a sufficient
>> alternate signal stack size, this check is still necessary to protect
>> binaries with insufficient alternate signal stack size from data
>> corruption.
> ...
>> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
>> index 3f6a0fcaa10c..ae60f838ebb9 100644
>> --- a/include/linux/sched/signal.h
>> +++ b/include/linux/sched/signal.h
>> @@ -537,6 +537,17 @@ static inline int kill_cad_pid(int sig, int priv)
>> #define SEND_SIG_NOINFO ((struct kernel_siginfo *) 0)
>> #define SEND_SIG_PRIV	((struct kernel_siginfo *) 1)
>>=20
>> +static inline int __on_sig_stack(unsigned long sp)
>> +{
>> +#ifdef CONFIG_STACK_GROWSUP
>> +	return sp >=3D current->sas_ss_sp &&
>> +		sp - current->sas_ss_sp < current->sas_ss_size;
>> +#else
>> +	return sp > current->sas_ss_sp &&
>> +		sp - current->sas_ss_sp <=3D current->sas_ss_size;
>> +#endif
>> +}
>> +
>=20
> Those don't look different enough.

The difference is on the SS_AUTODISARM flag check.  This refactoring was
suggested as on_sig_stack() brought confusion [3].

Thanks,
Chang

[1] https://lore.kernel.org/lkml/20210414120608.GE10709@zn.tnic/
[2] https://lore.kernel.org/lkml/CAJvTdKnpWL8y4N_BrCiK7fU0UXERwuuM8o84LUpp7=
Watxd8STw@mail.gmail.com/
[3] https://lore.kernel.org/lkml/20210325212733.GC32296@zn.tnic/