| Message ID | 20240529110804.3636963-1-oleg.sviridov@red-soft.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/net: prevent potential NULL dereference | expand |
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;
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;
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
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; >
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.
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;
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(+)