lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMZ6RqJ1N9E=yqJLcreVsqk5xtoHWQghBaezB+=TzjC3ChxxGA@mail.gmail.com> Date: Wed, 11 Sep 2024 13:45:50 +0900 From: Vincent MAILHOL <mailhol.vincent@...adoo.fr> To: Kees Cook <kees@...nel.org> Cc: David Laight <David.Laight@...lab.com>, "Gustavo A . R . Silva" <gustavoars@...nel.org>, linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v2] overflow: optimize struct_size() calculation On Wed. 11 Sep. 2024 at 09:36, Kees Cook <kees@...nel.org> wrote: > On Tue, Sep 10, 2024 at 11:49:52AM +0900, Vincent Mailhol wrote: > > If the offsetof() of a given flexible array member (fam) is smaller > > than the sizeof() of the containing struct, then the struct_size() > > macro reports a size which is too big. > > > > This occurs when the two conditions below are met: > > > > - there are padding bytes after the penultimate member (the member > > preceding the fam) > > - the alignment of the fam is less than or equal to the penultimate > > member's alignment > > > > In that case, the fam overlaps with the padding bytes of the > > penultimate member. This behaviour is not captured in the current > > struct_size() macro, potentially resulting in an overestimated size. > > > > Below example illustrates the issue: > > > > struct s { > > u64 foo; > > u32 count; > > u8 fam[] __counted_by(count); > > }; > > > > Assuming a 64 bits architecture: > > > > - there are 4 bytes of padding after s.count (the penultimate > > member) > > - sizeof(struct s) is 16 bytes > > - the offset of s.fam is 12 bytes > > - the alignment of s.fam is 1 byte > > > > The sizes are as below: > > > > s.count current struct_size() actual size > > ----------------------------------------------------------------------- > > 0 16 16 > > 1 17 16 > > 2 18 16 > > 3 19 16 > > 4 20 16 > > 5 21 17 > > . . . > > . . . > > . . . > > n sizeof(struct s) + n max(sizeof(struct s), > > offsetof(struct s, fam) + n) > > > > Change struct_size() from this pseudo code logic: > > > > sizeof(struct s) + sizeof(*s.fam) * s.count > > > > to that pseudo code logic: > > > > max(sizeof(struct s), offsetof(struct s, fam) + sizeof(*s.fam) * s.count) > > > > Here, the lowercase max*() macros can cause struct_size() to return a > > non constant integer expression which would break the DEFINE_FLEX() > > macro by making it declare a variable length array. Because of that, > > use the unsafe MAX() macro only if the expression is constant and use > > the safer max() otherwise. > > > > Reference: ISO/IEC 9899:2018 §6.7.2.1 "Structure and union specifiers" ¶18 > > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr> > > --- > > > > I also tried to think of whether the current struct_size() macro could > > be a security issue. > > > > The only example I can think of is if someone manually allocates the > > exact size but then use the current struct_size() macro. > > > > For example (reusing the struct s from above): > > > > u32 count = 5; > > struct s *s = kalloc(offsetof(typeof(*s), fam) + count); > > s->count = count; > > memset(s, 0, struct_size(s, fam, count)); /* 4 bytes buffer overflow */ > > > > If we have concerns that above pattern may actually exist, then this > > patch should also go to stable. I personally think that the above is a > > bit convoluted and, as such, I only suggest this patch to go to next. > > Yeah, this "over-estimation" has come up before, and my thinking as been > that while the above situation is certainly possible (but unlikely), > I've worried that the reverse situation (after this patch) is > potentially worse, where we allocate very precisely, but then manually > index too far: > > struct s *s = kmalloc(struct_size(s, fam, count), gfp); > typeof(*s->fam) *element; > element = (void *)s + sizeof(*s); > for (i = 0; i < count; i++) > element[i] = ...; To me, this pointer arithmetic is just a bug. Anyone in his right mind would write: typeof(*s->fam) *element = s->fam; When I compare your example to my example, I think that both are convoluted and unrealistic but with the *huge* difference that: - in my example, the code logic is correct and the thing breaks because of the current overestimation in struct_size(). See the clang documentation: https://clang.llvm.org/docs/AttributeReference.html#counted-by The clang guys are using the same calculation as in my patch. So I can think of a world in which someone with good intent would copy this example, and then (for example because of a collaborative work), someone else would use the struct_size() to do a copy or a memset resulting in an out of bound as illustrated in my example. - in your example, the code is incorrect. If we start to apply that "what if users do random pointer arithmetic" reasoning, then anything can be proven to be a security risk. To repeat, I think that my example was convoluted, but I see yours as even more convoluted. So, with this said, what do you think is the good trade-off? Return the exact size so that code with correct logic works as intended or keep the extra bytes in case someone does some crazy pointer arithmetic? > And for a max 7 byte savings, I'm concerned we can get bit much worse in > the above situation. It *should* be unlikely, but I've especially seen a > lot of manually calculated games especially for structs that have > effectively multiple trailing flexible arrays (wifi likes to do this, > for example). How does a struct with "multiple trailing flexible arrays" look like? By the C standard, structures can only have one single fam, so this isn't something I am aware of. Would the struct_size() macro still be relevant for those "multiple fam" structures or would the structure size also be hand calculated? Isn't this argument out of scope of this patch discussion? > So while I don't have very concrete evidence, my sensation is that we're > in a more defensive position leaving it over-estimated. So far, I think that my example is more concrete than your pointer arithmetic argument. Not by a lot, and this is why I just put that security argument at the end after the --- cutter. Yours sincerely, Vincent Mailhol
Powered by blists - more mailing lists