1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/net: prevent potential NULL dereference

Oleg Sviridov posted 1 patch 5 months, 4 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240529110804.3636963-1-oleg.sviridov@red-soft.ru
Maintainers: Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, David Gibson <david@gibson.dropbear.id.au>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Jason Wang <jasowang@redhat.com>
There is a newer version of this series
hw/net/spapr_llan.c | 4 ++++
1 file changed, 4 insertions(+)
[PATCH] hw/net: prevent potential NULL dereference
Posted by Oleg Sviridov 5 months, 4 weeks ago 
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
Re: [PATCH] hw/net: prevent potential NULL dereference
Posted by David Gibson 5 months, 4 weeks ago 
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
Re: [PATCH] hw/net: prevent potential NULL dereference
Posted by Peter Maydell 5 months, 4 weeks ago 
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
Re: [PATCH] hw/net: prevent potential NULL dereference
Posted by David Gibson 5 months, 3 weeks ago 
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
[PATCH v2] hw/net: prevent potential NULL dereference
Posted by Oleg Sviridov 5 months, 3 weeks ago 
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
Re: [PATCH v2] hw/net: prevent potential NULL dereference
Posted by Philippe Mathieu-Daudé 4 months, 3 weeks ago 
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.
Re: [PATCH] hw/net: prevent potential NULL dereference
Posted by Philippe Mathieu-Daudé 5 months, 4 weeks ago 
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;
Re: [PATCH] hw/net: prevent potential NULL dereference
Posted by Олег Свиридов 5 months, 4 weeks ago 
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;
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.