* [PATCH V1 1/3] asix: Add rx->ax_skb = NULL after usbnet_skb_return()
2017-08-07 8:50 [PATCH V1 0/3] asix: Improve robustness Dean Jenkins
@ 2017-08-07 8:50 ` Dean Jenkins
2017-08-07 8:50 ` [PATCH V1 2/3] asix: Ensure asix_rx_fixup_info members are all reset Dean Jenkins
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Dean Jenkins @ 2017-08-07 8:50 UTC (permalink / raw)
To: netdev, David Miller, Dean Jenkins; +Cc: linux-usb
In asix_rx_fixup_internal() there is a risk that rx->ax_skb gets
reused after passing the Ethernet frame into the network stack via
usbnet_skb_return().
The risks include:
a) asynchronously freeing rx->ax_skb after passing the netdev buffer
to the NAPI layer which might corrupt the backlog queue.
b) erroneously reusing rx->ax_skb such as calling skb_put_data() multiple
times which causes writing off the end of the netdev buffer.
Therefore add a defensive rx->ax_skb = NULL after usbnet_skb_return()
so that it is not possible to free rx->ax_skb or to apply
skb_put_data() too many times.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/net/usb/asix_common.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 7847436..6983b6b 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -168,8 +168,10 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
if (rx->ax_skb) {
skb_put_data(rx->ax_skb, skb->data + offset,
copy_length);
- if (!rx->remaining)
+ if (!rx->remaining) {
usbnet_skb_return(dev, rx->ax_skb);
+ rx->ax_skb = NULL;
+ }
}
offset += (copy_length + 1) & 0xfffe;
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH V1 2/3] asix: Ensure asix_rx_fixup_info members are all reset
2017-08-07 8:50 [PATCH V1 0/3] asix: Improve robustness Dean Jenkins
2017-08-07 8:50 ` [PATCH V1 1/3] asix: Add rx->ax_skb = NULL after usbnet_skb_return() Dean Jenkins
@ 2017-08-07 8:50 ` Dean Jenkins
2017-08-07 8:50 ` [PATCH V1 3/3] asix: Fix small memory leak in ax88772_unbind() Dean Jenkins
2017-08-07 17:11 ` [PATCH V1 0/3] asix: Improve robustness David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Dean Jenkins @ 2017-08-07 8:50 UTC (permalink / raw)
To: netdev, David Miller, Dean Jenkins; +Cc: linux-usb
There is a risk that the members of the structure asix_rx_fixup_info
become unsynchronised leading to the possibility of a malfunction.
For example, rx->split_head was not being set to false after an
error was detected so potentially could cause a malformed 32-bit
Data header word to be formed.
Therefore add function reset_asix_rx_fixup_info() to reset all the
members of asix_rx_fixup_info so that future processing will start
with known initial conditions.
Also, if (skb->len != offset) becomes true then call
reset_asix_rx_fixup_info() so that the processing of the next URB
starts with known initial conditions. Without the call, the check
does nothing which potentially could lead to a malfunction
when the next URB is processed.
In addition, for robustness, call reset_asix_rx_fixup_info() before
every error path's "return 0". This ensures that the next URB is
processed from known initial conditions.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/net/usb/asix_common.c | 34 +++++++++++++++++++++++++---------
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 6983b6b..fda74f3 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -75,6 +75,27 @@ void asix_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value, u16 index,
value, index, data, size);
}
+static void reset_asix_rx_fixup_info(struct asix_rx_fixup_info *rx)
+{
+ /* Reset the variables that have a lifetime outside of
+ * asix_rx_fixup_internal() so that future processing starts from a
+ * known set of initial conditions.
+ */
+
+ if (rx->ax_skb) {
+ /* Discard any incomplete Ethernet frame in the netdev buffer */
+ kfree_skb(rx->ax_skb);
+ rx->ax_skb = NULL;
+ }
+
+ /* Assume the Data header 32-bit word is at the start of the current
+ * or next URB socket buffer so reset all the state variables.
+ */
+ rx->remaining = 0;
+ rx->split_head = false;
+ rx->header = 0;
+}
+
int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
struct asix_rx_fixup_info *rx)
{
@@ -99,15 +120,7 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
if (size != ((~rx->header >> 16) & 0x7ff)) {
netdev_err(dev->net, "asix_rx_fixup() Data Header synchronisation was lost, remaining %d\n",
rx->remaining);
- if (rx->ax_skb) {
- kfree_skb(rx->ax_skb);
- rx->ax_skb = NULL;
- /* Discard the incomplete netdev Ethernet frame
- * and assume the Data header is at the start of
- * the current URB socket buffer.
- */
- }
- rx->remaining = 0;
+ reset_asix_rx_fixup_info(rx);
}
}
@@ -139,11 +152,13 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
if (size != ((~rx->header >> 16) & 0x7ff)) {
netdev_err(dev->net, "asix_rx_fixup() Bad Header Length 0x%x, offset %d\n",
rx->header, offset);
+ reset_asix_rx_fixup_info(rx);
return 0;
}
if (size > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
netdev_dbg(dev->net, "asix_rx_fixup() Bad RX Length %d\n",
size);
+ reset_asix_rx_fixup_info(rx);
return 0;
}
@@ -180,6 +195,7 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
if (skb->len != offset) {
netdev_err(dev->net, "asix_rx_fixup() Bad SKB Length %d, %d\n",
skb->len, offset);
+ reset_asix_rx_fixup_info(rx);
return 0;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH V1 3/3] asix: Fix small memory leak in ax88772_unbind()
2017-08-07 8:50 [PATCH V1 0/3] asix: Improve robustness Dean Jenkins
2017-08-07 8:50 ` [PATCH V1 1/3] asix: Add rx->ax_skb = NULL after usbnet_skb_return() Dean Jenkins
2017-08-07 8:50 ` [PATCH V1 2/3] asix: Ensure asix_rx_fixup_info members are all reset Dean Jenkins
@ 2017-08-07 8:50 ` Dean Jenkins
2017-08-07 17:11 ` [PATCH V1 0/3] asix: Improve robustness David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Dean Jenkins @ 2017-08-07 8:50 UTC (permalink / raw)
To: netdev, David Miller, Dean Jenkins; +Cc: linux-usb
When Ethernet frames span mulitple URBs, the netdev buffer memory
pointed to by the asix_rx_fixup_info structure remains allocated
during the time gap between the 2 executions of asix_rx_fixup_internal().
This means that if ax88772_unbind() is called within this time
gap to free the memory of the parent private data structure then
a memory leak of the part filled netdev buffer memory will occur.
Therefore, create a new function asix_rx_fixup_common_free() to
free the memory of the netdev buffer and add a call to
asix_rx_fixup_common_free() from inside ax88772_unbind().
Consequently when an unbind occurs part way through receiving
an Ethernet frame, the netdev buffer memory that is holding part
of the received Ethernet frame will now be freed.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/net/usb/asix.h | 1 +
drivers/net/usb/asix_common.c | 15 +++++++++++++++
drivers/net/usb/asix_devices.c | 1 +
3 files changed, 17 insertions(+)
diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
index d109242..9a4171b 100644
--- a/drivers/net/usb/asix.h
+++ b/drivers/net/usb/asix.h
@@ -209,6 +209,7 @@ void asix_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value,
int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
struct asix_rx_fixup_info *rx);
int asix_rx_fixup_common(struct usbnet *dev, struct sk_buff *skb);
+void asix_rx_fixup_common_free(struct asix_common_private *dp);
struct sk_buff *asix_tx_fixup(struct usbnet *dev, struct sk_buff *skb,
gfp_t flags);
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index fda74f3..522d290 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -210,6 +210,21 @@ int asix_rx_fixup_common(struct usbnet *dev, struct sk_buff *skb)
return asix_rx_fixup_internal(dev, skb, rx);
}
+void asix_rx_fixup_common_free(struct asix_common_private *dp)
+{
+ struct asix_rx_fixup_info *rx;
+
+ if (!dp)
+ return;
+
+ rx = &dp->rx_fixup_info;
+
+ if (rx->ax_skb) {
+ kfree_skb(rx->ax_skb);
+ rx->ax_skb = NULL;
+ }
+}
+
struct sk_buff *asix_tx_fixup(struct usbnet *dev, struct sk_buff *skb,
gfp_t flags)
{
diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index a3aa0a2..b2ff88e 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -764,6 +764,7 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
static void ax88772_unbind(struct usbnet *dev, struct usb_interface *intf)
{
+ asix_rx_fixup_common_free(dev->driver_priv);
kfree(dev->driver_priv);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH V1 0/3] asix: Improve robustness
2017-08-07 8:50 [PATCH V1 0/3] asix: Improve robustness Dean Jenkins
` (2 preceding siblings ...)
2017-08-07 8:50 ` [PATCH V1 3/3] asix: Fix small memory leak in ax88772_unbind() Dean Jenkins
@ 2017-08-07 17:11 ` David Miller
3 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2017-08-07 17:11 UTC (permalink / raw)
To: Dean_Jenkins; +Cc: netdev, linux-usb
From: Dean Jenkins <Dean_Jenkins@mentor.com>
Date: Mon, 7 Aug 2017 09:50:13 +0100
> Please consider taking these patches to improve the robustness of the ASIX USB
> to Ethernet driver.
I applied these to 'net' since they are bug fixes.
Please target bug fixes to 'net' instead of 'net-next' in the
future, unless they are fixing problems which only exist in
'net-next'.
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread