On 11/19/24 16:41, Vincent Mailhol wrote:
Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
I left comments on the comments. If you have time, it would be wonderful
if your comment on start_xmit() could be moved to can_dev_dropped_skb()
in a separate patch.
The code is good, so if such rework is not feasible, I am happy to take
it as-is.
Thanks for the review! Yes, if you are fine with taking it, I suggest
doing so now, and if I ever get to rework the comments, then I can look
into this as well.
The thing is, when writing this patch, I tried to put myself in Dan's
shoes. That is, someone unfamiliar with the code who is now looking at
it. There's some string manipulation going on (uh-oh!), and we need to
ensure that no buffers are overrun. Naturally, as a reviewer, I'd like
to know that indeed, frame->len <= 8. Hence why I added a comment to
that effect.
But security is not about trusting me, it's about proof. That's where
the comment in _xmit() comes in. And I have to say, while it's easy to
see that can_dev_dropped_skb() ensures len <= 8 in case of ETH_P_CAN,
the guarantee that we only receive ETH_P_CAN packets in the first place
is a whole different story. The MTU checks took a while for me to track
down and understand, so my intention is to leave some breadcrumbs for
the next poor soul digging into can327. Frankly, I think that's a sign
that the otherwise lean CAN stack has finally accumulated some tech debt
(or maybe I'm just bad at reading code), but oh well.
Now, as you say, this is sort of valid for all CAN drivers, and hence it
should be in some centralised documentation. Agreed. But then again, for
this driver, I wanted to make a point that we're dealing with ETH_P_CAN
only, so the comment is also written that way. If you really don't want
that there, I can shorten it and move it, but if it's okay as-is, then
let's leave it in, and at a later stage, we can use it as a template for
some more generic documentation :)
- snprintf(&local_txbuf[2 * i], sizeof(local_txbuf),
- "\r");
+ /* Send a regular CAN data frame.
+ *
+ * frame->len is guaranteed to be <= 8. Please refer
+ * to the comment in can327_netdev_start_xmit().
+ */
Nitpick, could be less verbose, e.g.:
/* frame->len <= 8 enforced in can327_netdev_start_xmit() */
IMHO that's a matter of point of view - for you it may be obvious that
the CAN stack only passes ETH_P_CAN to _xmit(), but to someone new it is
not. And this particular detail is *not* enforced in _xmit(), but in
can_send() in the CAN stack. I hope you can understand my wish to be
precise about this comment, in order to avoid confusion when inevitably
someone else comes looking for overflows. Or, well, if I want to touch
can327 again in six months' time, when I have forgotten about all of this :)
+ /* Why this driver can rely on frame->len <= 8:
+ *
+ * While can_dev_dropped_skb() sanity checks the skb to contain a
+ * CAN 2.0, CAN FD, or other CAN frame type supported by the CAN
+ * stack, it does not restrict these types of CAN frames.
+ *
+ * Instead, this driver is guaranteed to receive only classic CAN 2.0
+ * frames, with frame->len <= 8, by a chain of checks around the CAN
+ * device's MTU (as of v6.12):
+ *
+ * - can_changelink() sets the CAN device's MTU to CAN_MTU since we
+ * don't advertise CAN_CTRLMODE_FD support in ctrlmode_supported.
+ * - can_send() then refuses to pass any skb that exceeds CAN_MTU.
+ * - Since CAN_MTU is the smallest currently (v6.12) supported CAN
+ * MTU, it is clear that we are dealing with an ETH_P_CAN frame.
+ * - All ETH_P_CAN (classic CAN 2.0) frames have frame->len <= 8,
+ * as enforced by a call to can_is_can_skb() in can_send().
+ * - Thus for all CAN frames reaching this function, frame->len <= 8.
+ */
Actually, none of this is really specific to your can327 driver.
While this is a valuable piece of information, I would rather like to
see this added as a kdoc comment on top of can_dev_dropped_skb(). That
function already has a one line documentation, but maybe it deserves a
longer paragraph?
Agreed that the can_dev_dropped_skb() documentation should be improved.
However, I wouldn't remove the comment in can327 entirely - see above.
One of the key point is that the userland is able to bypass the CAN_RAW
layer (or any other CAN layers) by sending AF_PACKET. In which case, the
packet directly reaches the driver without any prior sanitization. So it
is critical to highlight that drivers must call can_dev_dropped_skb() at
the top of their start_xmit() function, typically to avoid buffer
overflows because of an out of band frame->len.
Agreed, and that's exactly the rabbit hole I went down yesterday. Where
would be the best place to document this? I guess that could go, er...
in the CAN driver writer's guide? Do we have something like that?
A bit off-topic, but since CAN_RAW came up: Why does CAN_RAW even
sanitise anything at all, instead of relying on checks on later layers?
It seems like quite a few checks are duplicated. And all the while it's
possible for userspace to do weird stuff like seemingly enabling CAN FD
on the socket via setsockopt() CAN_RAW_FD_FRAMES, but that's only
flipping a bit on the CAN_RAW socket, yet changes nothing underneath. It
was quite confusing to read. I suppose the explanation is "legacy"?
Thanks for your review!
Max
