From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f171.google.com (mail-pl1-f171.google.com [209.85.214.171]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 80D062F2E for ; Thu, 5 May 2022 19:27:34 +0000 (UTC) Received: by mail-pl1-f171.google.com with SMTP id j14so5322410plx.3 for ; Thu, 05 May 2022 12:27:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=/twjVtbcWPue2Jf3IhiwlWWY7Rof9WWSi6Cv9vIoV/Q=; b=Zw4fRetuqYTkqeq+yVUkdhxXm/8i81f3Tz5yiq/8IIDSZ024zV3L+pNdERes4bJaav 31/Q4CkRhBZLAixBgNaxyPQDOPUfRkG6V2Lznu+vqnnAE8TdezgewvauqqI6bgLcXe8G 8pjfVWQFjtIzIDipg+thK9X2Q1whG0Xu+Ovt4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=/twjVtbcWPue2Jf3IhiwlWWY7Rof9WWSi6Cv9vIoV/Q=; b=G7o2416wXHAFfZJituifT7NLSnlz0IlPnsZnOMZjPWNFEi8HOH5v5GhlQcFonQHTVF ab3fOKS9MvpNFY7rxa2/RwHsjpYy3NQ4COrDoBrTbvzH8sgk3XcWoUmue967F5nyEZuE 999LCx7jDj3r5Xr22kLrC8yNpMMa0SZoplGAx8klTqoKOz9F+54Tqb8mVhB+SDbRJS5G Sgkic0eIso1b1+E5sX3tr5wa0lhB+tdcS3ZA5A/eAjh8Ifsv7/SjhVvD+4+JDwHJ7sQd A0Kwjcr3woH6OhWsUROck4TpfkkdxoeU0JgWGwLQvcm7HzVjNfGszOsrco86Nys8LVVS mozQ== X-Gm-Message-State: AOAM533K2NmBIyzHP1JCQft9OSqaUxxMJeQWa39Ff7/i/o6UCJr+ZMoI ORf1OZ8XG+tDGrzdE32R2YM0fA== X-Google-Smtp-Source: ABdhPJyMt9gPdDTw+FpTk8LuZ7KJ4bSEOkJZBvChi1uFf0GDszQUqKrXsHdzONZd+UaK2I++GKA26A== X-Received: by 2002:a17:90b:1d11:b0:1dc:5dd1:b50e with SMTP id on17-20020a17090b1d1100b001dc5dd1b50emr7869285pjb.218.1651778853785; Thu, 05 May 2022 12:27:33 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id g4-20020a170902c38400b0015e8d4eb20bsm3383plg.85.2022.05.05.12.27.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 May 2022 12:27:33 -0700 (PDT) Date: Thu, 5 May 2022 12:27:32 -0700 From: Kees Cook To: Johannes Berg Cc: "Gustavo A . R . Silva" , Keith Packard , Francis Laniel , Daniel Axtens , Dan Williams , Vincenzo Frascino , Guenter Roeck , Daniel Vetter , Tadeusz Struk , Alexei Starovoitov , alsa-devel@alsa-project.org, Al Viro , Andrew Gabbasov , Andrew Morton , Andy Gross , Andy Lavr , Arend van Spriel , Baowen Zheng , Bjorn Andersson , Boris Ostrovsky , Bradley Grove , brcm80211-dev-list.pdl@broadcom.com, Christian Brauner , Christian =?iso-8859-1?Q?G=F6ttsche?= , Christian Lamparter , Chris Zankel , Cong Wang , David Gow , David Howells , "David S. Miller" , Dennis Dalessandro , devicetree@vger.kernel.org, Dexuan Cui , Dmitry Kasatkin , Eli Cohen , Eric Dumazet , Eric Paris , Eugeniu Rosca , Felipe Balbi , Frank Rowand , Franky Lin , Greg Kroah-Hartman , Gregory Greenman , Haiyang Zhang , Hante Meuleman , Herbert Xu , Hulk Robot , Jakub Kicinski , "James E.J. Bottomley" , James Morris , Jarkko Sakkinen , Jaroslav Kysela , Jason Gunthorpe , Jens Axboe , Johan Hedberg , John Keeping , Juergen Gross , Kalle Valo , keyrings@vger.kernel.org, kunit-dev@googlegroups.com, Kuniyuki Iwashima , "K. Y. Srinivasan" , Lars-Peter Clausen , Lee Jones , Leon Romanovsky , Liam Girdwood , linux1394-devel@lists.sourceforge.net, linux-afs@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-bluetooth@vger.kernel.org, linux-hardening@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-integrity@vger.kernel.org, linux-rdma@vger.kernel.org, linux-scsi@vger.kernel.org, linux-security-module@vger.kernel.org, linux-usb@vger.kernel.org, linux-wireless@vger.kernel.org, linux-xtensa@linux-xtensa.org, llvm@lists.linux.dev, Loic Poulain , Louis Peens , Luca Coelho , Luiz Augusto von Dentz , Marcel Holtmann , Mark Brown , "Martin K. Petersen" , Max Filippov , Mimi Zohar , Muchun Song , Nathan Chancellor , netdev@vger.kernel.org, Nick Desaulniers , Nuno =?iso-8859-1?Q?S=E1?= , Paolo Abeni , Paul Moore , Rich Felker , Rob Herring , Russell King , selinux@vger.kernel.org, "Serge E. Hallyn" , SHA-cyfmac-dev-list@infineon.com, Simon Horman , Stefano Stabellini , Stefan Richter , Steffen Klassert , Stephen Hemminger , Stephen Smalley , Takashi Iwai , Tom Rix , Udipto Goswami , wcn36xx@lists.infradead.org, Wei Liu , xen-devel@lists.xenproject.org, Xiu Jianfeng , Yang Yingliang Subject: Re: [PATCH 02/32] Introduce flexible array struct memcpy() helpers Message-ID: <202205051143.6B19E63983@keescook> References: <20220504014440.3697851-1-keescook@chromium.org> <20220504014440.3697851-3-keescook@chromium.org> <202205040819.DEA70BD@keescook> <970a674df04271b5fd1971b495c6b11a996c20c2.camel@sipsolutions.net> Precedence: bulk X-Mailing-List: llvm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <970a674df04271b5fd1971b495c6b11a996c20c2.camel@sipsolutions.net> On Thu, May 05, 2022 at 03:16:19PM +0200, Johannes Berg wrote: > On Wed, 2022-05-04 at 08:38 -0700, Kees Cook wrote: > > > > It seemed like requiring a structure be rearranged to take advantage of > > the "automatic layout introspection" wasn't very friendly. On the other > > hand, looking at the examples, most of them are already neighboring > > members. Hmmm. > > A lot of them are, and many could be, though not all. Yeah, I did a pass through them for the coming v2. Only a few have the struct order as part of an apparent hardware interface. > > And DECLARE_FLEX_ARRAY_ELEMENTS could actually be expanded to include > > the count_name too, so both methods could be "forward portable" to a > > future where C grew the syntax for bounded flex arrays. > > I guess I don't see that happening :) Well ... it's on my roadmap. ;) I want it for -fsanitize=array-bounds so that dynamic array indexing can be checked too. (Right now we can do constant-sized array index bounds checking at runtime, but the much harder to find problems tend to come from flex arrays.) > > Requiring instance to be NULL is debatable, but I feel pretty strongly > > about it because it does handle a class of mistakes (resource leaks), > > and it's not much of a burden to require a known-good starting state. > > Yeah, dunno, I guess I'm slightly more on the side of not requiring it, > since we don't do the same for kmalloc() etc. and probably really > wouldn't want to add kmalloc_s() that does it ;-) Well, I dislike all the *alloc APIs. :P > I mean, you _could_ go there: > > int kmalloc_s(void **ptr, size_t size, gfp_t gfp) Oh, and I really do (though as a macro, not a "real" function), since having type introspection would be _extremely_ useful. Though maybe it needs to be through some kind of type-of-lvalue thing... https://github.com/KSPP/linux/issues/189 https://github.com/KSPP/linux/issues/87 > So I'm not really sure why this aspect here should need to be different, > except of course that you already need the input argument for the magic. Right, and trying to move the kernel code closer to a form where the compiler can take more of the burden of handling code safety. > And btw, while I was writing it down I was looking to see if it should > be "size_t elements" or "size_t len" (like memcpy), it took me some time > to figure out, and I was looking at the examples: > > 1) most of them actually use __u8 or some variant thereof, so you > could probably add an even simpler macro like > BOUNDED_FLEX_DATA(int, bytes, data) > which has the u8 type internally. I didn't want these helpers to be "opinionated" about their types (just their API), so while it's true u8 is usually "good enough", I don't think it's common enough to make a special case for. > 2) Unless I'm confusing myself, you got the firewire change wrong, > because __mem_to_flex_dup takes the "elements_count", but the > memcpy() there wasn't multiplied by the sizeof(element)? Or maybe > the fact that it was declared as __u32 header[0] is wrong, and it > should be __u8, but it's all very confusing, and I'm really not > sure about this at all. Yes indeed; thanks for catching that. In fact, it's not a strict flex array struct, since, as you say, it's measuring bytes, not elements. Yeah, I'll see if that needs to be adjusted/dropped, etc. > One "perhaps you'll laugh me out of the room" suggestion might be to > actually be able to initialize the whole thing too? > > mydata = flex_struct_alloc(mydata, GFP_KERNEL, > variable_data, variable_len, > .member = 1, > .another = 2); > > (the ordering can't really be otherwise since you have to use > __VA_ARGS__). Oooh, that's a cool idea for the API. Hmmmm. > That might reduce some more code too, though I guess it's quite some > additional magic ... :) Yay preprocessor magic! > I was going to point to struct cfg80211_bss_ies, but I realize now > they're RCU-managed, so we never resize them anyway ... So maybe it's > less common than I thought it might be. > > I suppose you know better since you converted a lot of stuff already :-) Well, I've seen a lot of fragile code (usually in the form of exploitable flaws around flex arrays) and they do mostly look the same. Not everything fits perfectly into the forms this API tries to address, but my goal is to get it fitting well enough, and the weird stuff can be more carefully examined -- they're easier to find and audit if all the others are nicely wrapped up in some fancy flex*() API. Thanks for your thoughts on all of this! I'll continue to work on a v2... -Kees -- Kees Cook