[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <238df0b9-d1db-4f72-8238-828ea20ad1d9@embeddedor.com>
Date: Tue, 3 Dec 2024 10:20:41 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Johannes Berg <johannes@...solutions.net>, Haoyu Li <lihaoyu499@...il.com>
Cc: Kees Cook <kees@...nel.org>, "Gustavo A . R . Silva"
<gustavoars@...nel.org>, Jeff Johnson <quic_jjohnson@...cinc.com>,
linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] net: wireless: sme: Initialize n_channels before
accessing channels in cfg80211_conn_scan
On 03/12/24 09:25, Johannes Berg wrote:
> On Tue, 2024-12-03 at 23:20 +0800, Haoyu Li wrote:
>> With the new __counted_by annocation in cfg80211_scan_request struct,
>> the "n_channels" struct member must be set before accessing the
>> "channels" array. Failing to do so will trigger a runtime warning
>> when enabling CONFIG_UBSAN_BOUNDS and CONFIG_FORTIFY_SOURCE.
>>
>> Fixes: e3eac9f32ec0 ("wifi: cfg80211: Annotate struct cfg80211_scan_request with __counted_by")
>>
>> Signed-off-by: Haoyu Li <lihaoyu499@...il.com>
>
> nit: there should be no newline between these
>
> My tolerance for this is going WAY down, it seems it's all just busy-
> work, and then everyone complains and I need to handle "urgent fixes"
> because of it etc.
>
> I'm having severe second thoughts about ever having accepted the
> __counted_by annotations, I think we should just revert it. Experiment
> failed, we found ... that the code is fine but constantly needs changes
> to make the checkers happy.
Thanks for taking these changes! - This is improving :) See below.
"Right now, any addition of a counted_by annotation must also
include an open-coded assignment of the counter variable after
the allocation:
p = alloc(p, array, how_many);
p->counter = how_many;
In order to avoid the tedious and error-prone work of manually adding
the open-coded counted-by intializations everywhere in the Linux
kernel, a new GCC builtin __builtin_counted_by_ref will be very useful
to be added to help the adoption of the counted-by attribute.
-- Built-in Function: TYPE __builtin_counted_by_ref (PTR)
The built-in function '__builtin_counted_by_ref' checks whether the
array object pointed by the pointer PTR has another object
associated with it that represents the number of elements in the
array object through the 'counted_by' attribute (i.e. the
counted-by object). If so, returns a pointer to the corresponding
counted-by object. If such counted-by object does not exist,
returns a null pointer.
This built-in function is only available in C for now.
The argument PTR must be a pointer to an array. The TYPE of the
returned value is a pointer type pointing to the corresponding
type of the counted-by object or a void pointer type in case of a
null pointer being returned.
With this new builtin, the central allocator could be updated to:
#define MAX(A, B) (A > B) ? (A) : (B)
#define alloc(P, FAM, COUNT) ({ \
__auto_type __p = &(P); \
__auto_type __c = (COUNT); \
size_t __size = MAX (sizeof (*(*__p)),\
__builtin_offsetof (__typeof(*(*__p)),FAM) \
+ sizeof (*((*__p)->FAM)) * __c); \
if ((*__p = kmalloc(__size))) { \
__auto_type ret = __builtin_counted_by_ref((*__p)->FAM); \
*_Generic(ret, void *: &(size_t){0}, default: ret) = __c; \
} \
})
And then structs can gain the counted_by attribute without needing
additional open-coded counter assignments for each struct, and
unannotated structs could still use the same allocator."[1]
These changes have been merged already, and will likely be released
in coming GCC 15.
For Clang, see [2].
For the kmalloc-family changes, see [3] (a new version of this that includes
the __builtin_counted_by_ref() update is coming soon).
-Gustavo
[1] https://gcc.gnu.org/pipermail/gcc-patches/2024-October/665165.html
[2] https://github.com/llvm/llvm-project/issues/99774
[3] https://lore.kernel.org/linux-hardening/20240822231324.make.666-kees@kernel.org/
Powered by blists - more mailing lists