[PATCH] scsi_dh: only attach to SCSI devices

[PATCH] scsi_dh: only attach to SCSI devices

[Hannes Reinecke]

Any device might be setting a queuedata structure, so we need to
check if the queuedata really belongs to a SCSI device before
proceeding.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_dh.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
index b8d3b97..da104ed 100644
--- a/drivers/scsi/scsi_dh.c
+++ b/drivers/scsi/scsi_dh.c
@@ -226,7 +226,9 @@ static struct scsi_device *get_sdev_from_queue(struct request_queue *q)
 
 	spin_lock_irqsave(q->queue_lock, flags);
 	sdev = q->queuedata;
-	if (!sdev || !get_device(&sdev->sdev_gendev))
+	if (!sdev ||
+	    !scsi_is_sdev_device(&sdev->sdev_gendev) ||
+	    !get_device(&sdev->sdev_gendev))
 		sdev = NULL;
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
-- 
1.8.5.6

Re: [PATCH] scsi_dh: only attach to SCSI devices

[Bart Van Assche]

On Thu, 2017-02-16 at 08:33 +0100, Hannes Reinecke wrote:
> Any device might be setting a queuedata structure, so we need to
> check if the queuedata really belongs to a SCSI device before
> proceeding.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/scsi_dh.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
> index b8d3b97..da104ed 100644
> --- a/drivers/scsi/scsi_dh.c
> +++ b/drivers/scsi/scsi_dh.c
> @@ -226,7 +226,9 @@ static struct scsi_device *get_sdev_from_queue(struct request_queue *q)
>  
>  	spin_lock_irqsave(q->queue_lock, flags);
>  	sdev = q->queuedata;
> -	if (!sdev || !get_device(&sdev->sdev_gendev))
> +	if (!sdev ||
> +	    !scsi_is_sdev_device(&sdev->sdev_gendev) ||
> +	    !get_device(&sdev->sdev_gendev))
>  		sdev = NULL;
>  	spin_unlock_irqrestore(q->queue_lock, flags);

Hello Hannes,

Sorry but this approach looks wrong to me. A block driver can store any data
in .queuedata, even data that would cause the scsi_is_sdev_device() function
to crash.

Bart.

Re: [PATCH] scsi_dh: only attach to SCSI devices

[Christoph Hellwig]

On Thu, Feb 16, 2017 at 08:33:14AM +0100, Hannes Reinecke wrote:
> Any device might be setting a queuedata structure, so we need to
> check if the queuedata really belongs to a SCSI device before
> proceeding.

Can you add a comment next to the scsi_is_sdev_device call explaining
it?  The thing is a bit of a hack, but probably the best we can do for
now.

Re: [PATCH] scsi_dh: only attach to SCSI devices

[Bart Van Assche]

On Thu, 2017-02-16 at 15:32 +0100, Christoph Hellwig wrote:
> On Thu, Feb 16, 2017 at 08:33:14AM +0100, Hannes Reinecke wrote:
> > Any device might be setting a queuedata structure, so we need to
> > check if the queuedata really belongs to a SCSI device before
> > proceeding.
> 
> Can you add a comment next to the scsi_is_sdev_device call explaining
> it?  The thing is a bit of a hack, but probably the best we can do for
> now.

A much better approach would be to change the type of the argument of
get_sdev_from_queue().

Bart.

Re: [PATCH] scsi_dh: only attach to SCSI devices

[hch]

On Thu, Feb 16, 2017 at 02:30:34PM +0000, Bart Van Assche wrote:
> >  	sdev = q->queuedata;
> > -	if (!sdev || !get_device(&sdev->sdev_gendev))
> > +	if (!sdev ||
> > +	    !scsi_is_sdev_device(&sdev->sdev_gendev) ||
> > +	    !get_device(&sdev->sdev_gendev))
> >  		sdev = NULL;
> >  	spin_unlock_irqrestore(q->queue_lock, flags);
> 
> Hello Hannes,
> 
> Sorry but this approach looks wrong to me. A block driver can store any data
> in .queuedata, even data that would cause the scsi_is_sdev_device() function
> to crash.

Yes, I fear you're right.  I guess we need to call into the scsi_dh_*
functions through an indirection mediated by the block layer.

Re: [PATCH] scsi_dh: only attach to SCSI devices

[Hannes Reinecke]

On 02/16/2017 03:38 PM, hch@lst.de wrote:
> On Thu, Feb 16, 2017 at 02:30:34PM +0000, Bart Van Assche wrote:
>>>  	sdev = q->queuedata;
>>> -	if (!sdev || !get_device(&sdev->sdev_gendev))
>>> +	if (!sdev ||
>>> +	    !scsi_is_sdev_device(&sdev->sdev_gendev) ||
>>> +	    !get_device(&sdev->sdev_gendev))
>>>  		sdev = NULL;
>>>  	spin_unlock_irqrestore(q->queue_lock, flags);
>>
>> Hello Hannes,
>>
>> Sorry but this approach looks wrong to me. A block driver can store any data
>> in .queuedata, even data that would cause the scsi_is_sdev_device() function
>> to crash.
> 
> Yes, I fear you're right.  I guess we need to call into the scsi_dh_*
> functions through an indirection mediated by the block layer.
> 
Okay, will be tooling it up.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)