Skip to contentSkip to conversation

can: isotp: tx-path: zero initialize outgoing CAN frames

PATCH Superseded Oliver Hartkopp 2 participants 4 messages Mar 18, 2021 – Mar 19, 2021

Subsystems (2)
CAN network drivers CAN network layer

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Oliver Hartkopp

author maintainer
#0 lore raw

PATCH 1/1 can: isotp: tx-path: zero initialize outgoing CAN frames

Commit d4eb538e1f48 ("can: isotp: TX-path: ensure that CAN frame flags are initialized") ensured the TX flags to be properly set for outgoing CAN frames.

In fact the root cause of the issue results from a missing initialization of outgoing CAN frames created by isotp. This is no problem on the CAN bus as the CAN driver only picks the correctly defined content from the struct can(fd)_frame. But when the outgoing frames are monitored (e.g. with candump) we potentially leak some bytes in the unused content of struct can(fd)_frame.

Fixes e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Cc Marc Kleine-Budde <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>
---
 net/can/isotp.c | 3 +++
 1 file changed, 3 insertions(+)
net/can/isotp.c +3 -0
index 430976485d95..3eeb80b28ea8 100644
@@ -195,10 +195,11 @@ static int isotp_send_fc(struct sock *sk, int ae, u8 flowstatus)
 
 	nskb->dev = dev;
 	can_skb_set_owner(nskb, sk);
 	ncf = (struct canfd_frame *)nskb->data;
 	skb_put(nskb, so->ll.mtu);
+	memset(ncf, 0, so->ll.mtu);
 
 	/* create & send flow control reply */
 	ncf->can_id = so->txid;
 
 	if (so->opt.flags & CAN_ISOTP_TX_PADDING) {

@@ -778,10 +779,11 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
 		can_skb_prv(skb)->ifindex = dev->ifindex;
 		can_skb_prv(skb)->skbcnt = 0;
 
 		cf = (struct canfd_frame *)skb->data;
 		skb_put(skb, so->ll.mtu);
+		memset(cf, 0, so->ll.mtu);
 
 		/* create consecutive frame */
 		isotp_fill_dataframe(cf, so, ae, 0);
 
 		/* place consecutive frame N_PCI in appropriate index */

@@ -894,10 +896,11 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	so->tx.len = size;
 	so->tx.idx = 0;
 
 	cf = (struct canfd_frame *)skb->data;
 	skb_put(skb, so->ll.mtu);
+	memset(cf, 0, so->ll.mtu);
 
 	/* check for single frame transmission depending on TX_DL */
 	if (size <= so->tx.ll_dl - SF_PCI_SZ4 - ae - off) {
 		/* The message size generally fits into a SingleFrame - good.
 		 *
Signature · 1 line
2.30.1
maintainer
#1 lore raw
Quoted 9 lines · Oliver Hartkopp

Commit d4eb538e1f48 ("can: isotp: TX-path: ensure that CAN frame flags are initialized") ensured the TX flags to be properly set for outgoing CAN frames.

In fact the root cause of the issue results from a missing initialization of outgoing CAN frames created by isotp. This is no problem on the CAN bus as the CAN driver only picks the correctly defined content from the struct can(fd)_frame. But when the outgoing frames are monitored (e.g. with candump) we potentially leak some bytes in the unused content of struct can(fd)_frame.

Quoted 3 lines
Fixes e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Cc Marc Kleine-Budde <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>

What about skb_put_zero(), which I mentioned in my initial cover letter:

Quoted 3 lines
Quoted 3 lines

Note here the "B" and "E" flags are set. Another possibility is to use skb_put_zero() instead of skb_put(), but with a bigger overhead. A 3. option is to only memset() the non-data part of the struct canfd_frame.

http://lore.kernel.org/r/[email protected]

regards,
Marc

Signature · 4 lines
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 |
author maintainer
#2 lore raw
Quoted 10 lines · Marc Kleine-Budde
Quoted 9 lines · Oliver Hartkopp

Commit d4eb538e1f48 ("can: isotp: TX-path: ensure that CAN frame flags are initialized") ensured the TX flags to be properly set for outgoing CAN frames.

In fact the root cause of the issue results from a missing initialization of outgoing CAN frames created by isotp. This is no problem on the CAN bus as the CAN driver only picks the correctly defined content from the struct can(fd)_frame. But when the outgoing frames are monitored (e.g. with candump) we potentially leak some bytes in the unused content of struct can(fd)_frame.

Quoted 5 lines
Quoted 3 lines
Fixes e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Cc Marc Kleine-Budde <[email protected]>
Signed-off-by Oliver Hartkopp <[email protected]>

What about skb_put_zero(), which I mentioned in my initial cover letter:

Yes, that would indeed be more elegant. Will send a v2 patch.

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

Note here the "B" and "E" flags are set. Another possibility is to use skb_put_zero() instead of skb_put(), but with a bigger overhead. A 3. option is to only memset() the non-data part of the struct canfd_frame.

http://lore.kernel.org/r/[email protected]

I modified candump in a way that it always prints the entire frame independent from can(fd)_frame::len

candump.c +2 -2
index 7bb854a..9683fc9 100644
@@ -719,13 +719,13 @@ int main(int argc, char **argv)
                                perror("read");
                                return 1;
                        }
 
                        if ((size_t)nbytes == CAN_MTU)
-                               maxdlen = CAN_MAX_DLEN;
+                               frame.len = maxdlen = CAN_MAX_DLEN;
                        else if ((size_t)nbytes == CANFD_MTU)
-                               maxdlen = CANFD_MAX_DLEN;
+                               frame.len = maxdlen = CANFD_MAX_DLEN;
                        else {
                                fprintf(stderr, "read: incomplete CAN 

frame\n");
                                return 1;
                        }

And there you can see that in flow-control (FC) frames and consecutive frames (CF) especially at the end of the PDU you see uninitialized content.

So it does not help to only clear the non-data part of the struct canfd_frame.

Best,
Oliver

maintainer
#3 lore raw
Quoted 34 lines · Oliver Hartkopp
Quoted 1 line

What about skb_put_zero(), which I mentioned in my initial cover letter:

Yes, that would indeed be more elegant. Will send a v2 patch.

Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

Note here the "B" and "E" flags are set. Another possibility is to use skb_put_zero() instead of skb_put(), but with a bigger overhead. A 3. option is to only memset() the non-data part of the struct canfd_frame.

http://lore.kernel.org/r/[email protected]

I modified candump in a way that it always prints the entire frame independent from can(fd)_frame::len

candump.c +2 -2
index 7bb854a..9683fc9 100644
@@ -719,13 +719,13 @@ int main(int argc, char **argv)
                                perror("read");
                                return 1;
                        }
 
                        if ((size_t)nbytes == CAN_MTU)
-                               maxdlen = CAN_MAX_DLEN;
+                               frame.len = maxdlen = CAN_MAX_DLEN;
                        else if ((size_t)nbytes == CANFD_MTU)
-                               maxdlen = CANFD_MAX_DLEN;
+                               frame.len = maxdlen = CANFD_MAX_DLEN;
                        else {
                                fprintf(stderr, "read: incomplete CAN
 frame\n");
                                return 1;
                        }
Quoted 5 lines

And there you can see that in flow-control (FC) frames and consecutive frames (CF) especially at the end of the PDU you see uninitialized content.

So it does not help to only clear the non-data part of the struct canfd_frame.

Makes sense, my emphasis was on skb_put_zero() instead of skb_put().

regards,
Marc

Signature · 4 lines
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 |