Re: [PATCH v5 3/3] hw/nvme: add nvme management interface model
Hi Klaus,
On Tue, 2023-09-05 at 10:38 +0200, Klaus Jensen wrote:
> >
> > +static void nmi_handle_mi_config_get(NMIDevice *nmi, NMIRequest
> > *request)
> > +{
> > + uint32_t dw0 = le32_to_cpu(request->dw0);
> > + uint8_t identifier = FIELD_EX32(dw0,
> > NMI_CMD_CONFIGURATION_GET_DW0,
> > + IDENTIFIER);
> > + const uint8_t *buf;
> > +
> > + static const uint8_t smbus_freq[4] = {
> > + 0x00, /* success */
> > + 0x01, 0x00, 0x00, /* 100 kHz */
> > + };
> > +
> > + static const uint8_t mtu[4] = {
> > + 0x00, /* success */
> > + 0x40, 0x00, /* 64 */
> > + 0x00, /* reserved */
> > + };
> > +
> > + trace_nmi_handle_mi_config_get(identifier);
> > +
> > + switch (identifier) {
> > + case NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ:
> > + buf = smbus_freq;
> > + break;
> > +
> > + case NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT:
> > + buf = mtu;
> > + break;
> > +
> > + default:
> > + nmi_set_parameter_error(nmi, 0x0, offsetof(NMIRequest,
> > dw0));
> > + return;
> > + }
> > +
> > + nmi_scratch_append(nmi, buf, sizeof(buf));
> > +}
When I tried to build this patch I got:
```
In file included from /usr/include/string.h:535,
from /home/andrew/src/qemu.org/qemu/include/qemu/osdep.h:112,
from ../hw/nvme/nmi-i2c.c:12:
In function ‘memcpy’,
inlined from ‘nmi_scratch_append’ at ../hw/nvme/nmi-i2c.c:80:5,
inlined from ‘nmi_handle_mi_config_get’ at ../hw/nvme/nmi-i2c.c:246:5,
inlined from ‘nmi_handle_mi’ at ../hw/nvme/nmi-i2c.c:266:9,
inlined from ‘nmi_handle’ at ../hw/nvme/nmi-i2c.c:313:9:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error:
‘__builtin_memcpy’ forming offset [4, 7] is out of the bounds [0, 4]
[-Werror=array-bounds=]
29 | return __builtin___memcpy_chk (__dest, __src, __len,
| ^
30 | __glibc_objsize0 (__dest));
| ~~
```
It wasn't clear initially from the error that the source of the problem
was the size associated with the source buffer, especially as there is
some pointer arithmetic being done to derive `__dest`.
Anyway, what we're trying to express is that the size to copy from buf
is the size of the array pointed to by buf. However, buf is declared as
a pointer to uint8_t, which loses the length information. To fix that I
think we need:
- const uint8_t *buf;
+ const uint8_t (*buf)[4];
and then:
- nmi_scratch_append(nmi, buf, sizeof(buf));
+ nmi_scratch_append(nmi, buf, sizeof(*buf));
Andrew
Re: [PATCH v5 3/3] hw/nvme: add nvme management interface model
On Sep 12 13:50, Andrew Jeffery wrote:
> Hi Klaus,
>
> On Tue, 2023-09-05 at 10:38 +0200, Klaus Jensen wrote:
> > >
> > > +static void nmi_handle_mi_config_get(NMIDevice *nmi, NMIRequest
> > > *request)
> > > +{
> > > + uint32_t dw0 = le32_to_cpu(request->dw0);
> > > + uint8_t identifier = FIELD_EX32(dw0,
> > > NMI_CMD_CONFIGURATION_GET_DW0,
> > > + IDENTIFIER);
> > > + const uint8_t *buf;
> > > +
> > > + static const uint8_t smbus_freq[4] = {
> > > + 0x00, /* success */
> > > + 0x01, 0x00, 0x00, /* 100 kHz */
> > > + };
> > > +
> > > + static const uint8_t mtu[4] = {
> > > + 0x00, /* success */
> > > + 0x40, 0x00, /* 64 */
> > > + 0x00, /* reserved */
> > > + };
> > > +
> > > + trace_nmi_handle_mi_config_get(identifier);
> > > +
> > > + switch (identifier) {
> > > + case NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ:
> > > + buf = smbus_freq;
> > > + break;
> > > +
> > > + case NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT:
> > > + buf = mtu;
> > > + break;
> > > +
> > > + default:
> > > + nmi_set_parameter_error(nmi, 0x0, offsetof(NMIRequest,
> > > dw0));
> > > + return;
> > > + }
> > > +
> > > + nmi_scratch_append(nmi, buf, sizeof(buf));
> > > +}
>
> When I tried to build this patch I got:
>
> ```
> In file included from /usr/include/string.h:535,
> from /home/andrew/src/qemu.org/qemu/include/qemu/osdep.h:112,
> from ../hw/nvme/nmi-i2c.c:12:
> In function ‘memcpy’,
> inlined from ‘nmi_scratch_append’ at ../hw/nvme/nmi-i2c.c:80:5,
> inlined from ‘nmi_handle_mi_config_get’ at ../hw/nvme/nmi-i2c.c:246:5,
> inlined from ‘nmi_handle_mi’ at ../hw/nvme/nmi-i2c.c:266:9,
> inlined from ‘nmi_handle’ at ../hw/nvme/nmi-i2c.c:313:9:
> /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error:
> ‘__builtin_memcpy’ forming offset [4, 7] is out of the bounds [0, 4]
> [-Werror=array-bounds=]
>29 | return __builtin___memcpy_chk (__dest, __src, __len,
> | ^
>30 | __glibc_objsize0 (__dest));
> | ~~
> ```
>
> It wasn't clear initially from the error that the source of the problem
> was the size associated with the source buffer, especially as there is
> some pointer arithmetic being done to derive `__dest`.
>
> Anyway, what we're trying to express is that the size to copy from buf
> is the size of the array pointed to by buf. However, buf is declared as
> a pointer to uint8_t, which loses the length information. To fix that I
> think we need:
>
> - const uint8_t *buf;
> + const uint8_t (*buf)[4];
>
> and then:
>
> - nmi_scratch_append(nmi, buf, sizeof(buf));
> + nmi_scratch_append(nmi, buf, sizeof(*buf));
>
> Andrew
>
Hi Andrew,
Nice (and important) catch! Just curious, are you massaging QEMU's build
system into adding additional checks or how did your compiler catch
this?
Thanks,
Klaus
signature.asc
Description: PGP signature
Re: [PATCH v5 3/3] hw/nvme: add nvme management interface model
Hi Klaus,
On Thu, 2023-09-14 at 08:51 +0200, Klaus Jensen wrote:
> On Sep 12 13:50, Andrew Jeffery wrote:
> > Hi Klaus,
> >
> > On Tue, 2023-09-05 at 10:38 +0200, Klaus Jensen wrote:
> > > >
> > > > +static void nmi_handle_mi_config_get(NMIDevice *nmi, NMIRequest
> > > > *request)
> > > > +{
> > > > + uint32_t dw0 = le32_to_cpu(request->dw0);
> > > > + uint8_t identifier = FIELD_EX32(dw0,
> > > > NMI_CMD_CONFIGURATION_GET_DW0,
> > > > + IDENTIFIER);
> > > > + const uint8_t *buf;
> > > > +
> > > > + static const uint8_t smbus_freq[4] = {
> > > > + 0x00, /* success */
> > > > + 0x01, 0x00, 0x00, /* 100 kHz */
> > > > + };
> > > > +
> > > > + static const uint8_t mtu[4] = {
> > > > + 0x00, /* success */
> > > > + 0x40, 0x00, /* 64 */
> > > > + 0x00, /* reserved */
> > > > + };
> > > > +
> > > > + trace_nmi_handle_mi_config_get(identifier);
> > > > +
> > > > + switch (identifier) {
> > > > + case NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ:
> > > > + buf = smbus_freq;
> > > > + break;
> > > > +
> > > > + case NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT:
> > > > + buf = mtu;
> > > > + break;
> > > > +
> > > > + default:
> > > > + nmi_set_parameter_error(nmi, 0x0, offsetof(NMIRequest,
> > > > dw0));
> > > > + return;
> > > > + }
> > > > +
> > > > + nmi_scratch_append(nmi, buf, sizeof(buf));
> > > > +}
> >
> > When I tried to build this patch I got:
> >
> > ```
> > In file included from /usr/include/string.h:535,
> > from
> > /home/andrew/src/qemu.org/qemu/include/qemu/osdep.h:112,
> > from ../hw/nvme/nmi-i2c.c:12:
> > In function ‘memcpy’,
> > inlined from ‘nmi_scratch_append’ at ../hw/nvme/nmi-i2c.c:80:5,
> > inlined from ‘nmi_handle_mi_config_get’ at ../hw/nvme/nmi-i2c.c:246:5,
> > inlined from ‘nmi_handle_mi’ at ../hw/nvme/nmi-i2c.c:266:9,
> > inlined from ‘nmi_handle’ at ../hw/nvme/nmi-i2c.c:313:9:
> > /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error:
> > ‘__builtin_memcpy’ forming offset [4, 7] is out of the bounds [0, 4]
> > [-Werror=array-bounds=]
> >29 | return __builtin___memcpy_chk (__dest, __src, __len,
> > | ^
> >30 | __glibc_objsize0 (__dest));
> > | ~~
> > ```
> >
> > It wasn't clear initially from the error that the source of the problem
> > was the size associated with the source buffer, especially as there is
> > some pointer arithmetic being done to derive `__dest`.
> >
> > Anyway, what we're trying to express is that the size to copy from buf
> > is the size of the array pointed to by buf. However, buf is declared as
> > a pointer to uint8_t, which loses the length information. To fix that I
> > think we need:
> >
> > - const uint8_t *buf;
> > + const uint8_t (*buf)[4];
> >
> > and then:
> >
> > - nmi_scratch_append(nmi, buf, sizeof(buf));
> > + nmi_scratch_append(nmi, buf, sizeof(*buf));
> >
> > Andrew
> >
>
> Hi Andrew,
>
> Nice (and important) catch! Just curious, are you massaging QEMU's build
> system into adding additional checks or how did your compiler catch
> this?
No tricks to be honest, I just applied your patches on top of
9ef497755afc ("Merge tag 'pull-vfio-20230911' of
https://github.com/legoater/qemu into staging") using `b4 shazam ...`.
I'm building on Debian Bookworm:
$ gcc --version | head -n1
gcc (Debian 12.2.0-14) 12.2.0
Andrew
