| Message ID | 20240529122232.25360-1-yskelg@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/xenbus: handle potential dangling pointer issue in xen_pcibk_xenbus_probe | expand |
On 29.05.24 14:22, yskelg@gmail.com wrote: > From: Yunseong Kim <yskelg@gmail.com> > > If 'xen_pcibk_init_devices()' fails. This ensures that 'pdev->xdev' does > not point to 'xdev' when 'pdev' is freed. > > Signed-off-by: Yunseong Kim <yskelg@gmail.com> > --- > drivers/xen/xen-pciback/xenbus.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c > index b11e401f1b1e..348d6803b8c0 100644 > --- a/drivers/xen/xen-pciback/xenbus.c > +++ b/drivers/xen/xen-pciback/xenbus.c > @@ -54,6 +54,7 @@ static struct xen_pcibk_device *alloc_pdev(struct xenbus_device *xdev) > INIT_WORK(&pdev->op_work, xen_pcibk_do_op); > > if (xen_pcibk_init_devices(pdev)) { > + pdev->xdev = NULL; > kfree(pdev); > pdev = NULL; > } NAK. This doesn't make any sense, as pdev is freed. Juergen
On 5/29/24 9:27 오후, Jürgen Groß wrote: > On 29.05.24 14:22, yskelg@gmail.com wrote: >> From: Yunseong Kim <yskelg@gmail.com> >> >> If 'xen_pcibk_init_devices()' fails. This ensures that 'pdev->xdev' does >> not point to 'xdev' when 'pdev' is freed. >> >> Signed-off-by: Yunseong Kim <yskelg@gmail.com> >> --- >> drivers/xen/xen-pciback/xenbus.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/xen/xen-pciback/xenbus.c >> b/drivers/xen/xen-pciback/xenbus.c >> index b11e401f1b1e..348d6803b8c0 100644 >> --- a/drivers/xen/xen-pciback/xenbus.c >> +++ b/drivers/xen/xen-pciback/xenbus.c >> @@ -54,6 +54,7 @@ static struct xen_pcibk_device *alloc_pdev(struct >> xenbus_device *xdev) >> INIT_WORK(&pdev->op_work, xen_pcibk_do_op); >> if (xen_pcibk_init_devices(pdev)) { >> + pdev->xdev = NULL; >> kfree(pdev); >> pdev = NULL; >> } > > NAK. > > This doesn't make any sense, as pdev is freed. > > > Juergen Thanks for the code review Juergen! I don't know if it's a big deal, but I think that if pdev->xdev don't replace it with NULL, 'struct xenbus_device *xdev' address value stays in the kmalloc cached memory. And most of the kernel code I've seen does this (even when it's 'free_pdev()'), so I sent a patch. In the very unlikely event that. I was thinking if someone look at the value without initializing the kmalloced memory, someone might see the 'xenbus_device *xdev address'. Warm Regards, Yunseong Kim
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c index b11e401f1b1e..348d6803b8c0 100644 --- a/drivers/xen/xen-pciback/xenbus.c +++ b/drivers/xen/xen-pciback/xenbus.c @@ -54,6 +54,7 @@ static struct xen_pcibk_device *alloc_pdev(struct xenbus_device *xdev) INIT_WORK(&pdev->op_work, xen_pcibk_do_op); if (xen_pcibk_init_devices(pdev)) { + pdev->xdev = NULL; kfree(pdev); pdev = NULL; }