* [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 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-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-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).