wifi: libertas: use variable-size data in assoc req/resp cmd

Commit Message

Johannes Berg May 23, 2022, 4:02 p.m. UTC

From: Johannes Berg <[e-mail]>

The firmware has a 512 limit here, but we use less, so gcc
starts complaining about it:

drivers/net/wireless/marvell/libertas/cfg.c:1198:63: warning: array subscript ‘struct cmd_ds_802_11_associate_response[0]’ is partly outside array bounds of ‘unsigned char[203]’ [-Warray-bounds]
 1198 |                       "aid 0x%04x\n", status, le16_to_cpu(resp->statuscode),
      |                                                               ^~

Since we size the command and response buffer per our needs
and not per the firmware maximum, change to a variable size
data array and put the 512 only into a comment.

In the end, that's actually what the code always wanted, and
it simplifies the code that used to subtract the fixed size
buffer size in two places.

Reported-by: Jakub Kicinski <[e-mail]>
Signed-off-by: Johannes Berg <[e-mail]>
---
 drivers/net/wireless/marvell/libertas/cfg.c  | 4 +---
 drivers/net/wireless/marvell/libertas/host.h | 6 ++++--
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Jakub Kicinski May 25, 2022, 8:53 p.m. UTC | #1

On Mon, 23 May 2022 18:02:01 +0200 Johannes Berg wrote:
> From: Johannes Berg <[e-mail]>
> 
> The firmware has a 512 limit here, but we use less, so gcc
> starts complaining about it:
> 
> drivers/net/wireless/marvell/libertas/cfg.c:1198:63: warning: array subscript ‘struct cmd_ds_802_11_associate_response[0]’ is partly outside array bounds of ‘unsigned char[203]’ [-Warray-bounds]
>  1198 |                       "aid 0x%04x\n", status, le16_to_cpu(resp->statuscode),
>       |                                                               ^~
> 
> Since we size the command and response buffer per our needs
> and not per the firmware maximum, change to a variable size
> data array and put the 512 only into a comment.
> 
> In the end, that's actually what the code always wanted, and
> it simplifies the code that used to subtract the fixed size
> buffer size in two places.
> 
> Reported-by: Jakub Kicinski <[e-mail]>
> Signed-off-by: Johannes Berg <[e-mail]>

Is there a chance to get this into net before the merge window is over?

Johannes Berg May 25, 2022, 9:18 p.m. UTC | #2

On Wed, 2022-05-25 at 13:53 -0700, Jakub Kicinski wrote:
> On Mon, 23 May 2022 18:02:01 +0200 Johannes Berg wrote:
> > From: Johannes Berg <[e-mail]>
> > 
> > The firmware has a 512 limit here, but we use less, so gcc
> > starts complaining about it:
> > 
> > drivers/net/wireless/marvell/libertas/cfg.c:1198:63: warning: array subscript ‘struct cmd_ds_802_11_associate_response[0]’ is partly outside array bounds of ‘unsigned char[203]’ [-Warray-bounds]
> >  1198 |                       "aid 0x%04x\n", status, le16_to_cpu(resp->statuscode),
> >       |                                                               ^~
> > 
> > Since we size the command and response buffer per our needs
> > and not per the firmware maximum, change to a variable size
> > data array and put the 512 only into a comment.
> > 
> > In the end, that's actually what the code always wanted, and
> > it simplifies the code that used to subtract the fixed size
> > buffer size in two places.
> > 
> > Reported-by: Jakub Kicinski <[e-mail]>
> > Signed-off-by: Johannes Berg <[e-mail]>
> 
> Is there a chance to get this into net before the merge window is over?
> 
I guess you can just apply it. Kalle?

johannes

Kalle Valo May 27, 2022, 11:40 a.m. UTC | #3

Johannes Berg <[e-mail]> writes:

> On Wed, 2022-05-25 at 13:53 -0700, Jakub Kicinski wrote:
>> On Mon, 23 May 2022 18:02:01 +0200 Johannes Berg wrote:
>> > From: Johannes Berg <[e-mail]>
>> > 
>> > The firmware has a 512 limit here, but we use less, so gcc
>> > starts complaining about it:
>> > 
>> > drivers/net/wireless/marvell/libertas/cfg.c:1198:63: warning: array subscript ‘struct cmd_ds_802_11_associate_response[0]’ is partly outside array bounds of ‘unsigned char[203]’ [-Warray-bounds]
>> >  1198 |                       "aid 0x%04x\n", status, le16_to_cpu(resp->statuscode),
>> >       |                                                               ^~
>> > 
>> > Since we size the command and response buffer per our needs
>> > and not per the firmware maximum, change to a variable size
>> > data array and put the 512 only into a comment.
>> > 
>> > In the end, that's actually what the code always wanted, and
>> > it simplifies the code that used to subtract the fixed size
>> > buffer size in two places.
>> > 
>> > Reported-by: Jakub Kicinski <[e-mail]>
>> > Signed-off-by: Johannes Berg <[e-mail]>
>> 
>> Is there a chance to get this into net before the merge window is over?
>> 
> I guess you can just apply it. Kalle?

Yeah, Jakub can take this directly to net tree:

Acked-by: Kalle Valo <[e-mail]>

But I can also take this to wireless tree and send a pull request on
Wednesday. Whichever is better for you, just let me know.

Kalle Valo May 27, 2022, 11:43 a.m. UTC | #4

Johannes Berg <[e-mail]> writes:

> From: Johannes Berg <[e-mail]>
>
> The firmware has a 512 limit here, but we use less, so gcc
> starts complaining about it:
>
> drivers/net/wireless/marvell/libertas/cfg.c:1198:63: warning: array subscript ‘struct cmd_ds_802_11_associate_response[0]’ is partly outside array bounds of ‘unsigned char[203]’ [-Warray-bounds]
>  1198 |                       "aid 0x%04x\n", status, le16_to_cpu(resp->statuscode),
>       |                                                               ^~
>
> Since we size the command and response buffer per our needs
> and not per the firmware maximum, change to a variable size
> data array and put the 512 only into a comment.
>
> In the end, that's actually what the code always wanted, and
> it simplifies the code that used to subtract the fixed size
> buffer size in two places.
>
> Reported-by: Jakub Kicinski <[e-mail]>
> Signed-off-by: Johannes Berg <[e-mail]>

Can we now remove the no-array-bounds hack from libertas?

+# FIXME: temporarily silence -Warray-bounds on non W=1+ builds
+ifndef KBUILD_EXTRA_WARN
+CFLAGS_cfg.o += -Wno-array-bounds
+endif

Johannes Berg May 27, 2022, 11:44 a.m. UTC | #5

On Fri, 2022-05-27 at 14:43 +0300, Kalle Valo wrote:
> Johannes Berg <[e-mail]> writes:
> 
> > From: Johannes Berg <[e-mail]>
> > 
> > The firmware has a 512 limit here, but we use less, so gcc
> > starts complaining about it:
> > 
> > drivers/net/wireless/marvell/libertas/cfg.c:1198:63: warning: array subscript ‘struct cmd_ds_802_11_associate_response[0]’ is partly outside array bounds of ‘unsigned char[203]’ [-Warray-bounds]
> >  1198 |                       "aid 0x%04x\n", status, le16_to_cpu(resp->statuscode),
> >       |                                                               ^~
> > 
> > Since we size the command and response buffer per our needs
> > and not per the firmware maximum, change to a variable size
> > data array and put the 512 only into a comment.
> > 
> > In the end, that's actually what the code always wanted, and
> > it simplifies the code that used to subtract the fixed size
> > buffer size in two places.
> > 
> > Reported-by: Jakub Kicinski <[e-mail]>
> > Signed-off-by: Johannes Berg <[e-mail]>
> 
> Can we now remove the no-array-bounds hack from libertas?
> 

I think Jakub said he dropped it from the patches?

johannes

Kalle Valo May 27, 2022, 12:02 p.m. UTC | #6

Johannes Berg <[e-mail]> writes:

> On Fri, 2022-05-27 at 14:43 +0300, Kalle Valo wrote:
>> Johannes Berg <[e-mail]> writes:
>> 
>> > From: Johannes Berg <[e-mail]>
>> > 
>> > The firmware has a 512 limit here, but we use less, so gcc
>> > starts complaining about it:
>> > 
>> > drivers/net/wireless/marvell/libertas/cfg.c:1198:63: warning: array subscript ‘struct cmd_ds_802_11_associate_response[0]’ is partly outside array bounds of ‘unsigned char[203]’ [-Warray-bounds]
>> >  1198 |                       "aid 0x%04x\n", status, le16_to_cpu(resp->statuscode),
>> >       |                                                               ^~
>> > 
>> > Since we size the command and response buffer per our needs
>> > and not per the firmware maximum, change to a variable size
>> > data array and put the 512 only into a comment.
>> > 
>> > In the end, that's actually what the code always wanted, and
>> > it simplifies the code that used to subtract the fixed size
>> > buffer size in two places.
>> > 
>> > Reported-by: Jakub Kicinski <[e-mail]>
>> > Signed-off-by: Johannes Berg <[e-mail]>
>> 
>> Can we now remove the no-array-bounds hack from libertas?
>> 
>
> I think Jakub said he dropped it from the patches?

Ah, indeed. I missed that, sorry for the noise.

Jakub Kicinski May 27, 2022, 10:36 p.m. UTC | #7

On Fri, 27 May 2022 14:40:50 +0300 Kalle Valo wrote:
> >> Is there a chance to get this into net before the merge window is over?
> >>   
> > I guess you can just apply it. Kalle?  
> 
> Yeah, Jakub can take this directly to net tree:
> 
> Acked-by: Kalle Valo <[e-mail]>
> 
> But I can also take this to wireless tree and send a pull request on
> Wednesday. Whichever is better for you, just let me know.

PR on Wednesday sounds good, thanks! The patch didn't end up in our PW
instance, I'd have to apply it manually.

Kalle Valo May 30, 2022, 9:13 a.m. UTC | #8

Johannes Berg <[e-mail]> wrote:

> From: Johannes Berg <[e-mail]>
> 
> The firmware has a 512 limit here, but we use less, so gcc
> starts complaining about it:
> 
> drivers/net/wireless/marvell/libertas/cfg.c:1198:63: warning: array subscript ‘struct cmd_ds_802_11_associate_response[0]’ is partly outside array bounds of ‘unsigned char[203]’ [-Warray-bounds]
>  1198 |                       "aid 0x%04x\n", status, le16_to_cpu(resp->statuscode),
>       |                                                               ^~
> 
> Since we size the command and response buffer per our needs
> and not per the firmware maximum, change to a variable size
> data array and put the 512 only into a comment.
> 
> In the end, that's actually what the code always wanted, and
> it simplifies the code that used to subtract the fixed size
> buffer size in two places.
> 
> Reported-by: Jakub Kicinski <[e-mail]>
> Signed-off-by: Johannes Berg <[e-mail]>
> Acked-by: Kalle Valo <[e-mail]>

Patch applied to wireless.git, thanks.

d944e09ea839 wifi: libertas: use variable-size data in assoc req/resp cmd

Patch

diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c
index 4e3de684928b..b0b3f59dabc6 100644
--- a/drivers/net/wireless/marvell/libertas/cfg.c
+++ b/drivers/net/wireless/marvell/libertas/cfg.c
@@ -1053,7 +1053,6 @@  static int lbs_set_authtype(struct lbs_private *priv,
  */
 #define LBS_ASSOC_MAX_CMD_SIZE                     \
 	(sizeof(struct cmd_ds_802_11_associate)    \
-	 - 512 /* cmd_ds_802_11_associate.iebuf */ \
 	 + LBS_MAX_SSID_TLV_SIZE                   \
 	 + LBS_MAX_CHANNEL_TLV_SIZE                \
 	 + LBS_MAX_CF_PARAM_TLV_SIZE               \
@@ -1130,8 +1129,7 @@  static int lbs_associate(struct lbs_private *priv,
 	if (sme->ie && sme->ie_len)
 		pos += lbs_add_wpa_tlv(pos, sme->ie, sme->ie_len);
 
-	len = (sizeof(*cmd) - sizeof(cmd->iebuf)) +
-		(u16)(pos - (u8 *) &cmd->iebuf);
+	len = sizeof(*cmd) + (u16)(pos - (u8 *) &cmd->iebuf);
 	cmd->hdr.size = cpu_to_le16(len);
 
 	lbs_deb_hex(LBS_DEB_ASSOC, "ASSOC_CMD", (u8 *) cmd,
diff --git a/drivers/net/wireless/marvell/libertas/host.h b/drivers/net/wireless/marvell/libertas/host.h
index ceff4b92e7a1..a202b716ad5d 100644
--- a/drivers/net/wireless/marvell/libertas/host.h
+++ b/drivers/net/wireless/marvell/libertas/host.h
@@ -528,7 +528,8 @@  struct cmd_ds_802_11_associate {
 	__le16 listeninterval;
 	__le16 bcnperiod;
 	u8 dtimperiod;
-	u8 iebuf[512];    /* Enough for required and most optional IEs */
+	/* 512 permitted - enough for required and most optional IEs */
+	u8 iebuf[];
 } __packed;
 
 struct cmd_ds_802_11_associate_response {
@@ -537,7 +538,8 @@  struct cmd_ds_802_11_associate_response {
 	__le16 capability;
 	__le16 statuscode;
 	__le16 aid;
-	u8 iebuf[512];
+	/* max 512 */
+	u8 iebuf[];
 } __packed;
 
 struct cmd_ds_802_11_set_wep {