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

[Peter Maydell <peter.maydell@linaro.org>]

On Fri, 24 Jan 2020 at 23:20, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> From: Helge Deller <deller@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 <deller@gmx.de>
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> Message-Id: <20191220211512.3289-3-svens@stackframe.org>
> Signed-off-by: Richard Henderson <richard.henderson@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.

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
-- PMM