Re: [PATCH 6/6] sd: add support for TCG OPAL self encrypting disks

[Scott Bauer]

On Tue, Jun 06, 2017 at 11:59:55AM +0200, Christoph Hellwig wrote:
> On Mon, Jun 05, 2017 at 03:15:31PM -0600, Scott Bauer wrote:
> > I'm not familiar at all with ATA, but I noticed there was no unlock from 
> > suspend support
> > in the series. Does ATA not have a way to determine if we're coming out of 
> > a suspend?
> 
> I don't know, and not having a test system with a OPAL capable driver
> and suspend support I could not even test the code.
> 
> > I see there are some power-ops in scsi/sd.c, if you do want to add it you 
> > can mabe toss a
> > 
> > if (sdkp->security)
> >opal_unlock_from_suspend(sdpk->opal_dev)
> > 
> > somewhere in the resume path? We handle null opal_devs and no unlock from 
> > suspend list
> > so calling it when nothing is set up is just a no-op.
> 
> Yeah, maybe.  We'll just need someone who could test it first.

I was given a sata drive that apparently has opal enabled on it. If it actually 
has opal
I can run some tests.

Re: [PATCH 6/6] sd: add support for TCG OPAL self encrypting disks

[Scott Bauer]

On Sun, Jun 04, 2017 at 02:42:25PM +0200, Christoph Hellwig wrote:
> Just wire up the generic TCG OPAL infrastructure to the SCSI disk driver
> and the Security In/Out commands.
> 
> Note that I don't know of any actual SCSI disks that do support TCG OPAL,
> but this is required to support ATA disks through libata.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/scsi/sd.c | 44 
>  drivers/scsi/sd.h |  2 ++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b6bb4e0ce0e3..782f909a223c 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -50,6 +50,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -643,6 +644,26 @@ static void scsi_disk_put(struct scsi_disk *sdkp)
>   mutex_unlock(_ref_mutex);
>  }
>  
> +#ifdef CONFIG_BLK_SED_OPAL
> +static int sd_sec_submit(void *data, u16 spsp, u8 secp, void *buffer,
> + size_t len, bool send)
> +{
> + struct scsi_device *sdev = data;
> + u8 cdb[12] = { 0, };
> + int ret;
> +
> + cdb[0] = send ? SECURITY_PROTOCOL_OUT : SECURITY_PROTOCOL_IN;
> + cdb[1] = secp;
> + put_unaligned_be16(spsp, [2]);
> + put_unaligned_be32(len, [6]);
> +
> + ret = scsi_execute_req(sdev, cdb,
> + send ? DMA_TO_DEVICE : DMA_FROM_DEVICE,
> + buffer, len, NULL, SD_TIMEOUT, SD_MAX_RETRIES, NULL);
> + return ret <= 0 ? ret : -EIO;
> +}
> +#endif /* CONFIG_BLK_SED_OPAL */
> +
>  static unsigned char sd_setup_protect_cmnd(struct scsi_cmnd *scmd,
>  unsigned int dix, unsigned int dif)
>  {
> @@ -1454,6 +1475,9 @@ static int sd_ioctl(struct block_device *bdev, fmode_t 
> mode,
>   if (error)
>   goto out;
>  
> + if (is_sed_ioctl(cmd))
> + return sed_ioctl(sdkp->opal_dev, cmd, p);
> +
>   /*
>* Send SCSI addressing ioctls directly to mid level, send other
>* ioctls to block level and then onto mid level if they can't be
> @@ -3014,6 +3038,17 @@ static void sd_read_write_same(struct scsi_disk *sdkp, 
> unsigned char *buffer)
>   sdkp->ws10 = 1;
>  }
>  
> +static void sd_read_security(struct scsi_disk *sdkp, unsigned char *buffer)
> +{
> + struct scsi_device *sdev = sdkp->device;
> +
> + if (scsi_report_opcode(sdev, buffer, SD_BUF_SIZE,
> + SECURITY_PROTOCOL_IN) == 1 &&
> + scsi_report_opcode(sdev, buffer, SD_BUF_SIZE,
> + SECURITY_PROTOCOL_OUT) == 1)
> + sdkp->security = 1;
> +}
> +
>  /**
>   *   sd_revalidate_disk - called the first time a new disk is seen,
>   *   performs disk spin up, read_capacity, etc.
> @@ -3067,6 +3102,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>   sd_read_cache_type(sdkp, buffer);
>   sd_read_app_tag_own(sdkp, buffer);
>   sd_read_write_same(sdkp, buffer);
> + sd_read_security(sdkp, buffer);
>   }
>  
>   sdkp->first_scan = 0;
> @@ -3227,6 +3263,12 @@ static void sd_probe_async(void *data, async_cookie_t 
> cookie)
>  
>   sd_revalidate_disk(gd);
>  
> + if (sdkp->security) {
> + sdkp->opal_dev = init_opal_dev(sdp, _sec_submit);
> + if (sdkp->opal_dev)
> + sd_printk(KERN_NOTICE, sdkp, "supports TCG Opal\n");
> + }
> +
> 
I'm not familiar at all with ATA, but I noticed there was no unlock from 
suspend support
in the series. Does ATA not have a way to determine if we're coming out of a 
suspend?

I see there are some power-ops in scsi/sd.c, if you do want to add it you can 
mabe toss a

if (sdkp->security)
   opal_unlock_from_suspend(sdpk->opal_dev)

somewhere in the resume path? We handle null opal_devs and no unlock from 
suspend list
so calling it when nothing is set up is just a no-op.

Re: [PATCH v2] Avoid that scsi_exit_rq() triggers a use-after-free

[Scott Bauer]

On Thu, May 04, 2017 at 03:32:44PM +, Bart Van Assche wrote:
> On Thu, 2017-05-04 at 09:15 -0600, Scott Bauer wrote:
> > On Thu, May 04, 2017 at 03:26:37PM +, Bart Van Assche wrote:
> > > On Thu, 2017-05-04 at 09:30 +0200, Christoph Hellwig wrote:
> > > > Please just add a flag to ->flags instead of adding a whole new field.
> > > > 
> > > > Otherwise this looks good to me.
> > > 
> > > Hello Christoph,
> > > 
> > > Thanks for the feedback. I will make the proposed change and post a second
> > > version.
> > 
> > BTW, what branch was your last patch based off? I had some hunk errors while
> > applying it.
> 
> Hello Scott,
> 
> That patch was based off kernel v4.11. Did you perhaps use a different kernel
> version for your tests?
> 
> Bart.

I've been working on top of Jens' for-linus
https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/log/?h=for-linus

Re: [PATCH v2] Avoid that scsi_exit_rq() triggers a use-after-free

[Scott Bauer]

On Thu, May 04, 2017 at 03:26:37PM +, Bart Van Assche wrote:
> On Thu, 2017-05-04 at 09:30 +0200, Christoph Hellwig wrote:
> > Please just add a flag to ->flags instead of adding a whole new field.
> > 
> > Otherwise this looks good to me.
> 
> Hello Christoph,
> 
> Thanks for the feedback. I will make the proposed change and post a second
> version.
> 
> Bart.

BTW, what branch was your last patch based off? I had some hunk errors while
applying it.

Re: [PATCH] Avoid that scsi_exit_rq() triggers a use-after-free

[Scott Bauer]

On Tue, May 02, 2017 at 10:43:30AM -0700, Bart Van Assche wrote:
> This patch fixes the following KASAN complaint:
> 
> ==
> BUG: KASAN: use-after-free in scsi_exit_rq+0xf3/0x120 at addr 8802b7fedf00
> Read of size 1 by task rcuos/5/53
> CPU: 7 PID: 53 Comm: rcuos/6 Not tainted 4.11.0-rc5+ #13
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0 
> ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
> Call Trace:
>  dump_stack+0x63/0x8f
>  kasan_object_err+0x21/0x70
>  kasan_report.part.1+0x231/0x500
>  __asan_report_load1_noabort+0x2e/0x30
>  scsi_exit_rq+0xf3/0x120
>  free_request_size+0x44/0x60
>  mempool_destroy.part.6+0x9b/0x150
>  mempool_destroy+0x13/0x20
>  blk_exit_rl+0x36/0x40
>  blkg_free+0x146/0x200
>  __blkg_release_rcu+0x121/0x220
>  rcu_nocb_kthread+0x61f/0xca0
>  kthread+0x298/0x390
>  ret_from_fork+0x2c/0x40
> Object at 8802b7fedd80, in cache kmalloc-2048 size: 2048
> Allocated:
> PID = 3992
>  save_stack_trace+0x1b/0x20
>  save_stack+0x46/0xd0
>  kasan_kmalloc+0xad/0xe0
>  __kmalloc+0x134/0x220
>  scsi_host_alloc+0x6b/0x11c0
>  0xc101d94a
>  driver_probe_device+0x49e/0xc60
>  __device_attach_driver+0x1d3/0x2a0
>  bus_for_each_drv+0x11a/0x1d0
>  __device_attach+0x1e1/0x2c0
>  device_initial_probe+0x13/0x20
>  bus_probe_device+0x19b/0x240
>  device_add+0x86d/0x1450
>  device_register+0x1a/0x20
>  0xc10270ce
>  0xc1048a62
>  do_one_initcall+0xa7/0x250
>  do_init_module+0x1d0/0x55d
>  load_module+0x7c9f/0x9850
>  SYSC_finit_module+0x189/0x1c0
>  SyS_finit_module+0xe/0x10
>  entry_SYSCALL_64_fastpath+0x1a/0xa9
> Freed:
> PID = 4128
>  save_stack_trace+0x1b/0x20
>  save_stack+0x46/0xd0
>  kasan_slab_free+0x71/0xb0
>  kfree+0x8d/0x1b0
>  scsi_host_dev_release+0x2cb/0x430
>  device_release+0x76/0x1e0
>  kobject_release+0x107/0x370
>  kobject_put+0x56/0xb0
>  put_device+0x17/0x20
>  scsi_host_put+0x15/0x20
>  0xc101fcd7
>  device_release_driver_internal+0x26a/0x4e0
>  device_release_driver+0x12/0x20
>  bus_remove_device+0x2d0/0x590
>  device_del+0x55b/0x920
>  device_unregister+0x1a/0xa0
>  0xc101e0ca
>  0xc102fccc
>  SyS_delete_module+0x334/0x3e0
>  entry_SYSCALL_64_fastpath+0x1a/0xa9
> Memory state around the buggy address:
>  8802b7fede00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  8802b7fede80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> >8802b7fedf00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>    ^
>  ffff8802b7fedf80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>  8802b7fee000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ======
> 
> Reported-by: Scott Bauer <scott.ba...@intel.com>
> Fixes: e9c787e65c0c ("scsi: allocate scsi_cmnd structures as part of struct 
> request")
> Signed-off-by: Bart Van Assche <bart.vanass...@sandisk.com>
> Cc: Scott Bauer <scott.ba...@intel.com>
> Cc: Christoph Hellwig <h...@lst.de>
> Cc: Jan Kara <j...@suse.cz>
> Cc: Hannes Reinecke <h...@suse.com>
> Cc: <sta...@vger.kernel.org>
> ---
>  drivers/scsi/scsi_lib.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 15c9fe766071..d698364df072 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2095,11 +2095,14 @@ static int scsi_init_rq(struct request_queue *q, 
> struct request *rq, gfp_t gfp)
>   struct Scsi_Host *shost = q->rq_alloc_data;
>   struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
>  
> + if (!scsi_host_get(shost))
> + goto fail;
> +
>   memset(cmd, 0, sizeof(*cmd));
>  
>   cmd->sense_buffer = scsi_alloc_sense_buffer(shost, gfp, NUMA_NO_NODE);
>   if (!cmd->sense_buffer)
> - goto fail;
> + goto put;
>   cmd->req.sense = cmd->sense_buffer;
>  
>   if (scsi_host_get_prot(shost) >= SHOST_DIX_TYPE0_PROTECTION) {
> @@ -2112,6 +2115,8 @@ static int scsi_init_rq(struct request_queue *q, struct 
> request *rq, gfp_t gfp)
>  
>  fail_free_sense:
>   scsi_free_sense_buffer(shost, cmd->sense_buffer);
> +put:
> + scsi_host_put(shost);
>  fail:
>   return -ENOMEM;
>  }
> @@ -2124,6 +2129,7 @@ static void scsi_exit_rq(struct request_queue *q, 
> struct request *rq)
>   if (cmd->prot_sdb)
>   kmem_cache_free(scsi_sdb_cache, cmd->prot_sdb);
>   scsi_free_sense_buffer(shost, cmd->sense_

BUG: KASAN: use-after-free in scsi_exit_rq

[Scott Bauer]

Hi all,

While running xfs test testing some other features scheduled for 4.12
I came across this KASAN dump:


[  638.913813] XFS (nvme0n1): Mounting V5 Filesystem
[  638.917934] XFS (nvme0n1): Ending clean mount
[  639.035070] blk_update_request: I/O error, dev nvme1n1, sector 0
[  639.071764] XFS (nvme1n1): Mounting V5 Filesystem
[  639.077052] XFS (nvme1n1): Ending clean mount
[  639.110634] XFS (nvme0n1): Unmounting Filesystem
[  639.260132] XFS (nvme0n1): Mounting V5 Filesystem
[  639.264141] XFS (nvme0n1): Ending clean mount
[  639.282112] run fstests generic/108 at 2017-04-20 14:30:05
[  639.525274] XFS (nvme1n1): Unmounting Filesystem
[  639.570999] scsi host2: scsi_debug: version 1.86 [20160430]
[  639.570999]   dev_size_mb=128, opts=0x0, submit_queues=1, statistics=0
[  639.573698] scsi 2:0:0:0: Direct-Access Linuxscsi_debug   0186 
PQ: 0 ANSI: 7
[  639.595927] sd 2:0:0:0: Attached scsi generic sg1 type 0
[  639.598290] sd 2:0:0:0: [sda] 262144 512-byte logical blocks: (134 MB/128 
MiB)
[  639.599884] sd 2:0:0:0: [sda] Write Protect is off
[  639.600246] sd 2:0:0:0: [sda] Mode Sense: 73 00 10 08
[  639.602747] sd 2:0:0:0: [sda] Write cache: enabled, read cache: enabled, 
supports DPO and FUA
[  639.626440] sd 2:0:0:0: [sda] Attached SCSI disk
[  641.264666] XFS (dm-0): Mounting V5 Filesystem
[  641.278227] XFS (dm-0): Ending clean mount
[  641.394865] sd 2:0:0:0: rejecting I/O to offline device
[  641.395353] sd 2:0:0:0: rejecting I/O to offline device
[  641.395903] sd 2:0:0:0: rejecting I/O to offline device
[  641.396362] sd 2:0:0:0: rejecting I/O to offline device
[  641.396896] sd 2:0:0:0: rejecting I/O to offline device
[  641.397347] sd 2:0:0:0: rejecting I/O to offline device
[  641.397888] sd 2:0:0:0: rejecting I/O to offline device
[  641.398358] sd 2:0:0:0: rejecting I/O to offline device
[  641.49] sd 2:0:0:0: rejecting I/O to offline device
[  641.400378] blk_update_request: I/O error, dev sda, sector 0
[  641.423308] XFS (dm-0): Unmounting Filesystem
[  642.585631] sd 2:0:0:0: [sda] Synchronizing SCSI cache
[  642.637953] 
==
[  642.638860] BUG: KASAN: use-after-free in scsi_exit_rq+0xf3/0x120 at addr 
8802b7fedf00
[  642.639362] Read of size 1 by task rcuos/5/53
[  642.639713] CPU: 7 PID: 53 Comm: rcuos/6 Not tainted 4.11.0-rc5+ #13
[  642.640170] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
[  642.640923] Call Trace:
[  642.641080]  dump_stack+0x63/0x8f
[  642.641289]  kasan_object_err+0x21/0x70
[  642.641531]  kasan_report.part.1+0x231/0x500
[  642.641823]  ? scsi_exit_rq+0xf3/0x120
[  642.642054]  ? _raw_spin_unlock_irqrestore+0xe/0x10
[  642.642353]  ? free_percpu+0x1b7/0x340
[  642.642586]  ? put_task_stack+0x117/0x2b0
[  642.642837]  __asan_report_load1_noabort+0x2e/0x30
[  642.643138]  scsi_exit_rq+0xf3/0x120
[  642.643366]  free_request_size+0x44/0x60
[  642.643614]  mempool_destroy.part.6+0x9b/0x150
[  642.643899]  ? kasan_slab_free+0x87/0xb0
[  642.644152]  mempool_destroy+0x13/0x20
[  642.644394]  blk_exit_rl+0x36/0x40
[  642.644614]  blkg_free+0x146/0x200
[  642.644836]  __blkg_release_rcu+0x121/0x220
[  642.645112]  rcu_nocb_kthread+0x61f/0xca0
[  642.645376]  ? get_state_synchronize_rcu+0x20/0x20
[  642.645690]  ? pci_mmcfg_check_reserved+0x110/0x110
[  642.646011]  kthread+0x298/0x390
[  642.646224]  ? get_state_synchronize_rcu+0x20/0x20
[  642.646535]  ? kthread_park+0x160/0x160
[  642.646787]  ret_from_fork+0x2c/0x40
[  642.647020] Object at 8802b7fedd80, in cache kmalloc-2048 size: 2048
[  642.647435] Allocated:
[  642.647591] PID = 3992
[  642.647750]  save_stack_trace+0x1b/0x20
[  642.648000]  save_stack+0x46/0xd0
[  642.648208]  kasan_kmalloc+0xad/0xe0
[  642.648441]  __kmalloc+0x134/0x220
[  642.648664]  scsi_host_alloc+0x6b/0x11c0
[  642.648919]  0xc101d94a
[  642.649124]  driver_probe_device+0x49e/0xc60
[  642.649398]  __device_attach_driver+0x1d3/0x2a0
[  642.649684]  bus_for_each_drv+0x11a/0x1d0
[  642.649946]  __device_attach+0x1e1/0x2c0
[  642.650200]  device_initial_probe+0x13/0x20
[  642.650557]  bus_probe_device+0x19b/0x240
[  642.651029]  device_add+0x86d/0x1450
[  642.651472]  device_register+0x1a/0x20
[  642.651778]  0xc10270ce
[  642.651990]  0xc1048a62
[  642.652197]  do_one_initcall+0xa7/0x250
[  642.652445]  do_init_module+0x1d0/0x55d
[  642.652696]  load_module+0x7c9f/0x9850
[  642.652937]  SYSC_finit_module+0x189/0x1c0
[  642.653200]  SyS_finit_module+0xe/0x10
[  642.653441]  entry_SYSCALL_64_fastpath+0x1a/0xa9
[  642.653735] Freed:
[  642.653875] PID = 4128
[  642.654304]  save_stack_trace+0x1b/0x20
[  642.654991]  save_stack+0x46/0xd0
[  642.655200]  kasan_slab_free+0x71/0xb0
[  642.655434]  kfree+0x8d/0x1b0
[  642.655624]  scsi_host_dev_release+0x2cb/0x430
[  642.655898]  device_release+0x76/0x1e0
[  642.656141]  kobject_release+0x107/0x370