[PATCH 0/3] staging: rtl8723bs: Replace sprintf with scnprintf

[PATCH 0/3] staging: rtl8723bs: Replace sprintf with scnprintf

[Candy Febriyanto]

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

-- 
2.30.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/3] staging: rtl8723bs: core: Replace sprintf with scnprintf

[Candy Febriyanto]

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

[PATCH 2/3] staging: rtl8723bs: hal: Replace sprintf with scnprintf

[Candy Febriyanto]

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

[PATCH 3/3] staging: rtl8723bs: os_dep: Replace sprintf with scnprintf

[Candy Febriyanto]

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

Re: [PATCH 3/3] staging: rtl8723bs: os_dep: Replace sprintf with scnprintf

[Dan Carpenter]

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

Re: [PATCH 0/3] staging: rtl8723bs: Replace sprintf with scnprintf

[Hans de Goede]

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

Re: [PATCH 3/3] staging: rtl8723bs: os_dep: Replace sprintf with scnprintf

[Candy Febriyanto]

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

Re: [PATCH 0/3] staging: rtl8723bs: Replace sprintf with scnprintf

[Candy Febriyanto]

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