[Qemu-devel] [PATCH 0/2] ipmi: fix open() NULL argument

[Qemu-devel] [PATCH 0/2] ipmi: fix open() NULL argument

[Cédric Le Goater]

Hello,

Here is a small series fixing an NULL argument issue in the IPMI BMC
simulator and activating the IPMI tests under PPC64.

Thanks,

C.

Cédric Le Goater (2):
  tests: add IPMI tests for ppc64
  ipmi: don't try to open a NULL filename

 hw/ipmi/ipmi_bmc_sim.c | 5 +++++
 tests/Makefile.include | 2 ++
 2 files changed, 7 insertions(+)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] tests: add IPMI tests for ppc64

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 tests/Makefile.include | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 579ec07cce03..8ff22ccfbdde 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -287,6 +287,8 @@ check-qtest-ppc64-y += tests/pnv-xscom-test$(EXESUF)
 check-qtest-ppc64-y += tests/drive_del-test$(EXESUF)
 check-qtest-ppc64-y += tests/postcopy-test$(EXESUF)
 check-qtest-ppc64-y += tests/boot-serial-test$(EXESUF)
+check-qtest-ppc64-y += tests/ipmi-kcs-test$(EXESUF)
+check-qtest-ppc64-y += tests/ipmi-bt-test$(EXESUF)
 check-qtest-ppc64-y += tests/rtas-test$(EXESUF)
 check-qtest-ppc64-y += tests/pxe-test$(EXESUF)
 check-qtest-ppc64-y += tests/usb-hcd-ohci-test$(EXESUF)
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] ipmi: don't try to open a NULL filename

[Cédric Le Goater]

Currenlty, the code relies on the fact that open() handles NULL
filenames but that can cause an error with new clang:

  hw/core/loader.c:67:15: runtime error: null pointer passed as argument 1,
  which is declared to never be null

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ipmi/ipmi_bmc_sim.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 155561d06614..277c28cb40ed 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -1899,6 +1899,10 @@ static void ipmi_fru_init(IPMIFru *fru)
     int fsize;
     int size = 0;
 
+    if (!fru->filename) {
+        goto out;
+    }
+
     fsize = get_image_size(fru->filename);
     if (fsize > 0) {
         size = QEMU_ALIGN_UP(fsize, fru->areasize);
@@ -1910,6 +1914,7 @@ static void ipmi_fru_init(IPMIFru *fru)
         }
     }
 
+out:
     if (!fru->data) {
         /* give one default FRU */
         size = fru->areasize;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/2] ipmi: don't try to open a NULL filename

[Eric Blake]

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

On 04/25/2017 01:51 AM, Cédric Le Goater wrote:
> Currenlty, the code relies on the fact that open() handles NULL
> filenames but that can cause an error with new clang:

POSIX doesn't require it to work. Linux is nice and gives you EFAULT,
but there are systems where open(NULL) crashes with SIGSEGV.  The clang
warning is correct.

> 
>   hw/core/loader.c:67:15: runtime error: null pointer passed as argument 1,
>   which is declared to never be null
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ipmi/ipmi_bmc_sim.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/2] ipmi: don't try to open a NULL filename

[Peter Maydell]

On 25 April 2017 at 07:51, Cédric Le Goater <clg@kaod.org> wrote:
> Currenlty, the code relies on the fact that open() handles NULL
> filenames but that can cause an error with new clang:
>
>   hw/core/loader.c:67:15: runtime error: null pointer passed as argument 1,
>   which is declared to never be null

This isn't "new clang", incidentally, it's just clang with the
runtime-static-analysis enabled, which causes warnings to be
printed at runtime for undefined behaviours. You can enable
this by passing configure --extra-cflags=-fsanitize=undefined .
(I have clang 3.8.0 here.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] ipmi: don't try to open a NULL filename

[Cédric Le Goater]

On 04/25/2017 03:31 PM, Peter Maydell wrote:
> On 25 April 2017 at 07:51, Cédric Le Goater <clg@kaod.org> wrote:
>> Currenlty, the code relies on the fact that open() handles NULL
>> filenames but that can cause an error with new clang:
>>
>>   hw/core/loader.c:67:15: runtime error: null pointer passed as argument 1,
>>   which is declared to never be null
> 
> This isn't "new clang", incidentally, it's just clang with the
> runtime-static-analysis enabled, which causes warnings to be
> printed at runtime for undefined behaviours. You can enable
> this by passing configure --extra-cflags=-fsanitize=undefined .
> (I have clang 3.8.0 here.)

OK. So I have now reproduced the issue on a f24. That made be 
realize that the IPMI tests check for the arch they are being 
run on and that more changes are required for them to run on 
ppc64. 

We can drop patch 1/2. Tell me if you want a resend.

Thanks,

C.  

Re: [Qemu-devel] [PATCH 2/2] ipmi: don't try to open a NULL filename

[David Gibson]

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

On Tue, Apr 25, 2017 at 04:06:17PM +0200, Cédric Le Goater wrote:
> On 04/25/2017 03:31 PM, Peter Maydell wrote:
> > On 25 April 2017 at 07:51, Cédric Le Goater <clg@kaod.org> wrote:
> >> Currenlty, the code relies on the fact that open() handles NULL
> >> filenames but that can cause an error with new clang:
> >>
> >>   hw/core/loader.c:67:15: runtime error: null pointer passed as argument 1,
> >>   which is declared to never be null
> > 
> > This isn't "new clang", incidentally, it's just clang with the
> > runtime-static-analysis enabled, which causes warnings to be
> > printed at runtime for undefined behaviours. You can enable
> > this by passing configure --extra-cflags=-fsanitize=undefined .
> > (I have clang 3.8.0 here.)
> 
> OK. So I have now reproduced the issue on a f24. That made be 
> realize that the IPMI tests check for the arch they are being 
> run on and that more changes are required for them to run on 
> ppc64. 
> 
> We can drop patch 1/2. Tell me if you want a resend.

No, that's fine.  A patch to enable the tests on ppc properly ASAP
would be great, though.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [Qemu-devel] [PATCH 2/2] ipmi: don't try to open a NULL filename

[Cédric Le Goater]

On 04/26/2017 04:43 AM, David Gibson wrote:
> On Tue, Apr 25, 2017 at 04:06:17PM +0200, Cédric Le Goater wrote:
>> On 04/25/2017 03:31 PM, Peter Maydell wrote:
>>> On 25 April 2017 at 07:51, Cédric Le Goater <clg@kaod.org> wrote:
>>>> Currenlty, the code relies on the fact that open() handles NULL
>>>> filenames but that can cause an error with new clang:
>>>>
>>>>   hw/core/loader.c:67:15: runtime error: null pointer passed as argument 1,
>>>>   which is declared to never be null
>>>
>>> This isn't "new clang", incidentally, it's just clang with the
>>> runtime-static-analysis enabled, which causes warnings to be
>>> printed at runtime for undefined behaviours. You can enable
>>> this by passing configure --extra-cflags=-fsanitize=undefined .
>>> (I have clang 3.8.0 here.)
>>
>> OK. So I have now reproduced the issue on a f24. That made be 
>> realize that the IPMI tests check for the arch they are being 
>> run on and that more changes are required for them to run on 
>> ppc64. 
>>
>> We can drop patch 1/2. Tell me if you want a resend.
> 
> No, that's fine.  A patch to enable the tests on ppc properly ASAP
> would be great, though.

Yes, it would. I need to understand what is behind : 

	qtest_irq_intercept_in(...)

C.

Re: [Qemu-devel] [PATCH 2/2] ipmi: don't try to open a NULL filename

[David Gibson]

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

On Tue, Apr 25, 2017 at 08:51:41AM +0200, Cédric Le Goater wrote:
> Currenlty, the code relies on the fact that open() handles NULL
> filenames but that can cause an error with new clang:
> 
>   hw/core/loader.c:67:15: runtime error: null pointer passed as argument 1,
>   which is declared to never be null
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Since my ppc-for-2.10 pull request has been held up because of this
anyway, tather than just apply this on top, I've folded it into your
earlier patch which caused the bug - that way we won't break bisect.

> ---
>  hw/ipmi/ipmi_bmc_sim.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 155561d06614..277c28cb40ed 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -1899,6 +1899,10 @@ static void ipmi_fru_init(IPMIFru *fru)
>      int fsize;
>      int size = 0;
>  
> +    if (!fru->filename) {
> +        goto out;
> +    }
> +
>      fsize = get_image_size(fru->filename);
>      if (fsize > 0) {
>          size = QEMU_ALIGN_UP(fsize, fru->areasize);
> @@ -1910,6 +1914,7 @@ static void ipmi_fru_init(IPMIFru *fru)
>          }
>      }
>  
> +out:
>      if (!fru->data) {
>          /* give one default FRU */
>          size = fru->areasize;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [Qemu-devel] [PATCH 2/2] ipmi: don't try to open a NULL filename

[Cédric Le Goater]

On 04/26/2017 04:42 AM, David Gibson wrote:
> On Tue, Apr 25, 2017 at 08:51:41AM +0200, Cédric Le Goater wrote:
>> Currenlty, the code relies on the fact that open() handles NULL
>> filenames but that can cause an error with new clang:
>>
>>   hw/core/loader.c:67:15: runtime error: null pointer passed as argument 1,
>>   which is declared to never be null
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Since my ppc-for-2.10 pull request has been held up because of this
> anyway, tather than just apply this on top, I've folded it into your
> earlier patch which caused the bug - that way we won't break bisect.

Yes that is the best option. 

Thanks,

C.