* [PATCH 0/3] Fix several use after free bugs.
@ 2021-01-19 16:25 Vincent Mailhol
2021-01-19 16:25 ` [PATCH 1/3] can: dev: can_restart: fix use after free bug Vincent Mailhol
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Vincent Mailhol @ 2021-01-19 16:25 UTC (permalink / raw
To: Marc Kleine-Budde, Oliver Hartkopp, linux-can
Cc: Wolfgang Grandegger, Stephane Grosjean, Loris Fauster,
Alejandro Concepcion Rodriguez, Dan Carpenter, Vincent Mailhol
This series fix three bugs which all have the same root cause.
When calling netif_rx(skb) and its variants, the skb will eventually
get consumed (or freed) and thus it is unsafe to dereference it after
the call returns.
This remark especially applies to any variable with aliases the skb
memory which is the case of the can(fd)_frame.
The pattern is as this:
skb = alloc_can_skb(dev, &cf);
/* Do stuff */
netif_rx(skb);
stats->rx_bytes += cf->len;
Increasing the stats should be done *before* the call to netif_rx()
while the skb is still safe to use.
Vincent Mailhol (3):
can: dev: can_restart: fix use after free bug
can: vxcan: vxcan_xmit: fix use after free bug
can: peak_usb: fix use after free bugs
drivers/net/can/dev/dev.c | 4 ++--
drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 8 ++++----
drivers/net/can/vxcan.c | 6 ++++--
3 files changed, 10 insertions(+), 8 deletions(-)
base-commit: 1105592cb8fdfcc96f2c9c693ff4106bac5fac7c
prerequisite-patch-id: d9d54d9159b70a5ef179d19d5add20caffbae638
--
2.26.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] can: dev: can_restart: fix use after free bug
2021-01-19 16:25 [PATCH 0/3] Fix several use after free bugs Vincent Mailhol
@ 2021-01-19 16:25 ` Vincent Mailhol
2021-01-19 16:25 ` [PATCH 2/3] can: vxcan: vxcan_xmit: " Vincent Mailhol
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Vincent Mailhol @ 2021-01-19 16:25 UTC (permalink / raw
To: Marc Kleine-Budde, Oliver Hartkopp, linux-can
Cc: Wolfgang Grandegger, Stephane Grosjean, Loris Fauster,
Alejandro Concepcion Rodriguez, Dan Carpenter, Vincent Mailhol
After calling netif_rx_ni(skb), dereferencing skb is unsafe.
Especially, the can_frame cf which aliases skb memory is accessed
after the netif_rx_ni() in:
stats->rx_bytes += cf->len;
Reordering the lines solves the issue.
fixes: 39549eef3587 ("can: CAN Network device driver and Netlink interface")
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/dev/dev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index f580f0ac6497..01e4a194f187 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -153,11 +153,11 @@ static void can_restart(struct net_device *dev)
cf->can_id |= CAN_ERR_RESTARTED;
- netif_rx_ni(skb);
-
stats->rx_packets++;
stats->rx_bytes += cf->len;
+ netif_rx_ni(skb);
+
restart:
netdev_dbg(dev, "restarted\n");
priv->can_stats.restarts++;
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] can: vxcan: vxcan_xmit: fix use after free bug
2021-01-19 16:25 [PATCH 0/3] Fix several use after free bugs Vincent Mailhol
2021-01-19 16:25 ` [PATCH 1/3] can: dev: can_restart: fix use after free bug Vincent Mailhol
@ 2021-01-19 16:25 ` Vincent Mailhol
2021-01-19 17:07 ` Vincent MAILHOL
2021-01-19 16:25 ` [PATCH 3/3] can: peak_usb: fix use after free bugs Vincent Mailhol
2021-01-20 8:53 ` [PATCH 0/3] Fix several " Marc Kleine-Budde
3 siblings, 1 reply; 8+ messages in thread
From: Vincent Mailhol @ 2021-01-19 16:25 UTC (permalink / raw
To: Marc Kleine-Budde, Oliver Hartkopp, linux-can
Cc: Wolfgang Grandegger, Stephane Grosjean, Loris Fauster,
Alejandro Concepcion Rodriguez, Dan Carpenter, Vincent Mailhol
After calling netif_rx_ni(skb), dereferencing skb is unsafe.
Especially, the canfd_frame cfd which aliases skb memory is accessed
after the netif_rx_ni().
fixes: a8f820a380a2 ("can: add Virtual CAN Tunnel driver (vxcan)")
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/vxcan.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index fa47bab510bb..a525ef8d19b0 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -39,6 +39,7 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
struct net_device *peer;
struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
struct net_device_stats *peerstats, *srcstats = &dev->stats;
+ u8 len;
if (can_dropped_invalid_skb(dev, skb))
return NETDEV_TX_OK;
@@ -61,12 +62,13 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
skb->dev = peer;
skb->ip_summed = CHECKSUM_UNNECESSARY;
+ u8 len = cfd->len;
if (netif_rx_ni(skb) == NET_RX_SUCCESS) {
srcstats->tx_packets++;
- srcstats->tx_bytes += cfd->len;
+ srcstats->tx_bytes += len;
peerstats = &peer->stats;
peerstats->rx_packets++;
- peerstats->rx_bytes += cfd->len;
+ peerstats->rx_bytes += len;
}
out_unlock:
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] can: peak_usb: fix use after free bugs
2021-01-19 16:25 [PATCH 0/3] Fix several use after free bugs Vincent Mailhol
2021-01-19 16:25 ` [PATCH 1/3] can: dev: can_restart: fix use after free bug Vincent Mailhol
2021-01-19 16:25 ` [PATCH 2/3] can: vxcan: vxcan_xmit: " Vincent Mailhol
@ 2021-01-19 16:25 ` Vincent Mailhol
2021-01-20 8:53 ` [PATCH 0/3] Fix several " Marc Kleine-Budde
3 siblings, 0 replies; 8+ messages in thread
From: Vincent Mailhol @ 2021-01-19 16:25 UTC (permalink / raw
To: Marc Kleine-Budde, Oliver Hartkopp, linux-can
Cc: Wolfgang Grandegger, Stephane Grosjean, Loris Fauster,
Alejandro Concepcion Rodriguez, Dan Carpenter, Vincent Mailhol
After calling peak_usb_netif_rx_ni(skb), dereferencing skb is unsafe.
Especially, the can_frame cf which aliases skb memory is accessed
after the peak_usb_netif_rx_ni().
Reordering the lines solves the issue.
fixes: 0a25e1f4f185 ("can: peak_usb: add support for PEAK new CANFD USB adapters")
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
index 61631f4fd92a..f347ecc79aef 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
@@ -514,11 +514,11 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
else
memcpy(cfd->data, rm->d, cfd->len);
- peak_usb_netif_rx(skb, &usb_if->time_ref, le32_to_cpu(rm->ts_low));
-
netdev->stats.rx_packets++;
netdev->stats.rx_bytes += cfd->len;
+ peak_usb_netif_rx(skb, &usb_if->time_ref, le32_to_cpu(rm->ts_low));
+
return 0;
}
@@ -580,11 +580,11 @@ static int pcan_usb_fd_decode_status(struct pcan_usb_fd_if *usb_if,
if (!skb)
return -ENOMEM;
- peak_usb_netif_rx(skb, &usb_if->time_ref, le32_to_cpu(sm->ts_low));
-
netdev->stats.rx_packets++;
netdev->stats.rx_bytes += cf->len;
+ peak_usb_netif_rx(skb, &usb_if->time_ref, le32_to_cpu(sm->ts_low));
+
return 0;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] can: vxcan: vxcan_xmit: fix use after free bug
2021-01-19 16:25 ` [PATCH 2/3] can: vxcan: vxcan_xmit: " Vincent Mailhol
@ 2021-01-19 17:07 ` Vincent MAILHOL
2021-01-19 22:16 ` Oliver Hartkopp
0 siblings, 1 reply; 8+ messages in thread
From: Vincent MAILHOL @ 2021-01-19 17:07 UTC (permalink / raw
To: Marc Kleine-Budde, Oliver Hartkopp, linux-can
Cc: Wolfgang Grandegger, Stephane Grosjean, Loris Fauster,
Alejandro Concepcion Rodriguez, Dan Carpenter
On Wed. 20 Jan 2021 at 01:25, Vincent Mailhol
<mailhol.vincent@wanadoo.fr> wrote:
>
> After calling netif_rx_ni(skb), dereferencing skb is unsafe.
> Especially, the canfd_frame cfd which aliases skb memory is accessed
> after the netif_rx_ni().
>
> fixes: a8f820a380a2 ("can: add Virtual CAN Tunnel driver (vxcan)")
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> drivers/net/can/vxcan.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
> index fa47bab510bb..a525ef8d19b0 100644
> --- a/drivers/net/can/vxcan.c
> +++ b/drivers/net/can/vxcan.c
> @@ -39,6 +39,7 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
> struct net_device *peer;
> struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> struct net_device_stats *peerstats, *srcstats = &dev->stats;
> + u8 len;
>
> if (can_dropped_invalid_skb(dev, skb))
> return NETDEV_TX_OK;
> @@ -61,12 +62,13 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
> skb->dev = peer;
> skb->ip_summed = CHECKSUM_UNNECESSARY;
>
> + u8 len = cfd->len;
len = cfd->len;
Silly mistake: u8 not needed twice of course...
> if (netif_rx_ni(skb) == NET_RX_SUCCESS) {
> srcstats->tx_packets++;
> - srcstats->tx_bytes += cfd->len;
> + srcstats->tx_bytes += len;
> peerstats = &peer->stats;
> peerstats->rx_packets++;
> - peerstats->rx_bytes += cfd->len;
> + peerstats->rx_bytes += len;
> }
>
> out_unlock:
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] can: vxcan: vxcan_xmit: fix use after free bug
2021-01-19 17:07 ` Vincent MAILHOL
@ 2021-01-19 22:16 ` Oliver Hartkopp
2021-01-20 0:34 ` Vincent MAILHOL
0 siblings, 1 reply; 8+ messages in thread
From: Oliver Hartkopp @ 2021-01-19 22:16 UTC (permalink / raw
To: Vincent MAILHOL, Marc Kleine-Budde, linux-can
Cc: Wolfgang Grandegger, Stephane Grosjean, Loris Fauster,
Alejandro Concepcion Rodriguez, Dan Carpenter
On 19.01.21 18:07, Vincent MAILHOL wrote:
> On Wed. 20 Jan 2021 at 01:25, Vincent Mailhol
> <mailhol.vincent@wanadoo.fr> wrote:
>>
>> After calling netif_rx_ni(skb), dereferencing skb is unsafe.
>> Especially, the canfd_frame cfd which aliases skb memory is accessed
>> after the netif_rx_ni().
>>
>> fixes: a8f820a380a2 ("can: add Virtual CAN Tunnel driver (vxcan)")
>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>> ---
>> drivers/net/can/vxcan.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
>> index fa47bab510bb..a525ef8d19b0 100644
>> --- a/drivers/net/can/vxcan.c
>> +++ b/drivers/net/can/vxcan.c
>> @@ -39,6 +39,7 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
>> struct net_device *peer;
>> struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>> struct net_device_stats *peerstats, *srcstats = &dev->stats;
>> + u8 len;
>>
>> if (can_dropped_invalid_skb(dev, skb))
>> return NETDEV_TX_OK;
>> @@ -61,12 +62,13 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
>> skb->dev = peer;
>> skb->ip_summed = CHECKSUM_UNNECESSARY;
>>
>> + u8 len = cfd->len;
>
> len = cfd->len;
> Silly mistake: u8 not needed twice of course...
not tested -> compile tested -> runtime tested
Choose your level!
:-D
>
>> if (netif_rx_ni(skb) == NET_RX_SUCCESS) {
>> srcstats->tx_packets++;
>> - srcstats->tx_bytes += cfd->len;
>> + srcstats->tx_bytes += len;
>> peerstats = &peer->stats;
>> peerstats->rx_packets++;
>> - peerstats->rx_bytes += cfd->len;
>> + peerstats->rx_bytes += len;
>> }
>>
>> out_unlock:
>> --
>> 2.26.2
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] can: vxcan: vxcan_xmit: fix use after free bug
2021-01-19 22:16 ` Oliver Hartkopp
@ 2021-01-20 0:34 ` Vincent MAILHOL
0 siblings, 0 replies; 8+ messages in thread
From: Vincent MAILHOL @ 2021-01-20 0:34 UTC (permalink / raw
To: Oliver Hartkopp
Cc: Marc Kleine-Budde, linux-can, Wolfgang Grandegger,
Stephane Grosjean, Loris Fauster, Alejandro Concepcion Rodriguez,
Dan Carpenter
On Wed. 20 janv. 2021 at 07:16, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 19.01.21 18:07, Vincent MAILHOL wrote:
> > On Wed. 20 Jan 2021 at 01:25, Vincent Mailhol
> > <mailhol.vincent@wanadoo.fr> wrote:
> >>
> >> After calling netif_rx_ni(skb), dereferencing skb is unsafe.
> >> Especially, the canfd_frame cfd which aliases skb memory is accessed
> >> after the netif_rx_ni().
> >>
> >> fixes: a8f820a380a2 ("can: add Virtual CAN Tunnel driver (vxcan)")
> >> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >> ---
> >> drivers/net/can/vxcan.c | 6 ++++--
> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
> >> index fa47bab510bb..a525ef8d19b0 100644
> >> --- a/drivers/net/can/vxcan.c
> >> +++ b/drivers/net/can/vxcan.c
> >> @@ -39,6 +39,7 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
> >> struct net_device *peer;
> >> struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >> struct net_device_stats *peerstats, *srcstats = &dev->stats;
> >> + u8 len;
> >>
> >> if (can_dropped_invalid_skb(dev, skb))
> >> return NETDEV_TX_OK;
> >> @@ -61,12 +62,13 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
> >> skb->dev = peer;
> >> skb->ip_summed = CHECKSUM_UNNECESSARY;
> >>
> >> + u8 len = cfd->len;
> >
> > len = cfd->len;
> > Silly mistake: u8 not needed twice of course...
>
> not tested -> compile tested -> runtime tested
>
> Choose your level!
>
> :-D
I did all the tests on the first patch (can_restart). For the
other two patches, I greped for similar patterns and just test
compile, but I forgot to select the vxcan module thus could not
see the warning.
I take the blame :-)
> >
> >> if (netif_rx_ni(skb) == NET_RX_SUCCESS) {
> >> srcstats->tx_packets++;
> >> - srcstats->tx_bytes += cfd->len;
> >> + srcstats->tx_bytes += len;
> >> peerstats = &peer->stats;
> >> peerstats->rx_packets++;
> >> - peerstats->rx_bytes += cfd->len;
> >> + peerstats->rx_bytes += len;
> >> }
> >>
> >> out_unlock:
> >> --
> >> 2.26.2
> >>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] Fix several use after free bugs.
2021-01-19 16:25 [PATCH 0/3] Fix several use after free bugs Vincent Mailhol
` (2 preceding siblings ...)
2021-01-19 16:25 ` [PATCH 3/3] can: peak_usb: fix use after free bugs Vincent Mailhol
@ 2021-01-20 8:53 ` Marc Kleine-Budde
3 siblings, 0 replies; 8+ messages in thread
From: Marc Kleine-Budde @ 2021-01-20 8:53 UTC (permalink / raw
To: Vincent Mailhol
Cc: Oliver Hartkopp, linux-can, Wolfgang Grandegger,
Stephane Grosjean, Loris Fauster, Alejandro Concepcion Rodriguez,
Dan Carpenter
[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]
On Wed, Jan 20, 2021 at 01:25:09AM +0900, Vincent Mailhol wrote:
> This series fix three bugs which all have the same root cause.
>
> When calling netif_rx(skb) and its variants, the skb will eventually
> get consumed (or freed) and thus it is unsafe to dereference it after
> the call returns.
>
> This remark especially applies to any variable with aliases the skb
> memory which is the case of the can(fd)_frame.
>
> The pattern is as this:
> skb = alloc_can_skb(dev, &cf);
> /* Do stuff */
> netif_rx(skb);
> stats->rx_bytes += cf->len;
>
> Increasing the stats should be done *before* the call to netif_rx()
> while the skb is still safe to use.
>
> Vincent Mailhol (3):
> can: dev: can_restart: fix use after free bug
> can: vxcan: vxcan_xmit: fix use after free bug
> can: peak_usb: fix use after free bugs
>
> drivers/net/can/dev/dev.c | 4 ++--
> drivers/net/can/usb/peak_usb/pcan_usb_fd.c | 8 ++++----
> drivers/net/can/vxcan.c | 6 ++++--
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
>
> base-commit: 1105592cb8fdfcc96f2c9c693ff4106bac5fac7c
> prerequisite-patch-id: d9d54d9159b70a5ef179d19d5add20caffbae638
As this are fixes, this should go into net/master (and then be be backported to
the stable kernels). Please rebase to net/master.
Of course there will be a merge conflict, when net-next and net are merged, due
to the moving and splitting of dev.c. You anticipated this and made noted that
as a prerequisite. (BTW: I don't find a commit for
d9d54d9159b70a5ef179d19d5add20caffbae638).
The kernel way to deal with this is to inform the upstream of the problem. A
trivial merge conflict, can be I think described in words, like: "The dev.c
file has been moved, carry the patch forward." I don't know the procedure for
more complicated merges :)
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-20 8:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-19 16:25 [PATCH 0/3] Fix several use after free bugs Vincent Mailhol
2021-01-19 16:25 ` [PATCH 1/3] can: dev: can_restart: fix use after free bug Vincent Mailhol
2021-01-19 16:25 ` [PATCH 2/3] can: vxcan: vxcan_xmit: " Vincent Mailhol
2021-01-19 17:07 ` Vincent MAILHOL
2021-01-19 22:16 ` Oliver Hartkopp
2021-01-20 0:34 ` Vincent MAILHOL
2021-01-19 16:25 ` [PATCH 3/3] can: peak_usb: fix use after free bugs Vincent Mailhol
2021-01-20 8:53 ` [PATCH 0/3] Fix several " Marc Kleine-Budde
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.