2021-07-18 11:35:32

by Len Baker

Subject: [PATCH] staging/rtl8192u: Remove all strcpy() uses in favor of strscpy()

strcpy() performs no bounds checking on the destination buffer. This
could result in linear overflows beyond the end of the buffer, leading
to all kinds of misbehaviors. The safe replacement is strscpy().

Signed-off-by: Len Baker <[email protected]>
---
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
index ab885353f668..1a193f900779 100644
--- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
+++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
@@ -2226,7 +2226,8 @@ static void ieee80211_start_ibss_wq(struct work_struct *work)
mutex_lock(&ieee->wx_mutex);

if (ieee->current_network.ssid_len == 0) {
- strcpy(ieee->current_network.ssid, IEEE80211_DEFAULT_TX_ESSID);
+ strscpy(ieee->current_network.ssid, IEEE80211_DEFAULT_TX_ESSID,
+ sizeof(ieee->current_network.ssid));
ieee->current_network.ssid_len = strlen(IEEE80211_DEFAULT_TX_ESSID);
ieee->ssid_set = 1;
}
--
2.25.1

2021-07-19 09:05:57

by David Laight

Subject: RE: [PATCH] staging/rtl8192u: Remove all strcpy() uses in favor of strscpy()

From: Len Baker
> Sent: 18 July 2021 12:32
>
> strcpy() performs no bounds checking on the destination buffer. This
> could result in linear overflows beyond the end of the buffer, leading
> to all kinds of misbehaviors. The safe replacement is strscpy().
>
> Signed-off-by: Len Baker <[email protected]>
> ---
> drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> index ab885353f668..1a193f900779 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> @@ -2226,7 +2226,8 @@ static void ieee80211_start_ibss_wq(struct work_struct *work)
> mutex_lock(&ieee->wx_mutex);
>
> if (ieee->current_network.ssid_len == 0) {
> - strcpy(ieee->current_network.ssid, IEEE80211_DEFAULT_TX_ESSID);
> + strscpy(ieee->current_network.ssid, IEEE80211_DEFAULT_TX_ESSID,
> + sizeof(ieee->current_network.ssid));

I'm pretty sure that recent compiler releases know enough
about strcpy() to error overflows for strcpy() from quoted
strings into char[].

If these checks are enabled for kernel builds then they are
actually safer than the run-time check above
(which can be mistyped).

The compiler can (it may not) convert the strcpy() into a memcpy()
using the compile-time length of the quoted string.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

2021-07-23 18:18:28

by Len Baker

Subject: Re: [PATCH] staging/rtl8192u: Remove all strcpy() uses in favor of strscpy()

Hi,

On Mon, Jul 19, 2021 at 08:49:54AM +0000, David Laight wrote:
> From: Len Baker
> > Sent: 18 July 2021 12:32
> >
> > strcpy() performs no bounds checking on the destination buffer. This
> > could result in linear overflows beyond the end of the buffer, leading
> > to all kinds of misbehaviors. The safe replacement is strscpy().
> >
> > Signed-off-by: Len Baker <[email protected]>
> > ---
> > drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > index ab885353f668..1a193f900779 100644
> > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c
> > @@ -2226,7 +2226,8 @@ static void ieee80211_start_ibss_wq(struct work_struct *work)
> > mutex_lock(&ieee->wx_mutex);
> >
> > if (ieee->current_network.ssid_len == 0) {
> > - strcpy(ieee->current_network.ssid, IEEE80211_DEFAULT_TX_ESSID);
> > + strscpy(ieee->current_network.ssid, IEEE80211_DEFAULT_TX_ESSID,
> > + sizeof(ieee->current_network.ssid));
>
> I'm pretty sure that recent compiler releases know enough
> about strcpy() to error overflows for strcpy() from quoted
> strings into char[].
>
> If these checks are enabled for kernel builds then they are
> actually safer than the run-time check above
> (which can be mistyped).
>
> The compiler can (it may not) convert the strcpy() into a memcpy()
> using the compile-time length of the quoted string.

I agree, but if we want to remove this function entirely from the kernel [1]
we need to replace all the strcpy() uses as a previous step. And the safe
replacement is strscpy() [2].

[1] https://github.com/KSPP/linux/issues/88
[2] https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy

> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>

Thanks for the feedback,
Len