linux-hardening - Re: [PATCH 1/2] bcachefs: Remove incorrect __counted

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aBQmJIXygwIwj5Yy@archlinux>
Date: Fri, 2 May 2025 03:55:48 +0200
From: Jan Hendrik Farr <kernel@...rr.cc>
To: Alan Huang <mmpgouride@...il.com>
Cc: kent.overstreet@...ux.dev, kees@...nel.org, gustavoars@...nel.org,
	thorsten.blum@...lux.com, linux-bcachefs@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH 1/2] bcachefs: Remove incorrect __counted_by annotation

On 02 04:01:31, Alan Huang wrote:
> This actually reverts 86e92eeeb237 ("bcachefs: Annotate struct bch_xattr
> with __counted_by()").
> 
> After the x_name, there is a value. According to the disscussion[1],
> __counted_by assumes that the flexible array member contains exactly
> the amount of elements that are specified. Now there are users came across
> a false positive detection of an out of bounds write caused by
> the __counted_by here[2], so revert that.
> 
> [1] https://lore.kernel.org/lkml/Zv8VDKWN1GzLRT-_@archlinux/T/#m0ce9541c5070146320efd4f928cc1ff8de69e9b2
> [2] https://privatebin.net/?a0d4e97d590d71e1#9bLmp2Kb5NU6X6cZEucchDcu88HzUQwHUah8okKPReEt
> 
> Signed-off-by: Alan Huang <mmpgouride@...il.com>

Reviewed-by: Jan Hendrik Farr <kernel@...rr.cc>

> ---
>  fs/bcachefs/xattr_format.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/bcachefs/xattr_format.h b/fs/bcachefs/xattr_format.h
> index c7916011ef34..67426e33d04e 100644
> --- a/fs/bcachefs/xattr_format.h
> +++ b/fs/bcachefs/xattr_format.h
> @@ -13,7 +13,13 @@ struct bch_xattr {
>  	__u8			x_type;
>  	__u8			x_name_len;
>  	__le16			x_val_len;
> -	__u8			x_name[] __counted_by(x_name_len);
> +	/*
> +	 * x_name contains the name and value counted by
> +	 * x_name_len + x_val_len. The introduction of
> +	 * __counted_by(x_name_len) caused a false positive
> +	 * detection of an out of bounds write.
> +	 */

In my estimation the comment isn't strictly needed with the name
change in Patch 2/2, but it can also stay.

> +	__u8			x_name[];
>  } __packed __aligned(8);
>  
>  #endif /* _BCACHEFS_XATTR_FORMAT_H */
> -- 
> 2.48.1
> 

I was able to reproduce this issue with gcc 15.1.1 and bcachefs as
rootfs (and verify that this fixes it), but wasn't able to reproduce
using clang 19.1.7. Turns out there is one more difference in how
gcc and clang do __bdos. clang apparantly doesn't handle pointer
arithmetic done using + symbol instead of [] for the __counted_by
case. Here's a reproducer:
https://godbolt.org/z/136T98Wdz

Best Regards
Jan