Re: usercopy whitelist woe in scsi_sense_cache

[Douglas Gilbert <dgilbert@interlog.com>]

On 2018-04-04 04:21 PM, Kees Cook wrote:
> On Wed, Apr 4, 2018 at 12:07 PM, Oleksandr Natalenko
> <oleksandr@natalenko.name> 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