qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hw/net: prevent potential NULL dereference
@ 2024-05-29 11:07 Oleg Sviridov
  2024-05-29 13:52 ` Philippe Mathieu-Daudé
  2024-05-30  0:29 ` David Gibson
  0 siblings, 2 replies; 8+ messages in thread
From: Oleg Sviridov @ 2024-05-29 11:07 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: Oleg Sviridov, Daniel Henrique Barboza, David Gibson,
	Harsh Prateek Bora, Jason Wang, qemu-ppc, qemu-devel

Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is dereferenced immediately after.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Oleg Sviridov <oleg.sviridov@red-soft.ru>
---
 hw/net/spapr_llan.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index ecb30b7c76..f40b733229 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -770,6 +770,10 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU *cpu,
     SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
     int i;
 
+    if (!dev) {
+        return H_PARAMETER;
+    }
+
     for (i = 0; i < ETH_ALEN; i++) {
         dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
         macaddr >>= 8;
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/net: prevent potential NULL dereference
  2024-05-29 11:07 [PATCH] hw/net: prevent potential NULL dereference Oleg Sviridov
@ 2024-05-29 13:52 ` Philippe Mathieu-Daudé
  2024-05-30 14:38   ` Олег Свиридов
  2024-05-30  0:29 ` David Gibson
  1 sibling, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-05-29 13:52 UTC (permalink / raw)
  To: Oleg Sviridov, Nicholas Piggin
  Cc: Daniel Henrique Barboza, David Gibson, Harsh Prateek Bora,
	Jason Wang, qemu-ppc, qemu-devel

On 29/5/24 13:07, Oleg Sviridov wrote:
> Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is dereferenced immediately after.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Oleg Sviridov <oleg.sviridov@red-soft.ru>
> ---
>   hw/net/spapr_llan.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> index ecb30b7c76..f40b733229 100644
> --- a/hw/net/spapr_llan.c
> +++ b/hw/net/spapr_llan.c
> @@ -770,6 +770,10 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU *cpu,
>       SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
>       int i;
>   

Trying to change a MAC when no NIC is present is dubious, we could
at least report this using qemu_log_mask(LOG_GUEST_ERROR).

> +    if (!dev) {
> +        return H_PARAMETER;
> +    }
> +
>       for (i = 0; i < ETH_ALEN; i++) {
>           dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
>           macaddr >>= 8;



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/net: prevent potential NULL dereference
  2024-05-29 11:07 [PATCH] hw/net: prevent potential NULL dereference Oleg Sviridov
  2024-05-29 13:52 ` Philippe Mathieu-Daudé
@ 2024-05-30  0:29 ` David Gibson
  2024-05-30  9:03   ` Peter Maydell
  1 sibling, 1 reply; 8+ messages in thread
From: David Gibson @ 2024-05-30  0:29 UTC (permalink / raw)
  To: Oleg Sviridov
  Cc: Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora,
	Jason Wang, qemu-ppc, qemu-devel

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

On Wed, May 29, 2024 at 02:07:18PM +0300, Oleg Sviridov wrote:
> Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is dereferenced immediately after.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Oleg Sviridov <oleg.sviridov@red-soft.ru>
> ---
>  hw/net/spapr_llan.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> index ecb30b7c76..f40b733229 100644
> --- a/hw/net/spapr_llan.c
> +++ b/hw/net/spapr_llan.c
> @@ -770,6 +770,10 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU *cpu,
>      SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);

Hmm... I thought VIO_SPAPR_VLAN_DEVICE() was supposed to abort if sdev
was NULL or not of the right type.  Or have the rules for qom helpers
changed since I wrote this.

>      int i;
>  
> +    if (!dev) {
> +        return H_PARAMETER;
> +    }
> +
>      for (i = 0; i < ETH_ALEN; i++) {
>          dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
>          macaddr >>= 8;

-- 
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: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/net: prevent potential NULL dereference
  2024-05-30  0:29 ` David Gibson
@ 2024-05-30  9:03   ` Peter Maydell
  2024-05-31  4:52     ` David Gibson
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2024-05-30  9:03 UTC (permalink / raw)
  To: David Gibson
  Cc: Oleg Sviridov, Nicholas Piggin, Daniel Henrique Barboza,
	Harsh Prateek Bora, Jason Wang, qemu-ppc, qemu-devel

On Thu, 30 May 2024 at 01:52, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Wed, May 29, 2024 at 02:07:18PM +0300, Oleg Sviridov wrote:
> > Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is dereferenced immediately after.
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >
> > Signed-off-by: Oleg Sviridov <oleg.sviridov@red-soft.ru>
> > ---
> >  hw/net/spapr_llan.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> > index ecb30b7c76..f40b733229 100644
> > --- a/hw/net/spapr_llan.c
> > +++ b/hw/net/spapr_llan.c
> > @@ -770,6 +770,10 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU *cpu,
> >      SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
>
> Hmm... I thought VIO_SPAPR_VLAN_DEVICE() was supposed to abort if sdev
> was NULL or not of the right type.  Or have the rules for qom helpers
> changed since I wrote this.

QOM casts abort if the type is wrong, but a NULL pointer is
passed through as a NULL pointer.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/net: prevent potential NULL dereference
  2024-05-29 13:52 ` Philippe Mathieu-Daudé
@ 2024-05-30 14:38   ` Олег Свиридов
  0 siblings, 0 replies; 8+ messages in thread
From: Олег Свиридов @ 2024-05-30 14:38 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Nicholas Piggin
  Cc: Daniel Henrique Barboza, David Gibson, Harsh Prateek Bora,
	Jason Wang, qemu-ppc, qemu-devel

Thanks for review.

Would it be correct to use hcall_dprintf() as in other functions of the 
module? For example, in h_add_logical_lan_buffer().

Best regards,
Oleg.

29.05.2024 16:52, Philippe Mathieu-Daudé пишет:
> On 29/5/24 13:07, Oleg Sviridov wrote:
>> Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL 
>> and is dereferenced immediately after.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Signed-off-by: Oleg Sviridov <oleg.sviridov@red-soft.ru>
>> ---
>>   hw/net/spapr_llan.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
>> index ecb30b7c76..f40b733229 100644
>> --- a/hw/net/spapr_llan.c
>> +++ b/hw/net/spapr_llan.c
>> @@ -770,6 +770,10 @@ static target_ulong 
>> h_change_logical_lan_mac(PowerPCCPU *cpu,
>>       SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
>>       int i;
> 
> Trying to change a MAC when no NIC is present is dubious, we could
> at least report this using qemu_log_mask(LOG_GUEST_ERROR).
> 
>> +    if (!dev) {
>> +        return H_PARAMETER;
>> +    }
>> +
>>       for (i = 0; i < ETH_ALEN; i++) {
>>           dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
>>           macaddr >>= 8;
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] hw/net: prevent potential NULL dereference
  2024-05-30  9:03   ` Peter Maydell
@ 2024-05-31  4:52     ` David Gibson
  2024-05-31  7:36       ` [PATCH v2] " Oleg Sviridov
  0 siblings, 1 reply; 8+ messages in thread
From: David Gibson @ 2024-05-31  4:52 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Oleg Sviridov, Nicholas Piggin, Daniel Henrique Barboza,
	Harsh Prateek Bora, Jason Wang, qemu-ppc, qemu-devel

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

On Thu, May 30, 2024 at 10:03:51AM +0100, Peter Maydell wrote:
> On Thu, 30 May 2024 at 01:52, David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > On Wed, May 29, 2024 at 02:07:18PM +0300, Oleg Sviridov wrote:
> > > Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is dereferenced immediately after.
> > >
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > >
> > > Signed-off-by: Oleg Sviridov <oleg.sviridov@red-soft.ru>
> > > ---
> > >  hw/net/spapr_llan.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> > > index ecb30b7c76..f40b733229 100644
> > > --- a/hw/net/spapr_llan.c
> > > +++ b/hw/net/spapr_llan.c
> > > @@ -770,6 +770,10 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU *cpu,
> > >      SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
> >
> > Hmm... I thought VIO_SPAPR_VLAN_DEVICE() was supposed to abort if sdev
> > was NULL or not of the right type.  Or have the rules for qom helpers
> > changed since I wrote this.
> 
> QOM casts abort if the type is wrong, but a NULL pointer is
> passed through as a NULL pointer.

Ah, my mistake.  LGTM, then.

-- 
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: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] hw/net: prevent potential NULL dereference
  2024-05-31  4:52     ` David Gibson
@ 2024-05-31  7:36       ` Oleg Sviridov
  2024-07-01 15:02         ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 8+ messages in thread
From: Oleg Sviridov @ 2024-05-31  7:36 UTC (permalink / raw)
  To: Nicholas Piggin
  Cc: Oleg Sviridov, Daniel Henrique Barboza, David Gibson,
	Harsh Prateek Bora, Jason Wang, qemu-ppc, qemu-devel

Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is dereferenced immediately after.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Oleg Sviridov <oleg.sviridov@red-soft.ru>
---
v2: Debug message added
 hw/net/spapr_llan.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index ecb30b7c76..8af33d91b6 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -770,6 +770,12 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU *cpu,
     SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
     int i;
 
+    if (!dev) {
+        hcall_dprintf("H_CHANGE_LOGICAL_LAN_MAC called when "
+                      "no NIC is present\n");
+        return H_PARAMETER;
+    }
+
     for (i = 0; i < ETH_ALEN; i++) {
         dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
         macaddr >>= 8;
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] hw/net: prevent potential NULL dereference
  2024-05-31  7:36       ` [PATCH v2] " Oleg Sviridov
@ 2024-07-01 15:02         ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-07-01 15:02 UTC (permalink / raw)
  To: Oleg Sviridov, Nicholas Piggin
  Cc: Daniel Henrique Barboza, David Gibson, Harsh Prateek Bora,
	Jason Wang, qemu-ppc, qemu-devel

On 31/5/24 09:36, Oleg Sviridov wrote:
> Pointer, returned from function 'spapr_vio_find_by_reg', may be NULL and is dereferenced immediately after.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Oleg Sviridov <oleg.sviridov@red-soft.ru>
> ---
> v2: Debug message added
>   hw/net/spapr_llan.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
> index ecb30b7c76..8af33d91b6 100644
> --- a/hw/net/spapr_llan.c
> +++ b/hw/net/spapr_llan.c
> @@ -770,6 +770,12 @@ static target_ulong h_change_logical_lan_mac(PowerPCCPU *cpu,
>       SpaprVioVlan *dev = VIO_SPAPR_VLAN_DEVICE(sdev);
>       int i;
>   
> +    if (!dev) {
> +        hcall_dprintf("H_CHANGE_LOGICAL_LAN_MAC called when "
> +                      "no NIC is present\n");
> +        return H_PARAMETER;
> +    }
> +
>       for (i = 0; i < ETH_ALEN; i++) {
>           dev->nicconf.macaddr.a[ETH_ALEN - i - 1] = macaddr & 0xff;
>           macaddr >>= 8;

Thanks, patch queued.


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-07-01 15:05 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-29 11:07 [PATCH] hw/net: prevent potential NULL dereference Oleg Sviridov
2024-05-29 13:52 ` Philippe Mathieu-Daudé
2024-05-30 14:38   ` Олег Свиридов
2024-05-30  0:29 ` David Gibson
2024-05-30  9:03   ` Peter Maydell
2024-05-31  4:52     ` David Gibson
2024-05-31  7:36       ` [PATCH v2] " Oleg Sviridov
2024-07-01 15:02         ` Philippe Mathieu-Daudé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).