[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-5.2 v2 1/4] hw/net/can/ctucan: Don't allow guest to write
From: |
Pavel Pisa |
Subject: |
Re: [PATCH for-5.2 v2 1/4] hw/net/can/ctucan: Don't allow guest to write off end of tx_buffer |
Date: |
Tue, 10 Nov 2020 19:01:09 +0100 |
User-agent: |
KMail/1.9.10 |
Hello Peter,
On Tuesday 10 of November 2020 18:06:01 Peter Maydell wrote:
> The ctucan device has 4 CAN bus cores, each of which has a set of 20
> 32-bit registers for writing the transmitted data. The registers are
> however not contiguous; each core's buffers is 0x100 bytes after
> the last.
>
> We got the checks on the address wrong in the ctucan_mem_write()
> function:
> * the first "is addr in range at all" check allowed
> addr == CTUCAN_CORE_MEM_SIZE, which is actually the first
> byte off the end of the range
> * the decode of addresses into core-number plus offset in the
> tx buffer for that core failed to check that the offset was
> in range, so the guest could write off the end of the
> tx_buffer[] array
>
> NB: currently the values of CTUCAN_CORE_MEM_SIZE, CTUCAN_CORE_TXBUF_NUM,
> etc, make "buff_num >= CTUCAN_CORE_TXBUF_NUM" impossible, but we
> retain this as a runtime check rather than an assertion to permit
> those values to be changed in future (in hardware they are
> configurable synthesis parameters).
>
> Fix the top level check, and check the offset is within the buffer.
>
> Fixes: Coverity CID 1432874
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/net/can/ctucan_core.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/can/ctucan_core.c b/hw/net/can/ctucan_core.c
> index d20835cd7e9..538270e62f9 100644
> --- a/hw/net/can/ctucan_core.c
> +++ b/hw/net/can/ctucan_core.c
> @@ -303,7 +303,7 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
> uint64_t val, DPRINTF("write 0x%02llx addr 0x%02x\n",
> (unsigned long long)val, (unsigned int)addr);
>
> - if (addr > CTUCAN_CORE_MEM_SIZE) {
> + if (addr >= CTUCAN_CORE_MEM_SIZE) {
> return;
> }
Ack
> @@ -312,7 +312,8 @@ void ctucan_mem_write(CtuCanCoreState *s, hwaddr addr,
> uint64_t val, 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) {
> + if ((buff_num < CTUCAN_CORE_TXBUF_NUM) ||
> + (addr < sizeof(s->tx_buffer[buff_num].data))) {
should be &&
I would use
+ if (buff_num < CTUCAN_CORE_TXBUF_NUM &&
+ addr < CTUCAN_CORE_MSG_MAX_LEN) {
But that is equal. There can be problem that last three bytes of the uint32_t
type can fall after the end. The correct changes to fully support
unaligned writes is not so easy an dis unnecessary for actual drivers
and use. So suggest
+ addr &= ~3;
+ if ((buff_num < CTUCAN_CORE_TXBUF_NUM) &&
+ (addr < sizeof(s->tx_buffer[buff_num].data))) {
You can consider that as Acked by me
> uint32_t *bufp = (uint32_t *)(s->tx_buffer[buff_num].data +
> addr); *bufp = cpu_to_le32(val);
> }
[PATCH for-5.2 v2 3/4] hw/net/can/ctucan_core: Handle big-endian hosts, Peter Maydell, 2020/11/10
[PATCH for-5.2 v2 4/4] hw/net/can/ctucan_core: Use stl_le_p to write to tx_buffers, Peter Maydell, 2020/11/10