Re: [PULL v3 02/11] hppa: Add support for LASI chip with i82596 NIC

[Helge Deller]

On 17.02.20 18:56, Peter Maydell wrote:
> On Fri, 24 Jan 2020 at 23:20, Richard Henderson
> <address@hidden> wrote:
>>
>> From: Helge Deller <address@hidden>
>>
>> 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 <address@hidden>
>> Signed-off-by: Sven Schnelle <address@hidden>
>> Message-Id: <address@hidden>
>> Signed-off-by: Richard Henderson <address@hidden>
>
> 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