Date: Sat, 28 Feb 2015 14:17:37 -0600
From: David Fries <David@Fries.net>
To: "Thorsten.Bschorr" <thorsten@bschorr.de>
Cc: linux-kernel@vger.kernel.org, Evgeniy Polyakov <zbr@ioremap.net>
Subject: Re: [PATCH] Avoid null-pointer access in w1/slaves/w1_therm
Message-ID: <20150228201737.GU6151@spacedout.fries.net>
References: <CA+1k2cH5Y+RngvEbgE3CW5g+bO3V1ytDJC=u6DLVDZvbEOhu5A@mail.gmail.com>
 <54F02E22.5050901@bschorr.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <54F02E22.5050901@bschorr.de>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3152
Lines: 84

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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/