[PATCH] net: wireless: sme: Initialize n_channels before accessing channels in cfg80211_conn_scan

[PATCH] net: wireless: sme: Initialize n_channels before accessing channels in cfg80211_conn_scan

[Haoyu Li]

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@gmail.com>
---
 net/wireless/sme.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 431da30817a6..268171600087 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -83,6 +83,7 @@ static int cfg80211_conn_scan(struct wireless_dev *wdev)
 	if (!request)
 		return -ENOMEM;
 
+	request->n_channels = n_channels;
 	if (wdev->conn->params.channel) {
 		enum nl80211_band band = wdev->conn->params.channel->band;
 		struct ieee80211_supported_band *sband =
-- 
2.34.1

Re: [PATCH] net: wireless: sme: Initialize n_channels before accessing channels in cfg80211_conn_scan

[Johannes Berg]

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@gmail.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.

johannes

Re: [PATCH] net: wireless: sme: Initialize n_channels before accessing channels in cfg80211_conn_scan

[Gustavo A. R. Silva]

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@gmail.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/

Re: [PATCH] net: wireless: sme: Initialize n_channels before accessing channels in cfg80211_conn_scan

[Johannes Berg]

On Tue, 2024-12-03 at 10:20 -0600, Gustavo A. R. Silva wrote:
> 
> "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;

Not sure where you copied that from, but quite obviously Kees didn't
follow that guidance in e3eac9f32ec0 ("wifi: cfg80211: Annotate struct
cfg80211_scan_request with __counted_by"), otherwise we wouldn't have
this patch.

>   -- Built-in Function: TYPE __builtin_counted_by_ref (PTR)

Even with that though, we still have to actually implement it, and make
sure we use struct_size everywhere when we allocate these things... In
fact we probably need a new allocation function, not just struct_size,
but rather kzalloc_struct_size(...) or so.

Which e3eac9f32ec0 didn't do, and which anyway we still don't do e.g. in
nl80211_trigger_scan() because we have multiple variable things in the
allocation, so we *can't*.

That therefore doesn't even help here.

So that's not a very convincing argument. In a way moving again to "you
need the newest unreleased compiler" makes it *worse*, not *better*?

But of course if you do that now it'll basically mean again nobody is
running it and you get to kick the can further down the road ... I still
think it's a failed experiment. It didn't do any good here as far as I
can tell, and we've spent a ton of time on it.

johannes