Re: [PULL 02/29] hw/misc/ivshmem-flat: Add ivshmem-flat device

[Philippe Mathieu-Daudé]

Hi Gustavo,

On 31/12/24 21:22, Philippe Mathieu-Daudé wrote:
> From: Gustavo Romero <gustavo.romero@linaro.org>
>
> Add a new device, ivshmem-flat, which is similar to the ivshmem PCI but
> does not require a PCI bus. It's meant to be used on machines like those
> with Cortex-M MCUs, which usually lack a PCI/PCIe bus, e.g. lm3s6965evb
> and mps2-an385.
>
> The device currently only supports the sysbus bus.
>
> The new device, just like the ivshmem PCI device, supports both peer
> notification via hardware interrupts and shared memory.
>
> The device shared memory size can be set using the 'shmem-size' option
> and it defaults to 4 MiB, which is the default size of shmem allocated
> by the ivshmem server.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1134
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> [PMD: Rebased updating Property and using DEFINE_TYPES macro]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-ID: <20241216141818.111255-2-gustavo.romero@linaro.org>
> ---
>   docs/system/device-emulation.rst     |   1 +
>   docs/system/devices/ivshmem-flat.rst |  33 ++
>   include/hw/misc/ivshmem-flat.h       |  85 +++++
>   hw/misc/ivshmem-flat.c               | 459 +++++++++++++++++++++++++++
>   hw/misc/Kconfig                      |   5 +
>   hw/misc/meson.build                  |   2 +
>   hw/misc/trace-events                 |  16 +
>   7 files changed, 601 insertions(+)
>   create mode 100644 docs/system/devices/ivshmem-flat.rst
>   create mode 100644 include/hw/misc/ivshmem-flat.h
>   create mode 100644 hw/misc/ivshmem-flat.c


> diff --git a/hw/misc/ivshmem-flat.c b/hw/misc/ivshmem-flat.c
> new file mode 100644
> index 00000000000..33fc9425d20
> --- /dev/null
> +++ b/hw/misc/ivshmem-flat.c
> @@ -0,0 +1,459 @@
> +/*
> + * Inter-VM Shared Memory Flat Device
> + *
> + * SPDX-FileCopyrightText: 2023 Linaro Ltd.
> + * SPDX-FileContributor: Gustavo Romero <gustavo.romero@linaro.org>
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/units.h"
> +#include "qemu/error-report.h"
> +#include "qemu/module.h"
> +#include "qapi/error.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties-system.h"
> +#include "hw/sysbus.h"
> +#include "chardev/char-fe.h"
> +#include "exec/address-spaces.h"
> +#include "trace.h"
> +
> +#include "hw/misc/ivshmem-flat.h"
> +
> +static int64_t ivshmem_flat_recv_msg(IvshmemFTState *s, int *pfd)
> +{
> +    int64_t msg;
> +    int n, ret;
> +
> +    n = 0;
> +    do {
> +        ret = qemu_chr_fe_read_all(&s->server_chr, (uint8_t *)&msg + n,
> +                                   sizeof(msg) - n);

Coverity reported:

>>>     CID 1586089:  Insecure data handling  (INTEGER_OVERFLOW)
>>>     "8UL - n", which might have underflowed, is passed to 
"qemu_chr_fe_read_all(&s->server_chr, (uint8_t *)&msg + n, 8UL - n)".

Could you have a look?

> +        if (ret < 0) {
> +            if (ret == -EINTR) {
> +                continue;

Also as a future cleanup consider using the RETRY_ON_EINTR macro.

> +            }
> +            exit(1);
> +        }
> +        n += ret;
> +    } while (n < sizeof(msg));
> +
> +    if (pfd) {
> +        *pfd = qemu_chr_fe_get_msgfd(&s->server_chr);
> +    }
> +    return le64_to_cpu(msg);
> +}

Thanks!

Phil.