| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220514130430.44a2aa21.max@enpas.org>
Date: Sat, 14 May 2022 13:04:30 +0200
From: Max Staudt <max@...as.org>
To: Vincent Mailhol <vincent.mailhol@...il.com>
Cc: Wolfgang Grandegger <wg@...ndegger.com>,
Marc Kleine-Budde <mkl@...gutronix.de>,
linux-can@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Oliver Neukum <oneukum@...e.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6] can, tty: can327 CAN/ldisc driver for ELM327 based
OBD-II adapters
On Fri, 13 May 2022 11:38:31 +0900
Vincent Mailhol <vincent.mailhol@...il.com> wrote:
> Nice improvement since v3, thanks! Here are my new comments.
Well, thanks for and thanks to your review!
Replies below, a few style things (branching/goto) omitted.
> > +/* Bits in elm->cmds_todo */
> > +enum elm327_to_to_do_bits {
>
> to_to_do? Name looks like a typo. Also, do not see the value on the
> _bits suffix. I suggest:
>
> enum elm327_tx_do {
Indeed, a typo! It should have been elm327_to_to_do_bits. It's named
*_bits because the elem's indices are used to index bits in cmds_todo
in struct can327.
I guess your suggestion is nice, because it fits with the named indices.
> > +struct can327 {
> > + /* This must be the first member when using alloc_candev()
> > */
> > + struct can_priv can;
> > +
> > + struct can_rx_offload offload;
> > +
> > + /* TTY buffers */
> > + u8 rxbuf[ELM327_SIZE_RXBUF];
> > + u8 txbuf[ELM327_SIZE_TXBUF] ____cacheline_aligned;
>
> Out of curiosity, any rationale for the use of ____cacheline_aligned
> here?
Actually, I've checked now and it seems to not be necessary at all.
This was originally a kmalloc()'d buffer, in order to pass aligned
memory to tty->ops->write(). However, there are other (though few)
ldiscs that pass unaligned memory, and there aren't any checks in
serdev either, so I'll drop the forced alignment.
> > +static void elm327_kick_into_cmd_mode(struct can327 *elm)
> > +{
> > [...]
>
> (frame->can_id & CAN_EFF_FLAG) ^ (elm->can_frame_to_send.can_id &
> CAN_EFF_FLAG)
>
> can be factorized as:
>
> (frame->can_id ^ elm->can_frame_to_send.can_id) & CAN_EFF_FLAG
Yes :)
> > +/* Compare buffer to string length, then compare buffer to fixed
> > string.
> > + * This ensures two things:
> > + * - It flags cases where the fixed string is only the start of
> > the
> > + * buffer, rather than exactly all of it.
> > + * - It avoids byte comparisons in case the length doesn't match.
> > + *
> > + * strncmp() cannot be used here because it accepts the following
> > wrong case:
> > + * strncmp("CAN ER", "CAN ERROR", 6);
>
> What about:
> strncmp("CAN ER", "CAN ERROR", 7);
> ?
NAK, because this may overread the buffer by one byte (the NUL byte).
I am comparing naked bytes, not NUL-terminated strings.
> > +static void elm327_parse_error(struct can327 *elm, size_t len)
> > +{
> > + struct can_frame *frame;
> > + struct sk_buff *skb;
> > +
> > + lockdep_assert_held(&elm->lock);
> > +
> > + skb = alloc_can_err_skb(elm->dev, &frame);
> > + if (!skb)
> > + /* It's okay to return here:
> > + * The outer parsing loop will drop this UART
> > buffer.
> > + */
> > + return;
> > +
> > + /* Filter possible error messages based on length of RX'd
> > line */
> > + if (check_len_then_cmp(elm->rxbuf, len, "UNABLE TO
> > CONNECT")) {
>
> Is this equivalent?
> if (!strncmp(elm->rxbuf, "UNABLE TO CONNECT", len + 1)) {
No. We can use (len) bytes in elm->rxbuf, not (len+1).
There are no C strings in this buffer. There are no NUL chars in this
buffer. There are no trailing bytes we can overread into.
> > +static void elm327_handle_prompt(struct can327 *elm)
> > +{
> > + struct can_frame *frame = &elm->can_frame_to_send;
> > + /* Size this buffer for the largest ELM327 line we may
> > generate,
> > + * which is currently an 8 byte CAN frame's payload hexdump.
> > + * Items in elm327_init_script must fit here, too!
> > + */
> > + char local_txbuf[sizeof("0102030405060708\r")];
> > +
> > + lockdep_assert_held(&elm->lock);
> > +
> > + if (!elm->cmds_todo) {
> > + /* Enter CAN monitor mode */
> > + elm327_send(elm, "ATMA\r", 5);
> > + elm->state = ELM327_STATE_RECEIVING;
> > +
> > + /* We will be in the default state once this
> > command is
> > + * sent, so enable the TX packet queue.
> > + */
> > + netif_wake_queue(elm->dev);
> > +
> > + return;
> > + }
> > +
> > + /* Reconfigure ELM327 step by step as indicated by
> > elm->cmds_todo */
> > + if (test_bit(ELM327_TX_DO_INIT, &elm->cmds_todo)) {
> > + snprintf(local_txbuf, sizeof(local_txbuf),
> > + "%s",
> > + *elm->next_init_cmd);
> > +
> > + elm->next_init_cmd++;
> > + if (!(*elm->next_init_cmd)) {
> > + clear_bit(ELM327_TX_DO_INIT,
> > &elm->cmds_todo);
> > + /* Init finished. */
> > + }
> > +
> > + } else if (test_and_clear_bit(ELM327_TX_DO_SILENT_MONITOR,
> > &elm->cmds_todo)) {
> > + snprintf(local_txbuf, sizeof(local_txbuf),
> > + "ATCSM%i\r",
> > + !(!(elm->can.ctrlmode &
> > CAN_CTRLMODE_LISTENONLY)));
>
> The second pair of brackets look superficial:
> snprintf(local_txbuf, sizeof(local_txbuf),
> "ATCSM%i\r",
> !!(elm->can.ctrlmode &
> CAN_CTRLMODE_LISTENONLY));
True, thanks for catching this.
> > +static void elm327_parse_rxbuf(struct can327 *elm)
> > +{
> > + size_t len;
> > + int i;
> > +
> > + lockdep_assert_held(&elm->lock);
> > +
> > + switch (elm->state) {
> > + case ELM327_STATE_NOTINIT:
> > + elm->rxfill = 0;
> > + break;
> > +
> > + case ELM327_STATE_GETDUMMYCHAR:
> > + {
>
> Is this braket need?
No it's not, it's a leftover from before moving "int i;" to the
function head.
> > +
> > + case ELM327_STATE_GETPROMPT:
> > + /* Wait for '>' */
> > + if (elm327_is_ready_char(elm->rxbuf[elm->rxfill -
> > 1]))
> > + elm327_handle_prompt(elm);
> > +
> > + elm->rxfill = 0;
> > + break;
> > +
> > + case ELM327_STATE_RECEIVING:
> > + /* Find <CR> delimiting feedback lines. */
> > + for (len = 0;
> > + (len < elm->rxfill) && (elm->rxbuf[len] !=
> > '\r');
> > + len++) {
> > + /* empty loop */
>
> Question of taste but would prefer a while look with the len++ in the
> body (if you prever to do as above, no need to argue, just keep it
> like it is).
Good point, I think a while() would be easier on the eyes indeed.
>
> > + }
> > +
> > + if (len == ELM327_SIZE_RXBUF) {
> > + /* Line exceeds buffer. It's probably all
> > garbage.
> > + * Did we even connect at the right baud
> > rate?
> > + */
> > + netdev_err(elm->dev,
> > + "RX buffer overflow. Faulty
> > ELM327 or UART?\n");
> > + elm327_uart_side_failure(elm);
> > + break;
>
> Can you have just a single break at the end of the case instead of
> having it in every branches of the if/else?
Agreed, the status quo is ugly. I'll look into it.
> > +/* Send a can_frame to a TTY. */
> > +static netdev_tx_t can327_netdev_start_xmit(struct sk_buff *skb,
> > + struct net_device *dev)
> > +{
> > + struct can327 *elm = netdev_priv(dev);
> > + struct can_frame *frame = (struct can_frame *)skb->data;
> > +
> > + if (can_dropped_invalid_skb(dev, skb))
> > + return NETDEV_TX_OK;
> > +
> > + /* BHs are already disabled, so no spin_lock_bh().
> > + * See Documentation/networking/netdevices.txt
> > + */
> > + spin_lock(&elm->lock);
>
> Do you need to hold the lock here? Isn't it possible to move this line
> after the next if so that you do not have to unclock in the error
> path?
It is possible.
Even better: I forgot to remove the elm->tty check as part of PATCHv4,
and elm->uart_side_failure cannot be unset while the netdev is up. So
we can push the locking way further down after any remaining checks :)
> > + spin_unlock(&elm->lock);
> > + goto out;
> > + }
> > +
> > + netif_stop_queue(dev);
> > +
> > + elm327_send_frame(elm, frame);
> > + spin_unlock(&elm->lock);
> > +
> > + dev->stats.tx_packets++;
> > + dev->stats.tx_bytes += frame->len;
> > +
> > + can_led_event(dev, CAN_LED_EVENT_TX);
> > +
> > +out:
>
> The benefit of the goto label is to factorize code. If you have only
> one goto, you might as well prefer to remove the label and do the
> kfree_skb inside the if block.
But then I also have to duplicate the return... matter of taste :)
> > + kfree_skb(skb);
>
> Maybe you want to increment dev->stats here?
This already happens above, but only in case the frame is actually
queued on the UART.
> > + return NETDEV_TX_OK;
> > +}
> > +
> > +static const struct net_device_ops can327_netdev_ops = {
> > + .ndo_open = can327_netdev_open,
> > + .ndo_stop = can327_netdev_close,
> > + .ndo_start_xmit = can327_netdev_start_xmit,
> > + .ndo_change_mtu = can_change_mtu,
> > +};
> > +
> > +static bool can327_is_valid_rx_char(char c)
> > +{
> > + return (isdigit(c) ||
> > + isupper(c) ||
> > + c == ELM327_DUMMY_CHAR ||
> > + c == ELM327_READY_CHAR ||
> > + c == '<' ||
> > + c == 'a' ||
> > + c == 'b' ||
> > + c == 'v' ||
> > + c == '.' ||
> > + c == '?' ||
> > + c == '\r' ||
> > + c == ' ');
>
> Remark: if this function gets called a lot, you might what so create
> an lookup array and:
>
> return c < sizeof(can327_is_valid_rx_char_lookup) &&
> can327_is_valid_rx_char_lookup[c];
>
> This should be faster.
It does get called for every byte RX'd via UART. I'll have a look at
your LUT idea, it does sound good :)
> > + /* Check for stray characters on the UART
> > line.
> > + * Likely caused by bad hardware.
> > + */
> > + if (!can327_is_valid_rx_char(*cp)) {
> > + netdev_err(elm->dev,
> > + "Received illegal
> > character %02x.\n",
> > + *cp);
>
> Might make sense to put the netdev_err message inside
> can327_is_valid_rx_char().
Sorry, I prefer to keep can327_is_valid_rx_char() purely functional,
and the side effects in this function here. :)
> > + elm327_uart_side_failure(elm);
> > +
> > + goto out;
> > + }
> > +
> > + elm->rxbuf[elm->rxfill++] = *cp;
> > + }
> > +
> > + cp++;
> > + }
> > +
> > + if (count >= 0) {
> > + netdev_err(elm->dev, "Receive buffer overflowed.
> > Bad chip or wiring?"); +
> > + elm327_uart_side_failure(elm);
> > +
> > + goto out;
> > + }
> > +
> > + elm327_parse_rxbuf(elm);
> > +
> > +out:
> > + spin_unlock_bh(&elm->lock);
>
> If the out label has a single statement, can be better to just repalce
> all the goto out with spin_unlock_bh(&elm->lock);
It would be spin_unlock_bh() plus return...
Yeah, I guess it's easier to read.
> Question: did you try to send/receive DLC greater than 8? (c.f.
> CAN_CTRLMODE_CC_LEN8_DLC)
Yes, I have :)
It is entirely unsupported by the hardware:
- On RX, DLC > 8 is reported as 8.
- On TX, the DLC is constructed by the ELM327 depending on the
payload, so DLC > 8 is impossible.
Thanks for your review!
Max
Powered by blists - more mailing lists