On 19/11/2024 at 09:38, Max Staudt wrote:
> The hex dump encoding of outgoing payloads used snprintf() with a
> too large length of the target buffer. While the length was wrong, the
> buffer size and its filling logic were fine (sprintf() would have been
> sufficient), hence this is not security relevant.
>
> Still, it's a good opportunity to simplify the code, and since no
> length checking is required, let's implement it with bin2hex().
>
> Since bin2hex() outputs lowercase letters, this changes the spoken
> wire protocol with the ELM327 chip, resulting in a change in
> can327_is_valid_rx_char() because the ELM327 is set to echo the
> characters sent to it. The documentation says that this is fine, and
> I have verified the change on actual hardware.
Nice that the device accepts the lower case hexadecimals. This results
in a nice simplification.
> Finally, since the reporter's worry was that frame->len may be larger
> than 8, resulting in a buffer overflow in can327_handle_prompt()'s
> local_txbuf, a comment describes how the CAN stack prevents that. This
> is also the reason why the size passed to snprintf() was not relevant
> to preventing a buffer overflow, because there was no overflow possible
> in the first place.
>
> Fixes: 43da2f07622f ("can: can327: CAN/ldisc driver for ELM327 based OBD-II adapters")
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Tested-by: Max Staudt <max@xxxxxxxxx>
> Signed-off-by: Max Staudt <max@xxxxxxxxx>
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.
> ---
> drivers/net/can/can327.c | 46 ++++++++++++++++++++++++++++------------
> 1 file changed, 33 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c
> index 24af63961030..3ae7b4eb6ca5 100644
> --- a/drivers/net/can/can327.c
> +++ b/drivers/net/can/can327.c
> @@ -18,6 +18,7 @@
> #include <linux/bitops.h>
> #include <linux/ctype.h>
> #include <linux/errno.h>
> +#include <linux/hex.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> #include <linux/lockdep.h>
> @@ -622,17 +623,14 @@ static void can327_handle_prompt(struct can327 *elm)
> */
> snprintf(local_txbuf, sizeof(local_txbuf), "ATRTR\r");
> } else {
> - /* Send a regular CAN data frame */
> - int i;
> -
> - for (i = 0; i < frame->len; i++) {
> - snprintf(&local_txbuf[2 * i],
> - sizeof(local_txbuf), "%02X",
> - frame->data[i]);
> - }
> -
> - 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() */
> + bin2hex(local_txbuf, frame->data, frame->len);
> + local_txbuf[2 * frame->len] = '\r';
> + local_txbuf[2 * frame->len + 1] = '\0';
> }
>
> elm->drop_next_line = 1;
> @@ -815,6 +813,26 @@ static netdev_tx_t can327_netdev_start_xmit(struct sk_buff *skb,
> struct can327 *elm = netdev_priv(dev);
> struct can_frame *frame = (struct can_frame *)skb->data;
>
> + /* 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?
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.
> if (can_dev_dropped_skb(dev, skb))
> return NETDEV_TX_OK;
>
> @@ -871,8 +889,10 @@ static bool can327_is_valid_rx_char(u8 c)
> ['H'] = true, true, true, true, true, true, true,
> ['O'] = true, true, true, true, true, true, true,
> ['V'] = true, true, true, true, true,
> - ['a'] = true,
> - ['b'] = true,
> + /* Note: c-f are needed only if outgoing CAN payloads are
> + * sent as lowercase hex dumps instead of uppercase.
> + */
> + ['a'] = true, true, true, true, true, true,
> ['v'] = true,
> [CAN327_DUMMY_CHAR] = true,
> };
Yours sincerely,
Vincent Mailhol
