Re: [PULL 90/92] hw/net/can: CTU CAN FD IP open hardware core emulation.

[Peter Maydell]

On Thu, 24 Sep 2020 at 11:21, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Jan Charvat <charvj10@fel.cvut.cz>
>
> The implementation of the model of complete open-source/design/hardware
> CAN FD controller. The IP core project has been started and is maintained
> by Ondrej Ille at Czech Technical University in Prague.
>
> CTU CAN FD project pages:
>         https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core
>
> CAN bus CTU FEE Projects Listing page:
>         http://canbus.pages.fel.cvut.cz/
>
> The core is mapped to PCIe card same as on one of its real hardware
> adaptations. The device implementing two CTU CAN FD ip cores
> is instantiated after CAN bus definition
>
>         -object can-bus,id=canbus0-bus
>
> by QEMU parameters
>
>         -device ctucan_pci,canbus0=canbus0-bus,canbus1=canbus0-bus
>
> Signed-off-by: Jan Charvat <charvj10@fel.cvut.cz>
> Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> Message-Id: 
> <23e3ca4dcb2cc9900991016910a6cab7686c0e31.1600069689.git.pisa@cmp.felk.cvut.cz>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Hi; Coverity points out a couple of issues with this code,
one of which looks like a buffer overrun.

First, the minor one:

> +static void ctucan_send_ready_buffers(CtuCanCoreState *s)
> +{
> +    qemu_can_frame frame;
> +    uint8_t *pf;
> +    int buff2tx_idx;
> +    uint32_t tx_prio_max;
> +    unsigned int buff_st;
> +    uint32_t buff_st_mask;
> +
> +    if (!s->mode_settings.s.ena) {
> +        return;
> +    }
> +
> +    do {
> +        union ctu_can_fd_int_stat int_stat;
> +        int i;
> +        buff2tx_idx = -1;
> +        tx_prio_max = 0;
> +
> +        for (i = 0; i < CTUCAN_CORE_TXBUF_NUM; i++) {
> +            uint32_t prio;
> +
> +            buff_st_mask = 0xf << (i * 4);
> +            buff_st = (s->tx_status.u32 >> (i * 4)) & 0xf;

Here we write to buff_st_mask...

> +
> +            if (buff_st != TXT_RDY) {
> +                continue;
> +            }
> +            prio = (s->tx_priority.u32 >> (i * 4)) & 0x7;
> +            if (tx_prio_max < prio) {
> +                tx_prio_max = prio;
> +                buff2tx_idx = i;
> +            }
> +        }
> +        if (buff2tx_idx == -1) {
> +            break;
> +        }
> +        buff_st_mask = 0xf << (buff2tx_idx * 4);

...but we never use it before we overwrite with this assignment,
so the earlier assignment is dead code.

Is there a missing use of the variable in between, or should
the earlier assignment just be deleted ?

> +        buff_st = (s->tx_status.u32 >> (buff2tx_idx * 4)) & 0xf;
> +        int_stat.u32 = 0;
> +        buff_st = TXT_RDY;
> +        pf = s->tx_buffer[buff2tx_idx].data;
> +        ctucan_buff2frame(pf, &frame);
> +        s->status.s.idle = 0;
> +        s->status.s.txs = 1;
> +        can_bus_client_send(&s->bus_client, &frame, 1);
> +        s->status.s.idle = 1;
> +        s->status.s.txs = 0;
> +        s->tx_fr_ctr.s.tx_fr_ctr_val++;
> +        buff_st = TXT_TOK;
> +        int_stat.s.txi = 1;
> +        int_stat.s.txbhci = 1;
> +        s->int_stat.u32 |= int_stat.u32 & ~s->int_mask.u32;
> +        s->tx_status.u32 = (s->tx_status.u32 & ~buff_st_mask) |
> +                        (buff_st << (buff2tx_idx * 4));
> +    } while (1);
> +}

Now here's the buffer overrun:

> +void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr, uint64_t val,
> +                       unsigned size)
> +{
> +    int              i;
> +
> +    DPRINTF("write 0x%02llx addr 0x%02x\n",
> +            (unsigned long long)val, (unsigned int)addr);
> +
> +    if (addr > CTUCAN_CORE_MEM_SIZE) {
> +        return;

Side note, this check should almost certainly be > and not >=
since addresses 0x0..0x499 are OK but 0x500 not.

> +    }
> +
> +    if (addr >= CTU_CAN_FD_TXTB1_DATA_1) {
> +        int buff_num;
> +        addr -= CTU_CAN_FD_TXTB1_DATA_1;
> +        buff_num = addr / CTUCAN_CORE_TXBUFF_SPAN;
> +        addr %= CTUCAN_CORE_TXBUFF_SPAN;
> +        if (buff_num < CTUCAN_CORE_TXBUF_NUM) {
> +            uint32_t *bufp = (uint32_t *)(s->tx_buffer[buff_num].data + 
> addr);
> +            *bufp = cpu_to_le32(val);
> +        }

Here we take the address, and divide it into a buff_num
and an address inside that buffer. CTUCAN_CORE_TXBUFF_SPAN
is (CTU_CAN_FD_TXTB2_DATA_1 - CTU_CAN_FD_TXTB1_DATA_1),
which is 0x200 - 0x100 == 0x100 (256 in decimal).
So after the %= operation, addr can be between 0 and 255.
However, we then use this as an offset into the array
s->tx_buffer[buff_num].data.
tx_buffer is declared as:
    CtuCanCoreMsgBuffer tx_buffer[CTUCAN_CORE_TXBUF_NUM];
and CtuCanCoreMsgBuffer is
typedef struct CtuCanCoreMsgBuffer {
    uint8_t data[CTUCAN_CORE_MSG_MAX_LEN];
} CtuCanCoreMsgBuffer;

But CTUCAN_CORE_MSG_MAX_LEN is defined to be 0x50 (80 bytes),
so the data array is much smaller than the set of offsets
we allow the guest to make into it, and the guest can write
well off the end of the array.

Is the array too small, or should we be guarding the
permitted values of addr the way we do the value of
buff_num ?

Ironically, if we get the check against CTUCAN_CORE_MEM_SIZE
right then it's not actually possible for buff_num to
be >= CTUCAN_CORE_TXBUF_NUM, because CTUCAN_CORE_TXBUF_NUM
is 4, CTUCAN_CORE_MEM_SIZE is 0x500 and CTU_CAN_FD_TXTB1_DATA_1
is 0x100. So that should be assert()ed rather than tested,
I think.


> +        case CTU_CAN_FD_INT_STAT:
> +            s->int_stat.u32 &= ~(uint32_t)val;

Side note: using bitfields like this is non portable.
You have a lot of unions and ifdefs in your header file
as a result, and this in ctu_can_fd_regs.h suggests that
you're assuming little-endian order unconditionally:

#ifndef __LITTLE_ENDIAN_BITFIELD
#define __LITTLE_ENDIAN_BITFIELD 1
#endif

The minimum fix here is to define this only if
!defined(HOST_WORDS_BIGENDIAN) but really you ought
to replace all this code with extract32()/deposit32()
and other logical operations IMHO.

thanks
-- PMM