[PATCH v6] staging: rtl8723bs: fix heap buffer overflow in cfg80211_rtw_add_key()

[PATCH v6] staging: rtl8723bs: fix heap buffer overflow in cfg80211_rtw_add_key()

[Feng Ning]

The cfg80211 framework allows userspace to specify a key sequence
counter (NL80211_KEY_SEQ) of up to 16 bytes via NL80211_CMD_NEW_KEY
netlink messages, but ieee_param.crypt.seq is a fixed 8-byte buffer.
When cfg80211_rtw_add_key() copies the sequence counter via memcpy()
without checking seq_len, a heap buffer overflow of up to 8 bytes
occurs, overwriting bytes following seq within the same ieee_param
structure (key_len and the trailing key[] flexible array).

Cap the copy length at the buffer size using min_t().

Reviewed-by: Luka Gejak <luka.gejak@linux.dev>
Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
Cc: stable@vger.kernel.org
Signed-off-by: Feng Ning <feng@innora.ai>
---
Changes v5 -> v6:
  - Restore the changelog that was lost in v5 (per Greg KH feedback)
  - Add Cc: stable@vger.kernel.org (user-reachable heap overflow)
  - Tighten wording about which fields are clobbered (avoid implying
    a specific layout when padding may exist)
  - No code changes from v5; v4..v6 are byte-identical at the code
    level, so Luka Gejak's Reviewed-by from v4 is carried over

Changes v4 -> v5:
  - Rebase onto staging-next (line numbers and surrounding hashes
    refreshed; the hunk itself is unchanged)
  - No code changes from v4

Changes v3 -> v4:
  - Resend as plain text without PGP signature and public-key
    attachment (per Luka Gejak feedback)
  - No code changes from v3

Changes v2 -> v3:
  - Move the changelog below the cut line (per gregkh patch-bot)
  - No code changes from v2

Changes v1 -> v2:
  - Reformat as a proper kernel patch with Fixes: tag and
    Signed-off-by; address comments from the initial submission
  - No code changes from v1

 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 7cb0c6f22..4fba53c2d 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -883,8 +883,11 @@ static int cfg80211_rtw_add_key(struct wiphy *wiphy, struct net_device *ndev,

 	param->u.crypt.idx = key_index;

-	if (params->seq_len && params->seq)
-		memcpy(param->u.crypt.seq, (u8 *)params->seq, params->seq_len);
+	if (params->seq_len && params->seq) {
+		size_t seq_copy = min_t(size_t, params->seq_len,
+				       sizeof(param->u.crypt.seq));
+		memcpy(param->u.crypt.seq, (u8 *)params->seq, seq_copy);
+	}

 	if (params->key_len && params->key) {
 		param->u.crypt.key_len = params->key_len;
--
2.43.0

Re: [PATCH v6] staging: rtl8723bs: fix heap buffer overflow in cfg80211_rtw_add_key()

[Greg KH]

On Mon, Apr 27, 2026 at 11:17:45AM +0000, Feng Ning wrote:
> The cfg80211 framework allows userspace to specify a key sequence
> counter (NL80211_KEY_SEQ) of up to 16 bytes via NL80211_CMD_NEW_KEY
> netlink messages, but ieee_param.crypt.seq is a fixed 8-byte buffer.
> When cfg80211_rtw_add_key() copies the sequence counter via memcpy()
> without checking seq_len, a heap buffer overflow of up to 8 bytes
> occurs, overwriting bytes following seq within the same ieee_param
> structure (key_len and the trailing key[] flexible array).
> 
> Cap the copy length at the buffer size using min_t().
> 
> Reviewed-by: Luka Gejak <luka.gejak@linux.dev>
> Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Feng Ning <feng@innora.ai>
> ---

What about these review comments:
	https://sashiko.dev/#/patchset/20260427111738.33069-1-feng@innora.ai

Are they incorrect?

And was this tested on real hardware?

thanks,

greg k-h

Re: [PATCH v6] staging: rtl8723bs: fix heap buffer overflow in cfg80211_rtw_add_key()

[Feng Ning]

On Mon, May 04, 2026 at 04:12:44PM +0200, Greg KH wrote:
> What about these review comments:
>         https://sashiko.dev/#/patchset/20260427111738.33069-1-feng@innora.ai
>
> Are they incorrect?
>
> And was this tested on real hardware?

Hi Greg,

Thank you for the pointer to the Sashiko review.

Regarding the review comment (Medium): Sashiko suggests returning -EINVAL
when params->seq_len exceeds sizeof(param->u.crypt.seq), rather than
silently truncating with min_t().

The comment raises a valid point.  I chose min_t() for two reasons:

  1. The upstream cfg80211 framework does not enforce an upper bound on
     seq_len before reaching the driver, so a strict -EINVAL could
     break any existing userspace that happens to pass seq_len > 8
     (even if no standard cipher requires more than 6 bytes).

  2. Staging drivers historically favour silent clamping over hard
     rejections for parameters that are out of the ordinary but
     otherwise harmless -- the primary goal was to close the overflow,
     not to police the caller.

That said, I can see the argument for -EINVAL: it makes the contract
explicit and avoids installing a key with a truncated sequence counter
that could produce unexpected crypto behaviour.

I am happy to send v7 with -EINVAL if you prefer that approach.
Alternatively, if min_t() is acceptable as-is, I can add a brief
comment in the code explaining why truncation is intentional.

Please let me know which direction you prefer and I will follow up
promptly.

Regarding hardware testing: I do not currently have a physical
rtl8723bs device.  My verification was based on code review of the
cfg80211 key installation path and static analysis confirming that
ieee_param.crypt.seq is an 8-byte fixed buffer while params->seq_len
is fully userspace-controlled via NL80211_CMD_NEW_KEY.

I understand this is a limitation.  If hardware testing is required
before merge I can source a RTL8723BU/BS USB dongle (approximately
1-2 weeks), or alternatively a community member with the hardware
could confirm the fix.  Please advise on your preference.

Thanks,
Feng Ning

Re: [PATCH v6] staging: rtl8723bs: fix heap buffer overflow in cfg80211_rtw_add_key()

[Greg KH]

On Mon, May 04, 2026 at 03:48:30PM +0000, Feng Ning wrote:
> On Mon, May 04, 2026 at 04:12:44PM +0200, Greg KH wrote:
> > What about these review comments:
> >         https://sashiko.dev/#/patchset/20260427111738.33069-1-feng@innora.ai
> >
> > Are they incorrect?
> >
> > And was this tested on real hardware?
> 
> Hi Greg,
> 
> Thank you for the pointer to the Sashiko review.
> 
> Regarding the review comment (Medium): Sashiko suggests returning -EINVAL
> when params->seq_len exceeds sizeof(param->u.crypt.seq), rather than
> silently truncating with min_t().
> 
> The comment raises a valid point.  I chose min_t() for two reasons:
> 
>   1. The upstream cfg80211 framework does not enforce an upper bound on
>      seq_len before reaching the driver, so a strict -EINVAL could
>      break any existing userspace that happens to pass seq_len > 8
>      (even if no standard cipher requires more than 6 bytes).
> 
>   2. Staging drivers historically favour silent clamping over hard
>      rejections for parameters that are out of the ordinary but
>      otherwise harmless -- the primary goal was to close the overflow,
>      not to police the caller.

Let's fix this in a way that the code can be moved out of staging
someday please.

> That said, I can see the argument for -EINVAL: it makes the contract
> explicit and avoids installing a key with a truncated sequence counter
> that could produce unexpected crypto behaviour.

Yes, that is better.

> Regarding hardware testing: I do not currently have a physical
> rtl8723bs device.  My verification was based on code review of the
> cfg80211 key installation path and static analysis confirming that
> ieee_param.crypt.seq is an 8-byte fixed buffer while params->seq_len
> is fully userspace-controlled via NL80211_CMD_NEW_KEY.
> 
> I understand this is a limitation.  If hardware testing is required
> before merge I can source a RTL8723BU/BS USB dongle (approximately
> 1-2 weeks), or alternatively a community member with the hardware
> could confirm the fix.  Please advise on your preference.

Ideally someone can test this on the real hardware.  I'm loath to take
real patches for this driver without that happening.

thanks,

greg k-h

Re: [PATCH v6] staging: rtl8723bs: fix heap buffer overflow in cfg80211_rtw_add_key()

[Feng Ning]

On Mon, May 04, 2026 at 06:03:02PM +0200, Greg KH wrote:
> Let's fix this in a way that the code can be moved out of staging
> someday please.
>
> > That said, I can see the argument for -EINVAL: it makes the contract
> > explicit and avoids installing a key with a truncated sequence counter
> > that could produce unexpected crypto behaviour.
>
> Yes, that is better.
>
> > Regarding hardware testing: I do not currently have a physical
> > rtl8723bs device.
>
> Ideally someone can test this on the real hardware.  I'm loath to take
> real patches for this driver without that happening.

Hi Greg,

Thank you.  I will change the silent truncation to an explicit -EINVAL
when seq_len > sizeof(param->u.crypt.seq) for the next iteration.

Regarding testing: I do not have access to RTL8723BS/BU hardware to
verify this, and I will not resubmit as a regular PATCH without a
Tested-by from real hardware.

Would you prefer I send the -EINVAL revision as an RFC on
linux-staging and linux-wireless to ask for a community tester, or
should I drop the patch until someone with the hardware picks up the
thread?

I'm fine with either path -- whichever you prefer.

thanks,
Feng Ning

Re: [PATCH v6] staging: rtl8723bs: fix heap buffer overflow in cfg80211_rtw_add_key()

[Greg KH]

On Mon, May 04, 2026 at 04:38:35PM +0000, Feng Ning wrote:
> On Mon, May 04, 2026 at 06:03:02PM +0200, Greg KH wrote:
> > Let's fix this in a way that the code can be moved out of staging
> > someday please.
> >
> > > That said, I can see the argument for -EINVAL: it makes the contract
> > > explicit and avoids installing a key with a truncated sequence counter
> > > that could produce unexpected crypto behaviour.
> >
> > Yes, that is better.
> >
> > > Regarding hardware testing: I do not currently have a physical
> > > rtl8723bs device.
> >
> > Ideally someone can test this on the real hardware.  I'm loath to take
> > real patches for this driver without that happening.
> 
> Hi Greg,
> 
> Thank you.  I will change the silent truncation to an explicit -EINVAL
> when seq_len > sizeof(param->u.crypt.seq) for the next iteration.
> 
> Regarding testing: I do not have access to RTL8723BS/BU hardware to
> verify this, and I will not resubmit as a regular PATCH without a
> Tested-by from real hardware.
> 
> Would you prefer I send the -EINVAL revision as an RFC on
> linux-staging and linux-wireless to ask for a community tester, or
> should I drop the patch until someone with the hardware picks up the
> thread?

Submit the patch and ask for someone to test it.  I think Luka here said
they were getting a device, and I might have one somewhere around here
as well if I dig hard enough...

thanks,

greg k-h

Re: [PATCH v6] staging: rtl8723bs: fix heap buffer overflow in cfg80211_rtw_add_key()

[Luka Gejak]

On May 4, 2026 7:01:00 PM GMT+02:00, Greg KH <gregkh@linuxfoundation.org> wrote:
>On Mon, May 04, 2026 at 04:38:35PM +0000, Feng Ning wrote:
>> On Mon, May 04, 2026 at 06:03:02PM +0200, Greg KH wrote:
>> > Let's fix this in a way that the code can be moved out of staging
>> > someday please.
>> >
>> > > That said, I can see the argument for -EINVAL: it makes the contract
>> > > explicit and avoids installing a key with a truncated sequence counter
>> > > that could produce unexpected crypto behaviour.
>> >
>> > Yes, that is better.
>> >
>> > > Regarding hardware testing: I do not currently have a physical
>> > > rtl8723bs device.
>> >
>> > Ideally someone can test this on the real hardware.  I'm loath to take
>> > real patches for this driver without that happening.
>> 
>> Hi Greg,
>> 
>> Thank you.  I will change the silent truncation to an explicit -EINVAL
>> when seq_len > sizeof(param->u.crypt.seq) for the next iteration.
>> 
>> Regarding testing: I do not have access to RTL8723BS/BU hardware to
>> verify this, and I will not resubmit as a regular PATCH without a
>> Tested-by from real hardware.
>> 
>> Would you prefer I send the -EINVAL revision as an RFC on
>> linux-staging and linux-wireless to ask for a community tester, or
>> should I drop the patch until someone with the hardware picks up the
>> thread?
>
>Submit the patch and ask for someone to test it.  I think Luka here said
>they were getting a device, and I might have one somewhere around here
>as well if I dig hard enough...
>
>thanks,
>
>greg k-h

Hi Greg,
my hardware (medion akoya s2218 laptop) is currently on its way from 
Germany and should arrive in approximetely 10-14 days (approximation 
is based on time that it took other orders to arrive from same area).
Once it arrives I would be happy to serve as a tester if necessary.
Best regards,
Luka Gejak

Re: [PATCH v6] staging: rtl8723bs: fix heap buffer overflow in cfg80211_rtw_add_key()

[Luka Gejak]

On May 4, 2026 4:12:44 PM GMT+02:00, Greg KH <gregkh@linuxfoundation.org> wrote:
>On Mon, Apr 27, 2026 at 11:17:45AM +0000, Feng Ning wrote:
>> The cfg80211 framework allows userspace to specify a key sequence
>> counter (NL80211_KEY_SEQ) of up to 16 bytes via NL80211_CMD_NEW_KEY
>> netlink messages, but ieee_param.crypt.seq is a fixed 8-byte buffer.
>> When cfg80211_rtw_add_key() copies the sequence counter via memcpy()
>> without checking seq_len, a heap buffer overflow of up to 8 bytes
>> occurs, overwriting bytes following seq within the same ieee_param
>> structure (key_len and the trailing key[] flexible array).
>> 
>> Cap the copy length at the buffer size using min_t().
>> 
>> Reviewed-by: Luka Gejak <luka.gejak@linux.dev>
>> Fixes: 554c0a3abf21 ("staging: Add rtl8723bs sdio wifi driver")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Feng Ning <feng@innora.ai>
>> ---
>
>What about these review comments:
>	https://sashiko.dev/#/patchset/20260427111738.33069-1-feng@innora.ai
>
>Are they incorrect?
>
>And was this tested on real hardware?
>
>thanks,
>
>greg k-h

Hi Greg,
Is it better to let the driver attempt to function with a truncated 
key sequence (via min_t), or should we explicitly reject the request 
with -EINVAL to ensure we aren't installing a technically "broken" key
configuration? Which approach is more aligned with your preferences?
Best regards,
Luka Gejak