support > 16 byte CDBs for blocklayer SG_IO

support > 16 byte CDBs for blocklayer SG_IO

[Christoph Hellwig]

Follow the bsg and sg drivers to support large CDBs by allocation
the larger than 16 byte CDB array if nessecary.

[PATCH 1/2] block: cleanup error handling in sg_io

[Christoph Hellwig]

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 <hch@lst.de>
---
 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

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

[Christoph Hellwig]

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

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

[Boaz Harrosh]

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;
>  }
>  
> 

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

[Christoph Hellwig]

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.

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

[Boaz Harrosh]

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