[PATCH v3] staging: rtl8723bs: stop overreading monitor interface name

[PATCH v3] staging: rtl8723bs: stop overreading monitor interface name

[Amirhossein Hajimohammadi]

From: AmirHossein HajiMohammadi <info@hajimohammadi.net>

cfg80211 passes a NUL-terminated interface name. memcpy() always reads
IFNAMSIZ + 1 bytes, even when the caller supplied a shorter string.

Use strscpy() to stop at the terminator and bound the write to the
destination buffer.

Signed-off-by: AmirHossein HajiMohammadi <info@hajimohammadi.net>
---
Changes in v3:
- Add an explicit From header matching the Signed-off-by identity.

Changes in v2:
- Send the patch inline instead of as a MIME attachment.

 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 4416d0ec1..8f5032844 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -2158,7 +2158,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
 		goto out;
 
 	*ndev = pwdev_priv->pmon_ndev = mon_ndev;
-	memcpy(pwdev_priv->ifname_mon, name, IFNAMSIZ + 1);
+	strscpy(pwdev_priv->ifname_mon, name);
 
 out:
 	if (ret && mon_wdev) {
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v3] staging: rtl8723bs: stop overreading monitor interface name

[Greg KH]

On Tue, Sep 29, 2026 at 07:18:11PM +0330, Amirhossein Hajimohammadi wrote:
> From: AmirHossein HajiMohammadi <info@hajimohammadi.net>
> 
> cfg80211 passes a NUL-terminated interface name. memcpy() always reads
> IFNAMSIZ + 1 bytes, even when the caller supplied a shorter string.
> 
> Use strscpy() to stop at the terminator and bound the write to the
> destination buffer.
> 
> Signed-off-by: AmirHossein HajiMohammadi <info@hajimohammadi.net>
> ---
> Changes in v3:
> - Add an explicit From header matching the Signed-off-by identity.
> 
> Changes in v2:
> - Send the patch inline instead of as a MIME attachment.
> 
>  drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> index 4416d0ec1..8f5032844 100644
> --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> @@ -2158,7 +2158,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
>  		goto out;
>  
>  	*ndev = pwdev_priv->pmon_ndev = mon_ndev;
> -	memcpy(pwdev_priv->ifname_mon, name, IFNAMSIZ + 1);
> +	strscpy(pwdev_priv->ifname_mon, name);

But we are replacing str*() calls in the kernel with memcpy() calls
where it can happen, so why go backwards here?

What caused you to notice this change is needed?  Have you measured a
speed up in throughput with this change applied?  How was it tested?

thanks,

greg k-h

Re: [PATCH v3] staging: rtl8723bs: stop overreading monitor interface name

[Amirhossein Hajimohammadi]

You are right. I noticed this during manual source inspection and
incorrectly treated the fixed-size copy as something that should use a
string helper.

I did not measure any throughput improvement or test the change on
hardware. The original patch was only checked with checkpatch and by
applying the received email, so it is not sufficiently justified. Please
drop it.

On rechecking the code, ifname_mon is written and cleared but never read.
The appropriate cleanup is to remove the unused field and its dead stores
instead. I have compile-tested that cleanup with W=1 against staging-next
and will send it separately.

Sorry for the noise, and thanks for the review.

Re: [PATCH v3] staging: rtl8723bs: stop overreading monitor interface name

[Dan Carpenter]

On Tue, Sep 29, 2026 at 06:05:12PM +0200, Greg KH wrote:
> On Tue, Sep 29, 2026 at 07:18:11PM +0330, Amirhossein Hajimohammadi wrote:
> > From: AmirHossein HajiMohammadi <info@hajimohammadi.net>
> > 
> > cfg80211 passes a NUL-terminated interface name. memcpy() always reads
> > IFNAMSIZ + 1 bytes, even when the caller supplied a shorter string.
> > 
> > Use strscpy() to stop at the terminator and bound the write to the
> > destination buffer.
> > 
> > Signed-off-by: AmirHossein HajiMohammadi <info@hajimohammadi.net>
> > ---
> > Changes in v3:
> > - Add an explicit From header matching the Signed-off-by identity.
> > 
> > Changes in v2:
> > - Send the patch inline instead of as a MIME attachment.
> > 
> >  drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > index 4416d0ec1..8f5032844 100644
> > --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > @@ -2158,7 +2158,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
> >  		goto out;
> >  
> >  	*ndev = pwdev_priv->pmon_ndev = mon_ndev;
> > -	memcpy(pwdev_priv->ifname_mon, name, IFNAMSIZ + 1);
> > +	strscpy(pwdev_priv->ifname_mon, name);
> 
> But we are replacing str*() calls in the kernel with memcpy() calls
> where it can happen, so why go backwards here?
> 
> What caused you to notice this change is needed?  Have you measured a
> speed up in throughput with this change applied?  How was it tested?

This seems like a legit patch to me, although it's probably AI generated.

net/wireless/nl80211.c
  5199          wdev = rdev_add_virtual_intf(rdev,
  5200                                  nla_data(info->attrs[NL80211_ATTR_IFNAME]),
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This means that it is a string up to IFNAMSIZ (16) bytes long counting
the NUL terminator, but it could be less.  So doing a memcpy() is a
read overflow.

  5201                                  NET_NAME_USER, type, &params);

regards,
dan carpenter

Re: [PATCH v3] staging: rtl8723bs: stop overreading monitor interface name

[Dan Carpenter]

On Thu, Oct 01, 2026 at 12:59:59PM +0300, Dan Carpenter wrote:
> > > diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > > index 4416d0ec1..8f5032844 100644
> > > --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > > +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > > @@ -2158,7 +2158,7 @@ static int rtw_cfg80211_add_monitor_if(struct adapter *padapter, char *name, str
> > >  		goto out;
> > >  
> > >  	*ndev = pwdev_priv->pmon_ndev = mon_ndev;
> > > -	memcpy(pwdev_priv->ifname_mon, name, IFNAMSIZ + 1);
> > > +	strscpy(pwdev_priv->ifname_mon, name);
> > 
> > But we are replacing str*() calls in the kernel with memcpy() calls
> > where it can happen, so why go backwards here?
> > 
> > What caused you to notice this change is needed?  Have you measured a
> > speed up in throughput with this change applied?  How was it tested?
> 
> This seems like a legit patch to me, although it's probably AI generated.
> 
> net/wireless/nl80211.c
>   5199          wdev = rdev_add_virtual_intf(rdev,
>   5200                                  nla_data(info->attrs[NL80211_ATTR_IFNAME]),
>                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This means that it is a string up to IFNAMSIZ (16) bytes long counting
> the NUL terminator, but it could be less.  So doing a memcpy() is a
> read overflow.
> 
>   5201                                  NET_NAME_USER, type, &params);
> 

I tasked ChatGPT with writing a Smatch check for this and it noticed
that the "+ 1" in "IFNAMSIZ + 1" is wrong as well.  "name" can only
be 16 characters long (counting the NUL terminator).

drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c:2164 rtw_cfg80211_add_monitor_if()
warn: buffer 'name' too small user_len=1-16 for 17 byte copy

regards,
dan carpenter