linux-kernel - Re: [REGRESSION][BISECTED] erroneous buffer overflow detected in bch2_xattr

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <DD12E746-2B59-4AF5-B396-4700C6D099F9@gmail.com>
Date: Fri, 2 May 2025 02:18:33 +0800
From: Alan Huang <mmpgouride@...il.com>
To: Jan Hendrik Farr <kernel@...rr.cc>
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 May 2, 2025, at 01:58, Jan Hendrik Farr <kernel@...rr.cc> wrote:
> 
> 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.

OK, thanks, I’ll revert that.

> 
> [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