[PATCH] CDROM_SEND_PACKET bug

I just found Yet Another Bug in scsi_ioctl - CDROM_SEND_PACKET puts a
kernel pointer in hdr->cmdp, where sg_io() expects to find user address.
This worked up until recently because of the memcpy bug, but now it
doesn't because we do the proper copy_from_user().

This fix undoes the user copy code from sg_io, and instead makes the
SG_IO ioctl copy it locally.  This makes SG_IO and CDROM_SEND_PACKET
agree on the calling convention, and everybody is happy.

I've tested that both

   cdrecord -dev=/dev/hdc -inq

and

   cdrecord -dev=ATAPI:/dev/hdc -inq

works now.  The former will use SG_IO, the latter CDROM_SEND_PACKET (and
incidentally would work in both 2.4 and 2.6, if it wasn't for
CDROM_SEND_PACKET sucking badly in 2.4).

diff --git a/drivers/block/scsi_ioctl.c b/drivers/block/scsi_ioctl.c
index 098425adddf02b9dcac9f3dd526b1154ad94969c..0c02c5ab3eebfb6f6fbe89c4174120091ca48a35 100644 (file)
--- a/drivers/block/scsi_ioctl.c
+++ b/drivers/block/scsi_ioctl.c
@@ -150,7 +150,6 @@ static int sg_io(request_queue_t *q, struct block_device *bdev,
        struct request *rq;
        struct bio *bio;
        char sense[SCSI_SENSE_BUFFERSIZE];
-       unsigned char cdb[BLK_MAX_CDB];
        void *buffer;
 
        if (hdr->interface_id != 'S')
@@ -167,9 +166,6 @@ static int sg_io(request_queue_t *q, struct block_device *bdev,
        if (hdr->dxfer_len > (q->max_sectors << 9))
                return -EIO;
 
-       if (copy_from_user(cdb, hdr->cmdp, hdr->cmd_len))
-               return -EFAULT;
-
        reading = writing = 0;
        buffer = NULL;
        bio = NULL;
@@ -220,7 +216,7 @@ static int sg_io(request_queue_t *q, struct block_device *bdev,
         * fill in request structure
         */
        rq->cmd_len = hdr->cmd_len;
-       memcpy(rq->cmd, cdb, hdr->cmd_len);
+       memcpy(rq->cmd, hdr->cmdp, hdr->cmd_len);
        if (sizeof(rq->cmd) != hdr->cmd_len)
                memset(rq->cmd + hdr->cmd_len, 0, sizeof(rq->cmd) - hdr->cmd_len);
 
@@ -436,12 +432,23 @@ int scsi_cmd_ioctl(struct block_device *bdev, unsigned int cmd, unsigned long ar
                        break;
                case SG_IO: {
                        struct sg_io_hdr hdr;
+                       unsigned char cdb[BLK_MAX_CDB], *old_cdb;
 
-                       if (copy_from_user(&hdr, (struct sg_io_hdr *) arg, sizeof(hdr))) {
-                               err = -EFAULT;
+                       err = -EFAULT;
+                       if (copy_from_user(&hdr, (struct sg_io_hdr *) arg, sizeof(hdr)))
                                break;
-                       }
+                       err = -EINVAL;
+                       if (hdr.cmd_len > sizeof(rq->cmd))
+                               break;
+                       err = -EFAULT;
+                       if (copy_from_user(cdb, hdr.cmdp, hdr.cmd_len))
+                               break;
+
+                       old_cdb = hdr.cmdp;
+                       hdr.cmdp = cdb;
                        err = sg_io(q, bdev, &hdr);
+
+                       hdr.cmdp = old_cdb;
                        if (copy_to_user((struct sg_io_hdr *) arg, &hdr, sizeof(hdr)))
                                err = -EFAULT;
                        break;