Re: usercopy whitelist woe in scsi_sense_cache

[Douglas Gilbert]

On 2018-04-04 04:21 PM, Kees Cook wrote:
> On Wed, Apr 4, 2018 at 12:07 PM, Oleksandr Natalenko
> <oleksandr@xxxxxxxxxxxxxx> wrote:
> > With v4.16 I get the following dump while using smartctl:
> > [...]
> > [  261.262135] Bad or missing usercopy whitelist? Kernel memory exposure
> > attempt detected from SLUB object 'scsi_sense_cache' (offset 94, size 22)!
> > [...]
> > [  261.345976] Call Trace:
> > [  261.350620]  __check_object_size+0x130/0x1a0
> > [  261.355775]  sg_io+0x269/0x3f0
> > [  261.360729]  ? path_lookupat+0xaa/0x1f0
> > [  261.364027]  ? current_time+0x18/0x70
> > [  261.366684]  scsi_cmd_ioctl+0x257/0x410
> > [  261.369871]  ? xfs_bmapi_read+0x1c3/0x340 [xfs]
> > [  261.372231]  sd_ioctl+0xbf/0x1a0 [sd_mod]
> > [  261.375456]  blkdev_ioctl+0x8ca/0x990
> > [  261.381156]  ? read_null+0x10/0x10
> > [  261.384984]  block_ioctl+0x39/0x40
> > [  261.388739]  do_vfs_ioctl+0xa4/0x630
> > [  261.392624]  ? vfs_write+0x164/0x1a0
> > [  261.396658]  SyS_ioctl+0x74/0x80
> > [  261.399563]  do_syscall_64+0x74/0x190
> > [  261.402685]  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
>
> This is:
>
> sg_io+0x269/0x3f0:
> blk_complete_sghdr_rq at block/scsi_ioctl.c:280
>   (inlined by) sg_io at block/scsi_ioctl.c:376
>
> which is:
>
>          if (req->sense_len && hdr->sbp) {
>                  int len = min((unsigned int) hdr->mx_sb_len, req->sense_len);
>
>                  if (!copy_to_user(hdr->sbp, req->sense, len))
>                          hdr->sb_len_wr = len;
>                  else
>                          ret = -EFAULT;
>          }
>
> > [...]
> > I can easily reproduce it with a qemu VM and 2 virtual SCSI disks by calling
> > smartctl in a loop and doing some usual background I/O. The warning is
> > triggered within 3 minutes or so (not instantly).
> >
> > Initially, it was produced on my server after a kernel update (because disks
> > are monitored with smartctl via Zabbix).
> >
> > Looks like the thing was introduced with
> > 0afe76e88c57d91ef5697720aed380a339e3df70.
> >
> > Any idea how to deal with this please? If needed, I can provide any additional
> > info, and also I'm happy/ready to test any proposed patches.
>
> Interesting, and a little confusing. So, what's strange here is that
> the scsi_sense_cache already has a full whitelist:
>
>                         kmem_cache_create_usercopy("scsi_sense_cache",
>                                 SCSI_SENSE_BUFFERSIZE, 0, SLAB_HWCACHE_ALIGN,
>                                 0, SCSI_SENSE_BUFFERSIZE, NULL);
>
> Arg 2 is the buffer size, arg 5 is the whitelist offset (0), and the
> whitelist size (same as arg2). In other words, the entire buffer
> should be whitelisted.
>
> include/scsi/scsi_cmnd.h says:
>
> #define SCSI_SENSE_BUFFERSIZE  96
>
> That means scsi_sense_cache should be 96 bytes in size? But a 22 byte
> read starting at offset 94 happened? That seems like a 20 byte read
> beyond the end of the SLUB object? Though if it were reading past the
> actual end of the object, I'd expect the hardened usercopy BUG (rather
> than the WARN) to kick in. Ah, it looks like
> /sys/kernel/slab/scsi_sense_cache/slab_size shows this to be 128 bytes
> of actual allocation, so the 20 bytes doesn't strictly overlap another
> object (hence no BUG):
>
> /sys/kernel/slab/scsi_sense_cache# grep . object_size usersize slab_size
> object_size:96
> usersize:96
> slab_size:128
>
> Ah, right, due to SLAB_HWCACHE_ALIGN, the allocation is rounded up to
> the next cache line size, so there's 32 bytes of padding to reach 128.
>
> James or Martin, is this over-read "expected" behavior? i.e. does the
> sense cache buffer usage ever pull the ugly trick of silently
> expanding its allocation into the space the slab allocator has given
> it? If not, this looks like a real bug.
>
> What I don't see is how req->sense is _not_ at offset 0 in the
> scsi_sense_cache object...

Looking at the smartctl SCSI code it pulls 32 byte sense buffers.
Can't see 22 anywhere relevant in its code.

There are two types of sense: fixed and descriptor: with fixed you
seldom need more than 18 bytes (but it can only represent 32 bit
LBAs). The other type has a header and 0 or more variable length
descriptors. If decoding of descriptor sense went wrong you might
end up at offset 94. But not with smartctl ....

Doug Gilbert