On 02.04.2021 18:56:33, Vincent MAILHOL wrote:
> > > +int es58x_rx_err_msg(struct net_device *netdev, enum es58x_err error,
> > > + enum es58x_event event, u64 timestamp)
> > > +{
> > > + struct es58x_priv *priv = es58x_priv(netdev);
> > > + struct can_priv *can = netdev_priv(netdev);
> > > + struct can_device_stats *can_stats = &can->can_stats;
> > > + struct can_frame *cf;
> >
> > = NULL;
> >
> > So that the if (cf) properly works...
>
> I actually expected cfto be set to NULL when an error
> occurred (same as skb).
>
> But indeed, cf is not initialised if the allocation fails. I was
> wondering if it would not be better to make both alloc_can_skb()
> and and alloc_canfd_skb() set cf to NULL. That would remove a
> pitfall.
Makes sense.
> If you like the idea, I can submit a patch.
Sorry creating this drive by patch. The patch description took longer
than the actual patch :)
[...]
> > > +/**
> > > + * es58x_flush_cmd_buf() - Reset the buffer for URB commands.
> > > + * @es58x_dev: ES58X device.
> > > + */
> > > +static void es58x_flush_cmd_buf(struct es58x_device *es58x_dev)
> > > +{
> > > + memset(&es58x_dev->rx_cmd_buf, 0, es58x_dev->param->rx_urb_cmd_max_len);
> >
> > I think you can skip the memset, as you're overwriting the data anyways
> > and account the valid length.
>
> The idea was that this buffer might contain confidential
> data (e.g. an UDS seed and key pair or a VIN) so I cleared it out
> of precaution.
I see, I think the kernel takes care of explicitly clearing not key
material after using, but not unencrypted (i.e. plaintext) network
traffic.
> I recognize that this is useless as an attacker can just retrieve
> the information from a candump, no need to look into some kernel
> temporary buffer for that...
>
> I will remove it.
Ok.
[...]
> > > +static signed int es58x_split_urb(struct es58x_device *es58x_dev,
> > > + struct urb *urb)
> > > +{
> > > + const u8 es58x_is_alive = 0x11;
> >
> > We usually use #defines for this
>
> ACK.
>
> > > + union es58x_urb_cmd *urb_cmd;
> > > + u8 *raw_cmd = urb->transfer_buffer;
> > > + ssize_t raw_cmd_len = urb->actual_length;
> >
> > urb->actual_length is an u32
>
> Below, I test that (raw_cmd_len > 0) in a while loop. I want to
> make this variable signed to avoid an infinite loop in case of an
> off by one error (which should not occur, see this as a safety
> net).
makes sense
> I will make this a s32.
>
> Also, the size is represented by a u16 on the device so s32 is
> more than enough.
>
> > > + int ret;
> > > +
> > > + if (es58x_dev->rx_cmd_buf_len != 0) {
> > > + ret = es58x_handle_incomplete_cmd(es58x_dev, urb);
> > > + if (ret != -ENODATA)
> > > + es58x_flush_cmd_buf(es58x_dev);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + raw_cmd += ret;
> > > + raw_cmd_len -= ret;
> > > + }
> > > +
> > > + while (raw_cmd_len > 0) {
> > > + if (raw_cmd[0] == es58x_is_alive) {
> > > + raw_cmd++;
> > > + raw_cmd_len--;
> > > + continue;
> > > + }
> > > + urb_cmd = (union es58x_urb_cmd *)raw_cmd;
> > > + ret = es58x_check_rx_urb(es58x_dev, urb_cmd, raw_cmd_len);
> > > + if (ret > 0) {
> >
> > Here the length field in the usb_cmd is valid, it lies within the total
> > length of the rx'ed urb.
> >
> > > + es58x_handle_urb_cmd(es58x_dev, urb_cmd);
> >
> > As far as I see you're checking in ops->handle_urb_cmd() if the
> > urb_cmd's length is long enough for the command that's currently
> > processed, right?
>
> The check of the urb_cmd's length occurs in es58x_check_rx_urb():
> if (urb_cmd_len > param->rx_urb_cmd_max_len) {
> In that same function, I then check that urb_cmd's length is long enough:
> } else if (urb_actual_len < urb_cmd_len) {
> If it is not long enough, the not-yet-consumed data of the urb
> is buffered and once a new urb comes up, those are concatenated.
>
> After that, ops->handle_urb_cmd() will dispatch the command
> depending on the cmd_id and additional checks will be done on the
> size of the payload.
Good, this is how I understood the code.
[...]
> > > +/**
> > > + * es58x_open() - Open and start network device.
> > > + * @netdev: CAN network device.
> > > + *
> > > + * Called when the network transitions to the up state.
> > > + *
> > > + * Return: zero on success, errno when any error occurs.
> > > + */
> > > +static int es58x_open(struct net_device *netdev)
> > > +{
> > > + int ret;
> > > +
> > > + ret = open_candev(netdev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = es58x_enable_channel(netdev);
> > > + if (ret)
> > > + return ret;
> >
> > Please do an as complete as possible reset and configuration during
> > open(). If there is any error a ifdown/ifup should fix it. Here on a USB
> > device with multiple interfaces it's not as easy as on devices with only
> > one CAN interface.
>
> ACK.
>
> I will use a function as below to check if all interfaces are
> down.
>
> static bool es58x_are_all_channels_closed(struct es58x_device *es58x_dev)
> {
> int i;
>
> for (i = 0; i < es58x_dev->num_can_ch; i++) {
> struct can_priv *can_priv = netdev_priv(es58x_dev->netdev[i]);
> if (can_priv->state != CAN_STATE_STOPPED)
> return false;
> }
> return true;
> }
>
> I will modify both es58x_open() and es58x_close().
Have a look at
https://elixir.bootlin.com/linux/v5.11/source/include/linux/kref.h
I'm not sure if kref is overkill here :)
[...]
> > > +/**
> > > + * es58x_start_xmit() - Transmit an skb.
> > > + * @skb: socket buffer of a CAN message.
> > > + * @netdev: CAN network device.
> > > + *
> > > + * Called when a packet needs to be transmitted.
> > > + *
> > > + * This function relies on Byte Queue Limits (BQL). The main benefit
> > > + * it to increase the throughput by allowing bulk transfers
> > > + * (c.f. xmit_more flag).
> > > + *
> > > + * Queues up to tx_bulk_max messages in &tx_urb buffer and does
> > > + * a bulk send of all messages in one single URB.
> > > + *
> > > + * Return:
> > > + * NETDEV_TX_OK if we could manage the @skb (either transmit it or
> > > + * drop it)
> > > + *
> > > + * NETDEV_TX_BUSY if the device is busy (this is a bug, the network
> > > + * device stop/wake management should prevent this return code to
> > > + * occur).
> > > + */
> > > +static netdev_tx_t es58x_start_xmit(struct sk_buff *skb,
> > > + struct net_device *netdev)
> > > +{
> > > + struct es58x_priv *priv = es58x_priv(netdev);
> > > + struct es58x_device *es58x_dev = priv->es58x_dev;
> > > + unsigned int packet_idx, frame_len;
> > > + int ret;
> > > +
> > > + if (can_dropped_invalid_skb(netdev, skb)) {
> > > + if (priv->tx_urb)
> > > + goto xmit_commit;
> > > + return NETDEV_TX_OK;
> > > + }
> > > +
> > > + if (!priv->tx_urb) {
> > > + priv->tx_urb = es58x_get_tx_urb(es58x_dev);
> > > + if (!priv->tx_urb) {
> > > + ret = -ENOMEM;
> > > + goto drop_skb;
> > > + }
> > > + memset(priv->tx_urb->transfer_buffer, 0,
> > > + es58x_dev->param->tx_urb_cmd_max_len);
> >
> > Is this memset() needed?
>
> In current state, it is, but a small modification should be
> enough to have it removed.
nice!
> > > + priv->tx_urb->transfer_buffer_length =
> > > + es58x_dev->param->urb_cmd_header_len;
> > > + priv->tx_can_msg_cnt = 0;
> > > + priv->tx_can_msg_is_fd = can_is_canfd_skb(skb);
> > > + } else if (priv->tx_can_msg_is_fd != can_is_canfd_skb(skb)) {
> > > + /* Can not do bulk send with mixed CAN and CAN FD frames. */
> > > + ret = es58x_xmit_commit(netdev);
> > > + if (ret)
> > > + goto drop_skb;
> > > + return es58x_start_xmit(skb, netdev);
> >
> > In the kernel the stack is limited, does it make sense to re-arrange
> > this to avoid recursion?
>
> This is a tail recursion so with the good optimisation, it will
> not increase the stack. And also, the recursive call occurs one
> time max. So I thought it was acceptable.
This were my thoughts as well.
> Regardless, rearranging the code is trivial, so I will remove
> this recursion.
fine
[...]
> > > +/**
> > > + * struct es58x_priv - All information specific to a CAN channel.
> > > + * @can: struct can_priv must be the first member (Socket CAN relies
> > > + * on the fact that function netdev_priv() returns a pointer to
> > > + * a struct can_priv).
> > > + * @es58x_dev: pointer to the corresponding ES58X device.
> > > + * @tx_urb: Used as a buffer to concatenate the TX messages and to do
> > > + * a bulk send. Please refer to es58x_start_xmit() for more
> > > + * details.
> > > + * @tx_tail: Index of the oldest packet still pending for
> > > + * completion. @tx_tail & echo_skb_mask represents the beginning
> > > + * of the echo skb FIFO, i.e. index of the first element.
> > > + * @tx_head: Index of the next packet to be sent to the
> > > + * device. @tx_head & echo_skb_mask represents the end of the
> > > + * echo skb FIFO plus one, i.e. the first free index.
> >
> > Is this the description 100% correct? In xmit() you're using tx_head +
> > tx_can_msg_cnt to address the next packet to be send.
> >
> > >> + packet_idx = priv->tx_head + priv->tx_can_msg_cnt;
> >
> > But in xmit_more() you're not taking tx_can_msg_cnt into account.
>
> The idea was that in es58x_start_xmit() and es58x_xmit_more(),
> the changes were not yet committed and thus tx_head not yet
> updated.
>
> After a bit more thinking, it seems better to directly update
> tx_head each time a packet is queued in can_echo_skb[]. I will
> keep above description and modify the code.
fine
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:
signature.asc
Description: PGP signature
