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=-8.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,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 BF893C2D0A3 for ; Wed, 4 Nov 2020 10:59:19 +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 4BA0D221F8 for ; Wed, 4 Nov 2020 10:59:19 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="yh1tnJSe" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4BA0D221F8 Authentication-Results: mail.kernel.org; dmarc=fail (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=t136DGdf//bqI+qfcdKQ8plHmRpwJBcNJsLfT1KVRWs=; b=yh1tnJSefxCF82SKAWCC1GPw4 TRXfGVca3AL45crsjUL8apYp57cnPF7jd/JH8G+lchV0W/4fTBnfnUqr0O+ca2QvzvhahDUJApNNn 5Dg1V8dUfvboEaAuBIBFp6P+4GsEi9bkwBmRJK1iqGvPACgThd4+Yv9omu/wG3cj4+V882ZyXRqpS LBjgKNvUWGNascIHNY6XLVSL1w2YbAAfwwx+fMk3nijXNg+haXubhKywcz6RG6nZ1NerbfwmIA+0a zkKkpVmIVf4DXKn3byteQfC13b405GNRgB9xZCwK9ry+Oh1UW8pnOx5cpj7oh2g7h+mKl2b+DlmRq XtA6JOFcA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kaGUM-00031q-8c; Wed, 04 Nov 2020 10:58:02 +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 1kaGUH-00030Z-VM for linux-arm-kernel@lists.infradead.org; Wed, 04 Nov 2020 10:57:58 +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 0DF5E13D5; Wed, 4 Nov 2020 02:57:57 -0800 (PST) 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 143653F718; Wed, 4 Nov 2020 02:57:54 -0800 (PST) Date: Wed, 4 Nov 2020 10:57:51 +0000 From: Dave Martin To: Peter Collingbourne Subject: Re: [PATCH v13 7/8] signal: define the field siginfo.si_faultflags Message-ID: <20201104105750.GP6882@arm.com> References: <743fef80a8617378027d5d2b0538cfc36ea106a1.1604376407.git.pcc@google.com> <20201103175352.GA22573@gaia> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: 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-20201104_055758_133171_70061F71 X-CRM114-Status: GOOD ( 46.49 ) 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 , Catalin Marinas , Helge Deller , Kevin Brodsky , Oleg Nesterov , Linux API , "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 Tue, Nov 03, 2020 at 10:39:52AM -0800, Peter Collingbourne wrote: > On Tue, Nov 3, 2020 at 9:54 AM Catalin Marinas wrote: > > > > Hi Peter, > > > > On Mon, Nov 02, 2020 at 08:09:43PM -0800, Peter Collingbourne wrote: > > > This field will contain flags that may be used by signal handlers to > > > determine whether other fields in the _sigfault portion of siginfo are > > > valid. An example use case is the following patch, which introduces > > > the si_addr_tag_bits{,_mask} fields. > > > > > > A new sigcontext flag, SA_FAULTFLAGS, is introduced in order to allow > > > a signal handler to require the kernel to set the field (but note > > > that the field will be set anyway if the kernel supports the flag, > > > regardless of its value). In combination with the previous patches, > > > this allows a userspace program to determine whether the kernel will > > > set the field. > > > > As per patch 5, a user is supposed to call sigaction() twice to figure > > out whether _faultflags is meaningful. That's the part I'm not > > particularly fond of. Are the unused parts of siginfo always zeroed when > > the kernel delivers a signal? If yes, we could simply check the new > > field for non-zero bits. > > The unused parts of siginfo are zeroed in current kernels, but > unfortunately not in older kernels. The zeroing behavior was > introduced in commit c999b933faa5e281e3af2e110eccaf91698b0a81 which > first appeared in kernel version 4.18, and at least in Android land we > do need to support kernel versions older than that. > > > > It is possible for an si_faultflags-unaware program to cause a signal > > > handler in an si_faultflags-aware program to be called with a provided > > > siginfo data structure by using one of the following syscalls: > > > > > > - ptrace(PTRACE_SETSIGINFO) > > > - pidfd_send_signal > > > - rt_sigqueueinfo > > > - rt_tgsigqueueinfo > > > > > > So we need to prevent the si_faultflags-unaware program from causing an > > > uninitialized read of si_faultflags in the si_faultflags-aware program when > > > it uses one of these syscalls. > > > > > > The last three cases can be handled by observing that each of these > > > syscalls fails if si_code >= 0. We also observe that kill(2) and > > > tgkill(2) may be used to send a signal where si_code == 0 (SI_USER), > > > so we define si_faultflags to only be valid if si_code > 0. > > > > > > There is no such check on si_code in ptrace(PTRACE_SETSIGINFO), so > > > we make ptrace(PTRACE_SETSIGINFO) clear the si_faultflags field if it > > > detects that the signal would use the _sigfault layout, and introduce > > > a new ptrace request type, PTRACE_SETSIGINFO2, that a si_faultflags-aware > > > program may use to opt out of this behavior. > > > > I find this pretty fragile but maybe I have to read it a few more times > > to fully understand the implications ;). > > > > Could we instead copy all the fields, potentially uninitialised, and > > instead filter them when delivering the signal based on the > > SA_FAULTFLAGS? That means that the kernel only writes si_faultflags if > > the user requested it. > > I don't see how that would help. The goal is to protect new signal > handlers from old signal "injectors" that will have potentially > uninitialized data where the si_faultflags field is. The new signal > handler will have SA_FAULTFLAGS set so that wouldn't prevent the > signal handler from seeing the uninitialized data. > > > > v12: > > > - Change type of si_xflags to u32 to avoid increasing alignment > > [...] > > > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h > > > index 7aacf9389010..f43778355b77 100644 > > > --- a/include/uapi/asm-generic/siginfo.h > > > +++ b/include/uapi/asm-generic/siginfo.h > > > @@ -91,7 +91,9 @@ union __sifields { > > > char _dummy_pkey[__ADDR_BND_PKEY_PAD]; > > > __u32 _pkey; > > > } _addr_pkey; > > > + void *_pad[6]; > > > }; > > > + __u32 _faultflags; > > > } _sigfault; > > > > Sorry, I haven't checked the previous discussion on alignment here but > > don't we already require 64-bit alignment because of other members in > > the _sigfault union? We already have void * throughout this and with the > > next patch we just have a gap (unless I miscalculated the offsets). > > This is about avoiding increasing alignment on 32-bit platforms. > Currently the alignment is 4 but a u64 field would bump it to 8. > > Unfortunately we can't do much about the gap on 64-bit platforms. This > was previously a uintptr_t but that would mean that the upper 32 bits > cannot be used safely on all platforms so we would effectively end up > with a gap anyway. I suppose we could make this an int or long if that feels more natural, but unless we have different flag definitions for 32-bit and 64-bit platforms, it would be hard to make use of the high 32 bits on 64-bit. Cheers ---Dave _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel