staging/rtl8192u: Remove all strcpy() uses in favor of strscpy()

PATCH In review Len Baker 2 participants 3 messages Jul 18, 2021 – Jul 23, 2021 Staging

Share & tools

Len Baker

author

2021-07-18 11:32 UTC #0 lore raw

PATCH 1/1 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(-)

drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c +2 -1

index ab885353f668..1a193f900779 100644
@@ -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

David Laight

2021-07-19 08:49 UTC #1 lore raw

From: Len Baker

Quoted 17 lines

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(-)
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c +0 -0
index ab885353f668..1a193f900779 100644

drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c static void ieee80211_start_ibss_wq(struct work_struct *work) · line 2230

Quoted 6 lines

drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c +2 -1

@@ -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)

Len Baker

author

2021-07-23 18:16 UTC #2 lore raw

Hi,

Quoted 29 lines · David Laight

From: Len Baker
Quoted 17 lines
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(-)
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c +0 -0
index ab885353f668..1a193f900779 100644
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c static void ieee80211_start_ibss_wq(struct work_struct *work) · line 2230
Quoted 6 lines
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c +2 -1
@@ -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[].

Quoted 6 lines

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

Quoted 6 lines

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

Thanks for the feedback,
Len