Date:   Thu, 5 May 2022 12:27:32 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     "Gustavo A . R . Silva" <gustavoars@...nel.org>,
        Keith Packard <keithp@...thp.com>,
        Francis Laniel <laniel_francis@...vacyrequired.com>,
        Daniel Axtens <dja@...ens.net>,
        Dan Williams <dan.j.williams@...el.com>,
        Vincenzo Frascino <vincenzo.frascino@....com>,
        Guenter Roeck <linux@...ck-us.net>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Tadeusz Struk <tadeusz.struk@...aro.org>,
        Alexei Starovoitov <ast@...nel.org>,
        alsa-devel@...a-project.org, Al Viro <viro@...iv.linux.org.uk>,
        Andrew Gabbasov <andrew_gabbasov@...tor.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Andy Gross <agross@...nel.org>,
        Andy Lavr <andy.lavr@...il.com>,
        Arend van Spriel <aspriel@...il.com>,
        Baowen Zheng <baowen.zheng@...igine.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Bradley Grove <linuxdrivers@...otech.com>,
        brcm80211-dev-list.pdl@...adcom.com,
        Christian Brauner <brauner@...nel.org>,
        Christian Göttsche <cgzones@...glemail.com>,
        Christian Lamparter <chunkeey@...glemail.com>,
        Chris Zankel <chris@...kel.net>,
        Cong Wang <cong.wang@...edance.com>,
        David Gow <davidgow@...gle.com>,
        David Howells <dhowells@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>,
        devicetree@...r.kernel.org, Dexuan Cui <decui@...rosoft.com>,
        Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
        Eli Cohen <elic@...dia.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Eric Paris <eparis@...isplace.org>,
        Eugeniu Rosca <erosca@...adit-jv.com>,
        Felipe Balbi <balbi@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Franky Lin <franky.lin@...adcom.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Gregory Greenman <gregory.greenman@...el.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Hante Meuleman <hante.meuleman@...adcom.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Hulk Robot <hulkci@...wei.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        James Morris <jmorris@...ei.org>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Jason Gunthorpe <jgg@...pe.ca>, Jens Axboe <axboe@...nel.dk>,
        Johan Hedberg <johan.hedberg@...il.com>,
        John Keeping <john@...anate.com>,
        Juergen Gross <jgross@...e.com>, Kalle Valo <kvalo@...nel.org>,
        keyrings@...r.kernel.org, kunit-dev@...glegroups.com,
        Kuniyuki Iwashima <kuniyu@...zon.co.jp>,
        "K. Y. Srinivasan" <kys@...rosoft.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Lee Jones <lee.jones@...aro.org>,
        Leon Romanovsky <leon@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        linux1394-devel@...ts.sourceforge.net,
        linux-afs@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, linux-bluetooth@...r.kernel.org,
        linux-hardening@...r.kernel.org, linux-hyperv@...r.kernel.org,
        linux-integrity@...r.kernel.org, linux-rdma@...r.kernel.org,
        linux-scsi@...r.kernel.org, linux-security-module@...r.kernel.org,
        linux-usb@...r.kernel.org, linux-wireless@...r.kernel.org,
        linux-xtensa@...ux-xtensa.org, llvm@...ts.linux.dev,
        Loic Poulain <loic.poulain@...aro.org>,
        Louis Peens <louis.peens@...igine.com>,
        Luca Coelho <luciano.coelho@...el.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        Marcel Holtmann <marcel@...tmann.org>,
        Mark Brown <broonie@...nel.org>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Max Filippov <jcmvbkbc@...il.com>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        Muchun Song <songmuchun@...edance.com>,
        Nathan Chancellor <nathan@...nel.org>, netdev@...r.kernel.org,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Nuno Sá <nuno.sa@...log.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Paul Moore <paul@...l-moore.com>,
        Rich Felker <dalias@...ifal.cx>,
        Rob Herring <robh+dt@...nel.org>,
        Russell King <linux@...linux.org.uk>, selinux@...r.kernel.org,
        "Serge E. Hallyn" <serge@...lyn.com>,
        SHA-cyfmac-dev-list@...ineon.com,
        Simon Horman <simon.horman@...igine.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Stefan Richter <stefanr@...6.in-berlin.de>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Takashi Iwai <tiwai@...e.com>, Tom Rix <trix@...hat.com>,
        Udipto Goswami <quic_ugoswami@...cinc.com>,
        wcn36xx@...ts.infradead.org, Wei Liu <wei.liu@...nel.org>,
        xen-devel@...ts.xenproject.org,
        Xiu Jianfeng <xiujianfeng@...wei.com>,
        Yang Yingliang <yangyingliang@...wei.com>
Subject: Re: [PATCH 02/32] Introduce flexible array struct memcpy() helpers

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

Powered by blists - more mailing lists