[PATCH] can: isotp: tx-path: zero initialize outgoing CAN frames

[PATCH] can: isotp: tx-path: zero initialize outgoing CAN frames

[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.

Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 net/can/isotp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 430976485d95..3eeb80b28ea8 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -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.
 		 *
-- 
2.30.1

Re: [PATCH] can: isotp: tx-path: zero initialize outgoing CAN frames

[Marc Kleine-Budde]

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

On 18.03.2021 11:02:33, Oliver Hartkopp wrote:
> 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 <mkl@pengutronix.de>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

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

>> 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/20210218215434.1708249-1-mkl@pengutronix.de

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

Re: [PATCH] can: isotp: tx-path: zero initialize outgoing CAN frames

[Oliver Hartkopp]

On 19.03.21 09:26, Marc Kleine-Budde wrote:
> On 18.03.2021 11:02:33, Oliver Hartkopp wrote:
>> 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 <mkl@pengutronix.de>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> 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.

>>> 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/20210218215434.1708249-1-mkl@pengutronix.de

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

diff --git a/candump.c b/candump.c
index 7bb854a..9683fc9 100644
--- a/candump.c
+++ b/candump.c
@@ -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

Re: [PATCH] can: isotp: tx-path: zero initialize outgoing CAN frames

[Marc Kleine-Budde]

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

On 19.03.2021 10:55:07, Oliver Hartkopp wrote:
> > 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.
> 
> > > > 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/20210218215434.1708249-1-mkl@pengutronix.de
> 
> I modified candump in a way that it always prints the entire frame
> independent from can(fd)_frame::len
> 
> diff --git a/candump.c b/candump.c
> index 7bb854a..9683fc9 100644
> --- a/candump.c
> +++ b/candump.c
> @@ -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.

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

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