* [PATCH v2 0/3] staging: rtl8723bs: Replace sprintf with scnprintf
@ 2021-03-01 14:56 Candy Febriyanto
2021-03-01 14:58 ` [PATCH v2 1/3] staging: rtl8723bs: core: " Candy Febriyanto
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Candy Febriyanto @ 2021-03-01 14:56 UTC (permalink / raw)
To: devel; +Cc: gregkh, Larry.Finger, hdegoede
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 | 23 +++++-----
drivers/staging/rtl8723bs/os_dep/mlme_linux.c | 6 +--
5 files changed, 41 insertions(+), 40 deletions(-)
--
2.30.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/3] staging: rtl8723bs: core: Replace sprintf with scnprintf
2021-03-01 14:56 [PATCH v2 0/3] staging: rtl8723bs: Replace sprintf with scnprintf Candy Febriyanto
@ 2021-03-01 14:58 ` Candy Febriyanto
2021-03-01 15:00 ` [PATCH v2 3/3] staging: rtl8723bs: os_dep: " Candy Febriyanto
2021-03-01 15:01 ` [PATCH v2 2/3] staging: rtl8723bs: hal: " Candy Febriyanto
2 siblings, 0 replies; 6+ messages in thread
From: Candy Febriyanto @ 2021-03-01 14:58 UTC (permalink / raw)
To: devel; +Cc: gregkh, Larry.Finger, hdegoede
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>
Reviewed-by: Hans de Goede <hdegoede@redhat.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] 6+ messages in thread
* [PATCH v2 3/3] staging: rtl8723bs: os_dep: Replace sprintf with scnprintf
2021-03-01 14:56 [PATCH v2 0/3] staging: rtl8723bs: Replace sprintf with scnprintf Candy Febriyanto
2021-03-01 14:58 ` [PATCH v2 1/3] staging: rtl8723bs: core: " Candy Febriyanto
@ 2021-03-01 15:00 ` Candy Febriyanto
2021-03-01 15:07 ` Dan Carpenter
2021-03-01 15:01 ` [PATCH v2 2/3] staging: rtl8723bs: hal: " Candy Febriyanto
2 siblings, 1 reply; 6+ messages in thread
From: Candy Febriyanto @ 2021-03-01 15:00 UTC (permalink / raw)
To: devel; +Cc: gregkh, Larry.Finger, hdegoede
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>
---
Changed since V1:
- Dan Carpenter: assign the return value of scnprintf to the length
variable instead of calling strlen
.../staging/rtl8723bs/os_dep/ioctl_linux.c | 23 +++++++++----------
drivers/staging/rtl8723bs/os_dep/mlme_linux.c | 6 ++---
2 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
index 41389e266f54..78ba2423ed65 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,17 +367,16 @@ 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]);
memset(&iwe, 0, sizeof(iwe));
iwe.cmd = IWEVCUSTOM;
- iwe.u.data.length = strlen(buf);
+ iwe.u.data.length = scnprintf(buf, MAX_WPA_IE_LEN, "fm =%02X%02X", pos[1], pos[0]);
start = iwe_stream_add_point(info, start, stop, &iwe, buf);
kfree(buf);
}
@@ -5082,8 +5083,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]);
- len = strlen(str);
+ len = scnprintf(str, sizeof(str), "%d ", extra[j]);
output_len = strlen(output);
if ((output_len + len + 1) > 4096) {
err = -E2BIG;
@@ -5096,8 +5096,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]);
- len = strlen(str);
+ len = scnprintf(str, sizeof(str), "%d ", ((__s32 *)extra)[j]);
output_len = strlen(output);
if ((output_len + len + 1) > 4096) {
err = -E2BIG;
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] 6+ messages in thread
* [PATCH v2 2/3] staging: rtl8723bs: hal: Replace sprintf with scnprintf
2021-03-01 14:56 [PATCH v2 0/3] staging: rtl8723bs: Replace sprintf with scnprintf Candy Febriyanto
2021-03-01 14:58 ` [PATCH v2 1/3] staging: rtl8723bs: core: " Candy Febriyanto
2021-03-01 15:00 ` [PATCH v2 3/3] staging: rtl8723bs: os_dep: " Candy Febriyanto
@ 2021-03-01 15:01 ` Candy Febriyanto
2 siblings, 0 replies; 6+ messages in thread
From: Candy Febriyanto @ 2021-03-01 15:01 UTC (permalink / raw)
To: devel; +Cc: gregkh, Larry.Finger, hdegoede
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>
Reviewed-by: Hans de Goede <hdegoede@redhat.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] 6+ messages in thread
* Re: [PATCH v2 3/3] staging: rtl8723bs: os_dep: Replace sprintf with scnprintf
2021-03-01 15:00 ` [PATCH v2 3/3] staging: rtl8723bs: os_dep: " Candy Febriyanto
@ 2021-03-01 15:07 ` Dan Carpenter
2021-03-01 15:15 ` Candy Febriyanto
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-03-01 15:07 UTC (permalink / raw)
To: Candy Febriyanto; +Cc: devel, gregkh, hdegoede, Larry.Finger
On Mon, Mar 01, 2021 at 10:00:11PM +0700, Candy Febriyanto wrote:
> 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>
> ---
Looks good. TBH, v1 was also fine. I should have just acked it instead
of commenting...
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 3/3] staging: rtl8723bs: os_dep: Replace sprintf with scnprintf
2021-03-01 15:07 ` Dan Carpenter
@ 2021-03-01 15:15 ` Candy Febriyanto
0 siblings, 0 replies; 6+ messages in thread
From: Candy Febriyanto @ 2021-03-01 15:15 UTC (permalink / raw)
To: Dan Carpenter; +Cc: devel, gregkh, hdegoede, Larry.Finger
On Mon, Mar 01, 2021 at 06:07:25PM +0300, Dan Carpenter wrote:
> On Mon, Mar 01, 2021 at 10:00:11PM +0700, Candy Febriyanto wrote:
> > 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>
> > ---
>
> Looks good. TBH, v1 was also fine. I should have just acked it instead
> of commenting...
>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> regards,
> dan carpenter
>
Thanks for the review!
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-01 15:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-01 14:56 [PATCH v2 0/3] staging: rtl8723bs: Replace sprintf with scnprintf Candy Febriyanto
2021-03-01 14:58 ` [PATCH v2 1/3] staging: rtl8723bs: core: " Candy Febriyanto
2021-03-01 15:00 ` [PATCH v2 3/3] staging: rtl8723bs: os_dep: " Candy Febriyanto
2021-03-01 15:07 ` Dan Carpenter
2021-03-01 15:15 ` Candy Febriyanto
2021-03-01 15:01 ` [PATCH v2 2/3] staging: rtl8723bs: hal: " Candy Febriyanto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).