Re: [Xen-devel] [PATCH] xenbus: fix possible crash in xenbus_uevent_backend

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Mon, 2011-07-18 at 14:21 +0100, Jan Beulich wrote:
> >>> On 18.07.11 at 15:11, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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@xxxxxxxxx>
> >> 
> >> 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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel