From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 95270C2BC11 for ; Tue, 8 Sep 2020 20:06:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 67A5E207DE for ; Tue, 8 Sep 2020 20:06:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729037AbgIHUGH (ORCPT ); Tue, 8 Sep 2020 16:06:07 -0400 Received: from foss.arm.com ([217.140.110.172]:55394 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729576AbgIHPVm (ORCPT ); Tue, 8 Sep 2020 11:21:42 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 72B54169C; Tue, 8 Sep 2020 08:12:39 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 92E213F73C; Tue, 8 Sep 2020 08:12:37 -0700 (PDT) Date: Tue, 8 Sep 2020 16:12:35 +0100 From: Dave Martin To: Peter Collingbourne Cc: Catalin Marinas , Evgenii Stepanov , Kostya Serebryany , Vincenzo Frascino , Will Deacon , Oleg Nesterov , "Eric W. Biederman" , "James E.J. Bottomley" , linux-parisc@vger.kernel.org, Andrey Konovalov , Kevin Brodsky , David Spickett , Linux ARM , Richard Henderson Subject: Re: [PATCH v10 3/7] signal: clear non-uapi flag bits when passing/returning sa_flags Message-ID: <20200908151235.GT6642@arm.com> References: <868b8a89e4050d3f2d079bf28a18786e92b9c680.1598072840.git.pcc@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <868b8a89e4050d3f2d079bf28a18786e92b9c680.1598072840.git.pcc@google.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-parisc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-parisc@vger.kernel.org On Fri, Aug 21, 2020 at 10:10:13PM -0700, Peter Collingbourne wrote: > Previously we were not clearing non-uapi flag bits in > sigaction.sa_flags when storing the userspace-provided sa_flags or > when returning them via oldact. Start doing so. > > This allows userspace to detect missing support for flag bits and > allows the kernel to use non-uapi bits internally, as we are already > doing in arch/x86 for two flag bits. Now that this change is in > place, we no longer need the code in arch/x86 that was hiding these > bits from userspace, so remove it. > > This is technically a userspace-visible behavior change for sigaction, as > the unknown bits returned via oldact.sa_flags are no longer set. However, > we are free to define the behavior for unknown bits exactly because > their behavior is currently undefined, so for now we can define the > meaning of each of them to be "clear the bit in oldact.sa_flags unless > the bit becomes known in the future". Furthermore, this behavior is > consistent with OpenBSD [1], illumos [2] and XNU [3] (FreeBSD [4] and > NetBSD [5] fail the syscall if unknown bits are set). So there is some > precedent for this behavior in other kernels, and in particular in XNU, > which is probably the most popular kernel among those that I looked at, > which means that this change is less likely to be a compatibility issue. > > Link: [1] https://github.com/openbsd/src/blob/f634a6a4b5bf832e9c1de77f7894ae2625e74484/sys/kern/kern_sig.c#L278 > Link: [2] https://github.com/illumos/illumos-gate/blob/76f19f5fdc974fe5be5c82a556e43a4df93f1de1/usr/src/uts/common/syscall/sigaction.c#L86 > Link: [3] https://github.com/apple/darwin-xnu/blob/a449c6a3b8014d9406c2ddbdc81795da24aa7443/bsd/kern/kern_sig.c#L480 > Link: [4] https://github.com/freebsd/freebsd/blob/eded70c37057857c6e23fae51f86b8f8f43cd2d0/sys/kern/kern_sig.c#L699 > Link: [5] https://github.com/NetBSD/src/blob/3365779becdcedfca206091a645a0e8e22b2946e/sys/kern/sys_sig.c#L473 > Signed-off-by: Peter Collingbourne > --- > View this change in Gerrit: https://linux-review.googlesource.com/q/I35aab6f5be932505d90f3b3450c083b4db1eca86 > > v10: > - rename SA_UAPI_FLAGS -> UAPI_SA_FLAGS > - refactor how we define it to avoid mentioning flags more > than once > > arch/arm/include/asm/signal.h | 2 ++ > arch/parisc/include/asm/signal.h | 2 ++ > arch/x86/kernel/signal_compat.c | 7 ------- > include/linux/signal_types.h | 12 ++++++++++++ > kernel/signal.c | 10 ++++++++++ > 5 files changed, 26 insertions(+), 7 deletions(-) > > diff --git a/arch/arm/include/asm/signal.h b/arch/arm/include/asm/signal.h > index 65530a042009..430be7774402 100644 > --- a/arch/arm/include/asm/signal.h > +++ b/arch/arm/include/asm/signal.h > @@ -17,6 +17,8 @@ typedef struct { > unsigned long sig[_NSIG_WORDS]; > } sigset_t; > > +#define __ARCH_UAPI_SA_FLAGS (SA_THIRTYTWO | SA_RESTORER) > + > #define __ARCH_HAS_SA_RESTORER > > #include > diff --git a/arch/parisc/include/asm/signal.h b/arch/parisc/include/asm/signal.h > index 715c96ba2ec8..30dd1e43ef88 100644 > --- a/arch/parisc/include/asm/signal.h > +++ b/arch/parisc/include/asm/signal.h > @@ -21,6 +21,8 @@ typedef struct { > unsigned long sig[_NSIG_WORDS]; > } sigset_t; > > +#define __ARCH_UAPI_SA_FLAGS _SA_SIGGFAULT > + > #include > > #endif /* !__ASSEMBLY */ > diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c > index 9ccbf0576cd0..c599013ae8cb 100644 > --- a/arch/x86/kernel/signal_compat.c > +++ b/arch/x86/kernel/signal_compat.c > @@ -165,16 +165,9 @@ void sigaction_compat_abi(struct k_sigaction *act, struct k_sigaction *oact) > { > signal_compat_build_tests(); > > - /* Don't leak in-kernel non-uapi flags to user-space */ > - if (oact) > - oact->sa.sa_flags &= ~(SA_IA32_ABI | SA_X32_ABI); > - > if (!act) > return; > > - /* Don't let flags to be set from userspace */ > - act->sa.sa_flags &= ~(SA_IA32_ABI | SA_X32_ABI); > - > if (in_ia32_syscall()) > act->sa.sa_flags |= SA_IA32_ABI; > if (in_x32_syscall()) > diff --git a/include/linux/signal_types.h b/include/linux/signal_types.h > index f8a90ae9c6ec..a7887ad84d36 100644 > --- a/include/linux/signal_types.h > +++ b/include/linux/signal_types.h > @@ -68,4 +68,16 @@ struct ksignal { > int sig; > }; > > +#ifndef __ARCH_UAPI_SA_FLAGS > +#ifdef SA_RESTORER > +#define __ARCH_UAPI_SA_FLAGS SA_RESTORER > +#else > +#define __ARCH_UAPI_SA_FLAGS 0 > +#endif > +#endif > + > +#define UAPI_SA_FLAGS \ > + (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \ > + SA_NODEFER | SA_RESETHAND | __ARCH_UAPI_SA_FLAGS) > + Part of me wants this to be closer to the common flag definitions. But we don't really want to define this in the UAPI headers. Unless you can think of another good place to put it, this is probably OK as-is. > #endif /* _LINUX_SIGNAL_TYPES_H */ > diff --git a/kernel/signal.c b/kernel/signal.c > index 42b67d2cea37..f802c82c7bcc 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -3984,6 +3984,16 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact) > if (oact) > *oact = *k; > > + /* > + * Clear unknown flag bits in order to allow userspace to detect missing > + * support for flag bits and to allow the kernel to use non-uapi bits > + * internally. > + */ > + if (act) > + act->sa.sa_flags &= UAPI_SA_FLAGS; > + if (oact) > + oact->sa.sa_flags &= UAPI_SA_FLAGS; > + Seems reasonable. [...] Cheers ---Dave From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E90ABC433E2 for ; Tue, 8 Sep 2020 15:13:56 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 8F09123C90 for ; Tue, 8 Sep 2020 15:13:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="GHUI4jAL" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8F09123C90 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=g7I0r3clZhnw528ReG43FybFbkRKvgeQBjp5X0aER84=; b=GHUI4jALO2vB2Ve1mGfGumGhI XkSJwFWKuwoH/k+f6A0IePRoghMZ3jon3hY5NnaKEKzYCxC19jmmtpK4MzbLh1BqoWIuceZuzuYiv 6u5/hv9iFzWBw6BiCoYCulkgfI4a9XiwWOxHZBfKMrKncRmG8cgLvHiQavGXJbmLVLLirV07DlZvH RWxWlDaH+NAoWnYssTvLegmdBDl/LEax/kHm1RwYntPKPN9hvcwXtExLU0ps9CiCzIhF0454hFFB1 buUFc5IzJda9TWVMKQZbAtdHAsBm50cQ98RiFuVLFHYge+U1rKvBnBBFSyvGcfAwJrbJ7qDgOBgTb FtZEkIFig==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kFfIZ-0007VX-EX; Tue, 08 Sep 2020 15:12:43 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kFfIW-0007UI-0K for linux-arm-kernel@lists.infradead.org; Tue, 08 Sep 2020 15:12:41 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 72B54169C; Tue, 8 Sep 2020 08:12:39 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 92E213F73C; Tue, 8 Sep 2020 08:12:37 -0700 (PDT) Date: Tue, 8 Sep 2020 16:12:35 +0100 From: Dave Martin To: Peter Collingbourne Subject: Re: [PATCH v10 3/7] signal: clear non-uapi flag bits when passing/returning sa_flags Message-ID: <20200908151235.GT6642@arm.com> References: <868b8a89e4050d3f2d079bf28a18786e92b9c680.1598072840.git.pcc@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <868b8a89e4050d3f2d079bf28a18786e92b9c680.1598072840.git.pcc@google.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200908_111240_216249_3062B2D4 X-CRM114-Status: GOOD ( 33.25 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Linux ARM , linux-parisc@vger.kernel.org, Catalin Marinas , Kevin Brodsky , Oleg Nesterov , "James E.J. Bottomley" , Kostya Serebryany , "Eric W. Biederman" , Andrey Konovalov , David Spickett , Vincenzo Frascino , Will Deacon , Evgenii Stepanov , Richard Henderson Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Aug 21, 2020 at 10:10:13PM -0700, Peter Collingbourne wrote: > Previously we were not clearing non-uapi flag bits in > sigaction.sa_flags when storing the userspace-provided sa_flags or > when returning them via oldact. Start doing so. > > This allows userspace to detect missing support for flag bits and > allows the kernel to use non-uapi bits internally, as we are already > doing in arch/x86 for two flag bits. Now that this change is in > place, we no longer need the code in arch/x86 that was hiding these > bits from userspace, so remove it. > > This is technically a userspace-visible behavior change for sigaction, as > the unknown bits returned via oldact.sa_flags are no longer set. However, > we are free to define the behavior for unknown bits exactly because > their behavior is currently undefined, so for now we can define the > meaning of each of them to be "clear the bit in oldact.sa_flags unless > the bit becomes known in the future". Furthermore, this behavior is > consistent with OpenBSD [1], illumos [2] and XNU [3] (FreeBSD [4] and > NetBSD [5] fail the syscall if unknown bits are set). So there is some > precedent for this behavior in other kernels, and in particular in XNU, > which is probably the most popular kernel among those that I looked at, > which means that this change is less likely to be a compatibility issue. > > Link: [1] https://github.com/openbsd/src/blob/f634a6a4b5bf832e9c1de77f7894ae2625e74484/sys/kern/kern_sig.c#L278 > Link: [2] https://github.com/illumos/illumos-gate/blob/76f19f5fdc974fe5be5c82a556e43a4df93f1de1/usr/src/uts/common/syscall/sigaction.c#L86 > Link: [3] https://github.com/apple/darwin-xnu/blob/a449c6a3b8014d9406c2ddbdc81795da24aa7443/bsd/kern/kern_sig.c#L480 > Link: [4] https://github.com/freebsd/freebsd/blob/eded70c37057857c6e23fae51f86b8f8f43cd2d0/sys/kern/kern_sig.c#L699 > Link: [5] https://github.com/NetBSD/src/blob/3365779becdcedfca206091a645a0e8e22b2946e/sys/kern/sys_sig.c#L473 > Signed-off-by: Peter Collingbourne > --- > View this change in Gerrit: https://linux-review.googlesource.com/q/I35aab6f5be932505d90f3b3450c083b4db1eca86 > > v10: > - rename SA_UAPI_FLAGS -> UAPI_SA_FLAGS > - refactor how we define it to avoid mentioning flags more > than once > > arch/arm/include/asm/signal.h | 2 ++ > arch/parisc/include/asm/signal.h | 2 ++ > arch/x86/kernel/signal_compat.c | 7 ------- > include/linux/signal_types.h | 12 ++++++++++++ > kernel/signal.c | 10 ++++++++++ > 5 files changed, 26 insertions(+), 7 deletions(-) > > diff --git a/arch/arm/include/asm/signal.h b/arch/arm/include/asm/signal.h > index 65530a042009..430be7774402 100644 > --- a/arch/arm/include/asm/signal.h > +++ b/arch/arm/include/asm/signal.h > @@ -17,6 +17,8 @@ typedef struct { > unsigned long sig[_NSIG_WORDS]; > } sigset_t; > > +#define __ARCH_UAPI_SA_FLAGS (SA_THIRTYTWO | SA_RESTORER) > + > #define __ARCH_HAS_SA_RESTORER > > #include > diff --git a/arch/parisc/include/asm/signal.h b/arch/parisc/include/asm/signal.h > index 715c96ba2ec8..30dd1e43ef88 100644 > --- a/arch/parisc/include/asm/signal.h > +++ b/arch/parisc/include/asm/signal.h > @@ -21,6 +21,8 @@ typedef struct { > unsigned long sig[_NSIG_WORDS]; > } sigset_t; > > +#define __ARCH_UAPI_SA_FLAGS _SA_SIGGFAULT > + > #include > > #endif /* !__ASSEMBLY */ > diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c > index 9ccbf0576cd0..c599013ae8cb 100644 > --- a/arch/x86/kernel/signal_compat.c > +++ b/arch/x86/kernel/signal_compat.c > @@ -165,16 +165,9 @@ void sigaction_compat_abi(struct k_sigaction *act, struct k_sigaction *oact) > { > signal_compat_build_tests(); > > - /* Don't leak in-kernel non-uapi flags to user-space */ > - if (oact) > - oact->sa.sa_flags &= ~(SA_IA32_ABI | SA_X32_ABI); > - > if (!act) > return; > > - /* Don't let flags to be set from userspace */ > - act->sa.sa_flags &= ~(SA_IA32_ABI | SA_X32_ABI); > - > if (in_ia32_syscall()) > act->sa.sa_flags |= SA_IA32_ABI; > if (in_x32_syscall()) > diff --git a/include/linux/signal_types.h b/include/linux/signal_types.h > index f8a90ae9c6ec..a7887ad84d36 100644 > --- a/include/linux/signal_types.h > +++ b/include/linux/signal_types.h > @@ -68,4 +68,16 @@ struct ksignal { > int sig; > }; > > +#ifndef __ARCH_UAPI_SA_FLAGS > +#ifdef SA_RESTORER > +#define __ARCH_UAPI_SA_FLAGS SA_RESTORER > +#else > +#define __ARCH_UAPI_SA_FLAGS 0 > +#endif > +#endif > + > +#define UAPI_SA_FLAGS \ > + (SA_NOCLDSTOP | SA_NOCLDWAIT | SA_SIGINFO | SA_ONSTACK | SA_RESTART | \ > + SA_NODEFER | SA_RESETHAND | __ARCH_UAPI_SA_FLAGS) > + Part of me wants this to be closer to the common flag definitions. But we don't really want to define this in the UAPI headers. Unless you can think of another good place to put it, this is probably OK as-is. > #endif /* _LINUX_SIGNAL_TYPES_H */ > diff --git a/kernel/signal.c b/kernel/signal.c > index 42b67d2cea37..f802c82c7bcc 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -3984,6 +3984,16 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact) > if (oact) > *oact = *k; > > + /* > + * Clear unknown flag bits in order to allow userspace to detect missing > + * support for flag bits and to allow the kernel to use non-uapi bits > + * internally. > + */ > + if (act) > + act->sa.sa_flags &= UAPI_SA_FLAGS; > + if (oact) > + oact->sa.sa_flags &= UAPI_SA_FLAGS; > + Seems reasonable. [...] Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel