[PATCH v2][next] wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

[PATCH v2][next] wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

[Gustavo A. R. Silva]

Prepare for the coming implementation by GCC and Clang of the
__counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time
via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
(for strcpy/memcpy-family functions).

Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are
getting ready to enable it globally.

So, use the `DEFINE_FLEX()` helper for an on-stack definition of
a flexible structure where the size of the flexible-array member
is known at compile-time, and refactor the rest of the code,
accordingly.

So, with these changes, fix the following warning:
drivers/net/wireless/ath/wil6210/wmi.c:4018:49: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

Link: https://github.com/KSPP/linux/issues/202
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
 - Use __struct_size() to get the compile-time size of the flex-struct
   instance.

v1:
 - Link: https://lore.kernel.org/linux-hardening/ZgRsn72WkHzfCUsa@neat/

 drivers/net/wireless/ath/wil6210/wmi.c | 19 +++++++------------
 drivers/net/wireless/ath/wil6210/wmi.h |  2 +-
 2 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/wmi.c b/drivers/net/wireless/ath/wil6210/wmi.c
index 6fdb77d4c59e..8ff69dc72fb9 100644
--- a/drivers/net/wireless/ath/wil6210/wmi.c
+++ b/drivers/net/wireless/ath/wil6210/wmi.c
@@ -4014,28 +4014,23 @@ int wmi_set_cqm_rssi_config(struct wil6210_priv *wil,
 	struct net_device *ndev = wil->main_ndev;
 	struct wil6210_vif *vif = ndev_to_vif(ndev);
 	int rc;
-	struct {
-		struct wmi_set_link_monitor_cmd cmd;
-		s8 rssi_thold;
-	} __packed cmd = {
-		.cmd = {
-			.rssi_hyst = rssi_hyst,
-			.rssi_thresholds_list_size = 1,
-		},
-		.rssi_thold = rssi_thold,
-	};
 	struct {
 		struct wmi_cmd_hdr hdr;
 		struct wmi_set_link_monitor_event evt;
 	} __packed reply = {
 		.evt = {.status = WMI_FW_STATUS_FAILURE},
 	};
+	DEFINE_FLEX(struct wmi_set_link_monitor_cmd, cmd,
+		    rssi_thresholds_list, rssi_thresholds_list_size, 1);
+
+	cmd->rssi_hyst = rssi_hyst;
+	cmd->rssi_thresholds_list[0] = rssi_thold;
 
 	if (rssi_thold > S8_MAX || rssi_thold < S8_MIN || rssi_hyst > U8_MAX)
 		return -EINVAL;
 
-	rc = wmi_call(wil, WMI_SET_LINK_MONITOR_CMDID, vif->mid, &cmd,
-		      sizeof(cmd), WMI_SET_LINK_MONITOR_EVENTID,
+	rc = wmi_call(wil, WMI_SET_LINK_MONITOR_CMDID, vif->mid, cmd,
+		      __struct_size(cmd), WMI_SET_LINK_MONITOR_EVENTID,
 		      &reply, sizeof(reply), WIL_WMI_CALL_GENERAL_TO_MS);
 	if (rc) {
 		wil_err(wil, "WMI_SET_LINK_MONITOR_CMDID failed, rc %d\n", rc);
diff --git a/drivers/net/wireless/ath/wil6210/wmi.h b/drivers/net/wireless/ath/wil6210/wmi.h
index b47606d9068c..38f64524019e 100644
--- a/drivers/net/wireless/ath/wil6210/wmi.h
+++ b/drivers/net/wireless/ath/wil6210/wmi.h
@@ -3320,7 +3320,7 @@ struct wmi_set_link_monitor_cmd {
 	u8 rssi_hyst;
 	u8 reserved[12];
 	u8 rssi_thresholds_list_size;
-	s8 rssi_thresholds_list[];
+	s8 rssi_thresholds_list[] __counted_by(rssi_thresholds_list_size);
 } __packed;
 
 /* wmi_link_monitor_event_type */
-- 
2.34.1

Re: [PATCH v2][next] wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

[Jeff Johnson]

On 3/27/2024 2:43 PM, Gustavo A. R. Silva wrote:
> Prepare for the coming implementation by GCC and Clang of the
> __counted_by attribute. Flexible array members annotated with
> __counted_by can have their accesses bounds-checked at run-time
> via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
> (for strcpy/memcpy-family functions).
> 
> Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are
> getting ready to enable it globally.
> 
> So, use the `DEFINE_FLEX()` helper for an on-stack definition of
> a flexible structure where the size of the flexible-array member
> is known at compile-time, and refactor the rest of the code,
> accordingly.
> 
> So, with these changes, fix the following warning:
> drivers/net/wireless/ath/wil6210/wmi.c:4018:49: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 
> Link: https://github.com/KSPP/linux/issues/202
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Re: [PATCH v2][next] wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

[Kalle Valo]

"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> Prepare for the coming implementation by GCC and Clang of the
> __counted_by attribute. Flexible array members annotated with
> __counted_by can have their accesses bounds-checked at run-time
> via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
> (for strcpy/memcpy-family functions).
> 
> Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are
> getting ready to enable it globally.
> 
> So, use the `DEFINE_FLEX()` helper for an on-stack definition of
> a flexible structure where the size of the flexible-array member
> is known at compile-time, and refactor the rest of the code,
> accordingly.
> 
> So, with these changes, fix the following warning:
> drivers/net/wireless/ath/wil6210/wmi.c:4018:49: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 
> Link: https://github.com/KSPP/linux/issues/202
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

cbb0697e0ded wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/ZgSTCmdP+omePvWg@neat/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2][next] wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

[Kees Cook]

On Thu, Apr 04, 2024 at 10:12:28AM +0000, Kalle Valo wrote:
> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> 
> > Prepare for the coming implementation by GCC and Clang of the
> > __counted_by attribute. Flexible array members annotated with
> > __counted_by can have their accesses bounds-checked at run-time
> > via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
> > (for strcpy/memcpy-family functions).
> > 
> > Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are
> > getting ready to enable it globally.
> > 
> > So, use the `DEFINE_FLEX()` helper for an on-stack definition of
> > a flexible structure where the size of the flexible-array member
> > is known at compile-time, and refactor the rest of the code,
> > accordingly.
> > 
> > So, with these changes, fix the following warning:
> > drivers/net/wireless/ath/wil6210/wmi.c:4018:49: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> > 
> > Link: https://github.com/KSPP/linux/issues/202
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> 
> Patch applied to ath-next branch of ath.git, thanks.
> 
> cbb0697e0ded wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

Hi,

I was just walking through our patch tracker and noticed that I don't
see this patch include in -next yet (as of next-20240429). Is there a
flush of the ath-next queue planned soon? Or did I miss some change?

Thanks!

-Kees

-- 
Kees Cook

Re: [PATCH v2][next] wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

[Kalle Valo]

Kees Cook <keescook@chromium.org> writes:

> On Thu, Apr 04, 2024 at 10:12:28AM +0000, Kalle Valo wrote:
>
>> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
>> 
>> > Prepare for the coming implementation by GCC and Clang of the
>> > __counted_by attribute. Flexible array members annotated with
>> > __counted_by can have their accesses bounds-checked at run-time
>> > via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
>> > (for strcpy/memcpy-family functions).
>> > 
>> > Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are
>> > getting ready to enable it globally.
>> > 
>> > So, use the `DEFINE_FLEX()` helper for an on-stack definition of
>> > a flexible structure where the size of the flexible-array member
>> > is known at compile-time, and refactor the rest of the code,
>> > accordingly.
>> > 
>> > So, with these changes, fix the following warning:
>> > drivers/net/wireless/ath/wil6210/wmi.c:4018:49: warning: structure
>> > containing a flexible array member is not at the end of another
>> > structure [-Wflex-array-member-not-at-end]
>> > 
>> > Link: https://github.com/KSPP/linux/issues/202
>> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> > Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
>> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>> 
>> Patch applied to ath-next branch of ath.git, thanks.
>> 
>> cbb0697e0ded wifi: wil6210: wmi: Use __counted_by() in struct
>> wmi_set_link_monitor_cmd and avoid -Wfamnae warning
>
> Hi,
>
> I was just walking through our patch tracker and noticed that I don't
> see this patch include in -next yet (as of next-20240429). Is there a
> flush of the ath-next queue planned soon? Or did I miss some change?

Yeah, wireless-next was pulled last week so most likely we will create
ath-next pull request this week.

BTW we are planning to move ath.git to a new location, rename branches
etc. I think we'll see if we can also setup it so that it can be pulled
to linux-next, so that you don't need to ask this every time ;)

(Just joking of course, there a lot of benefits from having the tree in
linux-next)

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2][next] wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

[Kees Cook]

On Mon, Apr 29, 2024 at 08:25:56PM +0300, Kalle Valo wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
> > On Thu, Apr 04, 2024 at 10:12:28AM +0000, Kalle Valo wrote:
> >
> >> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> >> 
> >> > Prepare for the coming implementation by GCC and Clang of the
> >> > __counted_by attribute. Flexible array members annotated with
> >> > __counted_by can have their accesses bounds-checked at run-time
> >> > via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
> >> > (for strcpy/memcpy-family functions).
> >> > 
> >> > Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are
> >> > getting ready to enable it globally.
> >> > 
> >> > So, use the `DEFINE_FLEX()` helper for an on-stack definition of
> >> > a flexible structure where the size of the flexible-array member
> >> > is known at compile-time, and refactor the rest of the code,
> >> > accordingly.
> >> > 
> >> > So, with these changes, fix the following warning:
> >> > drivers/net/wireless/ath/wil6210/wmi.c:4018:49: warning: structure
> >> > containing a flexible array member is not at the end of another
> >> > structure [-Wflex-array-member-not-at-end]
> >> > 
> >> > Link: https://github.com/KSPP/linux/issues/202
> >> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> >> > Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> >> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> >> 
> >> Patch applied to ath-next branch of ath.git, thanks.
> >> 
> >> cbb0697e0ded wifi: wil6210: wmi: Use __counted_by() in struct
> >> wmi_set_link_monitor_cmd and avoid -Wfamnae warning
> >
> > Hi,
> >
> > I was just walking through our patch tracker and noticed that I don't
> > see this patch include in -next yet (as of next-20240429). Is there a
> > flush of the ath-next queue planned soon? Or did I miss some change?
> 
> Yeah, wireless-next was pulled last week so most likely we will create
> ath-next pull request this week.
> 
> BTW we are planning to move ath.git to a new location, rename branches
> etc. I think we'll see if we can also setup it so that it can be pulled
> to linux-next, so that you don't need to ask this every time ;)
> 
> (Just joking of course, there a lot of benefits from having the tree in
> linux-next)

Ah-ha! Thanks. Yeah, sorry if I keep asking about that. It's different
from other trees, so it doesn't stick in my head. :) I should keep
better notes!

-- 
Kees Cook

Re: [PATCH v2][next] wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

[Kalle Valo]

Kees Cook <keescook@chromium.org> writes:

> On Mon, Apr 29, 2024 at 08:25:56PM +0300, Kalle Valo wrote:
>
>> Kees Cook <keescook@chromium.org> writes:
>> 
>> > On Thu, Apr 04, 2024 at 10:12:28AM +0000, Kalle Valo wrote:
>> >
>> >> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
>> >> 
>> >> > Prepare for the coming implementation by GCC and Clang of the
>> >> > __counted_by attribute. Flexible array members annotated with
>> >> > __counted_by can have their accesses bounds-checked at run-time
>> >> > via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
>> >> > (for strcpy/memcpy-family functions).
>> >> > 
>> >> > Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are
>> >> > getting ready to enable it globally.
>> >> > 
>> >> > So, use the `DEFINE_FLEX()` helper for an on-stack definition of
>> >> > a flexible structure where the size of the flexible-array member
>> >> > is known at compile-time, and refactor the rest of the code,
>> >> > accordingly.
>> >> > 
>> >> > So, with these changes, fix the following warning:
>> >> > drivers/net/wireless/ath/wil6210/wmi.c:4018:49: warning: structure
>> >> > containing a flexible array member is not at the end of another
>> >> > structure [-Wflex-array-member-not-at-end]
>> >> > 
>> >> > Link: https://github.com/KSPP/linux/issues/202
>> >> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> >> > Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
>> >> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>> >> 
>> >> Patch applied to ath-next branch of ath.git, thanks.
>> >> 
>> >> cbb0697e0ded wifi: wil6210: wmi: Use __counted_by() in struct
>> >> wmi_set_link_monitor_cmd and avoid -Wfamnae warning
>> >
>> > Hi,
>> >
>> > I was just walking through our patch tracker and noticed that I don't
>> > see this patch include in -next yet (as of next-20240429). Is there a
>> > flush of the ath-next queue planned soon? Or did I miss some change?
>> 
>> Yeah, wireless-next was pulled last week so most likely we will create
>> ath-next pull request this week.
>> 
>> BTW we are planning to move ath.git to a new location, rename branches
>> etc. I think we'll see if we can also setup it so that it can be pulled
>> to linux-next, so that you don't need to ask this every time ;)
>> 
>> (Just joking of course, there a lot of benefits from having the tree in
>> linux-next)
>
> Ah-ha! Thanks. Yeah, sorry if I keep asking about that. It's different
> from other trees, so it doesn't stick in my head. :) I should keep
> better notes!

BTW I think all vendor specific wireless driver trees are not pulled to
linux-next: iwlwifi, mt76, rtw (Realtek) and ath. So with all of these it will
take a while before the commit is in linux-next.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2][next] wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

[Kees Cook]

On Mon, Apr 29, 2024 at 10:21:32PM +0300, Kalle Valo wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
> > On Mon, Apr 29, 2024 at 08:25:56PM +0300, Kalle Valo wrote:
> >
> >> Kees Cook <keescook@chromium.org> writes:
> >> 
> >> > On Thu, Apr 04, 2024 at 10:12:28AM +0000, Kalle Valo wrote:
> >> >
> >> >> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> >> >> 
> >> >> > Prepare for the coming implementation by GCC and Clang of the
> >> >> > __counted_by attribute. Flexible array members annotated with
> >> >> > __counted_by can have their accesses bounds-checked at run-time
> >> >> > via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE
> >> >> > (for strcpy/memcpy-family functions).
> >> >> > 
> >> >> > Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are
> >> >> > getting ready to enable it globally.
> >> >> > 
> >> >> > So, use the `DEFINE_FLEX()` helper for an on-stack definition of
> >> >> > a flexible structure where the size of the flexible-array member
> >> >> > is known at compile-time, and refactor the rest of the code,
> >> >> > accordingly.
> >> >> > 
> >> >> > So, with these changes, fix the following warning:
> >> >> > drivers/net/wireless/ath/wil6210/wmi.c:4018:49: warning: structure
> >> >> > containing a flexible array member is not at the end of another
> >> >> > structure [-Wflex-array-member-not-at-end]
> >> >> > 
> >> >> > Link: https://github.com/KSPP/linux/issues/202
> >> >> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> >> >> > Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> >> >> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> >> >> 
> >> >> Patch applied to ath-next branch of ath.git, thanks.
> >> >> 
> >> >> cbb0697e0ded wifi: wil6210: wmi: Use __counted_by() in struct
> >> >> wmi_set_link_monitor_cmd and avoid -Wfamnae warning
> >> >
> >> > Hi,
> >> >
> >> > I was just walking through our patch tracker and noticed that I don't
> >> > see this patch include in -next yet (as of next-20240429). Is there a
> >> > flush of the ath-next queue planned soon? Or did I miss some change?
> >> 
> >> Yeah, wireless-next was pulled last week so most likely we will create
> >> ath-next pull request this week.
> >> 
> >> BTW we are planning to move ath.git to a new location, rename branches
> >> etc. I think we'll see if we can also setup it so that it can be pulled
> >> to linux-next, so that you don't need to ask this every time ;)
> >> 
> >> (Just joking of course, there a lot of benefits from having the tree in
> >> linux-next)
> >
> > Ah-ha! Thanks. Yeah, sorry if I keep asking about that. It's different
> > from other trees, so it doesn't stick in my head. :) I should keep
> > better notes!
> 
> BTW I think all vendor specific wireless driver trees are not pulled to
> linux-next: iwlwifi, mt76, rtw (Realtek) and ath. So with all of these it will
> take a while before the commit is in linux-next.

How long is "a while"? And if the latency can be reduced for these, it'd
be nice since it would allow for longer bake-time in -next.

-- 
Kees Cook

Re: [PATCH v2][next] wifi: wil6210: wmi: Use __counted_by() in struct wmi_set_link_monitor_cmd and avoid -Wfamnae warning

[Kalle Valo]

Kees Cook <keescook@chromium.org> writes:

>> >> > I was just walking through our patch tracker and noticed that I don't
>> >> > see this patch include in -next yet (as of next-20240429). Is there a
>> >> > flush of the ath-next queue planned soon? Or did I miss some change?
>> >> 
>> >> Yeah, wireless-next was pulled last week so most likely we will create
>> >> ath-next pull request this week.
>> >> 
>> >> BTW we are planning to move ath.git to a new location, rename branches
>> >> etc. I think we'll see if we can also setup it so that it can be pulled
>> >> to linux-next, so that you don't need to ask this every time ;)
>> >> 
>> >> (Just joking of course, there a lot of benefits from having the tree in
>> >> linux-next)
>> >
>> > Ah-ha! Thanks. Yeah, sorry if I keep asking about that. It's different
>> > from other trees, so it doesn't stick in my head. :) I should keep
>> > better notes!
>> 
>> BTW I think all vendor specific wireless driver trees are not pulled to
>> linux-next: iwlwifi, mt76, rtw (Realtek) and ath. So with all of these it will
>> take a while before the commit is in linux-next.
>
> How long is "a while"?

The cadence can be anything from 1-4 times per release (~8 weeks).
Depends on the maintainer, how busy we are etc.

> And if the latency can be reduced for these, it'd be nice since it
> would allow for longer bake-time in -next.

Sure but our time is limited, as always :) There's extra overhead with
linux-next, like the rule that no updates during the merge window, so I
can understand why some maintainers have not included their tree to
linux-next builds.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches