| Message ID | 7d1bdccbf42a239ac1096393bfa1f7a248e1feba.1530783326.git.sbrivio@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: Fix stack out-of-bounds in smb{2,3}_create_lease_buf() | expand |
Hi Stefano, Good catch! But I think this bug hides a bigger more sinister bug... Stefano Brivio <sbrivio@redhat.com> writes: > - buf->lcontext.LeaseKeyLow = cpu_to_le64(*((u64 *)lease_key)); > - buf->lcontext.LeaseKeyHigh = cpu_to_le64(*((u64 *)(lease_key + 8))); > + get_random_bytes(&buf->lcontext.LeaseKey, SMB2_LEASE_KEY_SIZE); > buf->lcontext.LeaseState = map_oplock_to_lease(oplock); I know the value used to be kind of random and changing it to actual random bytes should be the same however we already have a function to generate a lease key in the generic version operations: /* generate new lease key */ void (*new_lease_key)(struct cifs_fid *); Which are used to set the lease key in a struct cifs_fid and are called on both create & open code paths (in cifs_create and cifs_atomic_open). I think the code is assuming being called by smb2_open_file(): __u8 smb2_oplock[17]; <--- space for oplock type + key buffer ... *smb2_oplock = SMB2_OPLOCK_LEVEL_BATCH; ... if (oparms->tcon->ses->server->capabilities & SMB2_GLOBAL_CAP_LEASING) memcpy(smb2_oplock + 1, fid->lease_key, SMB2_LEASE_KEY_SIZE); ... rc = SMB2_open(xid, oparms, smb2_path, smb2_oplock, smb2_data, NULL, NULL); So it seems all SMB2_open() callers that request an oplock MUST allocate this 17 bytes buffer and from a quick look many of them don't. The proper fix would be to pass the fid->lease_key when creating the context. Hope that was helpful enough for a v2 ;) Cheers,
Hi Aurélien, On Thu, 05 Jul 2018 13:30:43 +0200 Aurélien Aptel <aaptel@suse.com> wrote: > Hi Stefano, > > Good catch! But I think this bug hides a bigger more sinister bug... > > Stefano Brivio <sbrivio@redhat.com> writes: > > - buf->lcontext.LeaseKeyLow = cpu_to_le64(*((u64 *)lease_key)); > > - buf->lcontext.LeaseKeyHigh = cpu_to_le64(*((u64 *)(lease_key + 8))); > > + get_random_bytes(&buf->lcontext.LeaseKey, SMB2_LEASE_KEY_SIZE); > > buf->lcontext.LeaseState = map_oplock_to_lease(oplock); > > I know the value used to be kind of random and changing it to actual > random bytes should be the same however we already have a function to > generate a lease key in the generic version operations: > > /* generate new lease key */ > void (*new_lease_key)(struct cifs_fid *); > > Which are used to set the lease key in a struct cifs_fid and are called > on both create & open code paths (in cifs_create and cifs_atomic_open). > > I think the code is assuming being called by smb2_open_file(): > > __u8 smb2_oplock[17]; <--- space for oplock type + key buffer > ... > *smb2_oplock = SMB2_OPLOCK_LEVEL_BATCH; > ... > if (oparms->tcon->ses->server->capabilities & SMB2_GLOBAL_CAP_LEASING) > memcpy(smb2_oplock + 1, fid->lease_key, SMB2_LEASE_KEY_SIZE); > ... > rc = SMB2_open(xid, oparms, smb2_path, smb2_oplock, smb2_data, NULL, > NULL); > > So it seems all SMB2_open() callers that request an oplock MUST allocate this 17 bytes > buffer and from a quick look many of them don't. Aha! I didn't spot that. Everything makes sense now. Thanks for the explanation! > The proper fix would be to pass the fid->lease_key when creating the context. > > Hope that was helpful enough for a v2 ;) Indeed, that was extremely helpful. Preparing v2 now.
if this is as expected we will need to cc:stable as well On Thu, Jul 5, 2018 at 4:46 AM Stefano Brivio <sbrivio@redhat.com> wrote: > > smb{2,3}_create_lease_buf() store a lease key in the lease > context for later usage on a lease break. The key is currently > sourced from data that happens to be on the stack near oplock, > a local variable in open_shroot(). Its address is then passed > to create_lease_buf handlers via SMB2_open(), and 16 bytes near > oplock are used. This causes a stack out-of-bounds access as > reported by KASAN on SMB2.1 and SMB3 mounts (first out-of-bounds > access is shown here): > > [ 111.528823] BUG: KASAN: stack-out-of-bounds in smb3_create_lease_buf+0x399/0x3b0 [cifs] > [ 111.530815] Read of size 8 at addr ffff88010829f249 by task mount.cifs/985 > [ 111.532838] CPU: 3 PID: 985 Comm: mount.cifs Not tainted 4.18.0-rc3+ #91 > [ 111.534656] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 > [ 111.536838] Call Trace: > [ 111.537528] dump_stack+0xc2/0x16b > [ 111.540890] print_address_description+0x6a/0x270 > [ 111.542185] kasan_report+0x258/0x380 > [ 111.544701] smb3_create_lease_buf+0x399/0x3b0 [cifs] > [ 111.546134] SMB2_open+0x1ef8/0x4b70 [cifs] > [ 111.575883] open_shroot+0x339/0x550 [cifs] > [ 111.591969] smb3_qfs_tcon+0x32c/0x1e60 [cifs] > [ 111.617405] cifs_mount+0x4f3/0x2fc0 [cifs] > [ 111.674332] cifs_smb3_do_mount+0x263/0xf10 [cifs] > [ 111.677915] mount_fs+0x55/0x2b0 > [ 111.679504] vfs_kern_mount.part.22+0xaa/0x430 > [ 111.684511] do_mount+0xc40/0x2660 > [ 111.698301] ksys_mount+0x80/0xd0 > [ 111.701541] do_syscall_64+0x14e/0x4b0 > [ 111.711807] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > [ 111.713665] RIP: 0033:0x7f372385b5fa > [ 111.715311] Code: 48 8b 0d 99 78 2c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 66 78 2c 00 f7 d8 64 89 01 48 > [ 111.720330] RSP: 002b:00007ffff27049d8 EFLAGS: 00000206 ORIG_RAX: 00000000000000a5 > [ 111.722601] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f372385b5fa > [ 111.724842] RDX: 000055c2ecdc73b2 RSI: 000055c2ecdc73f9 RDI: 00007ffff270580f > [ 111.727083] RBP: 00007ffff2705804 R08: 000055c2ee976060 R09: 0000000000001000 > [ 111.729319] R10: 0000000000000000 R11: 0000000000000206 R12: 00007f3723f4d000 > [ 111.731615] R13: 000055c2ee976060 R14: 00007f3723f4f90f R15: 0000000000000000 > > [ 111.735448] The buggy address belongs to the page: > [ 111.737420] page:ffffea000420a7c0 count:0 mapcount:0 mapping:0000000000000000 index:0x0 > [ 111.739890] flags: 0x17ffffc0000000() > [ 111.741750] raw: 0017ffffc0000000 0000000000000000 dead000000000200 0000000000000000 > [ 111.744216] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000 > [ 111.746679] page dumped because: kasan: bad access detected > > [ 111.750482] Memory state around the buggy address: > [ 111.752562] ffff88010829f100: 00 f2 f2 f2 f2 f2 f2 f2 00 00 00 00 00 00 00 00 > [ 111.754991] ffff88010829f180: 00 00 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00 > [ 111.757401] >ffff88010829f200: 00 00 00 00 00 f1 f1 f1 f1 01 f2 f2 f2 f2 f2 f2 > [ 111.759801] ^ > [ 111.762034] ffff88010829f280: f2 02 f2 f2 f2 f2 f2 f2 f2 00 00 00 00 00 00 00 > [ 111.764486] ffff88010829f300: f2 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > [ 111.766913] ================================================================== > > Generate lease keys by properly gathering random data instead. > > Fixes: b8c32dbb0deb ("CIFS: Request SMB2.1 leases") > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> > --- > fs/cifs/cifsglob.h | 2 +- > fs/cifs/smb2ops.c | 13 +++++-------- > fs/cifs/smb2pdu.c | 2 +- > fs/cifs/smb2pdu.h | 6 ++---- > 4 files changed, 9 insertions(+), 14 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index bd78da59a4fd..8bb4748249dc 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -423,7 +423,7 @@ struct smb_version_operations { > void (*set_oplock_level)(struct cifsInodeInfo *, __u32, unsigned int, > bool *); > /* create lease context buffer for CREATE request */ > - char * (*create_lease_buf)(u8 *, u8); > + char * (*create_lease_buf)(u8); > /* parse lease context buffer and return oplock/epoch info */ > __u8 (*parse_lease_buf)(void *buf, unsigned int *epoch, char *lkey); > ssize_t (*copychunk_range)(const unsigned int, > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 0356b5559c71..d549704c54d5 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -2211,7 +2211,7 @@ map_oplock_to_lease(u8 oplock) > } > > static char * > -smb2_create_lease_buf(u8 *lease_key, u8 oplock) > +smb2_create_lease_buf(u8 oplock) > { > struct create_lease *buf; > > @@ -2219,8 +2219,7 @@ smb2_create_lease_buf(u8 *lease_key, u8 oplock) > if (!buf) > return NULL; > > - buf->lcontext.LeaseKeyLow = cpu_to_le64(*((u64 *)lease_key)); > - buf->lcontext.LeaseKeyHigh = cpu_to_le64(*((u64 *)(lease_key + 8))); > + get_random_bytes(&buf->lcontext.LeaseKey, SMB2_LEASE_KEY_SIZE); > buf->lcontext.LeaseState = map_oplock_to_lease(oplock); > > buf->ccontext.DataOffset = cpu_to_le16(offsetof > @@ -2238,7 +2237,7 @@ smb2_create_lease_buf(u8 *lease_key, u8 oplock) > } > > static char * > -smb3_create_lease_buf(u8 *lease_key, u8 oplock) > +smb3_create_lease_buf(u8 oplock) > { > struct create_lease_v2 *buf; > > @@ -2246,8 +2245,7 @@ smb3_create_lease_buf(u8 *lease_key, u8 oplock) > if (!buf) > return NULL; > > - buf->lcontext.LeaseKeyLow = cpu_to_le64(*((u64 *)lease_key)); > - buf->lcontext.LeaseKeyHigh = cpu_to_le64(*((u64 *)(lease_key + 8))); > + get_random_bytes(&buf->lcontext.LeaseKey, SMB2_LEASE_KEY_SIZE); > buf->lcontext.LeaseState = map_oplock_to_lease(oplock); > > buf->ccontext.DataOffset = cpu_to_le16(offsetof > @@ -2284,8 +2282,7 @@ smb3_parse_lease_buf(void *buf, unsigned int *epoch, char *lease_key) > if (lc->lcontext.LeaseFlags & SMB2_LEASE_FLAG_BREAK_IN_PROGRESS) > return SMB2_OPLOCK_LEVEL_NOCHANGE; > if (lease_key) > - memcpy(lease_key, &lc->lcontext.LeaseKeyLow, > - SMB2_LEASE_KEY_SIZE); > + memcpy(lease_key, &lc->lcontext.LeaseKey, SMB2_LEASE_KEY_SIZE); > return le32_to_cpu(lc->lcontext.LeaseState); > } > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 810b85787c91..7608c241b1ef 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -1712,7 +1712,7 @@ add_lease_context(struct TCP_Server_Info *server, struct kvec *iov, > struct smb2_create_req *req = iov[0].iov_base; > unsigned int num = *num_iovec; > > - iov[num].iov_base = server->ops->create_lease_buf(oplock+1, *oplock); > + iov[num].iov_base = server->ops->create_lease_buf(*oplock); > if (iov[num].iov_base == NULL) > return -ENOMEM; > iov[num].iov_len = server->vals->create_lease_size; > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h > index 824dddeee3f2..a671adcc44a6 100644 > --- a/fs/cifs/smb2pdu.h > +++ b/fs/cifs/smb2pdu.h > @@ -678,16 +678,14 @@ struct create_context { > #define SMB2_LEASE_KEY_SIZE 16 > > struct lease_context { > - __le64 LeaseKeyLow; > - __le64 LeaseKeyHigh; > + u8 LeaseKey[SMB2_LEASE_KEY_SIZE]; > __le32 LeaseState; > __le32 LeaseFlags; > __le64 LeaseDuration; > } __packed; > > struct lease_context_v2 { > - __le64 LeaseKeyLow; > - __le64 LeaseKeyHigh; > + u8 LeaseKey[SMB2_LEASE_KEY_SIZE]; > __le32 LeaseState; > __le32 LeaseFlags; > __le64 LeaseDuration; > -- > 2.15.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index bd78da59a4fd..8bb4748249dc 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -423,7 +423,7 @@ struct smb_version_operations { void (*set_oplock_level)(struct cifsInodeInfo *, __u32, unsigned int, bool *); /* create lease context buffer for CREATE request */ - char * (*create_lease_buf)(u8 *, u8); + char * (*create_lease_buf)(u8); /* parse lease context buffer and return oplock/epoch info */ __u8 (*parse_lease_buf)(void *buf, unsigned int *epoch, char *lkey); ssize_t (*copychunk_range)(const unsigned int, diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 0356b5559c71..d549704c54d5 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -2211,7 +2211,7 @@ map_oplock_to_lease(u8 oplock) } static char * -smb2_create_lease_buf(u8 *lease_key, u8 oplock) +smb2_create_lease_buf(u8 oplock) { struct create_lease *buf; @@ -2219,8 +2219,7 @@ smb2_create_lease_buf(u8 *lease_key, u8 oplock) if (!buf) return NULL; - buf->lcontext.LeaseKeyLow = cpu_to_le64(*((u64 *)lease_key)); - buf->lcontext.LeaseKeyHigh = cpu_to_le64(*((u64 *)(lease_key + 8))); + get_random_bytes(&buf->lcontext.LeaseKey, SMB2_LEASE_KEY_SIZE); buf->lcontext.LeaseState = map_oplock_to_lease(oplock); buf->ccontext.DataOffset = cpu_to_le16(offsetof @@ -2238,7 +2237,7 @@ smb2_create_lease_buf(u8 *lease_key, u8 oplock) } static char * -smb3_create_lease_buf(u8 *lease_key, u8 oplock) +smb3_create_lease_buf(u8 oplock) { struct create_lease_v2 *buf; @@ -2246,8 +2245,7 @@ smb3_create_lease_buf(u8 *lease_key, u8 oplock) if (!buf) return NULL; - buf->lcontext.LeaseKeyLow = cpu_to_le64(*((u64 *)lease_key)); - buf->lcontext.LeaseKeyHigh = cpu_to_le64(*((u64 *)(lease_key + 8))); + get_random_bytes(&buf->lcontext.LeaseKey, SMB2_LEASE_KEY_SIZE); buf->lcontext.LeaseState = map_oplock_to_lease(oplock); buf->ccontext.DataOffset = cpu_to_le16(offsetof @@ -2284,8 +2282,7 @@ smb3_parse_lease_buf(void *buf, unsigned int *epoch, char *lease_key) if (lc->lcontext.LeaseFlags & SMB2_LEASE_FLAG_BREAK_IN_PROGRESS) return SMB2_OPLOCK_LEVEL_NOCHANGE; if (lease_key) - memcpy(lease_key, &lc->lcontext.LeaseKeyLow, - SMB2_LEASE_KEY_SIZE); + memcpy(lease_key, &lc->lcontext.LeaseKey, SMB2_LEASE_KEY_SIZE); return le32_to_cpu(lc->lcontext.LeaseState); } diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 810b85787c91..7608c241b1ef 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -1712,7 +1712,7 @@ add_lease_context(struct TCP_Server_Info *server, struct kvec *iov, struct smb2_create_req *req = iov[0].iov_base; unsigned int num = *num_iovec; - iov[num].iov_base = server->ops->create_lease_buf(oplock+1, *oplock); + iov[num].iov_base = server->ops->create_lease_buf(*oplock); if (iov[num].iov_base == NULL) return -ENOMEM; iov[num].iov_len = server->vals->create_lease_size; diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h index 824dddeee3f2..a671adcc44a6 100644 --- a/fs/cifs/smb2pdu.h +++ b/fs/cifs/smb2pdu.h @@ -678,16 +678,14 @@ struct create_context { #define SMB2_LEASE_KEY_SIZE 16 struct lease_context { - __le64 LeaseKeyLow; - __le64 LeaseKeyHigh; + u8 LeaseKey[SMB2_LEASE_KEY_SIZE]; __le32 LeaseState; __le32 LeaseFlags; __le64 LeaseDuration; } __packed; struct lease_context_v2 { - __le64 LeaseKeyLow; - __le64 LeaseKeyHigh; + u8 LeaseKey[SMB2_LEASE_KEY_SIZE]; __le32 LeaseState; __le32 LeaseFlags; __le64 LeaseDuration;
smb{2,3}_create_lease_buf() store a lease key in the lease context for later usage on a lease break. The key is currently sourced from data that happens to be on the stack near oplock, a local variable in open_shroot(). Its address is then passed to create_lease_buf handlers via SMB2_open(), and 16 bytes near oplock are used. This causes a stack out-of-bounds access as reported by KASAN on SMB2.1 and SMB3 mounts (first out-of-bounds access is shown here): [ 111.528823] BUG: KASAN: stack-out-of-bounds in smb3_create_lease_buf+0x399/0x3b0 [cifs] [ 111.530815] Read of size 8 at addr ffff88010829f249 by task mount.cifs/985 [ 111.532838] CPU: 3 PID: 985 Comm: mount.cifs Not tainted 4.18.0-rc3+ #91 [ 111.534656] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 [ 111.536838] Call Trace: [ 111.537528] dump_stack+0xc2/0x16b [ 111.540890] print_address_description+0x6a/0x270 [ 111.542185] kasan_report+0x258/0x380 [ 111.544701] smb3_create_lease_buf+0x399/0x3b0 [cifs] [ 111.546134] SMB2_open+0x1ef8/0x4b70 [cifs] [ 111.575883] open_shroot+0x339/0x550 [cifs] [ 111.591969] smb3_qfs_tcon+0x32c/0x1e60 [cifs] [ 111.617405] cifs_mount+0x4f3/0x2fc0 [cifs] [ 111.674332] cifs_smb3_do_mount+0x263/0xf10 [cifs] [ 111.677915] mount_fs+0x55/0x2b0 [ 111.679504] vfs_kern_mount.part.22+0xaa/0x430 [ 111.684511] do_mount+0xc40/0x2660 [ 111.698301] ksys_mount+0x80/0xd0 [ 111.701541] do_syscall_64+0x14e/0x4b0 [ 111.711807] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 111.713665] RIP: 0033:0x7f372385b5fa [ 111.715311] Code: 48 8b 0d 99 78 2c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 66 78 2c 00 f7 d8 64 89 01 48 [ 111.720330] RSP: 002b:00007ffff27049d8 EFLAGS: 00000206 ORIG_RAX: 00000000000000a5 [ 111.722601] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f372385b5fa [ 111.724842] RDX: 000055c2ecdc73b2 RSI: 000055c2ecdc73f9 RDI: 00007ffff270580f [ 111.727083] RBP: 00007ffff2705804 R08: 000055c2ee976060 R09: 0000000000001000 [ 111.729319] R10: 0000000000000000 R11: 0000000000000206 R12: 00007f3723f4d000 [ 111.731615] R13: 000055c2ee976060 R14: 00007f3723f4f90f R15: 0000000000000000 [ 111.735448] The buggy address belongs to the page: [ 111.737420] page:ffffea000420a7c0 count:0 mapcount:0 mapping:0000000000000000 index:0x0 [ 111.739890] flags: 0x17ffffc0000000() [ 111.741750] raw: 0017ffffc0000000 0000000000000000 dead000000000200 0000000000000000 [ 111.744216] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000 [ 111.746679] page dumped because: kasan: bad access detected [ 111.750482] Memory state around the buggy address: [ 111.752562] ffff88010829f100: 00 f2 f2 f2 f2 f2 f2 f2 00 00 00 00 00 00 00 00 [ 111.754991] ffff88010829f180: 00 00 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00 [ 111.757401] >ffff88010829f200: 00 00 00 00 00 f1 f1 f1 f1 01 f2 f2 f2 f2 f2 f2 [ 111.759801] ^ [ 111.762034] ffff88010829f280: f2 02 f2 f2 f2 f2 f2 f2 f2 00 00 00 00 00 00 00 [ 111.764486] ffff88010829f300: f2 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 111.766913] ================================================================== Generate lease keys by properly gathering random data instead. Fixes: b8c32dbb0deb ("CIFS: Request SMB2.1 leases") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- fs/cifs/cifsglob.h | 2 +- fs/cifs/smb2ops.c | 13 +++++-------- fs/cifs/smb2pdu.c | 2 +- fs/cifs/smb2pdu.h | 6 ++---- 4 files changed, 9 insertions(+), 14 deletions(-)