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