staging: r8188eu: overflow in rtw_p2p_get_go_device_address()

PATCH In review Dan Carpenter 3 participants 4 messages Feb 3, 2014 – Feb 7, 2014

Subsystems (2)

Staging Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2014-02-03 22:38 UTC #0 lore raw

PATCH 2/2 staging: r8188eu: overflow in rtw_p2p_get_go_device_address()

The go_devadd_str[] array is two characters too small to hold the address so we corrupt memory.

I've changed the user space API slightly and I don't have a way to test if this breaks anything. In the original code we truncated away the last digit of the address and the NUL terminator so it was already a bit broken.

Signed-off-by Dan Carpenter <[email protected]>

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +4 -6

index dec992569476..4ad80ae1067f 100644
@@ -3164,9 +3164,7 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
 	u8 *p2pie;
 	uint p2pielen = 0, attr_contentlen = 0;
 	u8 attr_content[100] = {0x00};
-
-	u8 go_devadd_str[17 + 10] = {0x00};
-	/*  +10 is for the str "go_devadd =", we have to clear it at wrqu->data.pointer */
+	u8 go_devadd_str[17 + 12] = {};
 
 	/*	Commented by Albert 20121209 */
 	/*	The input data is the GO's interface address which the application wants to know its device address. */

@@ -3223,12 +3221,12 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
 	spin_unlock_bh(&pmlmepriv->scanned_queue.lock);
 
 	if (!blnMatch)
-		sprintf(go_devadd_str, "\n\ndev_add = NULL");
+		snprintf(go_devadd_str, sizeof(go_devadd_str), "\n\ndev_add = NULL");
 	else
-		sprintf(go_devadd_str, "\n\ndev_add =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
+		snprintf(go_devadd_str, sizeof(go_devadd_str), "\n\ndev_add =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
 			attr_content[0], attr_content[1], attr_content[2], attr_content[3], attr_content[4], attr_content[5]);
 
-	if (copy_to_user(wrqu->data.pointer, go_devadd_str, 10 + 17))
+	if (copy_to_user(wrqu->data.pointer, go_devadd_str, sizeof(go_devadd_str)))
 		return -EFAULT;
 	return ret;
 }

Larry Finger

Acked-by on v1 2/2

2014-02-07 02:51 UTC #1 lore raw

Quoted 9 lines · Dan Carpenter

The go_devadd_str[] array is two characters too small to hold the address so we corrupt memory.
I've changed the user space API slightly and I don't have a way to test if this breaks anything. In the original code we truncated away the last digit of the address and the NUL terminator so it was already a bit broken.
Signed-off-by Dan Carpenter <[email protected]>

Nothing seems to be broken.

Acked-by Larry Finger <[email protected]>

Larry

Quoted 33 lines

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +4 -6

index dec992569476..4ad80ae1067f 100644
@@ -3164,9 +3164,7 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
  	u8 *p2pie;
  	uint p2pielen = 0, attr_contentlen = 0;
  	u8 attr_content[100] = {0x00};
-
-	u8 go_devadd_str[17 + 10] = {0x00};
-	/*  +10 is for the str "go_devadd =", we have to clear it at wrqu->data.pointer */
+	u8 go_devadd_str[17 + 12] = {};
 
  	/*	Commented by Albert 20121209 */
  	/*	The input data is the GO's interface address which the application wants to know its device address. */

@@ -3223,12 +3221,12 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
  	spin_unlock_bh(&pmlmepriv->scanned_queue.lock);
 
  	if (!blnMatch)
-		sprintf(go_devadd_str, "\n\ndev_add = NULL");
+		snprintf(go_devadd_str, sizeof(go_devadd_str), "\n\ndev_add = NULL");
  	else
-		sprintf(go_devadd_str, "\n\ndev_add =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
+		snprintf(go_devadd_str, sizeof(go_devadd_str), "\n\ndev_add =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
  			attr_content[0], attr_content[1], attr_content[2], attr_content[3], attr_content[4], attr_content[5]);
 
-	if (copy_to_user(wrqu->data.pointer, go_devadd_str, 10 + 17))
+	if (copy_to_user(wrqu->data.pointer, go_devadd_str, sizeof(go_devadd_str)))
  		return -EFAULT;
  	return ret;
  }

walter harms

2014-02-07 10:46 UTC #2 lore raw

In reply to Dan Carpenter

Am 03.02.2014 23:38, schrieb Dan Carpenter:

Quoted 15 lines

The go_devadd_str[] array is two characters too small to hold the address so we corrupt memory.
I've changed the user space API slightly and I don't have a way to test if this breaks anything. In the original code we truncated away the last digit of the address and the NUL terminator so it was already a bit broken.
Signed-off-by Dan Carpenter <[email protected]>
drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +0 -0
index dec992569476..4ad80ae1067f 100644

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c static int rtw_p2p_get_go_device_address(struct net_device *dev, · line 3167

Quoted 7 lines

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +1 -3

@@ -3164,9 +3164,7 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
 	u8 *p2pie;
 	uint p2pielen = 0, attr_contentlen = 0;
 	u8 attr_content[100] = {0x00};
-
-	u8 go_devadd_str[17 + 10] = {0x00};
-	/*  +10 is for the str "go_devadd =", we have to clear it at wrqu->data.pointer */
+	u8 go_devadd_str[17 + 12] = {};

you are deleting the explanation for the magic numbers here,
- intentionally ?

NTL, it would be nice to have a full explanation like
10= space for "go_devadd ="
17= space for attr_content %.2X:%.2X:%.2X:%.2X:%.2X:%.2X
re,
wh

Quoted 23 lines

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +3 -4

 	/*	Commented by Albert 20121209 */
 	/*	The input data is the GO's interface address which the application wants to know its device address. */

@@ -3223,12 +3221,12 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
 	spin_unlock_bh(&pmlmepriv->scanned_queue.lock);
 
 	if (!blnMatch)
-		sprintf(go_devadd_str, "\n\ndev_add = NULL");
+		snprintf(go_devadd_str, sizeof(go_devadd_str), "\n\ndev_add = NULL");
 	else
-		sprintf(go_devadd_str, "\n\ndev_add =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
+		snprintf(go_devadd_str, sizeof(go_devadd_str), "\n\ndev_add =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
 			attr_content[0], attr_content[1], attr_content[2], attr_content[3], attr_content[4], attr_content[5]);
 
-	if (copy_to_user(wrqu->data.pointer, go_devadd_str, 10 + 17))
+	if (copy_to_user(wrqu->data.pointer, go_devadd_str, sizeof(go_devadd_str)))
 		return -EFAULT;
 	return ret;
 }
--

To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2014-02-07 11:21 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 13 lines · walter harms

Am 03.02.2014 23:38, schrieb Dan Carpenter:
Quoted 10 lines
The go_devadd_str[] array is two characters too small to hold the address so we corrupt memory.
I've changed the user space API slightly and I don't have a way to test if this breaks anything. In the original code we truncated away the last digit of the address and the NUL terminator so it was already a bit broken.
Signed-off-by Dan Carpenter <[email protected]>

Quoted 17 lines

drivers/staging/rtl8188eu/os_dep/ioctl_linux.c static int rtw_p2p_get_go_device_address(struct net_device *dev, · line 3167
Quoted 12 lines
drivers/staging/rtl8188eu/os_dep/ioctl_linux.c +1 -3
index dec992569476..4ad80ae1067f 100644
@@ -3164,9 +3164,7 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
 	u8 *p2pie;
 	uint p2pielen = 0, attr_contentlen = 0;
 	u8 attr_content[100] = {0x00};
-
-	u8 go_devadd_str[17 + 10] = {0x00};
-	/*  +10 is for the str "go_devadd =", we have to clear it at wrqu->data.pointer */
+	u8 go_devadd_str[17 + 12] = {};
you are deleting the explanation for the magic numbers here,
- intentionally ?

Yes. The old explanation was misleading because the string is "\n\ndev_add =" and not "go_devadd =". The buffer is only used in one location so the size is obvious and there is no need for a comment.

regards,
dan carpenter