CAN DLC field allows values higher than 8 in standard mode

[Martyx00]

Description of defect

The code responsible for handling incoming CAN messages allows DLC field values higher than 8. This is not in-line with the standard CAN protocol (such values are only used in CAN-FD). This report is created since several issues where developers assumed that the DLC value is maximum of 8 were found. Example of such an issue could be a simple memcpy operation like this: memcpy(dest_buffer,can_msg.data,can_msg.len) In such situation sending an arbitrary CAN message with DLC set to maximum value (15) would cause a buffer overflow.

Target(s) affected by this defect ?

Any code using CAN bus from MbedOS.

Toolchain(s) (name and version) displaying this defect ?

CAN interface handler

What version of Mbed-os are you using (tag or sha) ?

All versions of MbedOS up to current (https://github.com/ARMmbed/mbed-os/releases/tag/mbed-os-6.16.0)

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

Not Applicable

How is this defect reproduced ?

Any code working with CAN messages on the receiving end (example below was used in the PoC - see the attached screenshot). The sending side could be any Arduino board with CAN controller.

#include "mbed.h"
CAN can1(PB_8, PB_9);
// main() runs in its own thread in the OS
int main()
{
    CANMessage msg;
    printf("main()\n");
    can1.frequency(500000);

    while (true) {
        if (can1.read(msg)) {
            printf("Message received:\n ID: %d\n DLC: %d\n", msg.id,msg.len);
        } 
    }
}

[0xc0170]

Have you looked at where this lentgh is coming from?

Looking at CAN_Message, it's constructor sets length to maximum 8 bits. It is predefined to 8 in its ctor. Thus the example above should have msg.len set to 8. This is however not true if you pass it down to HAL.

In this case, I am checking STM can_api implementation:

msg->len = (uint8_t)0x0F & can->sFIFOMailBox[rxfifo_default].RDTR;

This is where the number > 8 comes from. Can you send a pull request fixing this? It's in can_api.c inside STM HAL, line 1027.

cc @jeromecoutant

[0xc0170]

The fix #15364 fixes the issue for particular implementation. I realized we could add one more check and fix in the upper C++ CAN driver.

This is the current implmentation.

int CAN::read(CANMessage &msg, int handle)
{
    lock();
    int ret = can_read(&_can, &msg, handle);
    unlock();
    return ret;
}

If msg->len is set to above 8, the error is propagated to the user and might cause issues.

What if we add a check for msg.len:

int CAN::read(CANMessage &msg, int handle)
{
    lock();
    int ret = can_read(&_can, &msg, handle);
    if (msg.len > 8) {
        msg.len = 8;
    }
    unlock();
    return ret;
}

Or potentionally we could just error() there if it's above 8bits as it shall not.

The error is better in this case as if anyone really did wrote more than 8 bits, it wrote to a place where it should not in any case. Let's add error() there.

[Martyx00]

Fixing in the upper layer seems more reasonable as this will address any similar situation in the HALs with CAN support that might be added in the future. I will cancel the pull request that I did with changes in the STM HAL. Will you update the upper driver?

[0xc0170]

I'll send PR today.

[0xc0170]

I will cancel the pull request that I did with changes in the STM HAL

It's still needed!

[Martyx00]

Having second thoughts on the fix in the upper layer, it has to allow CAN FD frames to propage DLC values which can be up to 15, thus the CAN type would have to be checked before throwing error() based on the length.

[0xc0170]

To accomodate anything above 8, the message structure CANmessage would need to be changed:

    unsigned char  data[8];            // Data field
    unsigned char  len;                // Length of data field in bytes

len in this case can be up to 8. I'll cover the current implementation.

[0xc0170]

I referenced above the fix, please review #15374

[Martyx00]

Closing this issue as it was resolved by mergin the https://github.com/ARMmbed/mbed-os/pull/15373 any subsequent actions related to possible issues when implementing FD CAN support are to be handled separately from this issue.