[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aBO2XVSisMXtU8nD@archlinux>
Date: Thu, 1 May 2025 19:58:53 +0200
From: Jan Hendrik Farr <kernel@...rr.cc>
To: Alan Huang <mmpgouride@...il.com>
Cc: kent.overstreet@...ux.dev, Thorsten Blum <thorsten.blum@...lux.com>,
Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
Nathan Chancellor <nathan@...nel.org>,
Bill Wendling <morbo@...gle.com>, Kees Cook <kees@...nel.org>,
regressions@...ts.linux.dev, linux-bcachefs@...r.kernel.org,
linux-hardening@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, ardb@...nel.org,
ojeda@...nel.org
Subject: Re: [REGRESSION][BISECTED] erroneous buffer overflow detected in
bch2_xattr_validate
On 02 01:28:28, Alan Huang wrote:
>
> Thanks,
> Alan
>
>
>
> > On May 2, 2025, at 01:22, Jan Hendrik Farr <kernel@...rr.cc> wrote:
> >
> >>
> >> I wonder if the __counted_by(x_name_len) in struct bch_xattr is needed, since there is also a value after x_name.
> >
> > Wait a minute. Are you saying that the value with length x_val_len
> > is behind the name (of length x_name_len) at the end of the struct.
> > So essentially the flexible array member x_name has a length of
> > x_name_len + x_val_len and contains both the name and value?
>
> Yes.
I assume you can't easily change the struct such that there exists a member
that contains the result of x_val_len + x_name_len, correct?
In that case the only available course of action at this time is to
remove the __counted_by, because it is incorrect.
In addition I would recommend changing the name of x_name to something
like x_name_and_val or similar. It's very misleading to call it x_name
when it also contains the value.
>
> >
> > If that's the case:
> >
> > 1. that's not at all clear from the struct definition
> > 2. __counted_by(x_name_len) is not correct in that case
> >
>
> Both clang and gcc say:
>
> • p->array has at least p->count number of elements available all the time.
>
> Note the at least here. Though I think the counted_by is misleading here.
>
Here's how clang defines __bdos language extension [1]. Also note the
attribute reference for __counted_by [2]. It assumes that the flexible array
member contains exactly the amount of elements that are specified.
I guess your quote from the gcc docs is misleading, as gcc's behavior
is like clang's.
The kernel uses the type & 2 == 0 case.
So let's say you have a simple struct like so:
struct foo{
int val_len;
char val[] __counted_by(val_len);
}
If val_len is 10 then foo->val[10] will be considered out of bounds.
Even if you did a malloc for enough space.
[1] https://github.com/llvm/llvm-project/blob/3b88805ca20018ae202afd3aea39f4fa856a8c64/clang/docs/LanguageExtensions.rst?plain=1#L5502-L5507
[2] https://clang.llvm.org/docs/AttributeReference.html#counted-by-counted-by-or-null-sized-by-sized-by-or-null
Best Regards
Jan
Powered by blists - more mailing lists