Mailing List Archive: [PATCH 2/2] block: support > 16 byte CDBs for SG

[PATCH 2/2] block: support > 16 byte CDBs for SG_IO

Jul 20, 2014, 3:23 AM

Post #1 of 4 (111 views)

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/scsi_ioctl.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index c4e6633..a804f3e 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -288,8 +288,6 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,

if (hdr->interface_id != 'S')
return -EINVAL;
- if (hdr->cmd_len > BLK_MAX_CDB)
- return -EINVAL;

if (hdr->dxfer_len > (queue_max_hw_sectors(q) << 9))
return -EIO;
@@ -306,14 +304,21 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
break;
}

+ ret = -ENOMEM;
rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL);
if (!rq)
- return -ENOMEM;
+ goto out;
blk_rq_set_block_pc(rq);

+ if (hdr->cmd_len > BLK_MAX_CDB) {
+ rq->cmd = kzalloc(hdr->cmd_len, GFP_KERNEL);
+ if (!rq->cmd)
+ goto out_put_request;
+ }
+
ret = -EFAULT;
if (blk_fill_sghdr_rq(q, rq, hdr, mode))
- goto out;
+ goto out_free_cdb;

if (hdr->iovec_count) {
size_t iov_data_len;
@@ -323,7 +328,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
0, NULL, &iov);
if (ret < 0) {
kfree(iov);
- goto out;
+ goto out_free_cdb;
}

iov_data_len = ret;
@@ -346,7 +351,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
GFP_KERNEL);

if (ret)
- goto out;
+ goto out_free_cdb;

bio = rq->bio;
memset(sense, 0, sizeof(sense));
@@ -365,8 +370,13 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
hdr->duration = jiffies_to_msecs(jiffies - start_time);

ret = blk_complete_sghdr_rq(rq, hdr, bio);
-out:
+
+out_free_cdb:
+ if (rq->cmd != rq->__cmd)
+ kfree(rq->cmd);
+out_put_request:
blk_put_request(rq);
+out:
return ret;
}

--
1.9.1

--
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/

Re: [PATCH 2/2] block: support > 16 byte CDBs for SG_IO [ In reply to ]

openosd at gmail

Jul 20, 2014, 4:47 AM

Post #2 of 4 (93 views)

Permalink

On 07/20/2014 01:23 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Hi Christoph I've quickly reviewed your code and have a few questions

> ---
> block/scsi_ioctl.c | 24 +++++++++++++++++-------
> 1 file changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index c4e6633..a804f3e 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -288,8 +288,6 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
>
> if (hdr->interface_id != 'S')
> return -EINVAL;
> - if (hdr->cmd_len > BLK_MAX_CDB)
> - return -EINVAL;
>
> if (hdr->dxfer_len > (queue_max_hw_sectors(q) << 9))
> return -EIO;
> @@ -306,14 +304,21 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
> break;
> }
>
> + ret = -ENOMEM;
> rq = blk_get_request(q, writing ? WRITE : READ, GFP_KERNEL);
> if (!rq)
> - return -ENOMEM;
> + goto out;
> blk_rq_set_block_pc(rq);
>
> + if (hdr->cmd_len > BLK_MAX_CDB) {
> + rq->cmd = kzalloc(hdr->cmd_len, GFP_KERNEL);

So two things here:
- hdr->cmd_len is char so can be MAX of 255. I understand that 4 bytes alignment is a SCSI
thing so you found no point of checking any max size?

- Why the zero alloc, if you are going to paste over it the exact same length. Now if like in scsi
you need 4 bytes alignment and zero padding yes, but here you do not do this (and probably shouldn't).
Hence why zero-alloc?

- Looking at sg.h where hdr->cmd_len is defined it stills has a comment of <= 16 you might want to
remove the comment by now.

> + if (!rq->cmd)
> + goto out_put_request;
> + }
> +
> ret = -EFAULT;
> if (blk_fill_sghdr_rq(q, rq, hdr, mode))

Inside here: blk_fill_sghdr_rq() calls blk_verify_command() which does:
(Below cmd[] is the buffer copied from user)

/* Anybody who can open the device can do a read-safe command */
if (test_bit(cmd[0], filter->read_ok))
return 0;

/* Write-safe commands require a writable open */
if (test_bit(cmd[0], filter->write_ok) && has_write_perm)
return 0;

Now I am not sure what type "Commands" you guys intend for these large commands
but if they are say SCSI-VARLEN this test will not work. Also a user might fool
the system with pretending to be "read" a device modifying command.

I would pass len to this test function and only permit these above if command is
<= 16. Any "special" large command is root only.

Thanks
Boaz

> - goto out;
> + goto out_free_cdb;
>
> if (hdr->iovec_count) {
> size_t iov_data_len;
> @@ -323,7 +328,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
> 0, NULL, &iov);
> if (ret < 0) {
> kfree(iov);
> - goto out;
> + goto out_free_cdb;
> }
>
> iov_data_len = ret;
> @@ -346,7 +351,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
> GFP_KERNEL);
>
> if (ret)
> - goto out;
> + goto out_free_cdb;
>
> bio = rq->bio;
> memset(sense, 0, sizeof(sense));
> @@ -365,8 +370,13 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
> hdr->duration = jiffies_to_msecs(jiffies - start_time);
>
> ret = blk_complete_sghdr_rq(rq, hdr, bio);
> -out:
> +
> +out_free_cdb:
> + if (rq->cmd != rq->__cmd)
> + kfree(rq->cmd);
> +out_put_request:
> blk_put_request(rq);
> +out:
> return ret;
> }
>
>

--
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/

Re: [PATCH 2/2] block: support > 16 byte CDBs for SG_IO [ In reply to ]

hch at lst

Jul 20, 2014, 6:27 AM

Post #3 of 4 (90 views)

Permalink

On Sun, Jul 20, 2014 at 02:47:49PM +0300, Boaz Harrosh wrote:
>
> So two things here:
> - hdr->cmd_len is char so can be MAX of 255. I understand that 4 bytes alignment is a SCSI
> thing so you found no point of checking any max size?

I don't see any point to force the aligmnet - the devices need to reject
garbage commands, and if for some reason we'd see future commands
that are > 252 and < 255 we are prepared to handle them.

> - Why the zero alloc, if you are going to paste over it the exact same length. Now if like in scsi
> you need 4 bytes alignment and zero padding yes, but here you do not do this (and probably shouldn't).
> Hence why zero-alloc?

No good reason except that's what sg and bsg do.

> - Looking at sg.h where hdr->cmd_len is defined it stills has a comment of <= 16 you might want to
> remove the comment by now.

The sg changes to support > 16 byte CDs remove the comment, take a look at my
core-for-3.17 tree which has them applied.

> Inside here: blk_fill_sghdr_rq() calls blk_verify_command() which does:
> (Below cmd[] is the buffer copied from user)
>
> /* Anybody who can open the device can do a read-safe command */
> if (test_bit(cmd[0], filter->read_ok))
> return 0;
>
> /* Write-safe commands require a writable open */
> if (test_bit(cmd[0], filter->write_ok) && has_write_perm)
> return 0;
>
> Now I am not sure what type "Commands" you guys intend for these large commands
> but if they are say SCSI-VARLEN this test will not work. Also a user might fool
> the system with pretending to be "read" a device modifying command.
>
> I would pass len to this test function and only permit these above if command is
> <= 16. Any "special" large command is root only.

Honestly that whole filter crap should never have been merged to start with,
you'll just need proper CAP_SYS_RAWIO for variable length commands.

--
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/

Re: [PATCH 2/2] block: support > 16 byte CDBs for SG_IO [ In reply to ]

openosd at gmail

Jul 20, 2014, 9:45 AM

Post #4 of 4 (87 views)

Permalink

On 07/20/2014 04:27 PM, Christoph Hellwig wrote:
> On Sun, Jul 20, 2014 at 02:47:49PM +0300, Boaz Harrosh wrote:
>>
>> So two things here:
>> - hdr->cmd_len is char so can be MAX of 255. I understand that 4 bytes alignment is a SCSI
>> thing so you found no point of checking any max size?
>
> I don't see any point to force the aligmnet - the devices need to reject
> garbage commands, and if for some reason we'd see future commands
> that are > 252 and < 255 we are prepared to handle them.
>

I agree

>> - Why the zero alloc, if you are going to paste over it the exact same length. Now if like in scsi
>> you need 4 bytes alignment and zero padding yes, but here you do not do this (and probably shouldn't).
>> Hence why zero-alloc?
>
> No good reason except that's what sg and bsg do.
>

Ha sorry didn't look there. Looks redundant to me that's all

<>
>> Inside here: blk_fill_sghdr_rq() calls blk_verify_command() which does:
>> (Below cmd[] is the buffer copied from user)
>>
>> /* Anybody who can open the device can do a read-safe command */
>> if (test_bit(cmd[0], filter->read_ok))
>> return 0;
>>
>> /* Write-safe commands require a writable open */
>> if (test_bit(cmd[0], filter->write_ok) && has_write_perm)
>> return 0;
>>
>> Now I am not sure what type "Commands" you guys intend for these large commands
>> but if they are say SCSI-VARLEN this test will not work. Also a user might fool
>> the system with pretending to be "read" a device modifying command.
>>
>> I would pass len to this test function and only permit these above if command is
>> <= 16. Any "special" large command is root only.
>
> Honestly that whole filter crap should never have been merged to start with,
> you'll just need proper CAP_SYS_RAWIO for variable length commands.
>
>

I agree. What I'm saying is - Are you sure that with current code as is we will not
pass on large commands without the proper CAP_SYS_RAWIO.

Should we not make sure and add:
/* root can do any command. */
if (capable(CAP_SYS_RAWIO))
return 0;
+
+ if (cmnd_len > BLK_MAX_CDB)
+ return -EPERM;

Rrrr you are right. I finally get the filter code. Anything that is not "white-listed"
is rejected. OK sorry for the noise.

Reviewed-by: Boaz Harrosh <boaz@plexistor.com>

Thanks
Boaz

--
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/