[PATCH] xenbus: fix possible crash in xenbus_uevent_backend

[PATCH] xenbus: fix possible crash in xenbus_uevent_backend

[Olaf Hering]

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

Re: [PATCH] xenbus: fix possible crash in xenbus_uevent_backend

[Ian Campbell]

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.

Re: [PATCH] xenbus: fix possible crash in xenbus_uevent_backend

[Jan Beulich]

>>> 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 */

Re: [PATCH] xenbus: fix possible crash in xenbus_uevent_backend

[Jan Beulich]

>>> 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

Re: [PATCH] xenbus: fix possible crash in xenbus_uevent_backend

[Ian Campbell]

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.

Re: [PATCH] xenbus: fix possible crash in xenbus_uevent_backend

[Olaf Hering]

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