[PATCH net-next v2 1/2] usbnet: ipheth: fix risk of NULL pointer deallocation

[PATCH net-next v2 1/2] usbnet: ipheth: fix risk of NULL pointer deallocation

[Foster Snowhill]

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. Rearrange the goto labels to
avoid this risk.

Signed-off-by: Georgi Valkov <gvalkov@gmail.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 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;
-- 
2.40.1

[PATCH net-next v2 2/2] usbnet: ipheth: add CDC NCM support

[Foster Snowhill]

Recent iOS releases support CDC NCM encapsulation on RX. This mode is
the default on macOS and Windows. In this mode, an iOS device may include
one or more Ethernet frames inside a single URB.

Freshly booted iOS devices start in legacy mode, but are put into
NCM mode by the official Apple driver. When reconnecting such a device
from a macOS/Windows machine to a Linux host, the device stays in
NCM mode, making it unusable with the legacy ipheth driver code.

To correctly support such a device, the driver has to either support
the NCM mode too, or put the device back into legacy mode.

To match the behaviour of the macOS/Windows driver, and since there
is no documented control command to revert to legacy mode, implement
NCM support. The device is attempted to be put into NCM mode by default,
and falls back to legacy mode if the attempt fails.

Signed-off-by: Foster Snowhill <forst@pen.gy>
Tested-by: Georgi Valkov <gvalkov@gmail.com>
---
v2:
  - Fix code formatting (RCS, 80 col width, remove redundant type casts)
  - Drop an unrelated goto label-related hunk from this patch
v1: https://lore.kernel.org/netdev/20230516210127.35841-1-forst@pen.gy/

v2 tested by me on net-next, amd64, Ubuntu 23.04, iPhone Xs Max, iOS 16.5.

There is an interest, specifically from Georgi, to have this patch
backported to v5.15 to then be used in OpenWrt.

Neither me nor Georgi are by any means skilled in developing for the
Linux kernel. Please review the patch carefully and advise if any
changes are needed in regards to security (e.g. data validation)
or code formatting.
---
 drivers/net/usb/ipheth.c | 185 ++++++++++++++++++++++++++++++++-------
 1 file changed, 152 insertions(+), 33 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 8875a3d0e..6677ac700 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -52,6 +52,7 @@
 #include <linux/ethtool.h>
 #include <linux/usb.h>
 #include <linux/workqueue.h>
+#include <linux/usb/cdc.h>
 
 #define USB_VENDOR_APPLE        0x05ac
 
@@ -59,8 +60,11 @@
 #define IPHETH_USBINTF_SUBCLASS 253
 #define IPHETH_USBINTF_PROTO    1
 
-#define IPHETH_BUF_SIZE         1514
 #define IPHETH_IP_ALIGN		2	/* padding at front of URB */
+#define IPHETH_NCM_HEADER_SIZE  (12 + 96) /* NCMH + NCM0 */
+#define IPHETH_TX_BUF_SIZE      ETH_FRAME_LEN
+#define IPHETH_RX_BUF_SIZE      65536
+
 #define IPHETH_TX_TIMEOUT       (5 * HZ)
 
 #define IPHETH_INTFNUM          2
@@ -71,6 +75,7 @@
 #define IPHETH_CTRL_TIMEOUT     (5 * HZ)
 
 #define IPHETH_CMD_GET_MACADDR   0x00
+#define IPHETH_CMD_ENABLE_NCM    0x04
 #define IPHETH_CMD_CARRIER_CHECK 0x45
 
 #define IPHETH_CARRIER_CHECK_TIMEOUT round_jiffies_relative(1 * HZ)
@@ -84,6 +89,8 @@ static const struct usb_device_id ipheth_table[] = {
 };
 MODULE_DEVICE_TABLE(usb, ipheth_table);
 
+static const char ipheth_start_packet[] = { 0x00, 0x01, 0x01, 0x00 };
+
 struct ipheth_device {
 	struct usb_device *udev;
 	struct usb_interface *intf;
@@ -97,6 +104,7 @@ struct ipheth_device {
 	u8 bulk_out;
 	struct delayed_work carrier_work;
 	bool confirmed_pairing;
+	int (*rcvbulk_callback)(struct urb *urb);
 };
 
 static int ipheth_rx_submit(struct ipheth_device *dev, gfp_t mem_flags);
@@ -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;
@@ -134,7 +142,7 @@ static int ipheth_alloc_urbs(struct ipheth_device *iphone)
 	return 0;
 
 free_tx_buf:
-	usb_free_coherent(iphone->udev, IPHETH_BUF_SIZE, tx_buf,
+	usb_free_coherent(iphone->udev, IPHETH_TX_BUF_SIZE, tx_buf,
 			  tx_urb->transfer_dma);
 free_rx_urb:
 	usb_free_urb(rx_urb);
@@ -146,9 +154,9 @@ static int ipheth_alloc_urbs(struct ipheth_device *iphone)
 
 static void ipheth_free_urbs(struct ipheth_device *iphone)
 {
-	usb_free_coherent(iphone->udev, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN, iphone->rx_buf,
+	usb_free_coherent(iphone->udev, IPHETH_RX_BUF_SIZE, iphone->rx_buf,
 			  iphone->rx_urb->transfer_dma);
-	usb_free_coherent(iphone->udev, IPHETH_BUF_SIZE, iphone->tx_buf,
+	usb_free_coherent(iphone->udev, IPHETH_TX_BUF_SIZE, iphone->tx_buf,
 			  iphone->tx_urb->transfer_dma);
 	usb_free_urb(iphone->rx_urb);
 	usb_free_urb(iphone->tx_urb);
@@ -160,14 +168,105 @@ static void ipheth_kill_urbs(struct ipheth_device *dev)
 	usb_kill_urb(dev->rx_urb);
 }
 
-static void ipheth_rcvbulk_callback(struct urb *urb)
+static int ipheth_consume_skb(char *buf, int len, struct ipheth_device *dev)
 {
-	struct ipheth_device *dev;
 	struct sk_buff *skb;
-	int status;
+
+	skb = dev_alloc_skb(len);
+	if (!skb) {
+		dev->net->stats.rx_dropped++;
+		return -ENOMEM;
+	}
+
+	skb_put_data(skb, buf, len);
+	skb->dev = dev->net;
+	skb->protocol = eth_type_trans(skb, dev->net);
+
+	dev->net->stats.rx_packets++;
+	dev->net->stats.rx_bytes += len;
+	netif_rx(skb);
+
+	return 0;
+}
+
+static int ipheth_rcvbulk_callback_legacy(struct urb *urb)
+{
+	struct ipheth_device *dev;
 	char *buf;
 	int len;
 
+	dev = urb->context;
+
+	if (urb->actual_length <= IPHETH_IP_ALIGN) {
+		dev->net->stats.rx_length_errors++;
+		return -EINVAL;
+	}
+	len = urb->actual_length - IPHETH_IP_ALIGN;
+	buf = urb->transfer_buffer + IPHETH_IP_ALIGN;
+
+	return ipheth_consume_skb(buf, len, dev);
+}
+
+static int ipheth_rcvbulk_callback_ncm(struct urb *urb)
+{
+	struct usb_cdc_ncm_nth16 *ncmh;
+	struct usb_cdc_ncm_ndp16 *ncm0;
+	struct usb_cdc_ncm_dpe16 *dpe;
+	struct ipheth_device *dev;
+	int retval = -EINVAL;
+	char *buf;
+	int len;
+
+	dev = urb->context;
+
+	if (urb->actual_length < IPHETH_NCM_HEADER_SIZE) {
+		dev->net->stats.rx_length_errors++;
+		return retval;
+	}
+
+	ncmh = urb->transfer_buffer;
+	if (ncmh->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN) ||
+	    le16_to_cpu(ncmh->wNdpIndex) >= urb->actual_length) {
+		dev->net->stats.rx_errors++;
+		return retval;
+	}
+
+	ncm0 = urb->transfer_buffer + le16_to_cpu(ncmh->wNdpIndex);
+	if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN) ||
+	    le16_to_cpu(ncmh->wHeaderLength) + le16_to_cpu(ncm0->wLength) >=
+	    urb->actual_length) {
+		dev->net->stats.rx_errors++;
+		return retval;
+	}
+
+	dpe = ncm0->dpe16;
+	while (le16_to_cpu(dpe->wDatagramIndex) != 0 &&
+	       le16_to_cpu(dpe->wDatagramLength) != 0) {
+		if (le16_to_cpu(dpe->wDatagramIndex) >= urb->actual_length ||
+		    le16_to_cpu(dpe->wDatagramIndex) +
+		    le16_to_cpu(dpe->wDatagramLength) > urb->actual_length) {
+			dev->net->stats.rx_length_errors++;
+			return retval;
+		}
+
+		buf = urb->transfer_buffer + le16_to_cpu(dpe->wDatagramIndex);
+		len = le16_to_cpu(dpe->wDatagramLength);
+
+		retval = ipheth_consume_skb(buf, len, dev);
+		if (retval != 0)
+			return retval;
+
+		dpe++;
+	}
+
+	return 0;
+}
+
+static void ipheth_rcvbulk_callback(struct urb *urb)
+{
+	struct ipheth_device *dev;
+	int retval, status;
+
 	dev = urb->context;
 	if (dev == NULL)
 		return;
@@ -187,29 +286,25 @@ static void ipheth_rcvbulk_callback(struct urb *urb)
 		return;
 	}
 
-	if (urb->actual_length <= IPHETH_IP_ALIGN) {
-		dev->net->stats.rx_length_errors++;
-		return;
-	}
-	len = urb->actual_length - IPHETH_IP_ALIGN;
-	buf = urb->transfer_buffer + IPHETH_IP_ALIGN;
-
-	skb = dev_alloc_skb(len);
-	if (!skb) {
-		dev_err(&dev->intf->dev, "%s: dev_alloc_skb: -ENOMEM\n",
-			__func__);
-		dev->net->stats.rx_dropped++;
+	/* The very first frame we receive from device has a fixed 4-byte value
+	 * We can safely skip it
+	 */
+	if (unlikely
+		(urb->actual_length == sizeof(ipheth_start_packet) &&
+		 memcmp(urb->transfer_buffer, ipheth_start_packet,
+			sizeof(ipheth_start_packet)) == 0
+	))
+		goto rx_submit;
+
+	retval = dev->rcvbulk_callback(urb);
+	if (retval != 0) {
+		dev_err(&dev->intf->dev, "%s: callback retval: %d\n",
+			__func__, retval);
 		return;
 	}
 
-	skb_put_data(skb, buf, len);
-	skb->dev = dev->net;
-	skb->protocol = eth_type_trans(skb, dev->net);
-
-	dev->net->stats.rx_packets++;
-	dev->net->stats.rx_bytes += len;
+rx_submit:
 	dev->confirmed_pairing = true;
-	netif_rx(skb);
 	ipheth_rx_submit(dev, GFP_ATOMIC);
 }
 
@@ -310,6 +405,27 @@ static int ipheth_get_macaddr(struct ipheth_device *dev)
 	return retval;
 }
 
+static int ipheth_enable_ncm(struct ipheth_device *dev)
+{
+	struct usb_device *udev = dev->udev;
+	int retval;
+
+	retval = usb_control_msg(udev,
+				 usb_sndctrlpipe(udev, IPHETH_CTRL_ENDP),
+				 IPHETH_CMD_ENABLE_NCM, /* request */
+				 0x40, /* request type */
+				 0x00, /* value */
+				 0x02, /* index */
+				 NULL,
+				 0,
+				 IPHETH_CTRL_TIMEOUT);
+
+	dev_info(&dev->intf->dev, "%s: usb_control_msg: %d\n",
+		 __func__, retval);
+
+	return retval;
+}
+
 static int ipheth_rx_submit(struct ipheth_device *dev, gfp_t mem_flags)
 {
 	struct usb_device *udev = dev->udev;
@@ -317,7 +433,7 @@ static int ipheth_rx_submit(struct ipheth_device *dev, gfp_t mem_flags)
 
 	usb_fill_bulk_urb(dev->rx_urb, udev,
 			  usb_rcvbulkpipe(udev, dev->bulk_in),
-			  dev->rx_buf, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN,
+			  dev->rx_buf, IPHETH_RX_BUF_SIZE,
 			  ipheth_rcvbulk_callback,
 			  dev);
 	dev->rx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
@@ -365,7 +481,7 @@ static netdev_tx_t ipheth_tx(struct sk_buff *skb, struct net_device *net)
 	int retval;
 
 	/* Paranoid */
-	if (skb->len > IPHETH_BUF_SIZE) {
+	if (skb->len > IPHETH_TX_BUF_SIZE) {
 		WARN(1, "%s: skb too large: %d bytes\n", __func__, skb->len);
 		dev->net->stats.tx_dropped++;
 		dev_kfree_skb_any(skb);
@@ -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;
@@ -450,6 +564,7 @@ static int ipheth_probe(struct usb_interface *intf,
 	dev->net = netdev;
 	dev->intf = intf;
 	dev->confirmed_pairing = false;
+	dev->rcvbulk_callback = ipheth_rcvbulk_callback_legacy;
 	/* Set up endpoints */
 	hintf = usb_altnum_to_altsetting(intf, IPHETH_ALT_INTFNUM);
 	if (hintf == NULL) {
@@ -481,6 +596,10 @@ static int ipheth_probe(struct usb_interface *intf,
 	if (retval)
 		goto err_get_macaddr;
 
+	retval = ipheth_enable_ncm(dev);
+	if (!retval)
+		dev->rcvbulk_callback = ipheth_rcvbulk_callback_ncm;
+
 	INIT_DELAYED_WORK(&dev->carrier_work, ipheth_carrier_check_work);
 
 	retval = ipheth_alloc_urbs(dev);
-- 
2.40.1

Re: [PATCH net-next v2 1/2] usbnet: ipheth: fix risk of NULL pointer deallocation

[Simon Horman]

On Thu, May 25, 2023 at 09:42:54PM +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. Rearrange the goto labels to
> avoid this risk.

Hi Georgi and Foster,

kfree will ignore a NULL argument, so I think the existing code is safe.
But given the name of the label I do agree there is scope for a cleanup
here.

Could you consider rewording the patch description accordingly?

> Signed-off-by: Georgi Valkov <gvalkov@gmail.com>

If Georgi is the author of the patch, which seems to be the case,
then the above is correct. But as the patch is being posted by Foster
I think it should be followed by a Signed-off-by line for Foster.

Link: https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed+off#developer-s-certificate-of-origin-1-1

> ---
>  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 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;

-- 
pw-bot: cr

Re: [PATCH net-next v2 1/2] usbnet: ipheth: fix risk of NULL pointer deallocation

[George Valkov]

[-- Attachment #1: Type: text/plain, Size: 1730 bytes --]


> On 26 May 2023, at 10:52 AM, Simon Horman <simon.horman@corigine.com> wrote:
> 
> On Thu, May 25, 2023 at 09:42:54PM +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. Rearrange the goto labels to
>> avoid this risk.
> 
> Hi Georgi and Foster,
> 
> kfree will ignore a NULL argument, so I think the existing code is safe.
> But given the name of the label I do agree there is scope for a cleanup
> here.

It’s good to know that precaution has been taken in kfree to avoid this, yet at my opinion knowingly attempting to free a NULL pointer is a red flag and bad design. Likely a misplaced label.

> Could you consider rewording the patch description accordingly?

What would you like me to use as title and description? Can I use this?

usbnet: ipheth: avoid kfree with a NULL pointer

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>
> 
> If Georgi is the author of the patch, which seems to be the case,
> then the above is correct. But as the patch is being posted by Foster
> I think it should be followed by a Signed-off-by line for Foster.

Yes, I discovered the potential issue and authored the patch to help. We’ll append Signed-off-by Foster as you suggested. Thanks Simon!

Something like that?


[-- Attachment #2: 0001-usbnet-ipheth-avoid-kfree-with-a-NULL-pointer.patch --]
[-- Type: application/octet-stream, Size: 1163 bytes --]

From 15dc5cec0d239d30856dc6d9b9a7a9528342cde0 Mon Sep 17 00:00:00 2001
From: Georgi Valkov <gvalkov@gmail.com>
Date: Thu, 25 May 2023 21:23:12 +0200
Subject: [PATCH net-next v3 1/2] usbnet: ipheth: avoid kfree with a NULL pointer

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>
---
 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 6a769df0b421..8875a3d0e6d9 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;
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 808 bytes --]




Georgi Valkov
httpstorm.com
nano RTOS

> Link: https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed+off#developer-s-certificate-of-origin-1-1
> 
>> ---
>> 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 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;
> 
> -- 
> pw-bot: cr

Re: [PATCH net-next v2 1/2] usbnet: ipheth: fix risk of NULL pointer deallocation

[Simon Horman]

On Fri, May 26, 2023 at 10:33:21AM +0200, George Valkov wrote:
> 
> > On 26 May 2023, at 10:52 AM, Simon Horman <simon.horman@corigine.com> wrote:
> >
> > On Thu, May 25, 2023 at 09:42:54PM +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. Rearrange the goto labels to
> >> avoid this risk.
> >
> > Hi Georgi and Foster,
> >
> > kfree will ignore a NULL argument, so I think the existing code is safe.
> > But given the name of the label I do agree there is scope for a cleanup
> > here.
> 
> It’s good to know that precaution has been taken in kfree to avoid this, yet at
> my opinion knowingly attempting to free a NULL pointer is a red flag and bad
> design. Likely a misplaced label.
> 
> > Could you consider rewording the patch description accordingly?
> 
> What would you like me to use as title and description? Can I use this?
> 
> usbnet: ipheth: avoid kfree with a NULL pointer
> 
> 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.

Thanks, that looks good to me.

> >> Signed-off-by: Georgi Valkov <gvalkov@gmail.com>
> >
> > If Georgi is the author of the patch, which seems to be the case,
> > then the above is correct. But as the patch is being posted by Foster
> > I think it should be followed by a Signed-off-by line for Foster.
> 
> Yes, I discovered the potential issue and authored the patch to help. We’ll
> append Signed-off-by Foster as you suggested. Thanks Simon!
> 
> Something like that?

Yes, I think that sounds good.

Please wait 24h before the posting of v2 before posting v3,
to allow time for more review of v3 (from others).

...