* [GIT PULL] Please pull NFS client changes for Linux 4.13 @ 2017-07-13 21:16 Anna Schumaker 2017-07-13 21:43 ` Linus Torvalds 2017-07-14 14:25 ` Dave Jones 0 siblings, 2 replies; 30+ messages in thread From: Anna Schumaker @ 2017-07-13 21:16 UTC (permalink / raw) To: torvalds Cc: Linux NFS Mailing List, linux-fsdevel, linux-kernel, J. Bruce Fields Hi Linus, The following changes since commit 32c1431eea4881a6b17bd7c639315010aeefa452: Linux 4.12-rc5 (2017-06-11 16:48:20 -0700) are available in the git repository at: git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1 for you to fetch changes up to b4f937cffa66b3d56eb8f586e620d0b223a281a3: NFS: Don't run wake_up_bit() when nobody is waiting... (2017-07-13 16:57:18 -0400) Stable bugfixes: - Fix -EACCESS on commit to DS handling - Fix initialization of nfs_page_array->npages - Only invalidate dentries that are actually invalid Features: - Enable NFSoRDMA transparent state migration - Add support for lookup-by-filehandle - Add support for nfs re-exporting Other bugfixes and cleanups: - Christoph cleaned up the way we declare NFS operations - Clean up various internal structures - Various cleanups to commits - Various improvements to error handling - Set the dt_type of . and .. entries in NFS v4 - Make slot allocation more reliable - Fix fscache stat printing - Fix uninitialized variable warnings - Fix potential list overrun in nfs_atomic_open() - Fix a race in NFSoRDMA RPC reply handler - Fix return size for nfs42_proc_copy() - Fix against MAC forgery timing attacks As Bruce mentioned in his pull request, we didn't do a good job coordinating with Christoph's patches from the beginning. I ended up rebasing my tree on top of Christoph's nfs-ops branch to prevent duplicate commits. Apologies if that was the wrong choice! Cheers, Anna ---------------------------------------------------------------- Anna Schumaker (1): NFS: Set FATTR4_WORD0_TYPE for . and .. entries Benjamin Coddington (3): NFS: convert flags to bool NFS: nfs_rename() - revalidate directories on -ERESTARTSYS NFS: Fix initialization of nfs_page_array->npages Christoph Hellwig (33): sunrpc: properly type argument to kxdreproc_t sunrpc: fix encoder callback prototypes lockd: fix encoder callback prototypes nfs: fix encoder callback prototypes nfsd: fix encoder callback prototypes sunrpc/auth_gss: nfsd: fix encoder callback prototypes sunrpc: properly type argument to kxdrdproc_t sunrpc: fix decoder callback prototypes sunrpc/auth_gss: fix decoder callback prototypes nfsd: fix decoder callback prototypes lockd: fix decoder callback prototypes nfs: fix decoder callback prototypes nfs: don't cast callback decode/proc/encode routines lockd: fix some weird indentation sunrpc: move p_count out of struct rpc_procinfo nfs: use ARRAY_SIZE() in the nfsacl_version3 declaration sunrpc: mark all struct rpc_procinfo instances as const nfsd4: const-ify nfs_cb_version4 nfsd: use named initializers in PROC() nfsd: remove the unused PROC() macro in nfs3proc.c sunrpc: properly type pc_func callbacks sunrpc: properly type pc_release callbacks sunrpc: properly type pc_decode callbacks sunrpc: properly type pc_encode callbacks sunrpc: remove kxdrproc_t nfsd4: properly type op_set_currentstateid callbacks nfsd4: properly type op_get_currentstateid callbacks nfsd4: remove nfsd4op_rsize nfsd4: properly type op_func callbacks sunrpc: move pc_count out of struct svc_procinfo sunrpc: mark all struct svc_procinfo instances as const sunrpc: mark all struct svc_version instances as const nfsd4: const-ify nfsd4_ops Chuck Lever (13): xprtrdma: On invalidation failure, remove MWs from rl_registered xprtrdma: Pre-mark remotely invalidated MRs xprtrdma: Pass only the list of registered MRs to ro_unmap_sync xprtrdma: Rename rpcrdma_req::rl_free xprtrdma: Fix client lock-up after application signal fires xprtrdma: Fix FRWR invalidation error recovery xprtrdma: Don't defer MR recovery if ro_map fails NFSv4.1: Handle EXCHGID4_FLAG_CONFIRMED_R during NFSv4.1 migration NFSv4.1: Use seqid returned by EXCHANGE_ID after state migration xprtrdma: Demote "connect" log messages xprtrdma: FMR does not need list_del_init() xprtrdma: Replace PAGE_MASK with offset_in_page() xprtrdma: Fix documenting comments in frwr_ops.c Dan Carpenter (1): NFS: silence a uninitialized variable warning Jason A. Donenfeld (1): sunrpc: use constant time memory comparison for mac Jeff Layton (1): nfs4: add NFSv4 LOOKUPP handlers NeilBrown (3): NFS: only invalidate dentrys that are clearly invalid. NFS: guard against confused server in nfs_atomic_open() NFS: check for nfs_refresh_inode() errors in nfs_fhget() Olga Kornievskaia (3): PNFS fix EACCESS on commit to DS handling PNFS for stateid errors retry against MDS first NFSv4.2 fix size storage for nfs42_proc_copy Peng Tao (3): nfs: replace d_add with d_splice_alias in atomic_open nfs: add a nfs_ilookup helper nfs: add export operations Trond Myklebust (5): SUNRPC: Make slot allocation more reliable NFS: Remove unused fields in the page I/O structures NFS: Ensure we commit after writeback is complete NFS: Fix commit policy for non-blocking calls to nfs_write_inode() NFS: Don't run wake_up_bit() when nobody is waiting... Tuo Chen Peng (1): nfs: Fix fscache stat printing in nfs_show_stats() fs/lockd/clnt4xdr.c | 34 ++- fs/lockd/clntxdr.c | 58 +++-- fs/lockd/mon.c | 38 +-- fs/lockd/svc.c | 38 +-- fs/lockd/svc4proc.c | 124 ++++++---- fs/lockd/svcproc.c | 124 ++++++---- fs/lockd/xdr.c | 43 ++-- fs/lockd/xdr4.c | 43 ++-- fs/nfs/Makefile | 2 +- fs/nfs/callback.c | 2 +- fs/nfs/callback.h | 27 +-- fs/nfs/callback_proc.c | 33 ++- fs/nfs/callback_xdr.c | 113 +++++---- fs/nfs/dir.c | 42 ++-- fs/nfs/export.c | 177 ++++++++++++++ fs/nfs/filelayout/filelayout.c | 31 +-- fs/nfs/flexfilelayout/flexfilelayout.c | 33 --- fs/nfs/inode.c | 36 ++- fs/nfs/internal.h | 18 +- fs/nfs/mount_clnt.c | 29 ++- fs/nfs/nfs2xdr.c | 72 ++++-- fs/nfs/nfs3proc.c | 2 +- fs/nfs/nfs3xdr.c | 153 ++++++++---- fs/nfs/nfs42proc.c | 2 +- fs/nfs/nfs42xdr.c | 52 +++-- fs/nfs/nfs4_fs.h | 6 +- fs/nfs/nfs4client.c | 5 + fs/nfs/nfs4idmap.c | 3 +- fs/nfs/nfs4proc.c | 109 ++++++--- fs/nfs/nfs4state.c | 16 +- fs/nfs/nfs4trace.h | 29 +++ fs/nfs/nfs4xdr.c | 408 ++++++++++++++++++++++---------- fs/nfs/pagelist.c | 23 +- fs/nfs/proc.c | 2 +- fs/nfs/super.c | 4 +- fs/nfs/unlink.c | 13 ++ fs/nfs/write.c | 59 ++++- fs/nfsd/current_stateid.h | 36 ++- fs/nfsd/nfs2acl.c | 116 +++++----- fs/nfsd/nfs3acl.c | 75 +++--- fs/nfsd/nfs3proc.c | 301 ++++++++++++------------ fs/nfsd/nfs3xdr.c | 164 +++++++------ fs/nfsd/nfs4callback.c | 32 ++- fs/nfsd/nfs4proc.c | 412 +++++++++++++++++---------------- fs/nfsd/nfs4state.c | 142 +++++++----- fs/nfsd/nfs4xdr.c | 13 +- fs/nfsd/nfsd.h | 6 +- fs/nfsd/nfsproc.c | 206 +++++++++-------- fs/nfsd/nfssvc.c | 24 +- fs/nfsd/nfsxdr.c | 92 +++++--- fs/nfsd/xdr.h | 50 ++-- fs/nfsd/xdr3.h | 100 +++----- fs/nfsd/xdr4.h | 78 +++---- include/linux/lockd/lockd.h | 4 +- include/linux/lockd/xdr.h | 26 +-- include/linux/lockd/xdr4.h | 26 +-- include/linux/nfs4.h | 1 + include/linux/nfs_fs.h | 1 + include/linux/nfs_fs_sb.h | 2 + include/linux/nfs_page.h | 4 +- include/linux/nfs_xdr.h | 31 ++- include/linux/sunrpc/clnt.h | 6 +- include/linux/sunrpc/sched.h | 2 +- include/linux/sunrpc/svc.h | 21 +- include/linux/sunrpc/xdr.h | 15 +- net/sunrpc/auth_gss/gss_krb5_crypto.c | 3 +- net/sunrpc/auth_gss/gss_rpc_upcall.c | 9 +- net/sunrpc/auth_gss/gss_rpc_xdr.c | 14 +- net/sunrpc/auth_gss/gss_rpc_xdr.h | 4 +- net/sunrpc/clnt.c | 16 +- net/sunrpc/rpcb_clnt.c | 82 ++++--- net/sunrpc/stats.c | 16 +- net/sunrpc/svc.c | 33 +-- net/sunrpc/xprt.c | 8 +- net/sunrpc/xprtrdma/fmr_ops.c | 47 ++-- net/sunrpc/xprtrdma/frwr_ops.c | 69 +++--- net/sunrpc/xprtrdma/rpc_rdma.c | 125 ++++++---- net/sunrpc/xprtrdma/transport.c | 3 +- net/sunrpc/xprtrdma/verbs.c | 55 ++--- net/sunrpc/xprtrdma/xprt_rdma.h | 40 +++- 80 files changed, 2733 insertions(+), 1780 deletions(-) create mode 100644 fs/nfs/export.c ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-13 21:16 [GIT PULL] Please pull NFS client changes for Linux 4.13 Anna Schumaker @ 2017-07-13 21:43 ` Linus Torvalds 2017-07-14 7:09 ` Christoph Hellwig 2017-07-14 14:25 ` Dave Jones 1 sibling, 1 reply; 30+ messages in thread From: Linus Torvalds @ 2017-07-13 21:43 UTC (permalink / raw) To: Anna Schumaker Cc: Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields On Thu, Jul 13, 2017 at 2:16 PM, Anna Schumaker <schumaker.anna@gmail.com> wrote: > > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1 Btw, your key seems to have expired, and doing a refresh on it doesn't fix it. I'm sure you've refreshed your key, but apparently that refresh hasn't been percolated to the public keyservers? Linus ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-13 21:43 ` Linus Torvalds @ 2017-07-14 7:09 ` Christoph Hellwig 2017-07-14 11:33 ` Anna Schumaker 0 siblings, 1 reply; 30+ messages in thread From: Christoph Hellwig @ 2017-07-14 7:09 UTC (permalink / raw) To: Linus Torvalds Cc: Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields On Thu, Jul 13, 2017 at 02:43:14PM -0700, Linus Torvalds wrote: > On Thu, Jul 13, 2017 at 2:16 PM, Anna Schumaker > <schumaker.anna@gmail.com> wrote: > > > > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1 > > Btw, your key seems to have expired, and doing a refresh on it doesn't fix it. > > I'm sure you've refreshed your key, but apparently that refresh hasn't > been percolated to the public keyservers? As someone who has run into an issue in that area recently: I manually had to refresh and re-upload my signing subkey and not just the primary key, which wa rather confusing and took a long time to sort out. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 7:09 ` Christoph Hellwig @ 2017-07-14 11:33 ` Anna Schumaker 0 siblings, 0 replies; 30+ messages in thread From: Anna Schumaker @ 2017-07-14 11:33 UTC (permalink / raw) To: Christoph Hellwig, Linus Torvalds Cc: Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields On 07/14/2017 03:09 AM, Christoph Hellwig wrote: > On Thu, Jul 13, 2017 at 02:43:14PM -0700, Linus Torvalds wrote: >> On Thu, Jul 13, 2017 at 2:16 PM, Anna Schumaker >> <schumaker.anna@gmail.com> wrote: >>> >>> git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1 >> >> Btw, your key seems to have expired, and doing a refresh on it doesn't fix it. >> >> I'm sure you've refreshed your key, but apparently that refresh hasn't >> been percolated to the public keyservers? I assumed I had refreshed it once git let me sign things again, but maybe I missed a step. > > As someone who has run into an issue in that area recently: I manually > had to refresh and re-upload my signing subkey and not just the primary > key, which wa rather confusing and took a long time to sort out. > I'll start here. Thanks for the tip! Anna ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-13 21:16 [GIT PULL] Please pull NFS client changes for Linux 4.13 Anna Schumaker 2017-07-13 21:43 ` Linus Torvalds @ 2017-07-14 14:25 ` Dave Jones 2017-07-14 16:36 ` J. Bruce Fields ` (2 more replies) 1 sibling, 3 replies; 30+ messages in thread From: Dave Jones @ 2017-07-14 14:25 UTC (permalink / raw) To: Anna Schumaker Cc: torvalds, Linux NFS Mailing List, linux-fsdevel, linux-kernel, J. Bruce Fields On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote: > Hi Linus, > > The following changes since commit 32c1431eea4881a6b17bd7c639315010aeefa452: > > Linux 4.12-rc5 (2017-06-11 16:48:20 -0700) > > are available in the git repository at: > > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1 > > for you to fetch changes up to b4f937cffa66b3d56eb8f586e620d0b223a281a3: > > NFS: Don't run wake_up_bit() when nobody is waiting... (2017-07-13 16:57:18 -0400) Since this landed, I'm seeing this during boot.. ================================================================== BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230 Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688 CPU: 0 PID: 688 Comm: nfsd Not tainted 4.12.0-firewall+ #14 Call Trace: dump_stack+0x68/0x94 print_address_description+0x2c/0x270 ? strscpy+0x4a/0x230 kasan_report+0x239/0x350 __asan_load8+0x55/0x90 strscpy+0x4a/0x230 __ip_map_lookup+0x85/0x150 ? ip_map_init+0x50/0x50 ? lock_acquire+0x270/0x270 svcauth_unix_set_client+0x9f3/0xdc0 ? svcauth_unix_set_client+0x5/0xdc0 ? unix_gid_parse+0x340/0x340 ? kasan_kmalloc+0xbb/0xf0 ? groups_alloc+0x29/0x80 ? __kmalloc+0x13b/0x360 ? groups_alloc+0x29/0x80 ? groups_alloc+0x48/0x80 ? svcauth_unix_accept+0x3a5/0x3c0 svc_set_client+0x50/0x60 svc_process+0x901/0x10b0 ? svc_register+0x430/0x430 ? __might_sleep+0x78/0xf0 ? preempt_count_sub+0xaf/0x120 ? __validate_process_creds+0x9e/0x160 nfsd+0x250/0x380 ? nfsd+0x5/0x380 kthread+0x1ab/0x200 ? nfsd_destroy+0x1f0/0x1f0 ? __kthread_create_on_node+0x340/0x340 ret_from_fork+0x27/0x40 The buggy address belongs to the variable: str__nfsd__trace_system_name+0x3a0/0x3e0 Memory state around the buggy address: ffffffffb4eeae00: 00 00 00 01 fa fa fa fa 00 00 00 00 00 04 fa fa ffffffffb4eeae80: fa fa fa fa 04 fa fa fa fa fa fa fa 04 fa fa fa >ffffffffb4eeaf00: fa fa fa fa 05 fa fa fa fa fa fa fa 00 00 00 00 ^ ffffffffb4eeaf80: 00 fa fa fa fa fa fa fa 00 00 05 fa fa fa fa fa ffffffffb4eeb000: 00 03 fa fa fa fa fa fa 00 07 fa fa fa fa fa fa ================================================================== ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 14:25 ` Dave Jones @ 2017-07-14 16:36 ` J. Bruce Fields 2017-07-14 19:05 ` Linus Torvalds 2017-07-16 21:15 ` Dave Jones 2 siblings, 0 replies; 30+ messages in thread From: J. Bruce Fields @ 2017-07-14 16:36 UTC (permalink / raw) To: Dave Jones, Anna Schumaker, torvalds, Linux NFS Mailing List, linux-fsdevel, linux-kernel On Fri, Jul 14, 2017 at 10:25:43AM -0400, Dave Jones wrote: > On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote: > > Hi Linus, > > > > The following changes since commit 32c1431eea4881a6b17bd7c639315010aeefa452: > > > > Linux 4.12-rc5 (2017-06-11 16:48:20 -0700) > > > > are available in the git repository at: > > > > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1 > > > > for you to fetch changes up to b4f937cffa66b3d56eb8f586e620d0b223a281a3: > > > > NFS: Don't run wake_up_bit() when nobody is waiting... (2017-07-13 16:57:18 -0400) > > Since this landed, I'm seeing this during boot.. __ip_map_lookup does have a strcpy, and it looks like that can be implemented in terms of strscpy. Based on that backtrace, it should just be copying from nfsd_program->pg_class, which is initialized to "nfsd" and never changed. I spent a few minutes trying to figure out the tracing macros that define str__nfsd__trace_system_name+0x3a0/0x3e0 and gave up. So I have no idea what's going on.... --b. > > ================================================================== > BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230 > Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688 > > CPU: 0 PID: 688 Comm: nfsd Not tainted 4.12.0-firewall+ #14 > Call Trace: > dump_stack+0x68/0x94 > print_address_description+0x2c/0x270 > ? strscpy+0x4a/0x230 > kasan_report+0x239/0x350 > __asan_load8+0x55/0x90 > strscpy+0x4a/0x230 > __ip_map_lookup+0x85/0x150 > ? ip_map_init+0x50/0x50 > ? lock_acquire+0x270/0x270 > svcauth_unix_set_client+0x9f3/0xdc0 > ? svcauth_unix_set_client+0x5/0xdc0 > ? unix_gid_parse+0x340/0x340 > ? kasan_kmalloc+0xbb/0xf0 > ? groups_alloc+0x29/0x80 > ? __kmalloc+0x13b/0x360 > ? groups_alloc+0x29/0x80 > ? groups_alloc+0x48/0x80 > ? svcauth_unix_accept+0x3a5/0x3c0 > svc_set_client+0x50/0x60 > svc_process+0x901/0x10b0 > ? svc_register+0x430/0x430 > ? __might_sleep+0x78/0xf0 > ? preempt_count_sub+0xaf/0x120 > ? __validate_process_creds+0x9e/0x160 > nfsd+0x250/0x380 > ? nfsd+0x5/0x380 > kthread+0x1ab/0x200 > ? nfsd_destroy+0x1f0/0x1f0 > ? __kthread_create_on_node+0x340/0x340 > ret_from_fork+0x27/0x40 > > The buggy address belongs to the variable: > str__nfsd__trace_system_name+0x3a0/0x3e0 > > Memory state around the buggy address: > ffffffffb4eeae00: 00 00 00 01 fa fa fa fa 00 00 00 00 00 04 fa fa > ffffffffb4eeae80: fa fa fa fa 04 fa fa fa fa fa fa fa 04 fa fa fa > >ffffffffb4eeaf00: fa fa fa fa 05 fa fa fa fa fa fa fa 00 00 00 00 > ^ > ffffffffb4eeaf80: 00 fa fa fa fa fa fa fa 00 00 05 fa fa fa fa fa > ffffffffb4eeb000: 00 03 fa fa fa fa fa fa 00 07 fa fa fa fa fa fa > ================================================================== ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 14:25 ` Dave Jones 2017-07-14 16:36 ` J. Bruce Fields @ 2017-07-14 19:05 ` Linus Torvalds 2017-07-14 19:43 ` Andrey Ryabinin 2017-07-14 19:48 ` Dave Jones 2017-07-16 21:15 ` Dave Jones 2 siblings, 2 replies; 30+ messages in thread From: Linus Torvalds @ 2017-07-14 19:05 UTC (permalink / raw) To: Dave Jones, Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov Cc: kasan-dev On Fri, Jul 14, 2017 at 7:25 AM, Dave Jones <davej@codemonkey.org.uk> wrote: > On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote: > > > > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1 > > Since this landed, I'm seeing this during boot.. > > ================================================================== > BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230 > Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688 Is KASAN aware that strscpy() does the word-at-a-time optimistic reads of the sources? The problem may be that the source is initialized from the global string "nfsd", and KASAN may be unhappy abotu the fact that we read 8 bytes from a 5-byte string (four plus NUL) as we do the word-at-a-time strscpy.. That said, we do check the size first (because we also *write* 8 bytes at a time), so maybe KASAN shouldn't even need to care. Hmm. it really looks to me like this is actually a compiler bug (I'm using current gcc in F26, which is gcc-7.1.1 - I'm assuming DaveJ is the same). This is the source code in __ip_map_lookup: struct ip_map ip; ..... strcpy(ip.m_class, class); and "m_class" is 8 bytes in size: struct ip_map { ... char m_class[8]; /* e.g. "nfsd" */ ... yet when I look at the generated code for __ip_map_lookup, I see movl $32, %edx #, movq %r13, %rsi # class, leaq 48(%rax), %rdi #, tmp126 call strscpy # what's the bug here? Look at that third argument - %rdx. It is initialized to 32. WTF? The code to turn "strcpy()" into "strscpy()" should pick the *smaller* of the two object sizes as the size argument. How the hell is that size argument 32? Am I missing something? DaveJ, do you see the same? Linus ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 19:05 ` Linus Torvalds @ 2017-07-14 19:43 ` Andrey Ryabinin 2017-07-14 19:58 ` Linus Torvalds 2017-07-14 19:48 ` Dave Jones 1 sibling, 1 reply; 30+ messages in thread From: Andrey Ryabinin @ 2017-07-14 19:43 UTC (permalink / raw) To: Linus Torvalds, Dave Jones, Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields, Alexander Potapenko, Dmitry Vyukov Cc: kasan-dev On 07/14/2017 10:05 PM, Linus Torvalds wrote: > On Fri, Jul 14, 2017 at 7:25 AM, Dave Jones <davej@codemonkey.org.uk> wrote: >> On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote: >> > >> > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1 >> >> Since this landed, I'm seeing this during boot.. >> >> ================================================================== >> BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230 >> Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688 > > Is KASAN aware that strscpy() does the word-at-a-time optimistic reads > of the sources? > Nope. > The problem may be that the source is initialized from the global > string "nfsd", and KASAN may be unhappy abotu the fact that we read 8 > bytes from a 5-byte string (four plus NUL) as we do the word-at-a-time > strscpy.. > Right. > That said, we do check the size first (because we also *write* 8 bytes > at a time), so maybe KASAN shouldn't even need to care. > Perhaps we could fallback to unoptimzed copy for KASAN case by setting max = 0 in strscpy(). > Hmm. it really looks to me like this is actually a compiler bug (I'm > using current gcc in F26, which is gcc-7.1.1 - I'm assuming DaveJ is > the same). > > This is the source code in __ip_map_lookup: > > struct ip_map ip; > ..... > strcpy(ip.m_class, class); > > and "m_class" is 8 bytes in size: > > struct ip_map { > ... > char m_class[8]; /* e.g. "nfsd" */ > ... > > yet when I look at the generated code for __ip_map_lookup, I see > > movl $32, %edx #, > movq %r13, %rsi # class, > leaq 48(%rax), %rdi #, tmp126 > call strscpy # > > what's the bug here? Look at that third argume8nt - %rdx. It is > initialized to 32. > > WTF? > It's not a compiler bug, it's a bug in our strcpy(). Whoever wrote this strcpy() into strscpy() code apparently didn't read carefully enough gcc manual about __builtin_object_size(). Summary from https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html : __builtin_object_size(ptr, type) returns a constant number of bytes from 'ptr' to the end of the object 'ptr' pointer points to. "type" is an integer constant from 0 to 3. If the least significant bit is clear, objects are whole variables, if it is set, a closest surrounding subobject is considered the object a pointer points to. The second bit determines if maximum or minimum of remaining bytes is computed. We have type = 0 in strcpy(), so the least significant bit is clear. So the 'ptr' is considered as a pointer to the whole variable i.e. pointer to struct ip_map ip; And the number of bytes from 'ip.m_class' to the end of the ip object is exactly 32. I suppose that changing the type to 1 should fix this bug. > The code to turn "strcpy()" into "strscpy()" should pick the *smaller* > of the two object sizes as the size argument. How the hell is that > size argument 32? > > Am I missing something? DaveJ, do you see the same? > > Linus > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 19:43 ` Andrey Ryabinin @ 2017-07-14 19:58 ` Linus Torvalds 2017-07-14 20:26 ` Andrey Rybainin ` (2 more replies) 0 siblings, 3 replies; 30+ messages in thread From: Linus Torvalds @ 2017-07-14 19:58 UTC (permalink / raw) To: Andrey Ryabinin, Daniel Micay, Kees Cook Cc: Dave Jones, Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields, Alexander Potapenko, Dmitry Vyukov, kasan-dev On Fri, Jul 14, 2017 at 12:43 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: > >> yet when I look at the generated code for __ip_map_lookup, I see >> >> movl $32, %edx #, >> movq %r13, %rsi # class, >> leaq 48(%rax), %rdi #, tmp126 >> call strscpy # >> >> what's the bug here? Look at that third argume8nt - %rdx. It is >> initialized to 32. > > It's not a compiler bug, it's a bug in our strcpy(). > Whoever wrote this strcpy() into strscpy() code apparently didn't read carefully > enough gcc manual about __builtin_object_size(). > > Summary from https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html : > > __builtin_object_size(ptr, type) returns a constant number of bytes from 'ptr' to the end of the object 'ptr' > pointer points to. "type" is an integer constant from 0 to 3. If the least significant bit is clear, objects > are whole variables, if it is set, a closest surrounding subobject is considered the object a pointer points to. > The second bit determines if maximum or minimum of remaining bytes is computed. > > We have type = 0 in strcpy(), so the least significant bit is clear. So the 'ptr' is considered as a pointer to the whole > variable i.e. pointer to struct ip_map ip; > And the number of bytes from 'ip.m_class' to the end of the ip object is exactly 32. > > I suppose that changing the type to 1 should fix this bug. Oh, that absolutely needs to be done. Because that "strcpy() -> strscpy()" conversion really depends on that size being the right size (well, in this case minimal safe size) for the actual accesses, exactly because "strscpy()" is perfectly willing to write *past* the end of the destination string within that given size limit (ie it reads and writes in the same 8-byte chunks). So if you have a small target string that is contained in a big object, then the "hardened" strcpy() code can actually end up overwriting things past the end of the strring, even if the string itself were to have fit in the buffer. I note that every single use in string.h is buggy, and it worries me that __compiletime_object_size() does this too. The only user of that seems to be check_copy_size(), and now I'm a bit worried what that bug may have hidden. I find "hardening" code that adds bugs to be particularly bad and ugly, the same way that I absolutely *hate* debugging code that turns out to make debugging impossible (we had that with the "better" stack tracing code that caused kernel panics to kill the machine entirely rather than show the backtrace, and I'm still bitter about it a decade after the fact). There is something actively *evil* about it. Daniel, Kees, please jump on this. Andrey, thanks for noticing this thing, Linus ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 19:58 ` Linus Torvalds @ 2017-07-14 20:26 ` Andrey Rybainin 2017-07-14 20:38 ` Daniel Micay 2017-07-14 23:59 ` Daniel Micay 2 siblings, 0 replies; 30+ messages in thread From: Andrey Rybainin @ 2017-07-14 20:26 UTC (permalink / raw) To: Linus Torvalds, Kees Cook Cc: Andrey Ryabinin, Daniel Micay, Dave Jones, Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields, Alexander Potapenko, Dmitry Vyukov, kasan-dev On 07/14/2017 10:58 PM, Linus Torvalds wrote: > On Fri, Jul 14, 2017 at 12:43 PM, Andrey Ryabinin > <aryabinin@virtuozzo.com> wrote: >> >>> yet when I look at the generated code for __ip_map_lookup, I see >>> >>> movl $32, %edx #, >>> movq %r13, %rsi # class, >>> leaq 48(%rax), %rdi #, tmp126 >>> call strscpy # >>> >>> what's the bug here? Look at that third argume8nt - %rdx. It is >>> initialized to 32. >> >> It's not a compiler bug, it's a bug in our strcpy(). >> Whoever wrote this strcpy() into strscpy() code apparently didn't read carefully >> enough gcc manual about __builtin_object_size(). >> >> Summary from https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html : >> >> __builtin_object_size(ptr, type) returns a constant number of bytes from 'ptr' to the end of the object 'ptr' >> pointer points to. "type" is an integer constant from 0 to 3. If the least significant bit is clear, objects >> are whole variables, if it is set, a closest surrounding subobject is considered the object a pointer points to. >> The second bit determines if maximum or minimum of remaining bytes is computed. >> >> We have type = 0 in strcpy(), so the least significant bit is clear. So the 'ptr' is considered as a pointer to the whole >> variable i.e. pointer to struct ip_map ip; >> And the number of bytes from 'ip.m_class' to the end of the ip object is exactly 32. >> >> I suppose that changing the type to 1 should fix this bug. > > Oh, that absolutely needs to be done. > > Because that "strcpy() -> strscpy()" conversion really depends on that > size being the right size (well, in this case minimal safe size) for > the actual accesses, exactly because "strscpy()" is perfectly willing > to write *past* the end of the destination string within that given > size limit (ie it reads and writes in the same 8-byte chunks). > > So if you have a small target string that is contained in a big > object, then the "hardened" strcpy() code can actually end up > overwriting things past the end of the strring, even if the string > itself were to have fit in the buffer. > > I note that every single use in string.h is buggy, and it worries me > that __compiletime_object_size() does this too. The only user of that > seems to be check_copy_size(), and now I'm a bit worried what that bug > may have hidden. > > I find "hardening" code that adds bugs to be particularly bad and > ugly, the same way that I absolutely *hate* debugging code that turns > out to make debugging impossible (we had that with the "better" stack > tracing code that caused kernel panics to kill the machine entirely > rather than show the backtrace, and I'm still bitter about it a decade > after the fact). > A have some more news to make you even more "happier" :) strcpy() choose to copy 32-bytes instead of smaller 5-bytes because it has one more bug :) GCC couldn't determine size of class (which is 5-byte string): strcpy(ip.m_class, class); So, p_size = 32 and q_size = -1, this "if (p_size == (size_t)-1 && q_size == (size_t)-1)" is false (because of bogus '&&', obviously we should have '||' here) and since (32 < (size_t)-1) if (strscpy(p, q, p_size < q_size ? p_size : q_size) < 0) we end up with 32-bytes strscpy(). Enjoy :) > There is something actively *evil* about it. Daniel, Kees, please jump on this. > > Andrey, thanks for noticing this thing, > > Linus > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 19:58 ` Linus Torvalds 2017-07-14 20:26 ` Andrey Rybainin @ 2017-07-14 20:38 ` Daniel Micay 2017-07-14 20:50 ` Linus Torvalds 2017-07-14 20:50 ` Daniel Micay 2017-07-14 23:59 ` Daniel Micay 2 siblings, 2 replies; 30+ messages in thread From: Daniel Micay @ 2017-07-14 20:38 UTC (permalink / raw) To: Linus Torvalds, Andrey Ryabinin, Kees Cook Cc: Dave Jones, Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields, Alexander Potapenko, Dmitry Vyukov, kasan-dev On Fri, 2017-07-14 at 12:58 -0700, Linus Torvalds wrote: > On Fri, Jul 14, 2017 at 12:43 PM, Andrey Ryabinin > <aryabinin@virtuozzo.com> wrote: > > > > > yet when I look at the generated code for __ip_map_lookup, I see > > > > > > movl $32, %edx #, > > > movq %r13, %rsi # class, > > > leaq 48(%rax), %rdi #, tmp126 > > > call strscpy # > > > > > > what's the bug here? Look at that third argume8nt - %rdx. It is > > > initialized to 32. > > > > It's not a compiler bug, it's a bug in our strcpy(). > > Whoever wrote this strcpy() into strscpy() code apparently didn't > > read carefully > > enough gcc manual about __builtin_object_size(). > > > > Summary from https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking > > .html : > > > > __builtin_object_size(ptr, type) returns a constant number > > of bytes from 'ptr' to the end of the object 'ptr' > > pointer points to. "type" is an integer constant from 0 to > > 3. If the least significant bit is clear, objects > > are whole variables, if it is set, a closest surrounding > > subobject is considered the object a pointer points to. > > The second bit determines if maximum or minimum of remaining > > bytes is computed. > > > > We have type = 0 in strcpy(), so the least significant bit is clear. > > So the 'ptr' is considered as a pointer to the whole > > variable i.e. pointer to struct ip_map ip; > > And the number of bytes from 'ip.m_class' to the end of the ip > > object is exactly 32. > > > > I suppose that changing the type to 1 should fix this bug. > > Oh, that absolutely needs to be done. > > Because that "strcpy() -> strscpy()" conversion really depends on that > size being the right size (well, in this case minimal safe size) for > the actual accesses, exactly because "strscpy()" is perfectly willing > to write *past* the end of the destination string within that given > size limit (ie it reads and writes in the same 8-byte chunks). > > So if you have a small target string that is contained in a big > object, then the "hardened" strcpy() code can actually end up > overwriting things past the end of the strring, even if the string > itself were to have fit in the buffer. > > I note that every single use in string.h is buggy, and it worries me > that __compiletime_object_size() does this too. The only user of that > seems to be check_copy_size(), and now I'm a bit worried what that bug > may have hidden. > > I find "hardening" code that adds bugs to be particularly bad and > ugly, the same way that I absolutely *hate* debugging code that turns > out to make debugging impossible (we had that with the "better" stack > tracing code that caused kernel panics to kill the machine entirely > rather than show the backtrace, and I'm still bitter about it a decade > after the fact). > > There is something actively *evil* about it. Daniel, Kees, please jump > on this. > > Andrey, thanks for noticing this thing, > > Linus The issue is the usage of strscpy then, not the __builtin_object_size type parameter. The type is set 0 rather than 1 to be more lenient by not detecting intra-object overflow, which is going to come later. If strscpy treats the count parameter as a *guarantee* of the dest size rather than a limit, it's wrong to use it there, whether or not the type parameter for __builtin_object_size is 0 or 1 since it can still return a larger size. It's a limit with no guaranteed minimum. My initial patch used strlcpy there, because I wasn't aware of strscpy before it was suggested: http://www.openwall.com/lists/kernel-hardening/2017/05/04/11 I was wrong to move it to strscpy. It could be switched back to strlcpy again unless the kernel considers the count parameter to be a guarantee that could be leveraged in the future. Using the fortified strlen + memcpy would provide the improvement that strscpy was meant to provide there over strlcpy. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 20:38 ` Daniel Micay @ 2017-07-14 20:50 ` Linus Torvalds 2017-07-14 21:01 ` Daniel Micay 2017-07-14 20:50 ` Daniel Micay 1 sibling, 1 reply; 30+ messages in thread From: Linus Torvalds @ 2017-07-14 20:50 UTC (permalink / raw) To: Daniel Micay Cc: Andrey Ryabinin, Kees Cook, Dave Jones, Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields, Alexander Potapenko, Dmitry Vyukov, kasan-dev On Fri, Jul 14, 2017 at 1:38 PM, Daniel Micay <danielmicay@gmail.com> wrote: > > If strscpy treats the count parameter as a *guarantee* of the dest size > rather than a limit, No, it's a *limit*. And by a *limit*, I mean that we know that we can access both source and destination within that limit. > My initial patch used strlcpy there, because I wasn't aware of strscpy > before it was suggested: Since I'm looking at this, I note that the "strlcpy()" code is complete garbage too, and has that same p_size == (size_t)-1 && q_size == (size_t)-1 check which is wrong. Of course, in strlcpy, q_size is never actually *used*, so the whole check seems bogus. But no, strlcpy() is complete garbage, and should never be used. It is truly a shit interface, and anybody who uses it is by definition buggy. Why? Because the return value of "strlcpy()" is defined to be ignoring the limit, so you FUNDAMENTALLY must not use that thing on untrusted source strings. But since the whole *point* of people using it is for untrusted sources, it by definition is garbage. Ergo: don't use strlcpy(). It's unbelievable crap. It's wrong. There's a reason we defined "strscpy()" as the way to do safe copies (strncpy(), of course, is broken for both lack of NUL termination _and_ for excessive NUL termination when a NUL did exist). So quite frankly, this hardening code needs to be looked at again. And no, if it uses "strlcpy()", then it's not hardering, it's just a pile of crap. Yes, I'm annoyed. I really get very very annoyed by "hardening" code that does nothing of the kind. Linus ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 20:50 ` Linus Torvalds @ 2017-07-14 21:01 ` Daniel Micay 2017-07-14 21:05 ` Daniel Micay 0 siblings, 1 reply; 30+ messages in thread From: Daniel Micay @ 2017-07-14 21:01 UTC (permalink / raw) To: Linus Torvalds Cc: Andrey Ryabinin, Kees Cook, Dave Jones, Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields, Alexander Potapenko, Dmitry Vyukov, kasan-dev On Fri, 2017-07-14 at 13:50 -0700, Linus Torvalds wrote: > On Fri, Jul 14, 2017 at 1:38 PM, Daniel Micay <danielmicay@gmail.com> > wrote: > > > > If strscpy treats the count parameter as a *guarantee* of the dest > > size > > rather than a limit, > > No, it's a *limit*. > > And by a *limit*, I mean that we know that we can access both source > and destination within that limit. FORTIFY_SOURCE needs to be able to pass a limit without implying that there's a minimum. That's the distinction I was trying to make. It's wrong to use anything where it's interpreted as a minimum here. Using __builtin_object_size(ptr, 0) vs. __builtin_object_size(ptr, 1) doesn't avoid the problem. __builtin_object_size(ptr, 1) returns a maximum among the possible buffer sizes just like 0. It's just stricter, i.e. catches intra-object overflow, which isn't desirable for the first take since it will cause compatibility issues. There's code using memcpy, copy_to_user, etc. to read / write multiple fields with a pointer to the first one passed as the source / destination. > > My initial patch used strlcpy there, because I wasn't aware of > > strscpy > > before it was suggested: > > Since I'm looking at this, I note that the "strlcpy()" code is > complete garbage too, and has that same > > p_size == (size_t)-1 && q_size == (size_t)-1 > > check which is wrong. Of course, in strlcpy, q_size is never actually > *used*, so the whole check seems bogus. That check is only an optimization. __builtin_object_size always returns a constant, and it's (size_t)-1 when no limit could be found. The reason q_size isn't used is because it doesn't yet prevent read overflow. The commit message mentions that among the current limitations along with __builtin_object_size(ptr, 1). > But no, strlcpy() is complete garbage, and should never be used. It is > truly a shit interface, and anybody who uses it is by definition > buggy. > > Why? Because the return value of "strlcpy()" is defined to be ignoring > the limit, so you FUNDAMENTALLY must not use that thing on untrusted > source strings. > > But since the whole *point* of people using it is for untrusted > sources, it by definition is garbage. > > Ergo: don't use strlcpy(). It's unbelievable crap. It's wrong. There's > a reason we defined "strscpy()" as the way to do safe copies > (strncpy(), of course, is broken for both lack of NUL termination > _and_ for excessive NUL termination when a NUL did exist). Sure, it doesn't prevent read overflow (but it's not worse than strcpy, which is the purpose) which is why I said this: "The fortified string functions should place a limit on reads from the source. For strcat/strcpy, these could just be a variant of strlcat / strlcpy using the size limit as a bound on both the source and destination, with the return value only reporting whether truncation occurred rather than providing the source length. It would be an easier API for developers to use too and not that it really matters but it would be more efficient for the case where truncation is intended." That's why strscpy was suggested and I switched to that + updated the commit message to only mention strcat, but it's wrong to use it here because __builtin_object_size(p, 0) / __builtin_object_size(p, 1) are only a guaranteed maximum length with no minimum guarantee. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 21:01 ` Daniel Micay @ 2017-07-14 21:05 ` Daniel Micay 0 siblings, 0 replies; 30+ messages in thread From: Daniel Micay @ 2017-07-14 21:05 UTC (permalink / raw) To: Linus Torvalds Cc: Andrey Ryabinin, Kees Cook, Dave Jones, Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields, Alexander Potapenko, Dmitry Vyukov, kasan-dev > The reason q_size isn't used is because it doesn't yet prevent read > overflow. The commit message mentions that among the current > limitations > along with __builtin_object_size(ptr, 1). Er rather, in strlcat, the q_size is unused after the fast path is because strnlen obtains the constant again itself. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 20:38 ` Daniel Micay 2017-07-14 20:50 ` Linus Torvalds @ 2017-07-14 20:50 ` Daniel Micay 1 sibling, 0 replies; 30+ messages in thread From: Daniel Micay @ 2017-07-14 20:50 UTC (permalink / raw) To: Linus Torvalds, Andrey Ryabinin, Kees Cook Cc: Dave Jones, Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields, Alexander Potapenko, Dmitry Vyukov, kasan-dev > My initial patch used strlcpy there, because I wasn't aware of strscpy > before it was suggested: > > http://www.openwall.com/lists/kernel-hardening/2017/05/04/11 > > I was wrong to move it to strscpy. It could be switched back to > strlcpy > again unless the kernel considers the count parameter to be a > guarantee > that could be leveraged in the future. Using the fortified strlen + > memcpy would provide the improvement that strscpy was meant to provide > there over strlcpy. Similarly, the FORTIFY_SOURCE strcat uses strlcat with the assumption that the count parameter is a limit, not a guarantee for a optimization. There's only a C implementation and it currently doesn't, but if it's meant to be a guarantee then the strcat needs to be changed too. I'll make a fix moving away both from the existing functions. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 19:58 ` Linus Torvalds 2017-07-14 20:26 ` Andrey Rybainin 2017-07-14 20:38 ` Daniel Micay @ 2017-07-14 23:59 ` Daniel Micay 2 siblings, 0 replies; 30+ messages in thread From: Daniel Micay @ 2017-07-14 23:59 UTC (permalink / raw) To: Linus Torvalds, Andrey Ryabinin, Kees Cook Cc: Dave Jones, Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields, Alexander Potapenko, Dmitry Vyukov, kasan-dev > I find "hardening" code that adds bugs to be particularly bad and > ugly, the same way that I absolutely *hate* debugging code that turns > out to make debugging impossible (we had that with the "better" stack > tracing code that caused kernel panics to kill the machine entirely > rather than show the backtrace, and I'm still bitter about it a decade > after the fact). Agree, it's very important for this code to be correct and the string functions have some subtleties so it needs scrutiny. I messed up strcpy between v1 and v2 trying to add a proper read overflow check. My fault for not looking more closely at strscpy before adopting it based on my misinterpretation of the API. This is primarily a bug finding feature right now and it has gotten a few fixed that actually matter (most were unimportant memcpy read past end of string constant but not all). I don't think it has another bug like this strscpy misuse itself, but there will need to be some more fixes for minor read overflows, etc. elsewhere in the tree before it'll actually make sense as a hardening feature because it can turn a benign read overflow into a DoS via BUG(). I think it will be fine for 4.13, but I definitely wouldn't propose 'default y' for a while, even if there was no performance cost (and there is). Fix for this issue is here in case anyone just looks only at this thread (realized I should have passed send-email a reply id): http://marc.info/?l=linux-fsdevel&m=150006772418003&w=2 ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 19:05 ` Linus Torvalds 2017-07-14 19:43 ` Andrey Ryabinin @ 2017-07-14 19:48 ` Dave Jones 1 sibling, 0 replies; 30+ messages in thread From: Dave Jones @ 2017-07-14 19:48 UTC (permalink / raw) To: Linus Torvalds Cc: Anna Schumaker, Linux NFS Mailing List, linux-fsdevel, Linux Kernel Mailing List, J. Bruce Fields, Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov, kasan-dev On Fri, Jul 14, 2017 at 12:05:02PM -0700, Linus Torvalds wrote: > On Fri, Jul 14, 2017 at 7:25 AM, Dave Jones <davej@codemonkey.org.uk> wrote: > > On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote: > > > > > > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1 > > > > Since this landed, I'm seeing this during boot.. > > > > ================================================================== > > BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230 > > Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688 > > Is KASAN aware that strscpy() does the word-at-a-time optimistic reads > of the sources? > > The problem may be that the source is initialized from the global > string "nfsd", and KASAN may be unhappy abotu the fact that we read 8 > bytes from a 5-byte string (four plus NUL) as we do the word-at-a-time > strscpy.. > > That said, we do check the size first (because we also *write* 8 bytes > at a time), so maybe KASAN shouldn't even need to care. > > Hmm. it really looks to me like this is actually a compiler bug (I'm > using current gcc in F26, which is gcc-7.1.1 - I'm assuming DaveJ is > the same). Debian's 6.4.0 > This is the source code in __ip_map_lookup: > > struct ip_map ip; > ..... > strcpy(ip.m_class, class); > > and "m_class" is 8 bytes in size: > > struct ip_map { > ... > char m_class[8]; /* e.g. "nfsd" */ > ... > > yet when I look at the generated code for __ip_map_lookup, I see > > movl $32, %edx #, > movq %r13, %rsi # class, > leaq 48(%rax), %rdi #, tmp126 > call strscpy # > > what's the bug here? Look at that third argument - %rdx. It is > initialized to 32. > > WTF? > > The code to turn "strcpy()" into "strscpy()" should pick the *smaller* > of the two object sizes as the size argument. How the hell is that > size argument 32? > > Am I missing something? DaveJ, do you see the same? My compiler seems to have replaced the call with an inlined copy afaics. 0000000000000be0 <__ip_map_lookup>: { be0: e8 00 00 00 00 callq be5 <__ip_map_lookup+0x5> be5: 55 push %rbp be6: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax bed: fc ff df bf0: 48 89 e5 mov %rsp,%rbp bf3: 41 57 push %r15 bf5: 41 56 push %r14 bf7: 4c 8d 32 lea (%rdx),%r14 if (strscpy(p, q, p_size < q_size ? p_size : q_size) < 0) bfa: ba 20 00 00 00 mov $0x20,%edx bff: 41 55 push %r13 c01: 4c 8d 2e lea (%rsi),%r13 c04: 41 54 push %r12 c06: 53 push %rbx c07: 48 8d 1f lea (%rdi),%rbx c0a: 48 8d a4 24 60 ff ff lea -0xa0(%rsp),%rsp c11: ff c12: 49 89 e4 mov %rsp,%r12 c15: 49 c1 ec 03 shr $0x3,%r12 c19: 48 c7 04 24 b3 8a b5 movq $0x41b58ab3,(%rsp) c20: 41 c21: 48 c7 44 24 08 00 00 movq $0x0,0x8(%rsp) c28: 00 00 c2a: 48 c7 44 24 10 00 00 movq $0x0,0x10(%rsp) c31: 00 00 c33: 48 8d 7c 24 50 lea 0x50(%rsp),%rdi c38: 4d 8d 24 04 lea (%r12,%rax,1),%r12 c3c: 41 c7 04 24 f1 f1 f1 movl $0xf1f1f1f1,(%r12) c43: f1 c44: 41 c7 44 24 0c 00 00 movl $0xf4f40000,0xc(%r12) c4b: f4 f4 c4d: 65 48 8b 04 25 28 00 mov %gs:0x28,%rax c54: 00 00 c56: 48 89 84 24 98 00 00 mov %rax,0x98(%rsp) c5d: 00 c5e: 31 c0 xor %eax,%eax c60: e8 00 00 00 00 callq c65 <__ip_map_lookup+0x85> c65: 48 85 c0 test %rax,%rax c68: 0f 88 a0 00 00 00 js d0e <__ip_map_lookup+0x12e> ip.m_addr = *addr; c6e: be 10 00 00 00 mov $0x10,%esi c73: 49 8d 3e lea (%r14),%rdi c76: e8 00 00 00 00 callq c7b <__ip_map_lookup+0x9b> c7b: 49 8b 56 08 mov 0x8(%r14),%rdx But that mov $0x20,%edx looks like it might be the same value we're talking about. Dave ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-14 14:25 ` Dave Jones 2017-07-14 16:36 ` J. Bruce Fields 2017-07-14 19:05 ` Linus Torvalds @ 2017-07-16 21:15 ` Dave Jones 2017-07-16 22:57 ` Trond Myklebust 2 siblings, 1 reply; 30+ messages in thread From: Dave Jones @ 2017-07-16 21:15 UTC (permalink / raw) To: Anna Schumaker, torvalds, Linux NFS Mailing List, linux-fsdevel, linux-kernel, J. Bruce Fields On Fri, Jul 14, 2017 at 10:25:43AM -0400, Dave Jones wrote: > On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote: > > Hi Linus, > > > > The following changes since commit 32c1431eea4881a6b17bd7c639315010aeefa452: > > > > Linux 4.12-rc5 (2017-06-11 16:48:20 -0700) > > > > are available in the git repository at: > > > > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1 > > > > for you to fetch changes up to b4f937cffa66b3d56eb8f586e620d0b223a281a3: > > > > NFS: Don't run wake_up_bit() when nobody is waiting... (2017-07-13 16:57:18 -0400) > > Since this landed, I'm seeing this during boot.. > > ================================================================== > BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230 > Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688 Now that this one got fixed, this one fell out instead.. Will dig deeper tomorrow. ================================================================== BUG: KASAN: global-out-of-bounds in call_start+0x93/0x100 Read of size 8 at addr ffffffff8d582588 by task kworker/0:1/22 CPU: 0 PID: 22 Comm: kworker/0:1 Not tainted 4.13.0-rc1-firewall+ #1 Workqueue: rpciod rpc_async_schedule Call Trace: dump_stack+0x68/0x94 print_address_description+0x2c/0x270 ? call_start+0x93/0x100 kasan_report+0x239/0x350 __asan_load8+0x55/0x90 call_start+0x93/0x100 ? rpc_default_callback+0x10/0x10 ? rpc_default_callback+0x10/0x10 __rpc_execute+0x170/0x740 ? rpc_wake_up_queued_task+0x50/0x50 ? __lock_is_held+0x9f/0x110 rpc_async_schedule+0x12/0x20 process_one_work+0x4ba/0xb10 ? process_one_work+0x401/0xb10 ? pwq_dec_nr_in_flight+0x120/0x120 worker_thread+0x91/0x670 ? __sched_text_start+0x8/0x8 kthread+0x1ab/0x200 ? process_one_work+0xb10/0xb10 ? __kthread_create_on_node+0x340/0x340 ret_from_fork+0x27/0x40 The buggy address belongs to the variable: nfs_cb_version+0x8/0x740 Memory state around the buggy address: ffffffff8d582480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffffffff8d582500: 00 00 00 00 00 00 00 00 00 00 fa fa fa fa fa fa >ffffffff8d582580: 00 fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00 ^ ffffffff8d582600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffffffff8d582680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ================================================================== ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-16 21:15 ` Dave Jones @ 2017-07-16 22:57 ` Trond Myklebust 2017-07-17 3:05 ` davej 0 siblings, 1 reply; 30+ messages in thread From: Trond Myklebust @ 2017-07-16 22:57 UTC (permalink / raw) To: torvalds, linux-kernel, bfields, linux-nfs, schumaker.anna, davej, linux-fsdevel Hi Dave, On Sun, 2017-07-16 at 17:15 -0400, Dave Jones wrote: > On Fri, Jul 14, 2017 at 10:25:43AM -0400, Dave Jones wrote: > > On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote: > > > Hi Linus, > > > > > > The following changes since commit > 32c1431eea4881a6b17bd7c639315010aeefa452: > > > > > > Linux 4.12-rc5 (2017-06-11 16:48:20 -0700) > > > > > > are available in the git repository at: > > > > > > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs- > for-4.13-1 > > > > > > for you to fetch changes up to > b4f937cffa66b3d56eb8f586e620d0b223a281a3: > > > > > > NFS: Don't run wake_up_bit() when nobody is waiting... (2017- > 07-13 16:57:18 -0400) > > > > Since this landed, I'm seeing this during boot.. > > > > ================================================================= > = > > BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230 > > Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688 > > Now that this one got fixed, this one fell out instead.. > Will dig deeper tomorrow. > > ================================================================== > BUG: KASAN: global-out-of-bounds in call_start+0x93/0x100 > Read of size 8 at addr ffffffff8d582588 by task kworker/0:1/22 > > CPU: 0 PID: 22 Comm: kworker/0:1 Not tainted 4.13.0-rc1-firewall+ #1 > Workqueue: rpciod rpc_async_schedule > Call Trace: > dump_stack+0x68/0x94 > print_address_description+0x2c/0x270 > ? call_start+0x93/0x100 > kasan_report+0x239/0x350 > __asan_load8+0x55/0x90 > call_start+0x93/0x100 > ? rpc_default_callback+0x10/0x10 > ? rpc_default_callback+0x10/0x10 > __rpc_execute+0x170/0x740 > ? rpc_wake_up_queued_task+0x50/0x50 > ? __lock_is_held+0x9f/0x110 > rpc_async_schedule+0x12/0x20 > process_one_work+0x4ba/0xb10 > ? process_one_work+0x401/0xb10 > ? pwq_dec_nr_in_flight+0x120/0x120 > worker_thread+0x91/0x670 > ? __sched_text_start+0x8/0x8 > kthread+0x1ab/0x200 > ? process_one_work+0xb10/0xb10 > ? __kthread_create_on_node+0x340/0x340 > ret_from_fork+0x27/0x40 > > The buggy address belongs to the variable: > nfs_cb_version+0x8/0x740 Does the following patch fix it? Cheers Trond 8<-------------------------------------- >From b9230cdfbbee90178a1318d20cd3373ffb758788 Mon Sep 17 00:00:00 2001 From: Trond Myklebust <trond.myklebust@primarydata.com> Date: Sun, 16 Jul 2017 18:52:18 -0400 Subject: [PATCH] nfsd: Fix a memory scribble in the callback channel The offset of the entry in struct rpc_version has to match the version number. Reported-by: Dave Jones <davej@codemonkey.org.uk> Fixes: 1c5876ddbdb4 ("sunrpc: move p_count out of struct rpc_procinfo") Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com> --- fs/nfsd/nfs4callback.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index b45083c0f9ae..49b0a9e7ff18 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -720,8 +720,8 @@ static const struct rpc_version nfs_cb_version4 = { .counts = nfs4_cb_counts, }; -static const struct rpc_version *nfs_cb_version[] = { - &nfs_cb_version4, +static const struct rpc_version *nfs_cb_version[2] = { + [1] = &nfs_cb_version4, }; static const struct rpc_program cb_program; @@ -795,7 +795,7 @@ static int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *c .saddress = (struct sockaddr *) &conn->cb_saddr, .timeout = &timeparms, .program = &cb_program, - .version = 0, + .version = 1, .flags = (RPC_CLNT_CREATE_NOPING | RPC_CLNT_CREATE_QUIET), }; struct rpc_clnt *client; -- 2.13.3 -- Trond Myklebust Linux NFS client maintainer, PrimaryData trond.myklebust@primarydata.com ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-16 22:57 ` Trond Myklebust @ 2017-07-17 3:05 ` davej 2017-07-17 19:02 ` Linus Torvalds 0 siblings, 1 reply; 30+ messages in thread From: davej @ 2017-07-17 3:05 UTC (permalink / raw) To: Trond Myklebust Cc: torvalds, linux-kernel, bfields, linux-nfs, schumaker.anna, linux-fsdevel On Sun, Jul 16, 2017 at 10:57:27PM +0000, Trond Myklebust wrote: > > BUG: KASAN: global-out-of-bounds in call_start+0x93/0x100 > > Read of size 8 at addr ffffffff8d582588 by task kworker/0:1/22 > > Does the following patch fix it? Yep, seems to do the trick! Dave ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-17 3:05 ` davej @ 2017-07-17 19:02 ` Linus Torvalds 2017-07-18 14:20 ` [GIT PULL] Please pull an nfsd bugfix for 4.13 bfields 2017-07-31 15:43 ` [GIT PULL] Please pull NFS client changes for Linux 4.13 davej 0 siblings, 2 replies; 30+ messages in thread From: Linus Torvalds @ 2017-07-17 19:02 UTC (permalink / raw) To: davej, Trond Myklebust, torvalds, linux-kernel, bfields, linux-nfs, schumaker.anna, linux-fsdevel On Sun, Jul 16, 2017 at 8:05 PM, davej@codemonkey.org.uk <davej@codemonkey.org.uk> wrote: > On Sun, Jul 16, 2017 at 10:57:27PM +0000, Trond Myklebust wrote: > > > > BUG: KASAN: global-out-of-bounds in call_start+0x93/0x100 > > > Read of size 8 at addr ffffffff8d582588 by task kworker/0:1/22 > > > > Does the following patch fix it? > > Yep, seems to do the trick! I'm assuming I'll get this fix through a future pull request, and am not applying the patch as-is. Just FYI. Linus ^ permalink raw reply [flat|nested] 30+ messages in thread
* [GIT PULL] Please pull an nfsd bugfix for 4.13 2017-07-17 19:02 ` Linus Torvalds @ 2017-07-18 14:20 ` bfields 2017-07-31 15:43 ` [GIT PULL] Please pull NFS client changes for Linux 4.13 davej 1 sibling, 0 replies; 30+ messages in thread From: bfields @ 2017-07-18 14:20 UTC (permalink / raw) To: Linus Torvalds Cc: davej, Trond Myklebust, linux-kernel, linux-nfs, schumaker.anna, linux-fsdevel Please pull: git://linux-nfs.org/~bfields/linux.git tags/nfsd-4.13-1 One fix for a problem introduced in the most recent merge window and found by Dave Jones and KASAN. --b. Trond Myklebust (1): nfsd: Fix a memory scribble in the callback channel fs/nfsd/nfs4callback.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-17 19:02 ` Linus Torvalds 2017-07-18 14:20 ` [GIT PULL] Please pull an nfsd bugfix for 4.13 bfields @ 2017-07-31 15:43 ` davej 2017-08-01 5:35 ` Linus Torvalds 1 sibling, 1 reply; 30+ messages in thread From: davej @ 2017-07-31 15:43 UTC (permalink / raw) To: Linus Torvalds Cc: Trond Myklebust, linux-kernel, bfields, linux-nfs, schumaker.anna, linux-fsdevel Another NFSv4 KASAN splat, this time from rc3. ================================================================== BUG: KASAN: use-after-free in nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4] Read of size 8 at addr ffff8804508af528 by task kworker/2:1/34 CPU: 2 PID: 34 Comm: kworker/2:1 Not tainted 4.13.0-rc3-think+ #1 Workqueue: rpciod rpc_async_schedule [sunrpc] Call Trace: dump_stack+0x68/0xa1 print_address_description+0xd9/0x270 kasan_report+0x257/0x370 ? nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4] check_memory_region+0x13a/0x1a0 __asan_loadN+0xf/0x20 nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4] ? nfs4_exchange_id_release+0xb0/0xb0 [nfsv4] rpc_exit_task+0x69/0x110 [sunrpc] ? rpc_destroy_wait_queue+0x20/0x20 [sunrpc] ? rpc_destroy_wait_queue+0x20/0x20 [sunrpc] __rpc_execute+0x1a0/0x840 [sunrpc] ? rpc_wake_up_queued_task+0x50/0x50 [sunrpc] ? __lock_is_held+0x9a/0x100 ? debug_lockdep_rcu_enabled.part.16+0x1a/0x30 rpc_async_schedule+0x12/0x20 [sunrpc] process_one_work+0x4d5/0xa70 ? flush_delayed_work+0x70/0x70 ? lock_acquire+0xfc/0x220 worker_thread+0x88/0x630 ? pci_mmcfg_check_reserved+0xc0/0xc0 kthread+0x1a6/0x1f0 ? process_one_work+0xa70/0xa70 ? kthread_create_on_node+0xc0/0xc0 ret_from_fork+0x27/0x40 Allocated by task 1: save_stack_trace+0x1b/0x20 save_stack+0x46/0xd0 kasan_kmalloc+0xad/0xe0 kasan_slab_alloc+0x12/0x20 kmem_cache_alloc+0xe0/0x2f0 getname_flags+0x43/0x220 getname+0x12/0x20 do_sys_open+0x14c/0x2b0 SyS_open+0x1e/0x20 do_syscall_64+0xea/0x260 return_from_SYSCALL_64+0x0/0x7a Freed by task 1: save_stack_trace+0x1b/0x20 save_stack+0x46/0xd0 kasan_slab_free+0x72/0xc0 kmem_cache_free+0xa8/0x300 putname+0x80/0x90 do_sys_open+0x22f/0x2b0 SyS_open+0x1e/0x20 do_syscall_64+0xea/0x260 return_from_SYSCALL_64+0x0/0x7a The buggy address belongs to the object at ffff8804508aeac0\x0a which belongs to the cache names_cache of size 4096 The buggy address is located 2664 bytes inside of\x0a 4096-byte region [ffff8804508aeac0, ffff8804508afac0) The buggy address belongs to the page: page:ffffea0011422a00 count:1 mapcount:0 mapping: (null) index:0x0 [CONT START] compound_mapcount: 0 flags: 0x8000000000008100(slab|head) raw: 8000000000008100 0000000000000000 0000000000000000 0000000100070007 raw: ffffea00113d6020 ffffea001136e220 ffff8804664f8040 0000000000000000 page dumped because: kasan: bad access detected Memory state around the buggy address: ffff8804508af400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ffff8804508af480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >ffff8804508af500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ^ ffff8804508af580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ffff8804508af600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ================================================================== ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-07-31 15:43 ` [GIT PULL] Please pull NFS client changes for Linux 4.13 davej @ 2017-08-01 5:35 ` Linus Torvalds 2017-08-01 15:51 ` davej 0 siblings, 1 reply; 30+ messages in thread From: Linus Torvalds @ 2017-08-01 5:35 UTC (permalink / raw) To: davej, Linus Torvalds, Trond Myklebust, linux-kernel, bfields, linux-nfs, schumaker.anna, linux-fsdevel On Mon, Jul 31, 2017 at 8:43 AM, davej@codemonkey.org.uk <davej@codemonkey.org.uk> wrote: > Another NFSv4 KASAN splat, this time from rc3. > > BUG: KASAN: use-after-free in nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4] Ugh. It's really hard to tell what access that it - KASAN doesn't actually give enough information. There's lots of 8-byte accesses there in that function. Any chance of getting the output from ./scripts/faddr2line vmlinux nfs4_exchange_id_done+0x3d7/0x8e0 or something? That would be extremely useful in general for stacktraces, but it's doubly useful for KASAN because most *other* stacktraces tend to have a very limited number of things that can warn (ie there's one or two WARN_ON() calls in a function), but KASAN can have tens or hundreds.. Linus > Read of size 8 at addr ffff8804508af528 by task kworker/2:1/34 > > CPU: 2 PID: 34 Comm: kworker/2:1 Not tainted 4.13.0-rc3-think+ #1 > Workqueue: rpciod rpc_async_schedule [sunrpc] > Call Trace: > dump_stack+0x68/0xa1 > print_address_description+0xd9/0x270 > kasan_report+0x257/0x370 > ? nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4] > check_memory_region+0x13a/0x1a0 > __asan_loadN+0xf/0x20 > nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4] > ? nfs4_exchange_id_release+0xb0/0xb0 [nfsv4] > rpc_exit_task+0x69/0x110 [sunrpc] > ? rpc_destroy_wait_queue+0x20/0x20 [sunrpc] > ? rpc_destroy_wait_queue+0x20/0x20 [sunrpc] > __rpc_execute+0x1a0/0x840 [sunrpc] > ? rpc_wake_up_queued_task+0x50/0x50 [sunrpc] > ? __lock_is_held+0x9a/0x100 > ? debug_lockdep_rcu_enabled.part.16+0x1a/0x30 > rpc_async_schedule+0x12/0x20 [sunrpc] > process_one_work+0x4d5/0xa70 > ? flush_delayed_work+0x70/0x70 > ? lock_acquire+0xfc/0x220 > worker_thread+0x88/0x630 > ? pci_mmcfg_check_reserved+0xc0/0xc0 > kthread+0x1a6/0x1f0 > ? process_one_work+0xa70/0xa70 > ? kthread_create_on_node+0xc0/0xc0 > ret_from_fork+0x27/0x40 > > Allocated by task 1: > save_stack_trace+0x1b/0x20 > save_stack+0x46/0xd0 > kasan_kmalloc+0xad/0xe0 > kasan_slab_alloc+0x12/0x20 > kmem_cache_alloc+0xe0/0x2f0 > getname_flags+0x43/0x220 > getname+0x12/0x20 > do_sys_open+0x14c/0x2b0 > SyS_open+0x1e/0x20 > do_syscall_64+0xea/0x260 > return_from_SYSCALL_64+0x0/0x7a > > Freed by task 1: > save_stack_trace+0x1b/0x20 > save_stack+0x46/0xd0 > kasan_slab_free+0x72/0xc0 > kmem_cache_free+0xa8/0x300 > putname+0x80/0x90 > do_sys_open+0x22f/0x2b0 > SyS_open+0x1e/0x20 > do_syscall_64+0xea/0x260 > return_from_SYSCALL_64+0x0/0x7a > > The buggy address belongs to the object at ffff8804508aeac0\x0a which belongs to the cache names_cache of size 4096 > The buggy address is located 2664 bytes inside of\x0a 4096-byte region [ffff8804508aeac0, ffff8804508afac0) > The buggy address belongs to the page: > page:ffffea0011422a00 count:1 mapcount:0 mapping: (null) index:0x0 > [CONT START] compound_mapcount: 0 > flags: 0x8000000000008100(slab|head) > raw: 8000000000008100 0000000000000000 0000000000000000 0000000100070007 > raw: ffffea00113d6020 ffffea001136e220 ffff8804664f8040 0000000000000000 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > ffff8804508af400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ffff8804508af480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb >>ffff8804508af500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ^ > ffff8804508af580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ffff8804508af600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ================================================================== > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-08-01 5:35 ` Linus Torvalds @ 2017-08-01 15:51 ` davej 2017-08-01 17:20 ` Linus Torvalds 0 siblings, 1 reply; 30+ messages in thread From: davej @ 2017-08-01 15:51 UTC (permalink / raw) To: Linus Torvalds Cc: Trond Myklebust, linux-kernel, bfields, linux-nfs, schumaker.anna, linux-fsdevel On Mon, Jul 31, 2017 at 10:35:45PM -0700, Linus Torvalds wrote: > On Mon, Jul 31, 2017 at 8:43 AM, davej@codemonkey.org.uk > <davej@codemonkey.org.uk> wrote: > > Another NFSv4 KASAN splat, this time from rc3. > > > > BUG: KASAN: use-after-free in nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4] > > Ugh. It's really hard to tell what access that it - KASAN doesn't > actually give enough information. There's lots of 8-byte accesses > there in that function. > > Any chance of getting the output from > > ./scripts/faddr2line vmlinux nfs4_exchange_id_done+0x3d7/0x8e0 Hm, that points to this.. 7463 /* Save the EXCHANGE_ID verifier session trunk tests */ 7464 memcpy(clp->cl_confirm.data, cdata->args.verifier->data, 7465 sizeof(clp->cl_confirm.data)); Dave ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-08-01 15:51 ` davej @ 2017-08-01 17:20 ` Linus Torvalds 2017-08-01 17:30 ` Trond Myklebust ` (2 more replies) 0 siblings, 3 replies; 30+ messages in thread From: Linus Torvalds @ 2017-08-01 17:20 UTC (permalink / raw) To: davej, Trond Myklebust, linux-kernel, bfields, linux-nfs, schumaker.anna, linux-fsdevel [-- Attachment #1: Type: text/plain, Size: 2322 bytes --] On Tue, Aug 1, 2017 at 8:51 AM, davej@codemonkey.org.uk <davej@codemonkey.org.uk> wrote: > On Mon, Jul 31, 2017 at 10:35:45PM -0700, Linus Torvalds wrote: > > Any chance of getting the output from > > > > ./scripts/faddr2line vmlinux nfs4_exchange_id_done+0x3d7/0x8e0 > > > Hm, that points to this.. > > 7463 /* Save the EXCHANGE_ID verifier session trunk tests */ > 7464 memcpy(clp->cl_confirm.data, cdata->args.verifier->data, > 7465 sizeof(clp->cl_confirm.data)); Ok, that certainly made no sense to me, because the KASAN report made it look like a stale pathname access (allocated in getname, freed in putname), but I think the issue is more fundamental than that. That cdata->args.verifier seems to be entirely broken. AT least for the "xprt == NULL" case, it does the following: - use the address of a local variable ("&verifier") - wait for the rpc completion using rpc_wait_for_completion_task(). That's unacceptably buggy crap. rpc_wait_for_completion_task() will happily exit on a deadly signal even if the rpc hasn't been completed, so now you'll have a stale pointer to a stack that has been freed. So I think the 'pathname' part may actually be entirely a red herring, and it's the underlying access itself that just picks up a random pointer from a stack that now contains something different. And KASAN didn't notice the stale stack access itself, because the stack slot is still valid - it's just no longer the original 'verifier' allocation. Or *something* like that. None of this looks even remotely new, though - the code seems to go back to 2009. Have you just changed what you're testing to trigger these things? I'm not even sure why it does that stupid stack allocation. It does a *real* allocation just a few lines later: struct nfs41_exchange_id_data *calldata ... calldata = kzalloc(sizeof(*calldata), GFP_NOFS); and the whole verifier structure could easily have been part of that same allocation as far as I can tell. And that really might seem to be the right thing to do. TOTALLY UNTESTED PROBABLY COMPLETE CRAP patch attatched. That patch compiles for me. It *might* even work. Or it might just be the ramblings of a diseased mind. Anna? Trond? So caveat probatorem, Linus [-- Attachment #2: patch.diff --] [-- Type: text/plain, Size: 1904 bytes --] fs/nfs/nfs4proc.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 18ca6879d8de..0712af3d38f8 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -7490,6 +7490,11 @@ static const struct rpc_call_ops nfs4_exchange_id_call_ops = { .rpc_release = nfs4_exchange_id_release, }; +struct verifier_and_calldata { + struct nfs41_exchange_id_data calldata; + nfs4_verifier verifier; +}; + /* * _nfs4_proc_exchange_id() * @@ -7498,7 +7503,8 @@ static const struct rpc_call_ops nfs4_exchange_id_call_ops = { static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred, u32 sp4_how, struct rpc_xprt *xprt) { - nfs4_verifier verifier; + struct verifier_and_calldata *vna; + nfs4_verifier *verifier; struct rpc_message msg = { .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_EXCHANGE_ID], .rpc_cred = cred, @@ -7516,14 +7522,17 @@ static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred, if (!atomic_inc_not_zero(&clp->cl_count)) return -EIO; - calldata = kzalloc(sizeof(*calldata), GFP_NOFS); - if (!calldata) { + vna = kzalloc(sizeof(*vna), GFP_NOFS); + if (!vna) { nfs_put_client(clp); return -ENOMEM; } + /* kfree() of calldata will also free the verifier */ + calldata = &vna->calldata; + verifier = &vna->verifier; if (!xprt) - nfs4_init_boot_verifier(clp, &verifier); + nfs4_init_boot_verifier(clp, verifier); status = nfs4_init_uniform_client_string(clp); if (status) @@ -7566,7 +7575,7 @@ static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred, RPC_TASK_SOFT|RPC_TASK_SOFTCONN|RPC_TASK_ASYNC; calldata->args.verifier = &clp->cl_confirm; } else { - calldata->args.verifier = &verifier; + calldata->args.verifier = verifier; } calldata->args.client = clp; #ifdef CONFIG_NFS_V4_1_MIGRATION ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-08-01 17:20 ` Linus Torvalds @ 2017-08-01 17:30 ` Trond Myklebust 2017-08-01 17:50 ` davej 2017-08-01 17:53 ` Linus Torvalds 2 siblings, 0 replies; 30+ messages in thread From: Trond Myklebust @ 2017-08-01 17:30 UTC (permalink / raw) To: torvalds, linux-kernel, bfields, linux-nfs, schumaker.anna, davej, linux-fsdevel On Tue, 2017-08-01 at 10:20 -0700, Linus Torvalds wrote: > On Tue, Aug 1, 2017 at 8:51 AM, davej@codemonkey.org.uk > <davej@codemonkey.org.uk> wrote: > > On Mon, Jul 31, 2017 at 10:35:45PM -0700, Linus Torvalds wrote: > > > Any chance of getting the output from > > > > > > ./scripts/faddr2line vmlinux > > nfs4_exchange_id_done+0x3d7/0x8e0 > > > > > > Hm, that points to this.. > > > > 7463 /* Save the EXCHANGE_ID verifier session trunk > > tests */ > > 7464 memcpy(clp->cl_confirm.data, cdata- > > >args.verifier->data, > > 7465 sizeof(clp->cl_confirm.data)); > > Ok, that certainly made no sense to me, because the KASAN report made > it look like a stale pathname access (allocated in getname, freed in > putname), but I think the issue is more fundamental than that. > > That cdata->args.verifier seems to be entirely broken. AT least for > the "xprt == NULL" case, it does the following: > > - use the address of a local variable ("&verifier") > > - wait for the rpc completion using rpc_wait_for_completion_task(). > > That's unacceptably buggy crap. rpc_wait_for_completion_task() will > happily exit on a deadly signal even if the rpc hasn't been > completed, > so now you'll have a stale pointer to a stack that has been freed. > > So I think the 'pathname' part may actually be entirely a red > herring, > and it's the underlying access itself that just picks up a random > pointer from a stack that now contains something different. And KASAN > didn't notice the stale stack access itself, because the stack slot > is > still valid - it's just no longer the original 'verifier' allocation. > > Or *something* like that. > > None of this looks even remotely new, though - the code seems to go > back to 2009. Have you just changed what you're testing to trigger > these things? > > I'm not even sure why it does that stupid stack allocation. It does a > *real* allocation just a few lines later: > > struct nfs41_exchange_id_data *calldata > ... > calldata = kzalloc(sizeof(*calldata), GFP_NOFS); > > and the whole verifier structure could easily have been part of that > same allocation as far as I can tell. > > And that really might seem to be the right thing to do. > > TOTALLY UNTESTED PROBABLY COMPLETE CRAP patch attatched. > > That patch compiles for me. It *might* even work. Or it might just be > the ramblings of a diseased mind. > > Anna? Trond? > I came to the same conclusion yesterday, and have a stable patch that does something similar. I just got distracted with the other bugs that were introduced by the exchangeid patch series in Linux-4.9 (including what looks like a duplicate free issue in nfs4_test_session_trunk()). I can pass a few of the more critical patches on to Anna for merging in this cycle, then I've got some clean ups ready for the 4.14 merge window. Cheers Trond -- Trond Myklebust Linux NFS client maintainer, PrimaryData trond.myklebust@primarydata.com ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-08-01 17:20 ` Linus Torvalds 2017-08-01 17:30 ` Trond Myklebust @ 2017-08-01 17:50 ` davej 2017-08-01 17:58 ` Trond Myklebust 2017-08-01 17:53 ` Linus Torvalds 2 siblings, 1 reply; 30+ messages in thread From: davej @ 2017-08-01 17:50 UTC (permalink / raw) To: Linus Torvalds Cc: Trond Myklebust, linux-kernel, bfields, linux-nfs, schumaker.anna, linux-fsdevel On Tue, Aug 01, 2017 at 10:20:31AM -0700, Linus Torvalds wrote: > So I think the 'pathname' part may actually be entirely a red herring, > and it's the underlying access itself that just picks up a random > pointer from a stack that now contains something different. And KASAN > didn't notice the stale stack access itself, because the stack slot is > still valid - it's just no longer the original 'verifier' allocation. > > Or *something* like that. > > None of this looks even remotely new, though - the code seems to go > back to 2009. Have you just changed what you're testing to trigger > these things? No idea why it only just showed up, but it isn't 100% reproducable either. A month or so ago I did disable the V4 code on the server completely (as I was using v3 everywhere else), so maybe I started hitting a fallback path somewhere. *shrug* Dave ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-08-01 17:50 ` davej @ 2017-08-01 17:58 ` Trond Myklebust 0 siblings, 0 replies; 30+ messages in thread From: Trond Myklebust @ 2017-08-01 17:58 UTC (permalink / raw) To: torvalds, davej Cc: bfields, linux-kernel, linux-nfs, schumaker.anna, linux-fsdevel On Tue, 2017-08-01 at 13:50 -0400, davej@codemonkey.org.uk wrote: > On Tue, Aug 01, 2017 at 10:20:31AM -0700, Linus Torvalds wrote: > > > So I think the 'pathname' part may actually be entirely a red > herring, > > and it's the underlying access itself that just picks up a random > > pointer from a stack that now contains something different. And > KASAN > > didn't notice the stale stack access itself, because the stack > slot is > > still valid - it's just no longer the original 'verifier' > allocation. > > > > Or *something* like that. > > > > None of this looks even remotely new, though - the code seems to > go > > back to 2009. Have you just changed what you're testing to trigger > > these things? > > No idea why it only just showed up, but it isn't 100% reproducable > either. A month or so ago I did disable the V4 code on the server > completely (as I was using v3 everywhere else), so maybe I started > hitting > a fallback path somewhere. *shrug* > I would only expect you too see it if you interrupt the wait on the asynchronous EXCHANGE_ID call (which would allow the RPC call to continue while the caller stack is trashed). Prior to commit 8d89bd70bc939, that code path was fully synchronous, so there was no issue with interrupting the call. -- Trond Myklebust Linux NFS client maintainer, PrimaryData trond.myklebust@primarydata.com ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [GIT PULL] Please pull NFS client changes for Linux 4.13 2017-08-01 17:20 ` Linus Torvalds 2017-08-01 17:30 ` Trond Myklebust 2017-08-01 17:50 ` davej @ 2017-08-01 17:53 ` Linus Torvalds 2 siblings, 0 replies; 30+ messages in thread From: Linus Torvalds @ 2017-08-01 17:53 UTC (permalink / raw) To: davej, Trond Myklebust, linux-kernel, bfields, linux-nfs, schumaker.anna, linux-fsdevel On Tue, Aug 1, 2017 at 10:20 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > So I think the 'pathname' part may actually be entirely a red herring, > and it's the underlying access itself that just picks up a random > pointer from a stack that now contains something different. And KASAN > didn't notice the stale stack access itself, because the stack slot is > still valid - it's just no longer the original 'verifier' allocation. > > Or *something* like that. I think the "something like that" is actually just reading the cdata->args.verifier->data pointer itself, and it *is* the stack access - but the stack page has been free'd (because of the same fatal signal that interrupted the rpc_wait_for_completion_task() call), and then re-allocated (and free'd again) as a pathname page. Maybe. Regardless, my patch still looks conceptually correct, even if it might have bugs due to total lack of testing. Linus ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2017-08-01 17:58 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-13 21:16 [GIT PULL] Please pull NFS client changes for Linux 4.13 Anna Schumaker 2017-07-13 21:43 ` Linus Torvalds 2017-07-14 7:09 ` Christoph Hellwig 2017-07-14 11:33 ` Anna Schumaker 2017-07-14 14:25 ` Dave Jones 2017-07-14 16:36 ` J. Bruce Fields 2017-07-14 19:05 ` Linus Torvalds 2017-07-14 19:43 ` Andrey Ryabinin 2017-07-14 19:58 ` Linus Torvalds 2017-07-14 20:26 ` Andrey Rybainin 2017-07-14 20:38 ` Daniel Micay 2017-07-14 20:50 ` Linus Torvalds 2017-07-14 21:01 ` Daniel Micay 2017-07-14 21:05 ` Daniel Micay 2017-07-14 20:50 ` Daniel Micay 2017-07-14 23:59 ` Daniel Micay 2017-07-14 19:48 ` Dave Jones 2017-07-16 21:15 ` Dave Jones 2017-07-16 22:57 ` Trond Myklebust 2017-07-17 3:05 ` davej 2017-07-17 19:02 ` Linus Torvalds 2017-07-18 14:20 ` [GIT PULL] Please pull an nfsd bugfix for 4.13 bfields 2017-07-31 15:43 ` [GIT PULL] Please pull NFS client changes for Linux 4.13 davej 2017-08-01 5:35 ` Linus Torvalds 2017-08-01 15:51 ` davej 2017-08-01 17:20 ` Linus Torvalds 2017-08-01 17:30 ` Trond Myklebust 2017-08-01 17:50 ` davej 2017-08-01 17:58 ` Trond Myklebust 2017-08-01 17:53 ` Linus Torvalds
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).