* [PATCH] xenbus: fix possible crash in xenbus_uevent_backend @ 2011-07-18 12:40 Olaf Hering 2011-07-18 13:11 ` Ian Campbell 2011-07-18 13:18 ` Jan Beulich 0 siblings, 2 replies; 6+ messages in thread From: Olaf Hering @ 2011-07-18 12:40 UTC (permalink / raw) To: xen-devel Fix possible NULL pointer crash in xenbus_uevent_backend(). The variable to check for should probably be bus. Signed-off-by: Olaf Hering <olaf@aepfle.de> Index: linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c =================================================================== --- linux-3.0-rc7-xen-kexec.orig/drivers/xen/xenbus/xenbus_probe_backend.c +++ linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c @@ -104,7 +104,7 @@ static int xenbus_uevent_backend(struct xdev = to_xenbus_device(dev); bus = container_of(xdev->dev.bus, struct xen_bus_type, bus); - if (xdev == NULL) + if (bus == NULL) return -ENODEV; /* stuff we want to pass to /sbin/hotplug */ Index: linux-3.0-rc7-xen-kexec/include/linux/compiler.h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xenbus: fix possible crash in xenbus_uevent_backend 2011-07-18 12:40 [PATCH] xenbus: fix possible crash in xenbus_uevent_backend Olaf Hering @ 2011-07-18 13:11 ` Ian Campbell 2011-07-18 13:21 ` Jan Beulich 2011-07-18 15:02 ` Olaf Hering 2011-07-18 13:18 ` Jan Beulich 1 sibling, 2 replies; 6+ messages in thread From: Ian Campbell @ 2011-07-18 13:11 UTC (permalink / raw) To: Olaf Hering; +Cc: xen-devel@lists.xensource.com On Mon, 2011-07-18 at 13:40 +0100, Olaf Hering wrote: > Fix possible NULL pointer crash in xenbus_uevent_backend(). > The variable to check for should probably be bus. > > Signed-off-by: Olaf Hering <olaf@aepfle.de> > > Index: linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c > =================================================================== > --- linux-3.0-rc7-xen-kexec.orig/drivers/xen/xenbus/xenbus_probe_backend.c > +++ linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c > @@ -104,7 +104,7 @@ static int xenbus_uevent_backend(struct > > xdev = to_xenbus_device(dev); > bus = container_of(xdev->dev.bus, struct xen_bus_type, bus); > - if (xdev == NULL) > + if (bus == NULL) > return -ENODEV; Is this fixing an actual crash which you observed of just something you noticed looking at the code? container_of is pure pointer arithmetic without dereferencing so to get bus == NULL you'd need xdev == offsetof(struct xen_bus_type, bus) or some such. I think the check of xdev is correct, although it might be clearer if it preceded the "bus = ... " it's not actively harmful where it is since container_of doesn't dereference the pointer. Ian. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xenbus: fix possible crash in xenbus_uevent_backend 2011-07-18 13:11 ` Ian Campbell @ 2011-07-18 13:21 ` Jan Beulich 2011-07-18 13:26 ` Ian Campbell 2011-07-18 15:02 ` Olaf Hering 1 sibling, 1 reply; 6+ messages in thread From: Jan Beulich @ 2011-07-18 13:21 UTC (permalink / raw) To: Olaf Hering, Ian Campbell; +Cc: xen-devel@lists.xensource.com >>> On 18.07.11 at 15:11, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Mon, 2011-07-18 at 13:40 +0100, Olaf Hering wrote: >> Fix possible NULL pointer crash in xenbus_uevent_backend(). >> The variable to check for should probably be bus. >> >> Signed-off-by: Olaf Hering <olaf@aepfle.de> >> >> Index: linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c >> =================================================================== >> --- linux-3.0-rc7-xen-kexec.orig/drivers/xen/xenbus/xenbus_probe_backend.c >> +++ linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c >> @@ -104,7 +104,7 @@ static int xenbus_uevent_backend(struct >> >> xdev = to_xenbus_device(dev); >> bus = container_of(xdev->dev.bus, struct xen_bus_type, bus); >> - if (xdev == NULL) >> + if (bus == NULL) >> return -ENODEV; > > Is this fixing an actual crash which you observed of just something you > noticed looking at the code? > > container_of is pure pointer arithmetic without dereferencing so to get > bus == NULL you'd need xdev == offsetof(struct xen_bus_type, bus) or > some such. -offsetof(struct xen_bus_type, bus) > I think the check of xdev is correct, although it might be clearer if it Not really, as it similarly is the result of a container_of(). > preceded the "bus = ... " it's not actively harmful where it is since > container_of doesn't dereference the pointer. Doesn't? "xdev->dev.bus" very much looks like a de-reference to me. Jan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xenbus: fix possible crash in xenbus_uevent_backend 2011-07-18 13:21 ` Jan Beulich @ 2011-07-18 13:26 ` Ian Campbell 0 siblings, 0 replies; 6+ messages in thread From: Ian Campbell @ 2011-07-18 13:26 UTC (permalink / raw) To: Jan Beulich; +Cc: Olaf Hering, xen-devel@lists.xensource.com On Mon, 2011-07-18 at 14:21 +0100, Jan Beulich wrote: > >>> On 18.07.11 at 15:11, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Mon, 2011-07-18 at 13:40 +0100, Olaf Hering wrote: > >> Fix possible NULL pointer crash in xenbus_uevent_backend(). > >> The variable to check for should probably be bus. > >> > >> Signed-off-by: Olaf Hering <olaf@aepfle.de> > >> > >> Index: linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c > >> =================================================================== > >> --- linux-3.0-rc7-xen-kexec.orig/drivers/xen/xenbus/xenbus_probe_backend.c > >> +++ linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c > >> @@ -104,7 +104,7 @@ static int xenbus_uevent_backend(struct > >> > >> xdev = to_xenbus_device(dev); > >> bus = container_of(xdev->dev.bus, struct xen_bus_type, bus); > >> - if (xdev == NULL) > >> + if (bus == NULL) > >> return -ENODEV; > > > > Is this fixing an actual crash which you observed of just something you > > noticed looking at the code? > > > > container_of is pure pointer arithmetic without dereferencing so to get > > bus == NULL you'd need xdev == offsetof(struct xen_bus_type, bus) or > > some such. > > -offsetof(struct xen_bus_type, bus) > > > I think the check of xdev is correct, although it might be clearer if it > > Not really, as it similarly is the result of a container_of(). So it is, didn't spot that. > > preceded the "bus = ... " it's not actively harmful where it is since > > container_of doesn't dereference the pointer. > > Doesn't? "xdev->dev.bus" very much looks like a de-reference to me. Oh, right. I'll argue that that's the parameter to container_of de-referencing, not the macro itself, to make myself look less dumb ;-P Ian. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xenbus: fix possible crash in xenbus_uevent_backend 2011-07-18 13:11 ` Ian Campbell 2011-07-18 13:21 ` Jan Beulich @ 2011-07-18 15:02 ` Olaf Hering 1 sibling, 0 replies; 6+ messages in thread From: Olaf Hering @ 2011-07-18 15:02 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel@lists.xensource.com On Mon, Jul 18, Ian Campbell wrote: > On Mon, 2011-07-18 at 13:40 +0100, Olaf Hering wrote: > > Fix possible NULL pointer crash in xenbus_uevent_backend(). > > The variable to check for should probably be bus. > > > > Signed-off-by: Olaf Hering <olaf@aepfle.de> > > > > Index: linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c > > =================================================================== > > --- linux-3.0-rc7-xen-kexec.orig/drivers/xen/xenbus/xenbus_probe_backend.c > > +++ linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c > > @@ -104,7 +104,7 @@ static int xenbus_uevent_backend(struct > > > > xdev = to_xenbus_device(dev); > > bus = container_of(xdev->dev.bus, struct xen_bus_type, bus); > > - if (xdev == NULL) > > + if (bus == NULL) > > return -ENODEV; > > Is this fixing an actual crash which you observed of just something you > noticed looking at the code? I was browsing the code. Thanks to you and Jan for reviewing my attempt to fix something thats not broken. I will prepare a better patch. Olaf ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xenbus: fix possible crash in xenbus_uevent_backend 2011-07-18 12:40 [PATCH] xenbus: fix possible crash in xenbus_uevent_backend Olaf Hering 2011-07-18 13:11 ` Ian Campbell @ 2011-07-18 13:18 ` Jan Beulich 1 sibling, 0 replies; 6+ messages in thread From: Jan Beulich @ 2011-07-18 13:18 UTC (permalink / raw) To: Olaf Hering; +Cc: xen-devel >>> On 18.07.11 at 14:40, Olaf Hering <olaf@aepfle.de> wrote: > Fix possible NULL pointer crash in xenbus_uevent_backend(). > The variable to check for should probably be bus. > > Signed-off-by: Olaf Hering <olaf@aepfle.de> > > Index: linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c > =================================================================== > --- linux-3.0-rc7-xen-kexec.orig/drivers/xen/xenbus/xenbus_probe_backend.c > +++ linux-3.0-rc7-xen-kexec/drivers/xen/xenbus/xenbus_probe_backend.c > @@ -104,7 +104,7 @@ static int xenbus_uevent_backend(struct > > xdev = to_xenbus_device(dev); > bus = container_of(xdev->dev.bus, struct xen_bus_type, bus); > - if (xdev == NULL) > + if (bus == NULL) How can the result of a container_of() be NULL if the passed in value is in any way meaningful (i.e. valid or NULL)? If any such check is really necessary, wouldn't you rather want to check xdev->dev.bus here? Looking at the code (and its 2.6.18 tree's counterpart xenbus_uevent_frontend()), I would rather suspect that this wasn't meant to check bus in the first place, but instead needlessly tries to check the to_xenbus_device() result, which likewise can't reasonably be NULL (as that's again the result of a container_of()). Jan > return -ENODEV; > > /* stuff we want to pass to /sbin/hotplug */ ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-07-18 15:02 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-07-18 12:40 [PATCH] xenbus: fix possible crash in xenbus_uevent_backend Olaf Hering 2011-07-18 13:11 ` Ian Campbell 2011-07-18 13:21 ` Jan Beulich 2011-07-18 13:26 ` Ian Campbell 2011-07-18 15:02 ` Olaf Hering 2011-07-18 13:18 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.