Follow the bsg and sg drivers to support large CDBs by allocation
the larger than 16 byte CDB array if nessecary.
Make sure we always clean up through the out label and just have
a single place to put the request.
Signed-off-by: Christoph Hellwig <[email protected]>
---
block/scsi_ioctl.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 14695c6..c4e6633 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -272,7 +272,6 @@ static int blk_complete_sghdr_rq(struct request *rq, struct sg_io_hdr *hdr,
r = blk_rq_unmap_user(bio);
if (!ret)
ret = r;
- blk_put_request(rq);
return ret;
}
@@ -312,10 +311,9 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
return -ENOMEM;
blk_rq_set_block_pc(rq);
- if (blk_fill_sghdr_rq(q, rq, hdr, mode)) {
- blk_put_request(rq);
- return -EFAULT;
- }
+ ret = -EFAULT;
+ if (blk_fill_sghdr_rq(q, rq, hdr, mode))
+ goto out;
if (hdr->iovec_count) {
size_t iov_data_len;
@@ -366,7 +364,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
hdr->duration = jiffies_to_msecs(jiffies - start_time);
- return blk_complete_sghdr_rq(rq, hdr, bio);
+ ret = blk_complete_sghdr_rq(rq, hdr, bio);
out:
blk_put_request(rq);
return ret;
--
1.9.1
Signed-off-by: Christoph Hellwig <[email protected]>
---
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
On 07/20/2014 01:23 PM, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <[email protected]>
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;
> }
>
>
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.
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 <[email protected]>
Thanks
Boaz