[Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs

[Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs

[Peter Maydell]

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);
-- 
2.19.0

Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs

[Philippe Mathieu-Daudé]

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);
> 

Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs

[Peter Maydell]

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);
> --

Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs

[Peter Maydell]

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);
>> --

Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs

[Michael S. Tsirkin]

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);
> >> --

Re: [Qemu-devel] [PATCH] hw/acpi/nvdimm: Don't take address of fields in packed structs

[Peter Maydell]

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