[PATCH AUTOSEL 5.13 06/21] USB: core: Fix incorrect pipe calculation in do_proc_control()

[PATCH AUTOSEL 5.13 06/21] USB: core: Fix incorrect pipe calculation in do_proc_control()

[Sasha Levin]

From: Alan Stern <stern@rowland.harvard.edu>

[ Upstream commit b0863f1927323110e3d0d69f6adb6a91018a9a3c ]

When the user submits a control URB via usbfs, the user supplies the
bRequestType value and the kernel uses it to compute the pipe value.
However, do_proc_control() performs this computation incorrectly in
the case where the bRequestType direction bit is set to USB_DIR_IN and
the URB's transfer length is 0: The pipe's direction is also set to IN
but it should be OUT, which is the direction the actual transfer will
use regardless of bRequestType.

Commit 5cc59c418fde ("USB: core: WARN if pipe direction != setup
packet direction") added a check to compare the direction bit in the
pipe value to a control URB's actual direction and to WARN if they are
different.  This can be triggered by the incorrect computation
mentioned above, as found by syzbot.

This patch fixes the computation, thus avoiding the WARNing.

Reported-and-tested-by: syzbot+72af3105289dcb4c055b@syzkaller.appspotmail.com
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Link: https://lore.kernel.org/r/20210712185436.GB326369@rowland.harvard.edu
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/usb/core/devio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 2218941d35a3..73b60f013b20 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1133,7 +1133,7 @@ static int do_proc_control(struct usb_dev_state *ps,
 		"wIndex=%04x wLength=%04x\n",
 		ctrl->bRequestType, ctrl->bRequest, ctrl->wValue,
 		ctrl->wIndex, ctrl->wLength);
-	if (ctrl->bRequestType & 0x80) {
+	if ((ctrl->bRequestType & USB_DIR_IN) && ctrl->wLength) {
 		pipe = usb_rcvctrlpipe(dev, 0);
 		snoop_urb(dev, NULL, pipe, ctrl->wLength, tmo, SUBMIT, NULL, 0);
 
-- 
2.30.2

Re: [PATCH AUTOSEL 5.13 06/21] USB: core: Fix incorrect pipe calculation in do_proc_control()

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 09:18:53AM -0400, Sasha Levin wrote:
> From: Alan Stern <stern@rowland.harvard.edu>
> 
> [ Upstream commit b0863f1927323110e3d0d69f6adb6a91018a9a3c ]
> 
> When the user submits a control URB via usbfs, the user supplies the
> bRequestType value and the kernel uses it to compute the pipe value.
> However, do_proc_control() performs this computation incorrectly in
> the case where the bRequestType direction bit is set to USB_DIR_IN and
> the URB's transfer length is 0: The pipe's direction is also set to IN
> but it should be OUT, which is the direction the actual transfer will
> use regardless of bRequestType.
> 
> Commit 5cc59c418fde ("USB: core: WARN if pipe direction != setup
> packet direction") added a check to compare the direction bit in the
> pipe value to a control URB's actual direction and to WARN if they are
> different.  This can be triggered by the incorrect computation
> mentioned above, as found by syzbot.
> 
> This patch fixes the computation, thus avoiding the WARNing.
> 
> Reported-and-tested-by: syzbot+72af3105289dcb4c055b@syzkaller.appspotmail.com
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Link: https://lore.kernel.org/r/20210712185436.GB326369@rowland.harvard.edu
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/usb/core/devio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 2218941d35a3..73b60f013b20 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -1133,7 +1133,7 @@ static int do_proc_control(struct usb_dev_state *ps,
>  		"wIndex=%04x wLength=%04x\n",
>  		ctrl->bRequestType, ctrl->bRequest, ctrl->wValue,
>  		ctrl->wIndex, ctrl->wLength);
> -	if (ctrl->bRequestType & 0x80) {
> +	if ((ctrl->bRequestType & USB_DIR_IN) && ctrl->wLength) {
>  		pipe = usb_rcvctrlpipe(dev, 0);
>  		snoop_urb(dev, NULL, pipe, ctrl->wLength, tmo, SUBMIT, NULL, 0);
>  
> -- 
> 2.30.2
> 

This is not needed in any kernel that does not also have 5cc59c418fde
("USB: core: WARN if pipe direction != setup packet direction"), which
showed up in 5.14-rc1, so please drop this from all of the AUTOSEL
trees.

thanks,

greg k-h

Re: [PATCH AUTOSEL 5.13 06/21] USB: core: Fix incorrect pipe calculation in do_proc_control()

[Sasha Levin]

On Tue, Jul 27, 2021 at 03:28:20PM +0200, Greg Kroah-Hartman wrote:
>On Tue, Jul 27, 2021 at 09:18:53AM -0400, Sasha Levin wrote:
>> From: Alan Stern <stern@rowland.harvard.edu>
>>
>> [ Upstream commit b0863f1927323110e3d0d69f6adb6a91018a9a3c ]
>>
>> When the user submits a control URB via usbfs, the user supplies the
>> bRequestType value and the kernel uses it to compute the pipe value.
>> However, do_proc_control() performs this computation incorrectly in
>> the case where the bRequestType direction bit is set to USB_DIR_IN and
>> the URB's transfer length is 0: The pipe's direction is also set to IN
>> but it should be OUT, which is the direction the actual transfer will
>> use regardless of bRequestType.
>>
>> Commit 5cc59c418fde ("USB: core: WARN if pipe direction != setup
>> packet direction") added a check to compare the direction bit in the
>> pipe value to a control URB's actual direction and to WARN if they are
>> different.  This can be triggered by the incorrect computation
>> mentioned above, as found by syzbot.
>>
>> This patch fixes the computation, thus avoiding the WARNing.
>>
>> Reported-and-tested-by: syzbot+72af3105289dcb4c055b@syzkaller.appspotmail.com
>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>> Link: https://lore.kernel.org/r/20210712185436.GB326369@rowland.harvard.edu
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> ---
>>  drivers/usb/core/devio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
>> index 2218941d35a3..73b60f013b20 100644
>> --- a/drivers/usb/core/devio.c
>> +++ b/drivers/usb/core/devio.c
>> @@ -1133,7 +1133,7 @@ static int do_proc_control(struct usb_dev_state *ps,
>>  		"wIndex=%04x wLength=%04x\n",
>>  		ctrl->bRequestType, ctrl->bRequest, ctrl->wValue,
>>  		ctrl->wIndex, ctrl->wLength);
>> -	if (ctrl->bRequestType & 0x80) {
>> +	if ((ctrl->bRequestType & USB_DIR_IN) && ctrl->wLength) {
>>  		pipe = usb_rcvctrlpipe(dev, 0);
>>  		snoop_urb(dev, NULL, pipe, ctrl->wLength, tmo, SUBMIT, NULL, 0);
>>
>> --
>> 2.30.2
>>
>
>This is not needed in any kernel that does not also have 5cc59c418fde
>("USB: core: WARN if pipe direction != setup packet direction"), which
>showed up in 5.14-rc1, so please drop this from all of the AUTOSEL
>trees.

Will do, thanks!

-- 
Thanks,
Sasha

[PATCH AUTOSEL 5.13 07/21] r8152: Fix potential PM refcount imbalance

[Sasha Levin]

From: Takashi Iwai <tiwai@suse.de>

[ Upstream commit 9c23aa51477a37f8b56c3c40192248db0663c196 ]

rtl8152_close() takes the refcount via usb_autopm_get_interface() but
it doesn't release when RTL8152_UNPLUG test hits.  This may lead to
the imbalance of PM refcount.  This patch addresses it.

Link: https://bugzilla.suse.com/show_bug.cgi?id=1186194
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/net/usb/r8152.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index e25bfb7021ed..8dcc55e4a5bc 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -6761,9 +6761,10 @@ static int rtl8152_close(struct net_device *netdev)
 		tp->rtl_ops.down(tp);
 
 		mutex_unlock(&tp->control);
+	}
 
+	if (!res)
 		usb_autopm_put_interface(tp->intf);
-	}
 
 	free_all_mem(tp);
 
-- 
2.30.2

[PATCH AUTOSEL 5.13 08/21] r8152: Fix a deadlock by doubly PM resume

[Sasha Levin]

From: Takashi Iwai <tiwai@suse.de>

[ Upstream commit 776ac63a986d211286230c4fd70f85390eabedcd ]

r8152 driver sets up the MAC address at reset-resume, while
rtl8152_set_mac_address() has the temporary autopm get/put.  This may
lead to a deadlock as the PM lock has been already taken for the
execution of the runtime PM callback.

This patch adds the workaround to avoid the superfluous autpm when
called from rtl8152_reset_resume().

Link: https://bugzilla.suse.com/show_bug.cgi?id=1186194
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/net/usb/r8152.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 8dcc55e4a5bc..2cf763b4ea84 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1550,7 +1550,8 @@ static int
 rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
 		  u32 advertising);
 
-static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
+static int __rtl8152_set_mac_address(struct net_device *netdev, void *p,
+				     bool in_resume)
 {
 	struct r8152 *tp = netdev_priv(netdev);
 	struct sockaddr *addr = p;
@@ -1559,9 +1560,11 @@ static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
 	if (!is_valid_ether_addr(addr->sa_data))
 		goto out1;
 
-	ret = usb_autopm_get_interface(tp->intf);
-	if (ret < 0)
-		goto out1;
+	if (!in_resume) {
+		ret = usb_autopm_get_interface(tp->intf);
+		if (ret < 0)
+			goto out1;
+	}
 
 	mutex_lock(&tp->control);
 
@@ -1573,11 +1576,17 @@ static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
 
 	mutex_unlock(&tp->control);
 
-	usb_autopm_put_interface(tp->intf);
+	if (!in_resume)
+		usb_autopm_put_interface(tp->intf);
 out1:
 	return ret;
 }
 
+static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
+{
+	return __rtl8152_set_mac_address(netdev, p, false);
+}
+
 /* Devices containing proper chips can support a persistent
  * host system provided MAC address.
  * Examples of this are Dell TB15 and Dell WD15 docks
@@ -1696,7 +1705,7 @@ static int determine_ethernet_addr(struct r8152 *tp, struct sockaddr *sa)
 	return ret;
 }
 
-static int set_ethernet_addr(struct r8152 *tp)
+static int set_ethernet_addr(struct r8152 *tp, bool in_resume)
 {
 	struct net_device *dev = tp->netdev;
 	struct sockaddr sa;
@@ -1709,7 +1718,7 @@ static int set_ethernet_addr(struct r8152 *tp)
 	if (tp->version == RTL_VER_01)
 		ether_addr_copy(dev->dev_addr, sa.sa_data);
 	else
-		ret = rtl8152_set_mac_address(dev, &sa);
+		ret = __rtl8152_set_mac_address(dev, &sa, in_resume);
 
 	return ret;
 }
@@ -8442,7 +8451,7 @@ static int rtl8152_reset_resume(struct usb_interface *intf)
 	clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 	tp->rtl_ops.init(tp);
 	queue_delayed_work(system_long_wq, &tp->hw_phy_work, 0);
-	set_ethernet_addr(tp);
+	set_ethernet_addr(tp, true);
 	return rtl8152_resume(intf);
 }
 
@@ -9562,7 +9571,7 @@ static int rtl8152_probe(struct usb_interface *intf,
 	tp->rtl_fw.retry = true;
 #endif
 	queue_delayed_work(system_long_wq, &tp->hw_phy_work, 0);
-	set_ethernet_addr(tp);
+	set_ethernet_addr(tp, false);
 
 	usb_set_intfdata(intf, tp);
 
-- 
2.30.2