Re: [PATCH] Avoid null-pointer access in w1/slaves/w1_therm

[David Fries <David@Fries.net>]

Thanks for preparing the patch, it looks like it will go another
round, but that happens to everyone.

To simplify to show a problem,

mutex_lock_interruptible(&dev->bus_mutex);

if (!sl->family_data)
	return -ENODEV;

If family_data is NULL, then dev->bus_mutex is left locked.  Your
earlier e-mails indicated that you weren't seeing any problems after
applying this patch, but I would have thought once this early return
was encountered, that bus would not be usable any longer because
bus_mutex would be locked.  I don't know if the bus controller went
away, so there is effectively a new bus, because I assume you are
still able to read the temperature once this condition triggers?

It looks like you need to add mutex_unlock(&dev->bus_mutex); before
return -ENODEV; and that's how it is handled elsewhere in the
function, but you might want to put a printk before the return,
trigger it and verify it is getting hung up on the bus_mutex, it would
also be telling if you can remove the wire and the other w1 modules,
they should not let you remove them while a mutex is held.


The patch needs to be run through the checkpatch script, and it's
listing two issues, adding a space after the if (I also prefer not
having it, but this is for consistency), and missing Signed-off-by:

That would look like this, only with your information, it's a
tracing of where the changes came from and agreeing to the copyright,
you can read more in Documentation/SubmittingPatches if you haven't
already.
Signed-off-by: David Fries <David@Fries.net>

scripts/checkpatch.pl /tmp/w1_null_patch.patch

Thanks, keep up improving things.

On Fri, Feb 27, 2015 at 09:43:14AM +0100, Thorsten.Bschorr wrote:
> w1_slave_show unlocks the bus while waiting for the sensor
> to convert the temperature. If the sensor gets disconnected
> during that time, sl->family_data could get NULL.
> Note: the w1_slave pointer inside w1_slave_show is protected by
> sysfs-mutex
> ---
>  drivers/w1/slaves/w1_therm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
> index 1f11a20..3a14cb1 100644
> --- a/drivers/w1/slaves/w1_therm.c
> +++ b/drivers/w1/slaves/w1_therm.c
> @@ -236,6 +236,12 @@ static ssize_t w1_slave_show(struct device *device,
>  				i = mutex_lock_interruptible(&dev->bus_mutex);
>  				if (i != 0)
>  					return i;
> +
> +				/* Ensure that device is still there -
> +				 * it could have gone while we unlocked the bus.
> +				 * Note: sl is still protected by sysfs-mutex */
> +				if(!sl->family_data)
> +					return -ENODEV;
>  			} else if (!w1_strong_pullup) {
>  				sleep_rem = msleep_interruptible(tm);
>  				if (sleep_rem != 0) {
> -- 
> 1.8.4.5
> 
> 
> ---
> Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
> http://www.avast.com
> 

-- 
David Fries <david@fries.net>    PGP pub CB1EE8F0
http://fries.net/~david/