diff mbox

[v2,17/30] scsi: Define usercopy region in scsi_sense_cache slab cache

Message ID 1503956111-36652-18-git-send-email-keescook@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Kees Cook Aug. 28, 2017, 9:34 p.m. UTC 
From: David Windsor <dave@nullcore.net>

SCSI sense buffers, stored in struct scsi_cmnd.sense and therefore
contained in the scsi_sense_cache slab cache, need to be copied to/from
userspace.

cache object allocation:
    drivers/scsi/scsi_lib.c:
        scsi_select_sense_cache(...):
            return ... ? scsi_sense_isadma_cache : scsi_sense_cache

        scsi_alloc_sense_buffer(...):
            return kmem_cache_alloc_node(scsi_select_sense_cache(), ...);

        scsi_init_request(...):
            ...
            cmd->sense_buffer = scsi_alloc_sense_buffer(...);
            ...
            cmd->req.sense = cmd->sense_buffer

example usage trace:

    block/scsi_ioctl.c:
        (inline from sg_io)
        blk_complete_sghdr_rq(...):
            struct scsi_request *req = scsi_req(rq);
            ...
            copy_to_user(..., req->sense, len)

        scsi_cmd_ioctl(...):
            sg_io(...);

In support of usercopy hardening, this patch defines a region in
the scsi_sense_cache slab cache in which userspace copy operations
are allowed.

This region is known as the slab cache's usercopy region.  Slab
caches can now check that each copy operation involving cache-managed
memory falls entirely within the slab's usercopy region.

Signed-off-by: David Windsor <dave@nullcore.net>
[kees: adjust commit log, provide usage trace]
Cc: "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/scsi/scsi_lib.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Bart Van Assche Aug. 28, 2017, 9:42 p.m. UTC | #1 
On Mon, 2017-08-28 at 14:34 -0700, Kees Cook wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c

> index f6097b89d5d3..f1c6bd56dd5b 100644

> --- a/drivers/scsi/scsi_lib.c

> +++ b/drivers/scsi/scsi_lib.c

> @@ -77,14 +77,15 @@ int scsi_init_sense_cache(struct Scsi_Host *shost)

>  	if (shost->unchecked_isa_dma) {

>  		scsi_sense_isadma_cache =

>  			kmem_cache_create("scsi_sense_cache(DMA)",

> -			SCSI_SENSE_BUFFERSIZE, 0,

> -			SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);

> +				SCSI_SENSE_BUFFERSIZE, 0,

> +				SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);

>  		if (!scsi_sense_isadma_cache)

>  			ret = -ENOMEM;


All this part of this patch does is to change source code indentation. Should
these changes really be included in this patch?

Thanks,

Bart.
Kees Cook Aug. 28, 2017, 9:52 p.m. UTC | #2 
On Mon, Aug 28, 2017 at 2:42 PM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
> On Mon, 2017-08-28 at 14:34 -0700, Kees Cook wrote:
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index f6097b89d5d3..f1c6bd56dd5b 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -77,14 +77,15 @@ int scsi_init_sense_cache(struct Scsi_Host *shost)
>>       if (shost->unchecked_isa_dma) {
>>               scsi_sense_isadma_cache =
>>                       kmem_cache_create("scsi_sense_cache(DMA)",
>> -                     SCSI_SENSE_BUFFERSIZE, 0,
>> -                     SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
>> +                             SCSI_SENSE_BUFFERSIZE, 0,
>> +                             SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
>>               if (!scsi_sense_isadma_cache)
>>                       ret = -ENOMEM;
>
> All this part of this patch does is to change source code indentation. Should
> these changes really be included in this patch?

I can certainly drop that hunk, but the existing alignment is really
ugly. :) Happy to do whatever.

-Kees
diff mbox

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f6097b89d5d3..f1c6bd56dd5b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -77,14 +77,15 @@  int scsi_init_sense_cache(struct Scsi_Host *shost)
 	if (shost->unchecked_isa_dma) {
 		scsi_sense_isadma_cache =
 			kmem_cache_create("scsi_sense_cache(DMA)",
-			SCSI_SENSE_BUFFERSIZE, 0,
-			SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
+				SCSI_SENSE_BUFFERSIZE, 0,
+				SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA, NULL);
 		if (!scsi_sense_isadma_cache)
 			ret = -ENOMEM;
 	} else {
 		scsi_sense_cache =
-			kmem_cache_create("scsi_sense_cache",
-			SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN, NULL);
+			kmem_cache_create_usercopy("scsi_sense_cache",
+				SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN,
+				0, SCSI_SENSE_BUFFERSIZE, NULL);
 		if (!scsi_sense_cache)
 			ret = -ENOMEM;
 	}