| Message ID | 20201110170604.5897-2-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/net/can/ctucan: fix Coverity and other issues | expand |
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); > }
On Tue, 10 Nov 2020 at 18:02, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote: > > 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 && Whoops, that's a silly mistake on my part. > 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))) { Hmm, yeah, the code is currently doing a 32-bit read regardless. > You can consider that as Acked by me OK, let's go with your version for 5.2. For unaligned accesses, for 6.0, I think the code for doing them to the txbuff at least is straightforward: if (buff_num < CTUCAN_CORE_TXBUF_NUM && (addr + size) < CTUCAN_CORE_MSG_MAX_LEN) { stn_le_p(s->tx_buffer[buff_num].data + addr, size, val); } (stn_le_p takes care of doing an appropriate-width write.) thanks -- PMM
Hello Peter, On Tuesday 10 of November 2020 19:24:03 Peter Maydell wrote: > For unaligned accesses, for 6.0, I think the code for doing > them to the txbuff at least is straightforward: > > if (buff_num < CTUCAN_CORE_TXBUF_NUM && > (addr + size) < CTUCAN_CORE_MSG_MAX_LEN) { > stn_le_p(s->tx_buffer[buff_num].data + addr, size, val); > } > > (stn_le_p takes care of doing an appropriate-width write.) Thanks, great to know, I like that much. Only small nitpicking, it should be (addr + size) <= CTUCAN_CORE_MSG_MAX_LEN So whole code I am testing now 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) && ((addr + size) <= sizeof(s->tx_buffer[buff_num].data))) { stn_le_p(s->tx_buffer[buff_num].data + addr, size, val); } } else { So I have applied you whole series with above update. All works correctly on x86_64 Linux host and with Linux x86_64 and MIPS big endian guests. Please update to this combination. I do not expect to have byte writes in our drivers but real core supports byte enable bus signals. Thanks much for teaching me QEMU stn_le_p. In the fact, we are discussion about similar slution of peripherals access for our https://github.com/cvut/QtMips/ education emulator (performance vise a total toy when compared to QEMU). It would worth to enable byte writes into registers as well. But I would not do it before release. It would be more complex. The reads supports bytes by reading 32/bit word and then shifting and masking right bits into result. Cross word unaligned reads are not supported. Again no reason for them now. You can add Tested-by: Pavel Pisa <pisa@cmp.felk.cvut.cz> to whole series. Thanks, Pavel
On Tue, 10 Nov 2020 at 19:32, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote: > > Hello Peter, > > On Tuesday 10 of November 2020 19:24:03 Peter Maydell wrote: > > For unaligned accesses, for 6.0, I think the code for doing > > them to the txbuff at least is straightforward: > > > > if (buff_num < CTUCAN_CORE_TXBUF_NUM && > > (addr + size) < CTUCAN_CORE_MSG_MAX_LEN) { > > stn_le_p(s->tx_buffer[buff_num].data + addr, size, val); > > } > > > > (stn_le_p takes care of doing an appropriate-width write.) > > Thanks, great to know, I like that much. > Only small nitpicking, it should be (addr + size) <= CTUCAN_CORE_MSG_MAX_LEN > > So whole code I am testing now > > 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) && > ((addr + size) <= sizeof(s->tx_buffer[buff_num].data))) { > stn_le_p(s->tx_buffer[buff_num].data + addr, size, val); > } > } else { > > So I have applied you whole series with above update. All works correctly > on x86_64 Linux host and with Linux x86_64 and MIPS big endian guests. > > Please update to this combination. If you've got a modified patch set that you've tested, would you mind sending it out to the list? That would avoid my possibly making mistakes in updating patches on my end and then requiring you to repeat the testing. thanks -- PMM
Hello Peter, On Tuesday 10 of November 2020 22:18:45 Peter Maydell wrote: > If you've got a modified patch set that you've tested, would > you mind sending it out to the list? That would avoid my > possibly making mistakes in updating patches on my end and > then requiring you to repeat the testing. OK, I have tried to send it with your authorship and my Signed-of-by at these patches which I have slightly modified and with Acked-by of these which should stay exactly same. If you prefer another style, send me a hint. Thanks much to help us to make our code better, Pavel Pisa
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; } @@ -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))) { uint32_t *bufp = (uint32_t *)(s->tx_buffer[buff_num].data + addr); *bufp = cpu_to_le32(val); }
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(-)