[PATCH net-next 1/5] usbnet: ipheth: race between ipheth_close and error handling

[PATCH net-next 1/5] usbnet: ipheth: race between ipheth_close and error handling

[Foster Snowhill]

From: Oliver Neukum <oneukum@suse.com>

ipheth_sndbulk_callback() can submit carrier_work
as a part of its error handling. That means that
the driver must make sure that the work is cancelled
after it has made sure that no more URB can terminate
with an error condition.

Hence the order of actions in ipheth_close() needs
to be inverted.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Foster Snowhill <forst@pen.gy>
Tested-by: Georgi Valkov <gvalkov@gmail.com>
---
v1:
  No code changes. Fixed two "ipeth" -> "ipheth" typos in commit msg.
RFC: https://lore.kernel.org/netdev/20231121144330.3990-1-oneukum@suse.com/
---
 drivers/net/usb/ipheth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 687d70cfc556..6eeef10edada 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -475,8 +475,8 @@ static int ipheth_close(struct net_device *net)
 {
 	struct ipheth_device *dev = netdev_priv(net);
 
-	cancel_delayed_work_sync(&dev->carrier_work);
 	netif_stop_queue(net);
+	cancel_delayed_work_sync(&dev->carrier_work);
 	return 0;
 }
 
-- 
2.45.1

[PATCH net-next 2/5] usbnet: ipheth: remove extraneous rx URB length check

[Foster Snowhill]

Rx URB length was already checked in ipheth_rcvbulk_callback_legacy()
and ipheth_rcvbulk_callback_ncm(), depending on the current mode.
The check in ipheth_rcvbulk_callback() was thus mostly a duplicate.

The only place in ipheth_rcvbulk_callback() where we care about the URB
length is for the initial control frame. These frames are always 4 bytes
long. This has been checked as far back as iOS 4.2.1 on iPhone 3G.

Remove the extraneous URB length check. For control frames, check for
the specific 4-byte length instead.

Signed-off-by: Foster Snowhill <forst@pen.gy>
Tested-by: Georgi Valkov <gvalkov@gmail.com>
---
 drivers/net/usb/ipheth.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 6eeef10edada..017255615508 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -286,11 +286,6 @@ static void ipheth_rcvbulk_callback(struct urb *urb)
 		return;
 	}
 
-	if (urb->actual_length <= IPHETH_IP_ALIGN) {
-		dev->net->stats.rx_length_errors++;
-		return;
-	}
-
 	/* RX URBs starting with 0x00 0x01 do not encapsulate Ethernet frames,
 	 * but rather are control frames. Their purpose is not documented, and
 	 * they don't affect driver functionality, okay to drop them.
@@ -298,7 +293,8 @@ static void ipheth_rcvbulk_callback(struct urb *urb)
 	 * URB received from the bulk IN endpoint.
 	 */
 	if (unlikely
-		(((char *)urb->transfer_buffer)[0] == 0 &&
+		(urb->actual_length == 4 &&
+		 ((char *)urb->transfer_buffer)[0] == 0 &&
 		 ((char *)urb->transfer_buffer)[1] == 1))
 		goto rx_submit;
 
-- 
2.45.1

Re: [PATCH net-next 2/5] usbnet: ipheth: remove extraneous rx URB length check

[Simon Horman]

On Tue, Aug 06, 2024 at 07:28:06PM +0200, Foster Snowhill wrote:
> Rx URB length was already checked in ipheth_rcvbulk_callback_legacy()
> and ipheth_rcvbulk_callback_ncm(), depending on the current mode.
> The check in ipheth_rcvbulk_callback() was thus mostly a duplicate.
> 
> The only place in ipheth_rcvbulk_callback() where we care about the URB
> length is for the initial control frame. These frames are always 4 bytes
> long. This has been checked as far back as iOS 4.2.1 on iPhone 3G.
> 
> Remove the extraneous URB length check. For control frames, check for
> the specific 4-byte length instead.

Hi Foster,

I am slightly concerned what happens if a frame that does not match the
slightly stricter check in this patch, is now passed to
dev->rcvbulk_callback().

I see that observations have been made that this does not happen.  But is
there no was to inject malicious packets, or for something to malfunction?

> 
> Signed-off-by: Foster Snowhill <forst@pen.gy>
> Tested-by: Georgi Valkov <gvalkov@gmail.com>
> ---
>  drivers/net/usb/ipheth.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
> index 6eeef10edada..017255615508 100644
> --- a/drivers/net/usb/ipheth.c
> +++ b/drivers/net/usb/ipheth.c
> @@ -286,11 +286,6 @@ static void ipheth_rcvbulk_callback(struct urb *urb)
>  		return;
>  	}
>  
> -	if (urb->actual_length <= IPHETH_IP_ALIGN) {
> -		dev->net->stats.rx_length_errors++;
> -		return;
> -	}
> -
>  	/* RX URBs starting with 0x00 0x01 do not encapsulate Ethernet frames,
>  	 * but rather are control frames. Their purpose is not documented, and
>  	 * they don't affect driver functionality, okay to drop them.
> @@ -298,7 +293,8 @@ static void ipheth_rcvbulk_callback(struct urb *urb)
>  	 * URB received from the bulk IN endpoint.
>  	 */
>  	if (unlikely
> -		(((char *)urb->transfer_buffer)[0] == 0 &&
> +		(urb->actual_length == 4 &&
> +		 ((char *)urb->transfer_buffer)[0] == 0 &&
>  		 ((char *)urb->transfer_buffer)[1] == 1))
>  		goto rx_submit;
>  
> -- 
> 2.45.1
> 
> 

Re: [PATCH net-next 2/5] usbnet: ipheth: remove extraneous rx URB length check

[Foster Snowhill]

Hello Simon,

Thank you very much for the feedback, and apologies for the delay.

On 2024-08-09 12:16, Simon Horman wrote:
> I am slightly concerned what happens if a frame that does not match the
> slightly stricter check in this patch, is now passed to
> dev->rcvbulk_callback().
> 
> I see that observations have been made that this does not happen.  But is
> there no was to inject malicious packets, or for something to malfunction?

Specifically for this patchset, in my opinion it shouldn't have had any
effect on dev->rcvbulk_callback(). The first thing that both of the
callbacks do is check the length of the incoming URB, to make sure they fit
the padding (for legacy callback) or the entirety of NTH16+NDP16 (for NCM).

However your message did prompt me to look at the code again with fresh
eyes, especially at the NCM implementation, and there is definitely need
for improvement in handling malicious URBs. I've submitted a patch a few
minutes ago [1] to address the issues I noticed.

What I think happened is I had two conflicting ideas in my head, one was
"make this generic enough to support arbitrary NCM header length and
location", and on the other hand "iOS has a very specific header size,
don't reimplement CDC NCM which already has a proper driver in cdc_ncm".
The implementation ended up somewhere in between, and resulted in code
that is susceptible to being negatively affected by malicious URBs.

With the latest patch [1] I decided that I should limit the NCM
implementation in ipheth to the iOS-specific URB payload format, and error
on anything else to be on the safe side. If there is ever need to make it
more generic (e.g. if iOS suddenly changes the URB payload structure),
a better idea could be to somehow reuse the existing code in the cdc_ncm
driver, which is a lot more careful in parsing incoming data. That would
possibly involve converting ipheth to use the usbnet framework.

[1]: https://lore.kernel.org/netdev/20240907230108.978355-1-forst@pen.gy/

Cheers,
Foster.

[PATCH net-next 3/5] usbnet: ipheth: drop RX URBs with no payload

[Foster Snowhill]

On iPhone 15 Pro Max one can observe periodic URBs with no payload
on the "bulk in" (RX) endpoint. These don't seem to do anything
meaningful. Reproduced on iOS 17.5.1 and 17.6.

This behaviour isn't observed on iPhone 11 on the same iOS version. The
nature of these zero-length URBs is so far unknown.

Drop RX URBs with no payload.

Signed-off-by: Foster Snowhill <forst@pen.gy>
---
 drivers/net/usb/ipheth.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 017255615508..f04c7bf79665 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -286,6 +286,12 @@ static void ipheth_rcvbulk_callback(struct urb *urb)
 		return;
 	}
 
+	/* iPhone may periodically send URBs with no payload
+	 * on the "bulk in" endpoint. It is safe to ignore them.
+	 */
+	if (urb->actual_length == 0)
+		goto rx_submit;
+
 	/* RX URBs starting with 0x00 0x01 do not encapsulate Ethernet frames,
 	 * but rather are control frames. Their purpose is not documented, and
 	 * they don't affect driver functionality, okay to drop them.
-- 
2.45.1

[PATCH net-next 4/5] usbnet: ipheth: do not stop RX on failing RX callback

[Foster Snowhill]

RX callbacks can fail for multiple reasons:

* Payload too short
* Payload formatted incorrecly (e.g. bad NCM framing)
* Lack of memory

None of these should cause the driver to seize up.

Make such failures non-critical and continue processing further
incoming URBs.

Signed-off-by: Foster Snowhill <forst@pen.gy>
---
 drivers/net/usb/ipheth.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index f04c7bf79665..cdc72559790a 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -308,7 +308,6 @@ static void ipheth_rcvbulk_callback(struct urb *urb)
 	if (retval != 0) {
 		dev_err(&dev->intf->dev, "%s: callback retval: %d\n",
 			__func__, retval);
-		return;
 	}
 
 rx_submit:
-- 
2.45.1

[PATCH net-next 5/5] usbnet: ipheth: fix carrier detection in modes 1 and 4

[Foster Snowhill]

Apart from the standard "configurations", "interfaces" and "alternate
interface settings" in USB, iOS devices also have a notion of
"modes". In different modes, the device exposes a different set of
available configurations.

Depending on the iOS version, and depending on the current mode, the
length and contents of the carrier state control message differs:

* 1 byte (seen on iOS 4.2.1, 8.4):
    * 03: carrier off (mode 0)
    * 04: carrier on (mode 0)
* 3 bytes (seen on iOS 10.3.4, 15.7.6):
    * 03 03 03: carrier off (mode 0)
    * 04 04 03: carrier on (mode 0)
* 4 bytes (seen on iOS 16.5, 17.6):
    * 03 03 03 00: carrier off (mode 0)
    * 04 03 03 00: carrier off (mode 1)
    * 06 03 03 00: carrier off (mode 4)
    * 04 04 03 04: carrier on (mode 0 and 1)
    * 06 04 03 04: carrier on (mode 4)

Before this change, the driver always used the first byte of the
response to determine carrier state.

From this larger sample, the first byte seems to indicate the number of
available USB configurations in the current mode (with the exception of
the default mode 0), and in some cases (namely mode 1 and 4) does not
correlate with the carrier state.

Previous logic erroneously counted `04 03 03 00` as "carrier on" and
`06 04 03 04` as "carrier off" on iOS versions that support mode 1 and
mode 4 respectively.

Only modes 0, 1 and 4 expose the USB Ethernet interfaces necessary for
the ipheth driver.

Check the second byte of the control message where possible, and fall
back to checking the first byte on older iOS versions.

Signed-off-by: Foster Snowhill <forst@pen.gy>
Tested-by: Georgi Valkov <gvalkov@gmail.com>
---
 drivers/net/usb/ipheth.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index cdc72559790a..46afb95ffabe 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -355,13 +355,14 @@ static int ipheth_carrier_set(struct ipheth_device *dev)
 			0x02, /* index */
 			dev->ctrl_buf, IPHETH_CTRL_BUF_SIZE,
 			IPHETH_CTRL_TIMEOUT);
-	if (retval < 0) {
+	if (retval <= 0) {
 		dev_err(&dev->intf->dev, "%s: usb_control_msg: %d\n",
 			__func__, retval);
 		return retval;
 	}
 
-	if (dev->ctrl_buf[0] == IPHETH_CARRIER_ON) {
+	if ((retval == 1 && dev->ctrl_buf[0] == IPHETH_CARRIER_ON) ||
+	    (retval >= 2 && dev->ctrl_buf[1] == IPHETH_CARRIER_ON)) {
 		netif_carrier_on(dev->net);
 		if (dev->tx_urb->status != -EINPROGRESS)
 			netif_wake_queue(dev->net);
-- 
2.45.1

Re: [PATCH net-next 1/5] usbnet: ipheth: race between ipheth_close and error handling

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Tue,  6 Aug 2024 19:28:05 +0200 you wrote:
> From: Oliver Neukum <oneukum@suse.com>
> 
> ipheth_sndbulk_callback() can submit carrier_work
> as a part of its error handling. That means that
> the driver must make sure that the work is cancelled
> after it has made sure that no more URB can terminate
> with an error condition.
> 
> [...]

Here is the summary with links:
  - [net-next,1/5] usbnet: ipheth: race between ipheth_close and error handling
    https://git.kernel.org/netdev/net/c/e5876b088ba0
  - [net-next,2/5] usbnet: ipheth: remove extraneous rx URB length check
    https://git.kernel.org/netdev/net/c/655b46d7a39a
  - [net-next,3/5] usbnet: ipheth: drop RX URBs with no payload
    https://git.kernel.org/netdev/net/c/94d7eeb6c0ef
  - [net-next,4/5] usbnet: ipheth: do not stop RX on failing RX callback
    https://git.kernel.org/netdev/net/c/74efed51e0a4
  - [net-next,5/5] usbnet: ipheth: fix carrier detection in modes 1 and 4
    https://git.kernel.org/netdev/net/c/67927a1b255d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html