Received: by 2002:a25:8b91:0:0:0:0:0 with SMTP id j17csp11074683ybl; Fri, 27 Dec 2019 07:17:01 -0800 (PST) X-Google-Smtp-Source: APXvYqz9JDzKKkqBSSczbuog50sa10XBx+ZimsHnODcYXrQHP0yEKbTLkOYYyDu9nZcO9KCKVuDO X-Received: by 2002:a9d:6f07:: with SMTP id n7mr55453265otq.112.1577459821152; Fri, 27 Dec 2019 07:17:01 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1577459821; cv=none; d=google.com; s=arc-20160816; b=x9iBMTbExxJWmyoEQE3szc1Ws2S0RKQa3UeQpLPVlljwp0KWGDlAmojnfW3tdhw0Za lEmOh51wE2l3kD+Jvg/0BJg8ablnVySiBR3Qe9gSNYiTT3E1oI0gs26cWoAnAP/w9m7y yezhjrLvtsBkoCKgoNKS46T2rlJfkjSOiSUsRFlV+kEARlaKG1kBfJwHvfrtzVOguP4z D9jvpJiYaZQ03gPqXDBkbMeQnK7uOC5Ewd52zSJGw4jVYvt8D0cOZbd4f/6XsJdpRWio wlDpuqvQw9V0RSi9YWAtv0HVAll1/YqKGvcAkGG7Gur9L5pLidjd5bKKjE/0cmX/OLtP r3Tg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:content-disposition :mime-version:references:message-id:subject:cc:to:from:date; bh=tuX0AMeEMCiE292GT31AZqzaa5w9FXcfc5591/vkGlc=; b=T58+g6T0BbN/jiGBKLXg/B9IFUJ4LNVUy99elodGKpQOmnCfdnjE6HlfkrtOFcr76Y GE7K0+EZ2drG8yMt8Clo/IcUapADMpT0nyf7tvZ/mfrfRsjd7D/vzZQU4D777o6wwztU 7PW7jakflHv9IBwovLfgWwM+1bQ1fYGyEfisXOA1MU5/BDlTOcTe1CWo5zB1TSdE1hZf kilDGdfJaESXhXdrbi5RyzuPsPDV2tOQa2IF68Z4+Ym8lRpdw20/erPp3BV6pSIM2NVI pCmylEnMsUrPFH1gWrA1wUHLE5/0M5j8JPXUGwzt9M582U1lSoN0dwueHPA005aeCYhX swxQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id n8si16220377otr.102.2019.12.27.07.16.36; Fri, 27 Dec 2019 07:17:01 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726971AbfL0PPX (ORCPT + 99 others); Fri, 27 Dec 2019 10:15:23 -0500 Received: from mout-p-102.mailbox.org ([80.241.56.152]:32214 "EHLO mout-p-102.mailbox.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726053AbfL0PPW (ORCPT ); Fri, 27 Dec 2019 10:15:22 -0500 Received: from smtp2.mailbox.org (smtp2.mailbox.org [80.241.60.241]) (using TLSv1.2 with cipher ECDHE-RSA-CHACHA20-POLY1305 (256/256 bits)) (No client certificate requested) by mout-p-102.mailbox.org (Postfix) with ESMTPS id 47kr3q4F8GzKmfB; Fri, 27 Dec 2019 16:15:19 +0100 (CET) X-Virus-Scanned: amavisd-new at heinlein-support.de Received: from smtp2.mailbox.org ([80.241.60.241]) by spamfilter03.heinlein-hosting.de (spamfilter03.heinlein-hosting.de [80.241.56.117]) (amavisd-new, port 10030) with ESMTP id FoRWcIzpKAMD; Fri, 27 Dec 2019 16:15:13 +0100 (CET) Date: Sat, 28 Dec 2019 02:15:01 +1100 From: Aleksa Sarai To: Sargun Dhillon Cc: Christian Brauner , LKML , Linux API , Tycho Andersen , Jann Horn , Kees Cook Subject: Re: [PATCH] seccomp: Check flags on seccomp_notif is unset Message-ID: <20191227151501.osy2m6o6p6odzk74@yavin.dot.cyphar.com> References: <20191225214530.GA27780@ircssh-2.c.rugged-nimbus-611.internal> <20191226115245.usf7z5dkui7ndp4w@wittgenstein> <20191226143229.sbopynwut2hhsiwn@yavin.dot.cyphar.com> <57C06925-0CC6-4251-AD57-8FF1BC28F049@ubuntu.com> <20191227022446.37e64ag4uaqms2w4@yavin.dot.cyphar.com> <20191227023131.klnobtlfgeqcmvbb@yavin.dot.cyphar.com> <20191227114725.xsacnaoaaxdv6yg3@wittgenstein> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="qp3epnoas5ruktga" Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --qp3epnoas5ruktga Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On 2019-12-27, Sargun Dhillon wrote: > On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner > wrote: > > > > On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote: > > > On 2019-12-27, Aleksa Sarai wrote: > > > > > > Scratch that -- as Tycho just mentioned, there is un-named padding in > > > the struct so check_zeroed_user() is the wrong thing to do. But this > > > > Hm, I don't think so. > > I understood Tycho's point as _if_ there ever is padding then this would > > not be zeroed. > > Right now, there is no padding since the struct is correctly padded: > > > > struct seccomp_data { > > int nr; > > __u32 arch; > > __u64 instruction_pointer; > > __u64 args[6]; > > }; > > > > struct seccomp_notif { > > __u64 id; > > __u32 pid; > > __u32 flags; > > struct seccomp_data data; > > }; > > > > which would be - using pahole: > > > > struct seccomp_data { > > int nr; /* 0 4= */ > > __u32 arch; /* 4 4= */ > > __u64 instruction_pointer; /* 8 8= */ > > __u64 args[6]; /* 16 48= */ > > > > /* size: 64, cachelines: 1, members: 4 */ > > }; > > struct seccomp_notif { > > __u64 id; /* 0 8= */ > > __u32 pid; /* 8 4= */ > > __u32 flags; /* 12 4= */ > > struct seccomp_data data; /* 16 64= */ > > > > /* size: 80, cachelines: 2, members: 4 */ > > /* last cacheline: 16 bytes */ > > }; > > > > The only worry would be a 2byte int type but there's no architecture > > we support which does this right now afaict. > > > > > also will make extensions harder to deal with because (presumably) th= ey > > > will also have un-named padding, making copy_struct_from_user() the > > > > This all will be a non-issue if we just use __u64 for extensions. > > > > My point about using copy_struct_from_user() was that we should verify > > that _all_ fields are uninitialized and not just the flags argument > > since we might introduce a flags argument that requires another already > > existing member in seccomp_notif to be set to a value. We should do this > > change now so we don't have to risk breaking someone in the future. > > > > I'm trying to get at least Mozilla/Firefox off of their crazy > > SECCOMP_RET_TRAP way of implementing their broker onto the user notifier > > and they will likely need some extensions. That includes the pidfd stuff > > for seccomp that Sargun will likely be doing and the new pidfd_getfd() > > syscall. So it's not unlikely that we might need other already existing > > fields in that struct to be set to some value. > > > > I don't particulary care how we do it: > > - We can do a simple copy_from_user() and check each field individually. >=20 > Just doing a simple copy_from_user, and for now, calling memchr_inv > on the whole thing. We can drop the memset, and just leave a note to > indicate that if unpadded fields are introduced in the future, this struc= ture > must be manually zeroed out. Although, this might be laying a trap for > ourselves. >=20 > This leaves us in a good position for introducing a flag field in the fut= ure. > All we have to do is change the memchr_inv from checking on an > entire struct basis to checking on a per-field basis. There is no need to do memchr_inv() on copy_from_user() to check for zero-ness. That's the entire point of check_zeroed_user() -- to not need to do it that way. > > - Use copy_struct_from_user(). > > That is safe to do right now since there is no padding afaict and > > it'll automatically verify new fields as well. > > If I understand the worry correctly then the argument against > > copy_struct_from_user() here is that there might be padding introduced > > and userspace will not do an explicit memset() but rather rely on an > > empty inializer {} and will _accidently_ pass down a struct which has > > __all fields cleared__ but __uninitialized padding__ and we tell them > > EINVAL? That can only happen if we introduce padding in the struct > > which I'd argue we just don't do. That'll be in line with what we > > require from our ABIs already anyway. --=20 Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH --qp3epnoas5ruktga Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYIAB0WIQSxZm6dtfE8gxLLfYqdlLljIbnQEgUCXgYf8wAKCRCdlLljIbnQ ElhMAP4zLWE+r++uvi/UC9ZcIHcddNH68CbKzBsKVgf88RALlAEAgjfH80XjQsXo QCVHPhWjgbUafWOoPCP7x97K25qTVAc= =aqat -----END PGP SIGNATURE----- --qp3epnoas5ruktga--