* [PATCH 1/3] staging: rtl8723bs: core: Replace sprintf with scnprintf
2021-03-01 13:12 [PATCH 0/3] staging: rtl8723bs: Replace sprintf with scnprintf Candy Febriyanto
@ 2021-03-01 13:12 ` Candy Febriyanto
2021-03-01 13:13 ` [PATCH 2/3] staging: rtl8723bs: hal: " Candy Febriyanto
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Candy Febriyanto @ 2021-03-01 13:12 UTC (permalink / raw)
To: gregkh, hdegoede, Larry.Finger, devel
The use of sprintf with format string here means that there is a risk
that the writes will go out of bounds, replace it with scnprintf.
In on_action_public_default the variable "cnt" isn't being used for
anything meaningful so remove it.
Signed-off-by: Candy Febriyanto <cfebriyanto@gmail.com>
---
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 3 +--
drivers/staging/rtl8723bs/core/rtw_pwrctrl.c | 4 ++--
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index fa4b0259c5ae..3443a5764c50 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -2084,7 +2084,6 @@ static unsigned int on_action_public_default(union recv_frame *precv_frame, u8 a
u8 *frame_body = pframe + sizeof(struct ieee80211_hdr_3addr);
u8 token;
struct adapter *adapter = precv_frame->u.hdr.adapter;
- int cnt = 0;
char msg[64];
token = frame_body[2];
@@ -2092,7 +2091,7 @@ static unsigned int on_action_public_default(union recv_frame *precv_frame, u8 a
if (rtw_action_public_decache(precv_frame, token) == _FAIL)
goto exit;
- cnt += sprintf((msg+cnt), "%s(token:%u)", action_public_str(action), token);
+ scnprintf(msg, sizeof(msg), "%s(token:%u)", action_public_str(action), token);
rtw_cfg80211_rx_action(adapter, pframe, frame_len, msg);
ret = _SUCCESS;
diff --git a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
index 5b05d1eaa328..c9f4a18b24b9 100644
--- a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
+++ b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
@@ -554,7 +554,7 @@ void LPS_Enter(struct adapter *padapter, const char *msg)
/* Idle for a while if we connect to AP a while ago. */
if (pwrpriv->LpsIdleCount >= 2) { /* 4 Sec */
if (pwrpriv->pwr_mode == PS_MODE_ACTIVE) {
- sprintf(buf, "WIFI-%s", msg);
+ scnprintf(buf, sizeof(buf), "WIFI-%s", msg);
pwrpriv->bpower_saving = true;
rtw_set_ps_mode(padapter, pwrpriv->power_mgnt, padapter->registrypriv.smart_ps, 0, buf);
}
@@ -584,7 +584,7 @@ void LPS_Leave(struct adapter *padapter, const char *msg)
if (pwrpriv->bLeisurePs) {
if (pwrpriv->pwr_mode != PS_MODE_ACTIVE) {
- sprintf(buf, "WIFI-%s", msg);
+ scnprintf(buf, sizeof(buf), "WIFI-%s", msg);
rtw_set_ps_mode(padapter, PS_MODE_ACTIVE, 0, 0, buf);
if (pwrpriv->pwr_mode == PS_MODE_ACTIVE)
--
2.30.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/3] staging: rtl8723bs: hal: Replace sprintf with scnprintf
2021-03-01 13:12 [PATCH 0/3] staging: rtl8723bs: Replace sprintf with scnprintf Candy Febriyanto
2021-03-01 13:12 ` [PATCH 1/3] staging: rtl8723bs: core: " Candy Febriyanto
@ 2021-03-01 13:13 ` Candy Febriyanto
2021-03-01 13:13 ` [PATCH 3/3] staging: rtl8723bs: os_dep: " Candy Febriyanto
2021-03-01 13:45 ` [PATCH 0/3] staging: rtl8723bs: " Hans de Goede
3 siblings, 0 replies; 8+ messages in thread
From: Candy Febriyanto @ 2021-03-01 13:13 UTC (permalink / raw)
To: gregkh, hdegoede, Larry.Finger, devel
The use of sprintf with format string here means that there is a risk
that the writes will go out of bounds, replace it with scnprintf.
Also avoid unnecessarily passing "%s" on some of the function calls.
Signed-off-by: Candy Febriyanto <cfebriyanto@gmail.com>
---
drivers/staging/rtl8723bs/hal/hal_com.c | 45 +++++++++++++------------
1 file changed, 24 insertions(+), 21 deletions(-)
diff --git a/drivers/staging/rtl8723bs/hal/hal_com.c b/drivers/staging/rtl8723bs/hal/hal_com.c
index 16b259acbe1a..173ccaba2537 100644
--- a/drivers/staging/rtl8723bs/hal/hal_com.c
+++ b/drivers/staging/rtl8723bs/hal/hal_com.c
@@ -40,47 +40,50 @@ void rtw_hal_data_deinit(struct adapter *padapter)
void dump_chip_info(HAL_VERSION ChipVersion)
{
- int cnt = 0;
- u8 buf[128];
+ char buf[128];
+ size_t cnt = 0;
+
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "Chip Version Info: CHIP_8723B_%s_",
+ IS_NORMAL_CHIP(ChipVersion) ? "Normal_Chip" : "Test_Chip");
- cnt += sprintf((buf+cnt), "Chip Version Info: CHIP_8723B_");
- cnt += sprintf((buf+cnt), "%s_", IS_NORMAL_CHIP(ChipVersion) ? "Normal_Chip" : "Test_Chip");
if (IS_CHIP_VENDOR_TSMC(ChipVersion))
- cnt += sprintf((buf+cnt), "%s_", "TSMC");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "TSMC_");
else if (IS_CHIP_VENDOR_UMC(ChipVersion))
- cnt += sprintf((buf+cnt), "%s_", "UMC");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "UMC_");
else if (IS_CHIP_VENDOR_SMIC(ChipVersion))
- cnt += sprintf((buf+cnt), "%s_", "SMIC");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "SMIC_");
if (IS_A_CUT(ChipVersion))
- cnt += sprintf((buf+cnt), "A_CUT_");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "A_CUT_");
else if (IS_B_CUT(ChipVersion))
- cnt += sprintf((buf+cnt), "B_CUT_");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "B_CUT_");
else if (IS_C_CUT(ChipVersion))
- cnt += sprintf((buf+cnt), "C_CUT_");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "C_CUT_");
else if (IS_D_CUT(ChipVersion))
- cnt += sprintf((buf+cnt), "D_CUT_");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "D_CUT_");
else if (IS_E_CUT(ChipVersion))
- cnt += sprintf((buf+cnt), "E_CUT_");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "E_CUT_");
else if (IS_I_CUT(ChipVersion))
- cnt += sprintf((buf+cnt), "I_CUT_");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "I_CUT_");
else if (IS_J_CUT(ChipVersion))
- cnt += sprintf((buf+cnt), "J_CUT_");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "J_CUT_");
else if (IS_K_CUT(ChipVersion))
- cnt += sprintf((buf+cnt), "K_CUT_");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "K_CUT_");
else
- cnt += sprintf((buf+cnt), "UNKNOWN_CUT(%d)_", ChipVersion.CUTVersion);
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt,
+ "UNKNOWN_CUT(%d)_", ChipVersion.CUTVersion);
if (IS_1T1R(ChipVersion))
- cnt += sprintf((buf+cnt), "1T1R_");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "1T1R_");
else if (IS_1T2R(ChipVersion))
- cnt += sprintf((buf+cnt), "1T2R_");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "1T2R_");
else if (IS_2T2R(ChipVersion))
- cnt += sprintf((buf+cnt), "2T2R_");
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "2T2R_");
else
- cnt += sprintf((buf+cnt), "UNKNOWN_RFTYPE(%d)_", ChipVersion.RFType);
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt,
+ "UNKNOWN_RFTYPE(%d)_", ChipVersion.RFType);
- cnt += sprintf((buf+cnt), "RomVer(%d)\n", ChipVersion.ROMVer);
+ cnt += scnprintf(buf + cnt, sizeof(buf) - cnt, "RomVer(%d)\n", ChipVersion.ROMVer);
DBG_871X("%s", buf);
}
--
2.30.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 3/3] staging: rtl8723bs: os_dep: Replace sprintf with scnprintf
2021-03-01 13:12 [PATCH 0/3] staging: rtl8723bs: Replace sprintf with scnprintf Candy Febriyanto
2021-03-01 13:12 ` [PATCH 1/3] staging: rtl8723bs: core: " Candy Febriyanto
2021-03-01 13:13 ` [PATCH 2/3] staging: rtl8723bs: hal: " Candy Febriyanto
@ 2021-03-01 13:13 ` Candy Febriyanto
2021-03-01 13:41 ` Dan Carpenter
2021-03-01 13:45 ` [PATCH 0/3] staging: rtl8723bs: " Hans de Goede
3 siblings, 1 reply; 8+ messages in thread
From: Candy Febriyanto @ 2021-03-01 13:13 UTC (permalink / raw)
To: gregkh, hdegoede, Larry.Finger, devel
The use of sprintf with format string here means that there is a risk
that the writes will go out of bounds, replace it with scnprintf.
In one block of the translate_scan function sprintf is only called once
(it's not being used to concatenate strings) so there is no need to keep
the pointer "p", remove it.
Signed-off-by: Candy Febriyanto <cfebriyanto@gmail.com>
---
.../staging/rtl8723bs/os_dep/ioctl_linux.c | 20 ++++++++++---------
drivers/staging/rtl8723bs/os_dep/mlme_linux.c | 6 +++---
2 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
index 41389e266f54..0bc3585d7097 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
@@ -240,9 +240,10 @@ static char *translate_scan(struct adapter *padapter,
return start;
if (wpa_len > 0) {
p = buf;
- p += sprintf(p, "wpa_ie =");
+ p += scnprintf(p, (MAX_WPA_IE_LEN * 2) - (p - buf), "wpa_ie =");
for (i = 0; i < wpa_len; i++)
- p += sprintf(p, "%02x", wpa_ie[i]);
+ p += scnprintf(p, (MAX_WPA_IE_LEN * 2) - (p - buf),
+ "%02x", wpa_ie[i]);
if (wpa_len > 100) {
printk("-----------------Len %d----------------\n", wpa_len);
@@ -265,9 +266,10 @@ static char *translate_scan(struct adapter *padapter,
if (rsn_len > 0) {
p = buf;
memset(buf, 0, MAX_WPA_IE_LEN*2);
- p += sprintf(p, "rsn_ie =");
+ p += scnprintf(p, (MAX_WPA_IE_LEN * 2) - (p - buf), "rsn_ie =");
for (i = 0; i < rsn_len; i++)
- p += sprintf(p, "%02x", rsn_ie[i]);
+ p += scnprintf(p, (MAX_WPA_IE_LEN * 2) - (p - buf),
+ "%02x", rsn_ie[i]);
memset(&iwe, 0, sizeof(iwe));
iwe.cmd = IWEVCUSTOM;
iwe.u.data.length = strlen(buf);
@@ -365,14 +367,14 @@ static char *translate_scan(struct adapter *padapter,
{
u8 *buf;
- u8 *p, *pos;
+ u8 *pos;
buf = kzalloc(MAX_WPA_IE_LEN, GFP_ATOMIC);
if (!buf)
goto exit;
- p = buf;
+
pos = pnetwork->network.Reserved;
- p += sprintf(p, "fm =%02X%02X", pos[1], pos[0]);
+ scnprintf(buf, MAX_WPA_IE_LEN, "fm =%02X%02X", pos[1], pos[0]);
memset(&iwe, 0, sizeof(iwe));
iwe.cmd = IWEVCUSTOM;
iwe.u.data.length = strlen(buf);
@@ -5082,7 +5084,7 @@ static int rtw_ioctl_wext_private(struct net_device *dev, union iwreq_data *wrq_
case IW_PRIV_TYPE_BYTE:
/* Display args */
for (j = 0; j < n; j++) {
- sprintf(str, "%d ", extra[j]);
+ scnprintf(str, sizeof(str), "%d ", extra[j]);
len = strlen(str);
output_len = strlen(output);
if ((output_len + len + 1) > 4096) {
@@ -5096,7 +5098,7 @@ static int rtw_ioctl_wext_private(struct net_device *dev, union iwreq_data *wrq_
case IW_PRIV_TYPE_INT:
/* Display args */
for (j = 0; j < n; j++) {
- sprintf(str, "%d ", ((__s32 *)extra)[j]);
+ scnprintf(str, sizeof(str), "%d ", ((__s32 *)extra)[j]);
len = strlen(str);
output_len = strlen(output);
if ((output_len + len + 1) > 4096) {
diff --git a/drivers/staging/rtl8723bs/os_dep/mlme_linux.c b/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
index d46c65ab384b..20899b2cff43 100644
--- a/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/mlme_linux.c
@@ -159,15 +159,15 @@ void rtw_report_sec_ie(struct adapter *adapter, u8 authmode, u8 *sec_ie)
}
p = buff;
- p += sprintf(p, "ASSOCINFO(ReqIEs =");
+ p += scnprintf(p, IW_CUSTOM_MAX - (p - buff), "ASSOCINFO(ReqIEs =");
len = sec_ie[1] + 2;
len = (len < IW_CUSTOM_MAX) ? len : IW_CUSTOM_MAX;
for (i = 0; i < len; i++)
- p += sprintf(p, "%02x", sec_ie[i]);
+ p += scnprintf(p, IW_CUSTOM_MAX - (p - buff), "%02x", sec_ie[i]);
- p += sprintf(p, ")");
+ p += scnprintf(p, IW_CUSTOM_MAX - (p - buff), ")");
memset(&wrqu, 0, sizeof(wrqu));
--
2.30.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] staging: rtl8723bs: os_dep: Replace sprintf with scnprintf
2021-03-01 13:13 ` [PATCH 3/3] staging: rtl8723bs: os_dep: " Candy Febriyanto
@ 2021-03-01 13:41 ` Dan Carpenter
2021-03-01 14:01 ` Candy Febriyanto
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2021-03-01 13:41 UTC (permalink / raw)
To: Candy Febriyanto; +Cc: devel, gregkh, Larry.Finger, hdegoede
On Mon, Mar 01, 2021 at 08:13:54PM +0700, Candy Febriyanto wrote:
> @@ -5082,7 +5084,7 @@ static int rtw_ioctl_wext_private(struct net_device *dev, union iwreq_data *wrq_
> case IW_PRIV_TYPE_BYTE:
> /* Display args */
> for (j = 0; j < n; j++) {
> - sprintf(str, "%d ", extra[j]);
> + scnprintf(str, sizeof(str), "%d ", extra[j]);
> len = strlen(str);
You could save a little code and combine the two statements:
len = scnprintf(str, sizeof(str), "%d ", extra[j]);
For bonus points, you could write a Coccinelle script to look for that
pattern of calling strlen() on a freshly sprintfed string.
> output_len = strlen(output);
> if ((output_len + len + 1) > 4096) {
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] staging: rtl8723bs: os_dep: Replace sprintf with scnprintf
2021-03-01 13:41 ` Dan Carpenter
@ 2021-03-01 14:01 ` Candy Febriyanto
0 siblings, 0 replies; 8+ messages in thread
From: Candy Febriyanto @ 2021-03-01 14:01 UTC (permalink / raw)
To: Dan Carpenter, devel, gregkh, hdegoede, Larry.Finger
On Mon, Mar 01, 2021 at 04:41:09PM +0300, Dan Carpenter wrote:
> On Mon, Mar 01, 2021 at 08:13:54PM +0700, Candy Febriyanto wrote:
> > @@ -5082,7 +5084,7 @@ static int rtw_ioctl_wext_private(struct net_device *dev, union iwreq_data *wrq_
> > case IW_PRIV_TYPE_BYTE:
> > /* Display args */
> > for (j = 0; j < n; j++) {
> > - sprintf(str, "%d ", extra[j]);
> > + scnprintf(str, sizeof(str), "%d ", extra[j]);
> > len = strlen(str);
>
> You could save a little code and combine the two statements:
>
> len = scnprintf(str, sizeof(str), "%d ", extra[j]);
Good catch, I'll send a V2.
>
> For bonus points, you could write a Coccinelle script to look for that
> pattern of calling strlen() on a freshly sprintfed string.
Sounds like a good idea, I don't know how to do it though so feel free.
>
> > output_len = strlen(output);
> > if ((output_len + len + 1) > 4096) {
>
> regards,
> dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] staging: rtl8723bs: Replace sprintf with scnprintf
2021-03-01 13:12 [PATCH 0/3] staging: rtl8723bs: Replace sprintf with scnprintf Candy Febriyanto
` (2 preceding siblings ...)
2021-03-01 13:13 ` [PATCH 3/3] staging: rtl8723bs: os_dep: " Candy Febriyanto
@ 2021-03-01 13:45 ` Hans de Goede
2021-03-01 14:02 ` Candy Febriyanto
3 siblings, 1 reply; 8+ messages in thread
From: Hans de Goede @ 2021-03-01 13:45 UTC (permalink / raw)
To: Candy Febriyanto, gregkh, Larry.Finger, devel
Hi,
On 3/1/21 2:12 PM, Candy Febriyanto wrote:
> This patchset replaces most calls to sprintf with scnprintf, thereby
> preventing potential buffer overflows. The rest I left alone because
> they write to a buffer passed by a caller that doesn't pass its size
> alongside it.
>
> Candy Febriyanto (3):
> staging: rtl8723bs: core: Replace sprintf with scnprintf
> staging: rtl8723bs: hal: Replace sprintf with scnprintf
> staging: rtl8723bs: os_dep: Replace sprintf with scnprintf
>
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 3 +-
> drivers/staging/rtl8723bs/core/rtw_pwrctrl.c | 4 +-
> drivers/staging/rtl8723bs/hal/hal_com.c | 45 ++++++++++---------
> .../staging/rtl8723bs/os_dep/ioctl_linux.c | 20 +++++----
> drivers/staging/rtl8723bs/os_dep/mlme_linux.c | 6 +--
> 5 files changed, 41 insertions(+), 37 deletions(-)
Thanks, the entire series looks good to me, for the series:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 0/3] staging: rtl8723bs: Replace sprintf with scnprintf
2021-03-01 13:45 ` [PATCH 0/3] staging: rtl8723bs: " Hans de Goede
@ 2021-03-01 14:02 ` Candy Febriyanto
0 siblings, 0 replies; 8+ messages in thread
From: Candy Febriyanto @ 2021-03-01 14:02 UTC (permalink / raw)
To: Hans de Goede, gregkh, Larry.Finger, devel
On Mon, Mar 01, 2021 at 02:45:00PM +0100, Hans de Goede wrote:
> Hi,
>
> On 3/1/21 2:12 PM, Candy Febriyanto wrote:
> > This patchset replaces most calls to sprintf with scnprintf, thereby
> > preventing potential buffer overflows. The rest I left alone because
> > they write to a buffer passed by a caller that doesn't pass its size
> > alongside it.
> >
> > Candy Febriyanto (3):
> > staging: rtl8723bs: core: Replace sprintf with scnprintf
> > staging: rtl8723bs: hal: Replace sprintf with scnprintf
> > staging: rtl8723bs: os_dep: Replace sprintf with scnprintf
> >
> > drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 3 +-
> > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c | 4 +-
> > drivers/staging/rtl8723bs/hal/hal_com.c | 45 ++++++++++---------
> > .../staging/rtl8723bs/os_dep/ioctl_linux.c | 20 +++++----
> > drivers/staging/rtl8723bs/os_dep/mlme_linux.c | 6 +--
> > 5 files changed, 41 insertions(+), 37 deletions(-)
>
> Thanks, the entire series looks good to me, for the series:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>
> Regards,
>
> Hans
>
Thank you Hans.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 8+ messages in thread