| Message ID | 20240715095939.72492-1-zhao1.liu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD() in nubus_virtio_mmio_realize() | expand |
Hi Philippe, If possible, can this one catch a ride with your PULL too? Many thanks! Zhao On Mon, Jul 15, 2024 at 05:59:37PM +0800, Zhao Liu wrote: > Date: Mon, 15 Jul 2024 17:59:37 +0800 > From: Zhao Liu <zhao1.liu@intel.com> > Subject: [PATCH] hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD() in > nubus_virtio_mmio_realize() > X-Mailer: git-send-email 2.34.1 > > As the comment in qapi/error, dereferencing @errp requires > ERRP_GUARD(): > > * = Why, when and how to use ERRP_GUARD() = > * > * Without ERRP_GUARD(), use of the @errp parameter is restricted: > * - It must not be dereferenced, because it may be null. > ... > * ERRP_GUARD() lifts these restrictions. > * > * To use ERRP_GUARD(), add it right at the beginning of the function. > * @errp can then be used without worrying about the argument being > * NULL or &error_fatal. > * > * Using it when it's not needed is safe, but please avoid cluttering > * the source with useless code. > > But in nubus_virtio_mmio_realize(), @errp is dereferenced without > ERRP_GUARD(). > > Although nubus_virtio_mmio_realize() - as a DeviceClass.realize() > method - doesn't get the NULL @errp parameter, it hasn't triggered the > bug that dereferencing the NULL @errp. It's still necessary to follow > the requirement of @errp, so add missing ERRP_GUARD() in > nubus_virtio_mmio_realize(). > > Cc: Laurent Vivier <laurent@vivier.eu> > Cc: Philippe Mathieu-Daudé <philmd@linaro.org> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > --- > hw/nubus/nubus-virtio-mmio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/nubus/nubus-virtio-mmio.c b/hw/nubus/nubus-virtio-mmio.c > index 58a63c84d0be..a5558d3ec28b 100644 > --- a/hw/nubus/nubus-virtio-mmio.c > +++ b/hw/nubus/nubus-virtio-mmio.c > @@ -23,6 +23,7 @@ static void nubus_virtio_mmio_set_input_irq(void *opaque, int n, int level) > > static void nubus_virtio_mmio_realize(DeviceState *dev, Error **errp) > { > + ERRP_GUARD(); > NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_GET_CLASS(dev); > NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(dev); > NubusDevice *nd = NUBUS_DEVICE(dev); > -- > 2.34.1 >
+Markus/Eric for review On 17/7/24 13:27, Zhao Liu wrote: > Hi Philippe, > > If possible, can this one catch a ride with your PULL too? > > Many thanks! > Zhao > > On Mon, Jul 15, 2024 at 05:59:37PM +0800, Zhao Liu wrote: >> Date: Mon, 15 Jul 2024 17:59:37 +0800 >> From: Zhao Liu <zhao1.liu@intel.com> >> Subject: [PATCH] hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD() in >> nubus_virtio_mmio_realize() >> X-Mailer: git-send-email 2.34.1 >> >> As the comment in qapi/error, dereferencing @errp requires >> ERRP_GUARD(): >> >> * = Why, when and how to use ERRP_GUARD() = >> * >> * Without ERRP_GUARD(), use of the @errp parameter is restricted: >> * - It must not be dereferenced, because it may be null. >> ... >> * ERRP_GUARD() lifts these restrictions. >> * >> * To use ERRP_GUARD(), add it right at the beginning of the function. >> * @errp can then be used without worrying about the argument being >> * NULL or &error_fatal. >> * >> * Using it when it's not needed is safe, but please avoid cluttering >> * the source with useless code. >> >> But in nubus_virtio_mmio_realize(), @errp is dereferenced without >> ERRP_GUARD(). >> >> Although nubus_virtio_mmio_realize() - as a DeviceClass.realize() >> method - doesn't get the NULL @errp parameter, it hasn't triggered the >> bug that dereferencing the NULL @errp. It's still necessary to follow >> the requirement of @errp, so add missing ERRP_GUARD() in >> nubus_virtio_mmio_realize(). >> >> Cc: Laurent Vivier <laurent@vivier.eu> >> Cc: Philippe Mathieu-Daudé <philmd@linaro.org> >> Signed-off-by: Zhao Liu <zhao1.liu@intel.com> >> --- >> hw/nubus/nubus-virtio-mmio.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/nubus/nubus-virtio-mmio.c b/hw/nubus/nubus-virtio-mmio.c >> index 58a63c84d0be..a5558d3ec28b 100644 >> --- a/hw/nubus/nubus-virtio-mmio.c >> +++ b/hw/nubus/nubus-virtio-mmio.c >> @@ -23,6 +23,7 @@ static void nubus_virtio_mmio_set_input_irq(void *opaque, int n, int level) >> >> static void nubus_virtio_mmio_realize(DeviceState *dev, Error **errp) >> { >> + ERRP_GUARD(); >> NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_GET_CLASS(dev); >> NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(dev); >> NubusDevice *nd = NUBUS_DEVICE(dev); >> -- >> 2.34.1 >>
Zhao Liu <zhao1.liu@intel.com> writes: > As the comment in qapi/error, dereferencing @errp requires Suggest "According to the comment in qapi/error.h". > ERRP_GUARD(): > > * = Why, when and how to use ERRP_GUARD() = > * > * Without ERRP_GUARD(), use of the @errp parameter is restricted: > * - It must not be dereferenced, because it may be null. > ... > * ERRP_GUARD() lifts these restrictions. > * > * To use ERRP_GUARD(), add it right at the beginning of the function. > * @errp can then be used without worrying about the argument being > * NULL or &error_fatal. > * > * Using it when it's not needed is safe, but please avoid cluttering > * the source with useless code. > > But in nubus_virtio_mmio_realize(), @errp is dereferenced without > ERRP_GUARD(). Suggest to scratch "But". > Although nubus_virtio_mmio_realize() - as a DeviceClass.realize() > method - doesn't get the NULL @errp parameter, it hasn't triggered the > bug that dereferencing the NULL @errp. It's still necessary to follow > the requirement of @errp, so add missing ERRP_GUARD() in > nubus_virtio_mmio_realize(). Suggest Although nubus_virtio_mmio_realize() - as a DeviceClass.realize() method - is never passed a null @errp argument, it should follow the rules on @errp usage. Add the ERRP_GUARD() there. > Cc: Laurent Vivier <laurent@vivier.eu> > Cc: Philippe Mathieu-Daudé <philmd@linaro.org> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > --- > hw/nubus/nubus-virtio-mmio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/nubus/nubus-virtio-mmio.c b/hw/nubus/nubus-virtio-mmio.c > index 58a63c84d0be..a5558d3ec28b 100644 > --- a/hw/nubus/nubus-virtio-mmio.c > +++ b/hw/nubus/nubus-virtio-mmio.c > @@ -23,6 +23,7 @@ static void nubus_virtio_mmio_set_input_irq(void *opaque, int n, int level) > > static void nubus_virtio_mmio_realize(DeviceState *dev, Error **errp) > { > + ERRP_GUARD(); > NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_GET_CLASS(dev); > NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(dev); > NubusDevice *nd = NUBUS_DEVICE(dev); SysBusDevice *sbd; int i, offset; nvmdc->parent_realize(dev, errp); Here's the dereference: if (*errp) { return; } Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks!
Hi Markus, On Tue, Jul 23, 2024 at 12:21:17PM +0200, Markus Armbruster wrote: > Date: Tue, 23 Jul 2024 12:21:17 +0200 > From: Markus Armbruster <armbru@redhat.com> > Subject: Re: [PATCH] hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD() > in nubus_virtio_mmio_realize() > > Zhao Liu <zhao1.liu@intel.com> writes: > > > As the comment in qapi/error, dereferencing @errp requires > > Suggest "According to the comment in qapi/error.h". Thanks! Good words. > > ERRP_GUARD(): > > > > * = Why, when and how to use ERRP_GUARD() = > > * > > * Without ERRP_GUARD(), use of the @errp parameter is restricted: > > * - It must not be dereferenced, because it may be null. > > ... > > * ERRP_GUARD() lifts these restrictions. > > * > > * To use ERRP_GUARD(), add it right at the beginning of the function. > > * @errp can then be used without worrying about the argument being > > * NULL or &error_fatal. > > * > > * Using it when it's not needed is safe, but please avoid cluttering > > * the source with useless code. > > > > But in nubus_virtio_mmio_realize(), @errp is dereferenced without > > ERRP_GUARD(). > > Suggest to scratch "But". No problem, will do. > > Although nubus_virtio_mmio_realize() - as a DeviceClass.realize() > > method - doesn't get the NULL @errp parameter, it hasn't triggered the > > bug that dereferencing the NULL @errp. It's still necessary to follow > > the requirement of @errp, so add missing ERRP_GUARD() in > > nubus_virtio_mmio_realize(). > > Suggest > > Although nubus_virtio_mmio_realize() - as a DeviceClass.realize() > method - is never passed a null @errp argument, it should follow the > rules on @errp usage. Add the ERRP_GUARD() there. Thanks for the text! It sounds much more authentic! > > Cc: Laurent Vivier <laurent@vivier.eu> > > Cc: Philippe Mathieu-Daudé <philmd@linaro.org> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > --- > > hw/nubus/nubus-virtio-mmio.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/nubus/nubus-virtio-mmio.c b/hw/nubus/nubus-virtio-mmio.c > > index 58a63c84d0be..a5558d3ec28b 100644 > > --- a/hw/nubus/nubus-virtio-mmio.c > > +++ b/hw/nubus/nubus-virtio-mmio.c > > @@ -23,6 +23,7 @@ static void nubus_virtio_mmio_set_input_irq(void *opaque, int n, int level) > > > > static void nubus_virtio_mmio_realize(DeviceState *dev, Error **errp) > > { > > + ERRP_GUARD(); > > NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_GET_CLASS(dev); > > NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(dev); > > NubusDevice *nd = NUBUS_DEVICE(dev); > SysBusDevice *sbd; > int i, offset; > > nvmdc->parent_realize(dev, errp); > > Here's the dereference: > > if (*errp) { > return; > } > > Reviewed-by: Markus Armbruster <armbru@redhat.com> > Thanks! Will refresh a v2 soon. Regards, Zhao
diff --git a/hw/nubus/nubus-virtio-mmio.c b/hw/nubus/nubus-virtio-mmio.c index 58a63c84d0be..a5558d3ec28b 100644 --- a/hw/nubus/nubus-virtio-mmio.c +++ b/hw/nubus/nubus-virtio-mmio.c @@ -23,6 +23,7 @@ static void nubus_virtio_mmio_set_input_irq(void *opaque, int n, int level) static void nubus_virtio_mmio_realize(DeviceState *dev, Error **errp) { + ERRP_GUARD(); NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_GET_CLASS(dev); NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(dev); NubusDevice *nd = NUBUS_DEVICE(dev);
As the comment in qapi/error, dereferencing @errp requires ERRP_GUARD(): * = Why, when and how to use ERRP_GUARD() = * * Without ERRP_GUARD(), use of the @errp parameter is restricted: * - It must not be dereferenced, because it may be null. ... * ERRP_GUARD() lifts these restrictions. * * To use ERRP_GUARD(), add it right at the beginning of the function. * @errp can then be used without worrying about the argument being * NULL or &error_fatal. * * Using it when it's not needed is safe, but please avoid cluttering * the source with useless code. But in nubus_virtio_mmio_realize(), @errp is dereferenced without ERRP_GUARD(). Although nubus_virtio_mmio_realize() - as a DeviceClass.realize() method - doesn't get the NULL @errp parameter, it hasn't triggered the bug that dereferencing the NULL @errp. It's still necessary to follow the requirement of @errp, so add missing ERRP_GUARD() in nubus_virtio_mmio_realize(). Cc: Laurent Vivier <laurent@vivier.eu> Cc: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- hw/nubus/nubus-virtio-mmio.c | 1 + 1 file changed, 1 insertion(+)