| Message ID | 20181016175236.5840-1-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/acpi/nvdimm: Don't take address of fields in packed structs | expand |
On Tue, Oct 16, 2018 at 06:52:36PM +0100, Peter Maydell wrote: > Taking the address of a field in a packed struct is a bad idea, because > it might not be actually aligned enough for that pointer type (and > thus cause a crash on dereference on some host architectures). Newer > versions of clang warn about this. Avoid the bug by not using the > "modify in place" byte swapping functions. > > Patch produced with scripts/coccinelle/inplace-byteswaps.cocci. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Automatically generated patch, tested with "make check" only. > > hw/acpi/nvdimm.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 16/10/2018 19:52, Peter Maydell wrote: > Taking the address of a field in a packed struct is a bad idea, because > it might not be actually aligned enough for that pointer type (and > thus cause a crash on dereference on some host architectures). Newer > versions of clang warn about this. Avoid the bug by not using the > "modify in place" byte swapping functions. > > Patch produced with scripts/coccinelle/inplace-byteswaps.cocci. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > Automatically generated patch, tested with "make check" only. > > hw/acpi/nvdimm.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > index 27eeb6609f5..e53b2cb6819 100644 > --- a/hw/acpi/nvdimm.c > +++ b/hw/acpi/nvdimm.c > @@ -581,7 +581,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in, > int size; > > read_fit = (NvdimmFuncReadFITIn *)in->arg3; > - le32_to_cpus(&read_fit->offset); > + read_fit->offset = le32_to_cpu(read_fit->offset); > > fit = fit_buf->fit; > > @@ -742,8 +742,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, > int size; > > get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3; > - le32_to_cpus(&get_label_data->offset); > - le32_to_cpus(&get_label_data->length); > + get_label_data->offset = le32_to_cpu(get_label_data->offset); > + get_label_data->length = le32_to_cpu(get_label_data->length); > > nvdimm_debug("Read Label Data: offset %#x length %#x.\n", > get_label_data->offset, get_label_data->length); > @@ -781,8 +781,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, > > set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3; > > - le32_to_cpus(&set_label_data->offset); > - le32_to_cpus(&set_label_data->length); > + set_label_data->offset = le32_to_cpu(set_label_data->offset); > + set_label_data->length = le32_to_cpu(set_label_data->length); > > nvdimm_debug("Write Label Data: offset %#x length %#x.\n", > set_label_data->offset, set_label_data->length); > @@ -877,9 +877,9 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > in = g_new(NvdimmDsmIn, 1); > cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in)); > > - le32_to_cpus(&in->revision); > - le32_to_cpus(&in->function); > - le32_to_cpus(&in->handle); > + in->revision = le32_to_cpu(in->revision); > + in->function = le32_to_cpu(in->function); > + in->handle = le32_to_cpu(in->handle); > > nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision, > in->handle, in->function); >
Ping? This patch got reviewed but does not seem to have made it into anybody's tree. thanks -- PMM On 16 October 2018 at 18:52, Peter Maydell <peter.maydell@linaro.org> wrote: > Taking the address of a field in a packed struct is a bad idea, because > it might not be actually aligned enough for that pointer type (and > thus cause a crash on dereference on some host architectures). Newer > versions of clang warn about this. Avoid the bug by not using the > "modify in place" byte swapping functions. > > Patch produced with scripts/coccinelle/inplace-byteswaps.cocci. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Automatically generated patch, tested with "make check" only. > > hw/acpi/nvdimm.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > index 27eeb6609f5..e53b2cb6819 100644 > --- a/hw/acpi/nvdimm.c > +++ b/hw/acpi/nvdimm.c > @@ -581,7 +581,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in, > int size; > > read_fit = (NvdimmFuncReadFITIn *)in->arg3; > - le32_to_cpus(&read_fit->offset); > + read_fit->offset = le32_to_cpu(read_fit->offset); > > fit = fit_buf->fit; > > @@ -742,8 +742,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, > int size; > > get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3; > - le32_to_cpus(&get_label_data->offset); > - le32_to_cpus(&get_label_data->length); > + get_label_data->offset = le32_to_cpu(get_label_data->offset); > + get_label_data->length = le32_to_cpu(get_label_data->length); > > nvdimm_debug("Read Label Data: offset %#x length %#x.\n", > get_label_data->offset, get_label_data->length); > @@ -781,8 +781,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, > > set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3; > > - le32_to_cpus(&set_label_data->offset); > - le32_to_cpus(&set_label_data->length); > + set_label_data->offset = le32_to_cpu(set_label_data->offset); > + set_label_data->length = le32_to_cpu(set_label_data->length); > > nvdimm_debug("Write Label Data: offset %#x length %#x.\n", > set_label_data->offset, set_label_data->length); > @@ -877,9 +877,9 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > in = g_new(NvdimmDsmIn, 1); > cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in)); > > - le32_to_cpus(&in->revision); > - le32_to_cpus(&in->function); > - le32_to_cpus(&in->handle); > + in->revision = le32_to_cpu(in->revision); > + in->function = le32_to_cpu(in->function); > + in->handle = le32_to_cpu(in->handle); > > nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision, > in->handle, in->function); > --
Since nobody responded to my ping of a week ago I propose to just apply this to master... thanks -- PMM On 5 November 2018 at 14:40, Peter Maydell <peter.maydell@linaro.org> wrote: > Ping? This patch got reviewed but does not seem to have > made it into anybody's tree. > > thanks > -- PMM > > On 16 October 2018 at 18:52, Peter Maydell <peter.maydell@linaro.org> wrote: >> Taking the address of a field in a packed struct is a bad idea, because >> it might not be actually aligned enough for that pointer type (and >> thus cause a crash on dereference on some host architectures). Newer >> versions of clang warn about this. Avoid the bug by not using the >> "modify in place" byte swapping functions. >> >> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> Automatically generated patch, tested with "make check" only. >> >> hw/acpi/nvdimm.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index 27eeb6609f5..e53b2cb6819 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -581,7 +581,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in, >> int size; >> >> read_fit = (NvdimmFuncReadFITIn *)in->arg3; >> - le32_to_cpus(&read_fit->offset); >> + read_fit->offset = le32_to_cpu(read_fit->offset); >> >> fit = fit_buf->fit; >> >> @@ -742,8 +742,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, >> int size; >> >> get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3; >> - le32_to_cpus(&get_label_data->offset); >> - le32_to_cpus(&get_label_data->length); >> + get_label_data->offset = le32_to_cpu(get_label_data->offset); >> + get_label_data->length = le32_to_cpu(get_label_data->length); >> >> nvdimm_debug("Read Label Data: offset %#x length %#x.\n", >> get_label_data->offset, get_label_data->length); >> @@ -781,8 +781,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, >> >> set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3; >> >> - le32_to_cpus(&set_label_data->offset); >> - le32_to_cpus(&set_label_data->length); >> + set_label_data->offset = le32_to_cpu(set_label_data->offset); >> + set_label_data->length = le32_to_cpu(set_label_data->length); >> >> nvdimm_debug("Write Label Data: offset %#x length %#x.\n", >> set_label_data->offset, set_label_data->length); >> @@ -877,9 +877,9 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) >> in = g_new(NvdimmDsmIn, 1); >> cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in)); >> >> - le32_to_cpus(&in->revision); >> - le32_to_cpus(&in->function); >> - le32_to_cpus(&in->handle); >> + in->revision = le32_to_cpu(in->revision); >> + in->function = le32_to_cpu(in->function); >> + in->handle = le32_to_cpu(in->handle); >> >> nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision, >> in->handle, in->function); >> --
On Mon, Nov 12, 2018 at 02:42:16PM +0000, Peter Maydell wrote: > Since nobody responded to my ping of a week ago I propose to just > apply this to master... > > thanks > -- PMM Sorry. My LPC talk proposal suddenly got accepted and I was scrambling to get ready. Please feel free to apply this for now: Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Generally I think we need to rethink our approach to endian-ness. I think we want to tag fields with specific endian-ness and use static checkers to verify it. That is how Linux does it. In particular this will mean no swapping bytes in place. But that's a subject for another day. > On 5 November 2018 at 14:40, Peter Maydell <peter.maydell@linaro.org> wrote: > > Ping? This patch got reviewed but does not seem to have > > made it into anybody's tree. > > > > thanks > > -- PMM > > > > On 16 October 2018 at 18:52, Peter Maydell <peter.maydell@linaro.org> wrote: > >> Taking the address of a field in a packed struct is a bad idea, because > >> it might not be actually aligned enough for that pointer type (and > >> thus cause a crash on dereference on some host architectures). Newer > >> versions of clang warn about this. Avoid the bug by not using the > >> "modify in place" byte swapping functions. > >> > >> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci. > >> > >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > >> --- > >> Automatically generated patch, tested with "make check" only. > >> > >> hw/acpi/nvdimm.c | 16 ++++++++-------- > >> 1 file changed, 8 insertions(+), 8 deletions(-) > >> > >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > >> index 27eeb6609f5..e53b2cb6819 100644 > >> --- a/hw/acpi/nvdimm.c > >> +++ b/hw/acpi/nvdimm.c > >> @@ -581,7 +581,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in, > >> int size; > >> > >> read_fit = (NvdimmFuncReadFITIn *)in->arg3; > >> - le32_to_cpus(&read_fit->offset); > >> + read_fit->offset = le32_to_cpu(read_fit->offset); > >> > >> fit = fit_buf->fit; > >> > >> @@ -742,8 +742,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, > >> int size; > >> > >> get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3; > >> - le32_to_cpus(&get_label_data->offset); > >> - le32_to_cpus(&get_label_data->length); > >> + get_label_data->offset = le32_to_cpu(get_label_data->offset); > >> + get_label_data->length = le32_to_cpu(get_label_data->length); > >> > >> nvdimm_debug("Read Label Data: offset %#x length %#x.\n", > >> get_label_data->offset, get_label_data->length); > >> @@ -781,8 +781,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, > >> > >> set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3; > >> > >> - le32_to_cpus(&set_label_data->offset); > >> - le32_to_cpus(&set_label_data->length); > >> + set_label_data->offset = le32_to_cpu(set_label_data->offset); > >> + set_label_data->length = le32_to_cpu(set_label_data->length); > >> > >> nvdimm_debug("Write Label Data: offset %#x length %#x.\n", > >> set_label_data->offset, set_label_data->length); > >> @@ -877,9 +877,9 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > >> in = g_new(NvdimmDsmIn, 1); > >> cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in)); > >> > >> - le32_to_cpus(&in->revision); > >> - le32_to_cpus(&in->function); > >> - le32_to_cpus(&in->handle); > >> + in->revision = le32_to_cpu(in->revision); > >> + in->function = le32_to_cpu(in->function); > >> + in->handle = le32_to_cpu(in->handle); > >> > >> nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision, > >> in->handle, in->function); > >> --
On 12 November 2018 at 15:01, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Nov 12, 2018 at 02:42:16PM +0000, Peter Maydell wrote: >> Since nobody responded to my ping of a week ago I propose to just >> apply this to master... >> >> thanks >> -- PMM > > Sorry. My LPC talk proposal suddenly got accepted and > I was scrambling to get ready. > > Please feel free to apply this for now: > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Thanks. > Generally I think we need to rethink our approach to endian-ness. I > think we want to tag fields with specific endian-ness and use static > checkers to verify it. That is how Linux does it. > In particular this will mean no swapping bytes in place. Yeah, I agree that swapping-in-place is definitely not ideal. Personally I prefer to avoid having structs that try to match in-memory binary layouts entirely (ie no packed structs). > But that's a subject for another day. Indeed. -- PMM
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index 27eeb6609f5..e53b2cb6819 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -581,7 +581,7 @@ static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in, int size; read_fit = (NvdimmFuncReadFITIn *)in->arg3; - le32_to_cpus(&read_fit->offset); + read_fit->offset = le32_to_cpu(read_fit->offset); fit = fit_buf->fit; @@ -742,8 +742,8 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, int size; get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3; - le32_to_cpus(&get_label_data->offset); - le32_to_cpus(&get_label_data->length); + get_label_data->offset = le32_to_cpu(get_label_data->offset); + get_label_data->length = le32_to_cpu(get_label_data->length); nvdimm_debug("Read Label Data: offset %#x length %#x.\n", get_label_data->offset, get_label_data->length); @@ -781,8 +781,8 @@ static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in, set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3; - le32_to_cpus(&set_label_data->offset); - le32_to_cpus(&set_label_data->length); + set_label_data->offset = le32_to_cpu(set_label_data->offset); + set_label_data->length = le32_to_cpu(set_label_data->length); nvdimm_debug("Write Label Data: offset %#x length %#x.\n", set_label_data->offset, set_label_data->length); @@ -877,9 +877,9 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) in = g_new(NvdimmDsmIn, 1); cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in)); - le32_to_cpus(&in->revision); - le32_to_cpus(&in->function); - le32_to_cpus(&in->handle); + in->revision = le32_to_cpu(in->revision); + in->function = le32_to_cpu(in->function); + in->handle = le32_to_cpu(in->handle); nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision, in->handle, in->function);
Taking the address of a field in a packed struct is a bad idea, because it might not be actually aligned enough for that pointer type (and thus cause a crash on dereference on some host architectures). Newer versions of clang warn about this. Avoid the bug by not using the "modify in place" byte swapping functions. Patch produced with scripts/coccinelle/inplace-byteswaps.cocci. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Automatically generated patch, tested with "make check" only. hw/acpi/nvdimm.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)