On 17.02.20 18:56, Peter Maydell wrote:
> On Fri, 24 Jan 2020 at 23:20, Richard Henderson
> <richard.hender...@linaro.org> wrote:
>>
>> From: Helge Deller <del...@gmx.de>
>>
>> LASI is a built-in multi-I/O chip which supports serial, parallel,
>> network (Intel i82596 Apricot), sound and other functionalities.
>> LASI has been used in many HP PARISC machines.
>> This patch adds the necessary parts to allow Linux and HP-UX to detect
>> LASI and the network card.
>>
>> Signed-off-by: Helge Deller <del...@gmx.de>
>> Signed-off-by: Sven Schnelle <sv...@stackframe.org>
>> Message-Id: <20191220211512.3289-3-sv...@stackframe.org>
>> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
>
> Hi; Coverity has an issue with this code (CID 1419396):
>
>
>> +#define ETHER_TYPE_LEN 2
>> +#define VLAN_TCI_LEN 2
>> +#define VLAN_HLEN (ETHER_TYPE_LEN + VLAN_TCI_LEN)
>
>
>> +#define MIN_BUF_SIZE 60
>> +
>> +ssize_t i82596_receive(NetClientState *nc, const uint8_t *buf, size_t sz)
>> +{
>> + I82596State *s = qemu_get_nic_opaque(nc);
>> + uint32_t rfd_p;
>> + uint32_t rbd;
>> + uint16_t is_broadcast = 0;
>> + size_t len = sz;
>> + uint32_t crc;
>> + uint8_t *crc_ptr;
>> + uint8_t buf1[MIN_BUF_SIZE + VLAN_HLEN];
>
> This buffer is 60 + 2 + 2 == 64 bytes large...
>
>> + /* if too small buffer, then expand it */
>> + if (len < MIN_BUF_SIZE + VLAN_HLEN) {
>> + memcpy(buf1, buf, len);
>> + memset(buf1 + len, 0, MIN_BUF_SIZE + VLAN_HLEN - len);
>> + buf = buf1;
>> + if (len < MIN_BUF_SIZE) {
>> + len = MIN_BUF_SIZE;
>> + }
>
> ...here if we're using the buf1[] buffer then len must
> be >= MIN_BUF_SIZE (60) and < MIN_BUF_SIZE + VLAN_HLEN (64),
> so it's in the range 60 to 63...
>
>> + }
>> +
>> + /* Calculate the ethernet checksum (4 bytes) */
>> + len += 4;
>> + crc = cpu_to_be32(crc32(~0, buf, sz));
>> + crc_ptr = (uint8_t *) &crc;
>
> ...but then we add 4 to len here, so it's now 64 to 67...
>
>> + while (len) {
>
>
>
>> + num = buffer_size & SIZE_MASK;
>> + if (num > len) {
>> + num = len;
>> + }
>
> ...before using len as the cap on how many bytes we write...
>
>> + rba = get_uint32(rbd + 8);
>> + /* printf("rba is 0x%x\n", rba); */
>> + address_space_rw(&address_space_memory, rba,
>> + MEMTXATTRS_UNSPECIFIED, (void *)buf, num, 1);
>
> ...from the buffer into guest memory here.
>
> So we could be reading off the end of the buffer.
>
> I don't know whether the buffer should be 4 bytes
> larger to allow for the checksum, or if the len calculation
> is wrong.
I'm working on a bigger patch which will improve this driver.
It still has some issues with the emulation in Linux and HP-UX.
With the patch I will take try to fix those out-of-bounds
accesses too.
It will take some time though, until I will send the patch.
> PS: I think calling address_space_write() with a constant
> final argument is confusing;
> "address_space_rw(as, addr, attrs, buf, len, 1)"
> is equivalent to
> "address_space_write(as, addr, attrs, buf, len)",
> except that it's more obvious that it's a write rather
> than a read, and it avoids an extra layer of function
> call. We do seem to have a surprisingly large number of
> places in the codebase that call address_space_rw() with a
> constant final argument, though...
Thanks for cleaning this up in the meantime!
Helge
