Skip to contentSkip to conversation

scsi_dh: potential null dereference in

PATCH In review Dan Carpenter 3 participants 5 messages Jan 3, 2011 – Jan 4, 2011

Subsystems (2)
SCSI Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 scsi_dh: potential null dereference in

We assumed "sdev" could be NULL ealier, so lets check it here as well.

Signed-off-by Dan Carpenter <[email protected]>
drivers/scsi/device_handler/scsi_dh.c +1 -1
index b837c5b..ff340e3 100644
@@ -446,7 +446,7 @@ int scsi_dh_activate(struct request_queue *q, activate_complete fn, void *data)
 	    sdev->sdev_state = SDEV_CANCEL ||
 	    sdev->sdev_state = SDEV_DEL)
 		err = SCSI_DH_NOSYS;
-	if (sdev->sdev_state = SDEV_OFFLINE)
+	if (sdev && sdev->sdev_state = SDEV_OFFLINE)
 		err = SCSI_DH_DEV_OFFLINED;
 	spin_unlock_irqrestore(q->queue_lock, flags);
#1 lore raw

Looks good to me.

Quoted 28 lines

-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Sunday, January 02, 2011 11:49 PM
To: James E.J. Bottomley
Cc: Mike Snitzer; Menny Hamburger; Moger, Babu; linux- [email protected]; [email protected]
Subject: [patch] [SCSI] scsi_dh: potential null dereference in scsi_dh_activate()

We assumed "sdev" could be NULL ealier, so lets check it here as well.

Signed-off-by Dan Carpenter <[email protected]>
drivers/scsi/device_handler/scsi_dh.c +1 -1
index b837c5b..ff340e3 100644
@@ -446,7 +446,7 @@ int scsi_dh_activate(struct request_queue *q,
 activate_complete fn, void *data)
 	    sdev->sdev_state == SDEV_CANCEL ||
 	    sdev->sdev_state == SDEV_DEL)
 		err = SCSI_DH_NOSYS;
-	if (sdev->sdev_state == SDEV_OFFLINE)
+	if (sdev && sdev->sdev_state == SDEV_OFFLINE)
 		err = SCSI_DH_DEV_OFFLINED;
 	spin_unlock_irqrestore(q->queue_lock, flags);
#2 lore raw
Quoted 1 line · Babu

Looks good to me.

It does? The first check is the bogus one, surely. The queue is created and destroyed by scsi_alloc_sdev(), so queuedata can never be NULL for a SCSI queue. There's no check anywhere in the rest of SCSI, so there shouldn't be one here, should there?

James

#3 lore raw
Quoted 10 lines

-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Tuesday, January 04, 2011 10:25 AM
To: Moger, Babu
Cc: Dan Carpenter; Mike Snitzer; Menny Hamburger; linux- [email protected]; [email protected]
Subject: RE: [patch] [SCSI] scsi_dh: potential null dereference in scsi_dh_activate()

Quoted 7 lines
Quoted 1 line · Babu

Looks good to me.

It does? The first check is the bogus one, surely. The queue is created and destroyed by scsi_alloc_sdev(), so queuedata can never be NULL for a SCSI queue. There's no check anywhere in the rest of SCSI, so there shouldn't be one here, should there?

You are right. This check may not be required.

But I am not sure why there is a check in scsi_device_from_queue.
Is there a possibility of request_fn other than scsi_request_fn for scsi device? I don’t know. Here is the code..

struct scsi_device *scsi_device_from_queue(struct request_queue *q)

{
        struct scsi_device *sdev = NULL;

        if (q->request_fn == scsi_request_fn)
                sdev = q->queuedata;

        return sdev;
}
#4 lore raw
Quoted 22 lines · Babu
Quoted 9 lines

-----Original Message-----
From: James Bottomley [mailto:[email protected]]
Sent: Tuesday, January 04, 2011 10:25 AM
To: Moger, Babu
Cc: Dan Carpenter; Mike Snitzer; Menny Hamburger; linux- [email protected]; [email protected]
Subject: RE: [patch] [SCSI] scsi_dh: potential null dereference in scsi_dh_activate()

Quoted 7 lines
Quoted 1 line · Babu

Looks good to me.

It does? The first check is the bogus one, surely. The queue is created and destroyed by scsi_alloc_sdev(), so queuedata can never be NULL for a SCSI queue. There's no check anywhere in the rest of SCSI, so there shouldn't be one here, should there?

You are right. This check may not be required.

But I am not sure why there is a check in scsi_device_from_queue.
Is there a possibility of request_fn other than scsi_request_fn for scsi device? I don’t know. Here is the code..

Quoted 8 lines

struct scsi_device *scsi_device_from_queue(struct request_queue *q)

{
        struct scsi_device *sdev = NULL;

        if (q->request_fn = scsi_request_fn)
                sdev = q->queuedata;

        return sdev;

It can be called for any queue so it returns NULL for a non-SCSI queue. If you think the queue scsi_dh_activate() is called on may be a non-scsi one, then you need to use scsi_device_from_queue() and check the result. Checking q->queuedata for NULL isn't sufficient because other devices are perfectly entitled to use q->queuedata for their own purposes. If you know you have a SCSI queue, you can just pick sdev out of q->queuedata.

James

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html