On 08/08/2018 02:12 PM, Florian Ferg wrote:
> 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).
Where?
If I understand the code correctly:
If urb->actual_length is 66, you've processed and the first 64 bytes
(read_size == 64) and base.size is 1 the above check will not fail.
So accessing base.flags will outside of the buffer.
> 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.
Yes.
> 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.
You do a memcpy with a certain length which comes from the dlc, which is
also from the device. Please check that, too.
> Do you think we should add the above plausability checks anyway?
As msg_end is a unsigned variable it can underflow while doing the -=.
Better kill msg_end and compare read_size + sizeof (can_msg->base) <=
urb->actual_length.
>> + msg_type = le32_to_cpu(can_msg->base.flags);
>> + msg_type &= IXXAT_USB_MSG_FLAGS_TYPE;
>> +
>> + switch (msg_type) {
Either check the read_size before calling the ixxat_usb_handle_*() if
it's big enough or check the size _inside_ the ixxat_usb_handle_*().
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Attachment:
signature.asc
Description: OpenPGP digital signature
