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

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

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

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.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/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 */


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

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


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

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.


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

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

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