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