| Message ID | 20230527130309.34090-1-forst@pen.gy |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next,v3,1/2] usbnet: ipheth: fix risk of NULL pointer deallocation | expand |
Thanks Paolo! Something like that? Georgi Valkov httpstorm.com nano RTOS > On 30 May 2023, at 2:02 PM, Paolo Abeni <pabeni@redhat.com> wrote: > > Hi, > > On Sat, 2023-05-27 at 15:03 +0200, Foster Snowhill wrote: >> From: Georgi Valkov <gvalkov@gmail.com> >> >> The cleanup precedure in ipheth_probe will attempt to free a >> NULL pointer in dev->ctrl_buf if the memory allocation for >> this buffer is not successful. While kfree ignores NULL pointers, >> and the existing code is safe, it is a better design to rearrange >> the goto labels and avoid this. >> >> Signed-off-by: Georgi Valkov <gvalkov@gmail.com> >> Signed-off-by: Foster Snowhill <forst@pen.gy> > > If you are going to repost (due to changes in patch 2) please update > this patch subj, too. Currently is a bit confusing, something alike > "cleanup the initialization error path" would be more clear. > > Thanks, > > Paolo >
Georgi Valkov httpstorm.com nano RTOS > On 30 May 2023, at 1:58 PM, Paolo Abeni <pabeni@redhat.com> wrote: > > Hi, > > On Sat, 2023-05-27 at 15:03 +0200, Foster Snowhill wrote: >> @@ -116,12 +124,12 @@ static int ipheth_alloc_urbs(struct ipheth_device *iphone) >> if (rx_urb == NULL) >> goto free_tx_urb; >> >> - tx_buf = usb_alloc_coherent(iphone->udev, IPHETH_BUF_SIZE, >> + tx_buf = usb_alloc_coherent(iphone->udev, IPHETH_TX_BUF_SIZE, >> GFP_KERNEL, &tx_urb->transfer_dma); >> if (tx_buf == NULL) >> goto free_rx_urb; >> >> - rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN, >> + rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_RX_BUF_SIZE, >> GFP_KERNEL, &rx_urb->transfer_dma); >> if (rx_buf == NULL) >> goto free_tx_buf; > > Here the driver already knows if the device is in NCM or legacy mode, > so perhaps we could select the buffer size accordingly? You would > probably need to store the actual buffer size somewhere to keep the > buffer handling consistent and simple in later code. > >> @@ -373,12 +489,10 @@ static netdev_tx_t ipheth_tx(struct sk_buff *skb, struct net_device *net) >> } >> >> memcpy(dev->tx_buf, skb->data, skb->len); >> - if (skb->len < IPHETH_BUF_SIZE) >> - memset(dev->tx_buf + skb->len, 0, IPHETH_BUF_SIZE - skb->len); >> >> usb_fill_bulk_urb(dev->tx_urb, udev, >> usb_sndbulkpipe(udev, dev->bulk_out), >> - dev->tx_buf, IPHETH_BUF_SIZE, >> + dev->tx_buf, skb->len, >> ipheth_sndbulk_callback, >> dev); >> dev->tx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; > > This chunk looks unrelated from NCM support, and unconditionally > changes the established behaviour even with legacy mode, why? > > Does that works even with old(er) devices? Yes, Tested-on: iPhone 7 Plus, iOS 15.7.6 Testen-on: iPhone 4s, iOS 8.4 Tested-on: iPhone 3G, iOS 4.2.1 All work without any issues.
Hello Paolo, Thank you for the review! >> - rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN, >> + rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_RX_BUF_SIZE, > > Here the driver already knows if the device is in NCM or legacy mode, > so perhaps we could select the buffer size accordingly? You would > probably need to store the actual buffer size somewhere to keep the > buffer handling consistent and simple in later code. Agreed, I will make the buffer size dynamic in the next revision. The RX buffer size is 1516 bytes for legacy mode (2 bytes padding + 1514 bytes Ethernet frame), and 65536 bytes for NCM mode. >> memcpy(dev->tx_buf, skb->data, skb->len); >> - if (skb->len < IPHETH_BUF_SIZE) >> - memset(dev->tx_buf + skb->len, 0, IPHETH_BUF_SIZE - skb->len); >> >> usb_fill_bulk_urb(dev->tx_urb, udev, >> usb_sndbulkpipe(udev, dev->bulk_out), >> - dev->tx_buf, IPHETH_BUF_SIZE, >> + dev->tx_buf, skb->len, >> ipheth_sndbulk_callback, >> dev); >> dev->tx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; > > This chunk looks unrelated from NCM support, and unconditionally > changes the established behaviour even with legacy mode, why? > > Does that works even with old(er) devices? I see Georgi Valkov said he tested v3 of the patch on older iOS devices and confirmed it working. I'll chat with him to get some USB traffic captures, to check what is macOS' behaviour with such devices (to make sure we behave the same way as the official driver). I also wanted to investigate a bit, when was NCM support even added to iOS. Personally I remember testing this in legacy mode a while ago, before I implemented NCM. I will re-test it again in legacy mode in addition to Georgi's efforts.
On Wed, 2023-05-31 at 17:10 +0200, Foster Snowhill wrote: > > > > > memcpy(dev->tx_buf, skb->data, skb->len); > > > - if (skb->len < IPHETH_BUF_SIZE) > > > - memset(dev->tx_buf + skb->len, 0, IPHETH_BUF_SIZE - skb->len); > > > > > > usb_fill_bulk_urb(dev->tx_urb, udev, > > > usb_sndbulkpipe(udev, dev->bulk_out), > > > - dev->tx_buf, IPHETH_BUF_SIZE, > > > + dev->tx_buf, skb->len, > > > ipheth_sndbulk_callback, > > > dev); > > > dev->tx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; > > > > This chunk looks unrelated from NCM support, and unconditionally > > changes the established behaviour even with legacy mode, why? > > > > Does that works even with old(er) devices? > > I see Georgi Valkov said he tested v3 of the patch on older iOS devices > and confirmed it working. I'll chat with him to get some USB traffic > captures, to check what is macOS' behaviour with such devices (to make > sure we behave the same way as the official driver). I also wanted to > investigate a bit, when was NCM support even added to iOS. > > Personally I remember testing this in legacy mode a while ago, before > I implemented NCM. I will re-test it again in legacy mode in addition > to Georgi's efforts. > > From my side, I think it's reasonable to split this out into a separate > patch, since it technically applies to the legacy mode as well, and > doesn't (directly) relate to NCM support, as you pointed out. I think that would be the best option, so we have a clear separation between what is needed for NCM support and other improvements. Thanks! Paolo
diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c index 6a769df0b..8875a3d0e 100644 --- a/drivers/net/usb/ipheth.c +++ b/drivers/net/usb/ipheth.c @@ -510,8 +510,8 @@ static int ipheth_probe(struct usb_interface *intf, ipheth_free_urbs(dev); err_alloc_urbs: err_get_macaddr: -err_alloc_ctrl_buf: kfree(dev->ctrl_buf); +err_alloc_ctrl_buf: err_endpoints: free_netdev(netdev); return retval;