Hello Marc,
On Fri, 20 Jul 2018, Marc Kleine-Budde wrote:
> > +static int ixxat_usb_decode_buf(struct urb *urb)
> > +{
> > + struct ixxat_usb_device *dev = urb->context;
> > + struct net_device *netdev = dev->netdev;
> > + struct ixxat_can_msg *can_msg;
> > + int err = 0;
> > + u32 msg_end = urb->actual_length;
> > + u32 read_size = 0;
> > + u8 *data = urb->transfer_buffer;
> > +
> > + while (msg_end > 0) {
> > + u8 msg_type;
> > +
> > + can_msg = (struct ixxat_can_msg *)&data[read_size];
> > +
> > + if (!can_msg || !can_msg->base.size) {
> > + err = -ENOTSUPP;
> > + netdev_err(netdev, "Error %d: Unsupported usb msg\n",
> > + err);
> > + break;
> > + }
> > +
> > + if ((read_size + can_msg->base.size + 1) > urb->actual_length) {
>
> You trust here, that base.size doesn't lie. You don't ensure that
> read_size + sizeof(struct struct ixxat_can_msg) [1] <= urb->actual_length.
>
> [1] It's more complicated, as not all of sizeof(struct struct
> ixxat_can_msg) is accessed, depeding on the msg_type and adapter
> version. Please don't consider dating coming from outside trustworthy,
> especially when it comes to length :) We don't want to access kernel
> memory outside of the rx'ed USB message.
We already prevent accessing memory outside of the USB message (We never
exceed urb->actual_length).
Checking if
- status messages have exactly status message size
- error messages have exactly error message size
- can messages have not more than maximum can message size
would be more of a plausability check.
And then still the base.size field could lie about can messages which are
smaller than maximum can message size. Can messages are truncated to
their actual length by the device. They don't necessarily have exactly 8
or 64 data bytes. So we can never be 100% sure about message plausabilty.
Do you think we should add the above plausability checks anyway?
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
