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@xxxxxxxxxxxxxx>
Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
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@xxxxxxxxxxxxxx
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
