| Message ID | 20221017043107.never.457-kees@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] NFS: Avoid memcpy() run-time warning for struct sockaddr overflows | expand |
On Sun, Oct 16, 2022 at 09:36:50PM -0700, Kees Cook wrote: > The 'nfs_server' and 'mount_server' structures include a union of > 'struct sockaddr' (with the older 16 bytes max address size) and > 'struct sockaddr_storage' which is large enough to hold all the > supported sa_family types (128 bytes max size). The runtime memcpy() > buffer overflow checker is seeing attempts to write beyond the 16 > bytes as an overflow, but the actual expected size is that of 'struct > sockaddr_storage'. Plumb the use of 'struct sockaddr_storage' more > completely through-out NFS, which results in adjusting the memcpy() > buffers to the correct union members. Avoids this false positive run-time > warning under CONFIG_FORTIFY_SOURCE: > > memcpy: detected field-spanning write (size 28) of single field "&ctx->nfs_server.address" at fs/nfs/namespace.c:178 (size 16) > > Reported-by: kernel test robot <yujie.liu@intel.com> > Link: https://lore.kernel.org/all/202210110948.26b43120-yujie.liu@intel.com > Cc: Trond Myklebust <trond.myklebust@hammerspace.com> > Cc: Anna Schumaker <anna@kernel.org> Friendly ping -- this needs to land in v6.1 to avoid these warnings. Should I carry this in the hardening tree instead? Thanks! -Kees
> On Oct 26, 2022, at 19:32, Kees Cook <keescook@chromium.org> wrote: > > On Sun, Oct 16, 2022 at 09:36:50PM -0700, Kees Cook wrote: >> The 'nfs_server' and 'mount_server' structures include a union of >> 'struct sockaddr' (with the older 16 bytes max address size) and >> 'struct sockaddr_storage' which is large enough to hold all the >> supported sa_family types (128 bytes max size). The runtime memcpy() >> buffer overflow checker is seeing attempts to write beyond the 16 >> bytes as an overflow, but the actual expected size is that of 'struct >> sockaddr_storage'. Plumb the use of 'struct sockaddr_storage' more >> completely through-out NFS, which results in adjusting the memcpy() >> buffers to the correct union members. Avoids this false positive run-time >> warning under CONFIG_FORTIFY_SOURCE: >> >> memcpy: detected field-spanning write (size 28) of single field "&ctx->nfs_server.address" at fs/nfs/namespace.c:178 (size 16) >> >> Reported-by: kernel test robot <yujie.liu@intel.com> >> Link: https://lore.kernel.org/all/202210110948.26b43120-yujie.liu@intel.com >> Cc: Trond Myklebust <trond.myklebust@hammerspace.com> >> Cc: Anna Schumaker <anna@kernel.org> > > Friendly ping -- this needs to land in v6.1 to avoid these warnings. > Should I carry this in the hardening tree instead? > > Thanks! Anna, this is your call since you’re the ‘6.1 nfs client maintainer’...
On Thu, Oct 27, 2022 at 12:03:07AM +0000, Trond Myklebust wrote: > > > > On Oct 26, 2022, at 19:32, Kees Cook <keescook@chromium.org> wrote: > > > > On Sun, Oct 16, 2022 at 09:36:50PM -0700, Kees Cook wrote: > >> The 'nfs_server' and 'mount_server' structures include a union of > >> 'struct sockaddr' (with the older 16 bytes max address size) and > >> 'struct sockaddr_storage' which is large enough to hold all the > >> supported sa_family types (128 bytes max size). The runtime memcpy() > >> buffer overflow checker is seeing attempts to write beyond the 16 > >> bytes as an overflow, but the actual expected size is that of 'struct > >> sockaddr_storage'. Plumb the use of 'struct sockaddr_storage' more > >> completely through-out NFS, which results in adjusting the memcpy() > >> buffers to the correct union members. Avoids this false positive run-time > >> warning under CONFIG_FORTIFY_SOURCE: > >> > >> memcpy: detected field-spanning write (size 28) of single field "&ctx->nfs_server.address" at fs/nfs/namespace.c:178 (size 16) > >> > >> Reported-by: kernel test robot <yujie.liu@intel.com> > >> Link: https://lore.kernel.org/all/202210110948.26b43120-yujie.liu@intel.com > >> Cc: Trond Myklebust <trond.myklebust@hammerspace.com> > >> Cc: Anna Schumaker <anna@kernel.org> > > > > Friendly ping -- this needs to land in v6.1 to avoid these warnings. > > Should I carry this in the hardening tree instead? > > > > Thanks! > > Anna, this is your call since you’re the ‘6.1 nfs client maintainer’... And just to remind, this came up in -rc1 as well: https://lore.kernel.org/lkml/202210162144.76FBC7271@keescook/
Hi all,
Reviewed-by: Petr Vorel <pvorel@suse.cz>
It looks like it fixed all kernel oops triggered when running LTP NFS tests
(net.nfs runtest file).
First one was reported by Dave Jones
(https://lore.kernel.org/all/Y0zEzZwhOxTDcBTB@codemonkey.org.uk/)
and both were addressed in by this patchset.
BTW There are some other errors lockd related (nfslock* tests), I'll investigate
them during this week:
[ 967.545642] lockd: cannot monitor 10.0.0.2
[ 974.016225] xs_tcp_setup_socket: connect returned unhandled error -107
Thanks Kees!
Kind regards,
Petr
[ 125.774063] ------------[ cut here ]------------
[ 125.775460] memcpy: detected field-spanning write (size 28) of single field "request.sap" at fs/nfs/super.c:857 (size 18446744073709551615)
[ 125.777904] WARNING: CPU: 0 PID: 3217 at fs/nfs/super.c:857 nfs_request_mount.constprop.0.isra.0+0x1cf/0x1e0 [nfs]
[ 125.779823] Modules linked in: rpcsec_gss_krb5(E) nfsv4(E) dns_resolver(E) nfsv3(E) nfs(E) fscache(E) netfs(E) xfrm_user(E) xfrm_algo(E) veth(E) af_packet(E) rfkill(E) qrtr(E) intel_rapl_msr(E) intel_rapl_common(E) intel_pmc_core_pltdrv(E) intel_pmc_core(E) kvm_intel(E) snd_hda_codec_generic(E) ledtrig_audio(E) snd_hda_intel(E) snd_intel_dspcfg(E) snd_intel_sdw_acpi(E) iTCO_wdt(E) intel_pmc_bxt(E) iTCO_vendor_support(E) snd_hda_codec(E) kvm(E) irqbypass(E) pcspkr(E) snd_hda_core(E) snd_hwdep(E) snd_pcm(E) snd_timer(E) joydev(E) i2c_i801(E) lpc_ich(E) i2c_smbus(E) snd(E) soundcore(E) virtio_net(E) virtio_balloon(E) net_failover(E) failover(E) tiny_power_button(E) button(E) nfsd(E) auth_rpcgss(E) nfs_acl(E) lockd(E) grace(E) fuse(E) configfs(E) sunrpc(E) ip_tables(E) x_tables(E) hid_generic(E) usbhid(E) crct10dif_pclmul(E) crc32_pclmul(E) polyval_clmulni(E) polyval_generic(E) gf128mul(E) ghash_clmulni_intel(E) sha512_ssse3(E) aesni_intel(E) xhci_pci(E) crypto_simd(E)
[ 125.779892] xhci_pci_renesas(E) cryptd(E) serio_raw(E) xhci_hcd(E) virtio_rng(E) virtio_blk(E) usbcore(E) virtio_gpu(E) virtio_dma_buf(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_intel(E) xor(E) raid6_pq(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) qemu_fw_cfg(E)
[ 125.793324] Unloaded tainted modules: intel_tcc_cooling(E):2 pcc_cpufreq(E):2 acpi_cpufreq(E):2
[ 125.799336] CPU: 0 PID: 3217 Comm: mount.nfs Tainted: G E 6.1.0-rc3-0.g3b74e67-default #1 openSUSE Tumbleweed (unreleased) 83d9036cc26826a41b4e9f7bea95f22c34b218fc
[ 125.802007] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.0-0-gd239552-rebuilt.opensuse.org 04/01/2014
[ 125.804599] RIP: 0010:nfs_request_mount.constprop.0.isra.0+0x1cf/0x1e0 [nfs]
[ 125.806374] Code: 0f 85 2d ff ff ff 48 c7 c1 ff ff ff ff 48 c7 c2 80 4e da c0 48 89 ee 48 c7 c7 b0 4e da c0 c6 05 68 91 04 00 01 e8 43 e8 d7 ca <0f> 0b 48 8b 3c 24 e9 fe fe ff ff e8 91 01 dd ca 90 0f 1f 44 00 00
[ 125.809487] RSP: 0018:ffffac0a0157fce0 EFLAGS: 00010282
[ 125.810651] RAX: 0000000000000000 RBX: ffff9819c3cd5c00 RCX: 0000000000000027
[ 125.811999] RDX: ffff981a1e4224c8 RSI: 0000000000000001 RDI: ffff981a1e4224c0
[ 125.813296] RBP: 000000000000001c R08: 0000000000000000 R09: ffffac0a0157fb88
[ 125.814622] R10: 0000000000000003 R11: ffffffff8c958348 R12: ffff9819c3cd5d30
[ 125.815971] R13: ffff9819c64c1340 R14: ffff9819c39ae0c0 R15: 0000000000000001
[ 125.817269] FS: 00007f0283539100(0000) GS:ffff981a1e400000(0000) knlGS:0000000000000000
[ 125.818816] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 125.820120] CR2: 00005580d080bc68 CR3: 0000000003c6c003 CR4: 0000000000370ef0
[ 125.821557] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 125.822924] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 125.824417] Call Trace:
[ 125.825286] <TASK>
[ 125.826030] nfs_try_get_tree+0xa7/0x200 [nfs 25f29e807864c924ae56dc3fe894ed7d36f5a514]
[ 125.827520] ? nfs_get_tree+0x1ec/0x6b0 [nfs 25f29e807864c924ae56dc3fe894ed7d36f5a514]
[ 125.829262] ? __kmalloc_node_track_caller+0x48/0x150
[ 125.830466] ? get_nfs_version+0x5f/0x130 [nfs 25f29e807864c924ae56dc3fe894ed7d36f5a514]
[ 125.831941] ? nfs_get_tree+0x447/0x6b0 [nfs 25f29e807864c924ae56dc3fe894ed7d36f5a514]
[ 125.833408] vfs_get_tree+0x22/0xc0
[ 125.834446] path_mount+0x458/0xa60
[ 125.835614] __x64_sys_mount+0x105/0x140
[ 125.836786] do_syscall_64+0x58/0x80
[ 125.837681] ? exit_to_user_mode_prepare+0x166/0x1f0
[ 125.838793] ? syscall_exit_to_user_mode+0x17/0x40
[ 125.840130] ? do_syscall_64+0x67/0x80
[ 125.841241] ? do_syscall_64+0x67/0x80
[ 125.842641] ? exc_page_fault+0x66/0x150
[ 125.844412] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ 125.845895] RIP: 0033:0x7f02838f268e
[ 125.846862] Code: 48 8b 0d 85 e7 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 52 e7 0c 00 f7 d8 64 89 01 48
[ 125.850094] RSP: 002b:00007ffee724dfd8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
[ 125.851555] RAX: ffffffffffffffda RBX: 00007ffee724e1f0 RCX: 00007f02838f268e
[ 125.852917] RDX: 0000563edbf74070 RSI: 0000563edbf74ed0 RDI: 0000563edbf74f40
[ 125.854350] RBP: 0000563edbf99f70 R08: 0000563edbf99f70 R09: 0000563edbf99f70
[ 125.855671] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffee724e1f0
[ 125.857006] R13: 00007ffee724e0d0 R14: 0000563edbf99ed0 R15: 0000563edbf77170
[ 125.858302] </TASK>
[ 125.859007] ---[ end trace 0000000000000000 ]---
export LTPROOT=/opt/ltp; runltp-ng -r net.nfs
[ 84.451572] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.0-0-gd239552-rebuilt.opensuse.org 04/01/2014
[ 84.454466] RIP: 0010:nfs_d_automount+0x24c/0x290 [nfs]
[ 84.456065] Code: 10 00 00 00 4c 89 ee 4c 89 04 24 48 c7 c2 48 52 da c0 48 c7 c7 88 52 da c0 48 89 44 24 08 c6 05 8c d3 03 00 01 e8 66 2a 77 ce <0f> 0b 48 8b 44 24 08 4c 8b 04 24 e9 b7 fe ff ff be 01 00 00 00 e8
[ 84.459311] RSP: 0018:ffffa50b811039f0 EFLAGS: 00010286
[ 84.460532] RAX: 0000000000000000 RBX: ffff88ad83696000 RCX: 0000000000000000
[ 84.461886] RDX: ffff88adde42efa8 RSI: ffff88adde4224c0 RDI: ffff88adde4224c0
[ 84.463367] RBP: ffff88ad9c862240 R08: 0000000000000000 R09: ffffa50b81103898
[ 84.464727] R10: 0000000000000003 R11: ffffffff90358348 R12: 000000000001e848
[ 84.466115] R13: 000000000000001c R14: ffff88ad82f09000 R15: ffff88ad83694c00
[ 84.467942] FS: 00007f46a8761100(0000) GS:ffff88adde400000(0000) knlGS:0000000000000000
[ 84.469531] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 84.470678] CR2: 0000555c1ce960d8 CR3: 0000000005782006 CR4: 0000000000370ef0
[ 84.471999] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 84.473420] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 84.474894] Call Trace:
[ 84.475701] <TASK>
[ 84.476446] __traverse_mounts+0x8c/0x210
[ 84.477494] step_into+0x339/0x760
[ 84.478461] link_path_walk.part.0.constprop.0+0x246/0x3b0
[ 84.479583] ? path_init+0x5c/0x3c0
[ 84.480479] path_lookupat+0x3e/0x190
[ 84.481439] filename_lookup+0xd0/0x1d0
[ 84.482341] ? getname_kernel+0x25/0x110
[ 84.483262] ? kmem_cache_alloc+0x16a/0x360
[ 84.484258] ? alloc_mnt_ns+0x57/0x170
[ 84.485522] vfs_path_lookup+0x4e/0x80
[ 84.486628] mount_subtree+0x8b/0x130
[ 84.487542] do_nfs4_mount+0x269/0x360 [nfsv4 59ca3c817e039994690a920010d1ce7d3d851090]
[ 84.488996] nfs4_try_get_tree+0x44/0xb0 [nfsv4 59ca3c817e039994690a920010d1ce7d3d851090]
[ 84.490854] vfs_get_tree+0x22/0xc0
[ 84.491735] path_mount+0x458/0xa60
[ 84.492588] __x64_sys_mount+0x105/0x140
[ 84.493513] do_syscall_64+0x58/0x80
[ 84.494343] ? exit_to_user_mode_prepare+0x166/0x1f0
[ 84.495328] ? syscall_exit_to_user_mode+0x17/0x40
[ 84.496318] ? do_syscall_64+0x67/0x80
[ 84.497737] ? syscall_exit_to_user_mode+0x17/0x40
[ 84.498716] ? do_syscall_64+0x67/0x80
[ 84.499634] ? do_syscall_64+0x67/0x80
[ 84.500462] ? do_user_addr_fault+0x1dc/0x690
[ 84.501534] ? exc_page_fault+0x66/0x150
[ 84.502373] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ 84.503350] RIP: 0033:0x7f46a8b1a68e
[ 84.504098] Code: 48 8b 0d 85 e7 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 52 e7 0c 00 f7 d8 64 89 01 48
[ 84.507186] RSP: 002b:00007ffd9cc0c9c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
[ 84.508695] RAX: ffffffffffffffda RBX: 00007ffd9cc0cbf0 RCX: 00007f46a8b1a68e
[ 84.509955] RDX: 000055f08e126070 RSI: 000055f08e126ed0 RDI: 000055f08e126f40
[ 84.511158] RBP: 000055f08e129400 R08: 000055f08e129400 R09: 00313a3a313a313a
[ 84.512318] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffd9cc0cbf0
[ 84.513522] R13: 000055f08e129120 R14: 0000000000000005 R15: 000055f08dfdc8f8
[ 84.514798] </TASK>
[ 84.515652] ---[ end trace 0000000000000000 ]---
...
[ 84.451572] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.0-0-gd239552-rebuilt.opensuse.org 04/01/2014
[ 84.454466] RIP: 0010:nfs_d_automount+0x24c/0x290 [nfs]
[ 84.456065] Code: 10 00 00 00 4c 89 ee 4c 89 04 24 48 c7 c2 48 52 da c0 48 c7 c7 88 52 da c0 48 89 44 24 08 c6 05 8c d3 03 00 01 e8 66 2a 77 ce <0f> 0b 48 8b 44 24 08 4c 8b 04 24 e9 b7 fe ff ff be 01 00 00 00 e8
[ 84.459311] RSP: 0018:ffffa50b811039f0 EFLAGS: 00010286
[ 84.460532] RAX: 0000000000000000 RBX: ffff88ad83696000 RCX: 0000000000000000
[ 84.461886] RDX: ffff88adde42efa8 RSI: ffff88adde4224c0 RDI: ffff88adde4224c0
[ 84.463367] RBP: ffff88ad9c862240 R08: 0000000000000000 R09: ffffa50b81103898
[ 84.464727] R10: 0000000000000003 R11: ffffffff90358348 R12: 000000000001e848
[ 84.466115] R13: 000000000000001c R14: ffff88ad82f09000 R15: ffff88ad83694c00
[ 84.467942] FS: 00007f46a8761100(0000) GS:ffff88adde400000(0000) knlGS:0000000000000000
[ 84.469531] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 84.470678] CR2: 0000555c1ce960d8 CR3: 0000000005782006 CR4: 0000000000370ef0
[ 84.471999] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 84.473420] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 84.474894] Call Trace:
[ 84.475701] <TASK>
[ 84.476446] __traverse_mounts+0x8c/0x210
[ 84.477494] step_into+0x339/0x760
[ 84.478461] link_path_walk.part.0.constprop.0+0x246/0x3b0
[ 84.479583] ? path_init+0x5c/0x3c0
[ 84.480479] path_lookupat+0x3e/0x190
[ 84.481439] filename_lookup+0xd0/0x1d0
[ 84.482341] ? getname_kernel+0x25/0x110
[ 84.483262] ? kmem_cache_alloc+0x16a/0x360
[ 84.484258] ? alloc_mnt_ns+0x57/0x170
[ 84.485522] vfs_path_lookup+0x4e/0x80
[ 84.486628] mount_subtree+0x8b/0x130
[ 84.487542] do_nfs4_mount+0x269/0x360 [nfsv4 59ca3c817e039994690a920010d1ce7d3d851090]
[ 84.488996] nfs4_try_get_tree+0x44/0xb0 [nfsv4 59ca3c817e039994690a920010d1ce7d3d851090]
[ 84.490854] vfs_get_tree+0x22/0xc0
[ 84.491735] path_mount+0x458/0xa60
[ 84.492588] __x64_sys_mount+0x105/0x140
[ 84.493513] do_syscall_64+0x58/0x80
[ 84.494343] ? exit_to_user_mode_prepare+0x166/0x1f0
[ 84.495328] ? syscall_exit_to_user_mode+0x17/0x40
[ 84.496318] ? do_syscall_64+0x67/0x80
[ 84.497737] ? syscall_exit_to_user_mode+0x17/0x40
[ 84.498716] ? do_syscall_64+0x67/0x80
[ 84.499634] ? do_syscall_64+0x67/0x80
[ 84.500462] ? do_user_addr_fault+0x1dc/0x690
[ 84.501534] ? exc_page_fault+0x66/0x150
[ 84.502373] entry_SYSCALL_64_after_hwframe+0x63/0xcd
[ 84.503350] RIP: 0033:0x7f46a8b1a68e
[ 84.504098] Code: 48 8b 0d 85 e7 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 52 e7 0c 00 f7 d8 64 89 01 48
[ 84.507186] RSP: 002b:00007ffd9cc0c9c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
[ 84.508695] RAX: ffffffffffffffda RBX: 00007ffd9cc0cbf0 RCX: 00007f46a8b1a68e
[ 84.509955] RDX: 000055f08e126070 RSI: 000055f08e126ed0 RDI: 000055f08e126f40
[ 84.511158] RBP: 000055f08e129400 R08: 000055f08e129400 R09: 00313a3a313a313a
[ 84.512318] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffd9cc0cbf0
[ 84.513522] R13: 000055f08e129120 R14: 0000000000000005 R15: 000055f08dfdc8f8
[ 84.514798] </TASK>
[ 84.515652] ---[ end trace 0000000000000000 ]---
diff --git a/fs/nfs/client.c b/fs/nfs/client.c index da8da5cdbbc1..f50e025ae406 100644 --- a/fs/nfs/client.c +++ b/fs/nfs/client.c @@ -280,7 +280,7 @@ EXPORT_SYMBOL_GPL(nfs_put_client); static struct nfs_client *nfs_match_client(const struct nfs_client_initdata *data) { struct nfs_client *clp; - const struct sockaddr *sap = data->addr; + const struct sockaddr *sap = (struct sockaddr *)data->addr; struct nfs_net *nn = net_generic(data->net, nfs_net_id); int error; @@ -666,7 +666,7 @@ static int nfs_init_server(struct nfs_server *server, struct rpc_timeout timeparms; struct nfs_client_initdata cl_init = { .hostname = ctx->nfs_server.hostname, - .addr = (const struct sockaddr *)&ctx->nfs_server.address, + .addr = &ctx->nfs_server._address, .addrlen = ctx->nfs_server.addrlen, .nfs_mod = ctx->nfs_mod, .proto = ctx->nfs_server.protocol, diff --git a/fs/nfs/dns_resolve.c b/fs/nfs/dns_resolve.c index e87d500ad95a..6603b5cee029 100644 --- a/fs/nfs/dns_resolve.c +++ b/fs/nfs/dns_resolve.c @@ -16,8 +16,9 @@ #include "dns_resolve.h" ssize_t nfs_dns_resolve_name(struct net *net, char *name, size_t namelen, - struct sockaddr *sa, size_t salen) + struct sockaddr_storage *ss, size_t salen) { + struct sockaddr *sa = (struct sockaddr *)ss; ssize_t ret; char *ip_addr = NULL; int ip_len; @@ -341,7 +342,7 @@ static int do_cache_lookup_wait(struct cache_detail *cd, } ssize_t nfs_dns_resolve_name(struct net *net, char *name, - size_t namelen, struct sockaddr *sa, size_t salen) + size_t namelen, struct sockaddr_storage *ss, size_t salen) { struct nfs_dns_ent key = { .hostname = name, @@ -354,7 +355,7 @@ ssize_t nfs_dns_resolve_name(struct net *net, char *name, ret = do_cache_lookup_wait(nn->nfs_dns_resolve, &key, &item); if (ret == 0) { if (salen >= item->addrlen) { - memcpy(sa, &item->addr, item->addrlen); + memcpy(ss, &item->addr, item->addrlen); ret = item->addrlen; } else ret = -EOVERFLOW; diff --git a/fs/nfs/dns_resolve.h b/fs/nfs/dns_resolve.h index 576ff4b54c82..fe3b172c4de1 100644 --- a/fs/nfs/dns_resolve.h +++ b/fs/nfs/dns_resolve.h @@ -32,6 +32,6 @@ extern void nfs_dns_resolver_cache_destroy(struct net *net); #endif extern ssize_t nfs_dns_resolve_name(struct net *net, char *name, - size_t namelen, struct sockaddr *sa, size_t salen); + size_t namelen, struct sockaddr_storage *sa, size_t salen); #endif diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c index 4da701fd1424..09833ec102fc 100644 --- a/fs/nfs/fs_context.c +++ b/fs/nfs/fs_context.c @@ -273,9 +273,9 @@ static const struct constant_table nfs_secflavor_tokens[] = { * Address family must be initialized, and address must not be * the ANY address for that family. */ -static int nfs_verify_server_address(struct sockaddr *addr) +static int nfs_verify_server_address(struct sockaddr_storage *addr) { - switch (addr->sa_family) { + switch (addr->ss_family) { case AF_INET: { struct sockaddr_in *sa = (struct sockaddr_in *)addr; return sa->sin_addr.s_addr != htonl(INADDR_ANY); @@ -969,7 +969,7 @@ static int nfs23_parse_monolithic(struct fs_context *fc, { struct nfs_fs_context *ctx = nfs_fc2context(fc); struct nfs_fh *mntfh = ctx->mntfh; - struct sockaddr *sap = (struct sockaddr *)&ctx->nfs_server.address; + struct sockaddr_storage *sap = &ctx->nfs_server._address; int extra_flags = NFS_MOUNT_LEGACY_INTERFACE; int ret; @@ -1044,7 +1044,7 @@ static int nfs23_parse_monolithic(struct fs_context *fc, memcpy(sap, &data->addr, sizeof(data->addr)); ctx->nfs_server.addrlen = sizeof(data->addr); ctx->nfs_server.port = ntohs(data->addr.sin_port); - if (sap->sa_family != AF_INET || + if (sap->ss_family != AF_INET || !nfs_verify_server_address(sap)) goto out_no_address; @@ -1200,7 +1200,7 @@ static int nfs4_parse_monolithic(struct fs_context *fc, struct nfs4_mount_data *data) { struct nfs_fs_context *ctx = nfs_fc2context(fc); - struct sockaddr *sap = (struct sockaddr *)&ctx->nfs_server.address; + struct sockaddr_storage *sap = &ctx->nfs_server._address; int ret; char *c; @@ -1314,7 +1314,7 @@ static int nfs_fs_context_validate(struct fs_context *fc) { struct nfs_fs_context *ctx = nfs_fc2context(fc); struct nfs_subversion *nfs_mod; - struct sockaddr *sap = (struct sockaddr *)&ctx->nfs_server.address; + struct sockaddr_storage *sap = &ctx->nfs_server._address; int max_namelen = PAGE_SIZE; int max_pathlen = NFS_MAXPATHLEN; int port = 0; @@ -1540,7 +1540,7 @@ static int nfs_init_fs_context(struct fs_context *fc) ctx->version = nfss->nfs_client->rpc_ops->version; ctx->minorversion = nfss->nfs_client->cl_minorversion; - memcpy(&ctx->nfs_server.address, &nfss->nfs_client->cl_addr, + memcpy(&ctx->nfs_server._address, &nfss->nfs_client->cl_addr, ctx->nfs_server.addrlen); if (fc->net_ns != net) { diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h index d914d609b85b..647fc3f547cb 100644 --- a/fs/nfs/internal.h +++ b/fs/nfs/internal.h @@ -69,7 +69,7 @@ static inline fmode_t flags_to_mode(int flags) struct nfs_client_initdata { unsigned long init_flags; const char *hostname; /* Hostname of the server */ - const struct sockaddr *addr; /* Address of the server */ + const struct sockaddr_storage *addr; /* Address of the server */ const char *nodename; /* Hostname of the client */ const char *ip_addr; /* IP address of the client */ size_t addrlen; @@ -180,7 +180,7 @@ static inline struct nfs_fs_context *nfs_fc2context(const struct fs_context *fc) /* mount_clnt.c */ struct nfs_mount_request { - struct sockaddr *sap; + struct sockaddr_storage *sap; size_t salen; char *hostname; char *dirpath; @@ -223,7 +223,7 @@ extern void nfs4_server_set_init_caps(struct nfs_server *); extern struct nfs_server *nfs4_create_server(struct fs_context *); extern struct nfs_server *nfs4_create_referral_server(struct fs_context *); extern int nfs4_update_server(struct nfs_server *server, const char *hostname, - struct sockaddr *sap, size_t salen, + struct sockaddr_storage *sap, size_t salen, struct net *net); extern void nfs_free_server(struct nfs_server *server); extern struct nfs_server *nfs_clone_server(struct nfs_server *, @@ -235,7 +235,7 @@ extern int nfs_client_init_status(const struct nfs_client *clp); extern int nfs_wait_client_init_complete(const struct nfs_client *clp); extern void nfs_mark_client_ready(struct nfs_client *clp, int state); extern struct nfs_client *nfs4_set_ds_client(struct nfs_server *mds_srv, - const struct sockaddr *ds_addr, + const struct sockaddr_storage *ds_addr, int ds_addrlen, int ds_proto, unsigned int ds_timeo, unsigned int ds_retrans, @@ -243,7 +243,7 @@ extern struct nfs_client *nfs4_set_ds_client(struct nfs_server *mds_srv, extern struct rpc_clnt *nfs4_find_or_create_ds_client(struct nfs_client *, struct inode *); extern struct nfs_client *nfs3_set_ds_client(struct nfs_server *mds_srv, - const struct sockaddr *ds_addr, int ds_addrlen, + const struct sockaddr_storage *ds_addr, int ds_addrlen, int ds_proto, unsigned int ds_timeo, unsigned int ds_retrans); #ifdef CONFIG_PROC_FS @@ -894,13 +894,13 @@ static inline bool nfs_error_is_fatal_on_server(int err) * Select between a default port value and a user-specified port value. * If a zero value is set, then autobind will be used. */ -static inline void nfs_set_port(struct sockaddr *sap, int *port, +static inline void nfs_set_port(struct sockaddr_storage *sap, int *port, const unsigned short default_port) { if (*port == NFS_UNSPEC_PORT) *port = default_port; - rpc_set_port(sap, *port); + rpc_set_port((struct sockaddr *)sap, *port); } struct nfs_direct_req { diff --git a/fs/nfs/mount_clnt.c b/fs/nfs/mount_clnt.c index c5e3b6b3366a..68e76b626371 100644 --- a/fs/nfs/mount_clnt.c +++ b/fs/nfs/mount_clnt.c @@ -158,7 +158,7 @@ int nfs_mount(struct nfs_mount_request *info, int timeo, int retrans) struct rpc_create_args args = { .net = info->net, .protocol = info->protocol, - .address = info->sap, + .address = (struct sockaddr *)info->sap, .addrsize = info->salen, .timeout = &mnt_timeout, .servername = info->hostname, @@ -245,7 +245,7 @@ void nfs_umount(const struct nfs_mount_request *info) struct rpc_create_args args = { .net = info->net, .protocol = IPPROTO_UDP, - .address = info->sap, + .address = (struct sockaddr *)info->sap, .addrsize = info->salen, .timeout = &nfs_umnt_timeout, .servername = info->hostname, diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c index 3295af4110f1..2f336ace7555 100644 --- a/fs/nfs/namespace.c +++ b/fs/nfs/namespace.c @@ -175,7 +175,7 @@ struct vfsmount *nfs_d_automount(struct path *path) } /* for submounts we want the same server; referrals will reassign */ - memcpy(&ctx->nfs_server.address, &client->cl_addr, client->cl_addrlen); + memcpy(&ctx->nfs_server._address, &client->cl_addr, client->cl_addrlen); ctx->nfs_server.addrlen = client->cl_addrlen; ctx->nfs_server.port = server->port; diff --git a/fs/nfs/nfs3client.c b/fs/nfs/nfs3client.c index b49359afac88..669cda757a5c 100644 --- a/fs/nfs/nfs3client.c +++ b/fs/nfs/nfs3client.c @@ -78,7 +78,7 @@ struct nfs_server *nfs3_clone_server(struct nfs_server *source, * the MDS. */ struct nfs_client *nfs3_set_ds_client(struct nfs_server *mds_srv, - const struct sockaddr *ds_addr, int ds_addrlen, + const struct sockaddr_storage *ds_addr, int ds_addrlen, int ds_proto, unsigned int ds_timeo, unsigned int ds_retrans) { struct rpc_timeout ds_timeout; @@ -98,7 +98,7 @@ struct nfs_client *nfs3_set_ds_client(struct nfs_server *mds_srv, char buf[INET6_ADDRSTRLEN + 1]; /* fake a hostname because lockd wants it */ - if (rpc_ntop(ds_addr, buf, sizeof(buf)) <= 0) + if (rpc_ntop((struct sockaddr *)ds_addr, buf, sizeof(buf)) <= 0) return ERR_PTR(-EINVAL); cl_init.hostname = buf; diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h index 400a71e75238..cfef738d765e 100644 --- a/fs/nfs/nfs4_fs.h +++ b/fs/nfs/nfs4_fs.h @@ -281,7 +281,7 @@ struct rpc_clnt *nfs4_negotiate_security(struct rpc_clnt *, struct inode *, int nfs4_submount(struct fs_context *, struct nfs_server *); int nfs4_replace_transport(struct nfs_server *server, const struct nfs4_fs_locations *locations); -size_t nfs_parse_server_name(char *string, size_t len, struct sockaddr *sa, +size_t nfs_parse_server_name(char *string, size_t len, struct sockaddr_storage *ss, size_t salen, struct net *net, int port); /* nfs4proc.c */ extern int nfs4_handle_exception(struct nfs_server *, int, struct nfs4_exception *); diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c index 7a5162afa5c0..3b9b53481309 100644 --- a/fs/nfs/nfs4client.c +++ b/fs/nfs/nfs4client.c @@ -889,7 +889,7 @@ nfs4_find_client_sessionid(struct net *net, const struct sockaddr *addr, */ static int nfs4_set_client(struct nfs_server *server, const char *hostname, - const struct sockaddr *addr, + const struct sockaddr_storage *addr, const size_t addrlen, const char *ip_addr, int proto, const struct rpc_timeout *timeparms, @@ -924,7 +924,7 @@ static int nfs4_set_client(struct nfs_server *server, __set_bit(NFS_CS_MIGRATION, &cl_init.init_flags); if (test_bit(NFS_MIG_TSM_POSSIBLE, &server->mig_status)) __set_bit(NFS_CS_TSM_POSSIBLE, &cl_init.init_flags); - server->port = rpc_get_port(addr); + server->port = rpc_get_port((struct sockaddr *)addr); /* Allocate or find a client reference we can use */ clp = nfs_get_client(&cl_init); @@ -960,7 +960,7 @@ static int nfs4_set_client(struct nfs_server *server, * the MDS. */ struct nfs_client *nfs4_set_ds_client(struct nfs_server *mds_srv, - const struct sockaddr *ds_addr, int ds_addrlen, + const struct sockaddr_storage *ds_addr, int ds_addrlen, int ds_proto, unsigned int ds_timeo, unsigned int ds_retrans, u32 minor_version) { @@ -980,7 +980,7 @@ struct nfs_client *nfs4_set_ds_client(struct nfs_server *mds_srv, }; char buf[INET6_ADDRSTRLEN + 1]; - if (rpc_ntop(ds_addr, buf, sizeof(buf)) <= 0) + if (rpc_ntop((struct sockaddr *)ds_addr, buf, sizeof(buf)) <= 0) return ERR_PTR(-EINVAL); cl_init.hostname = buf; @@ -1148,7 +1148,7 @@ static int nfs4_init_server(struct nfs_server *server, struct fs_context *fc) /* Get a client record */ error = nfs4_set_client(server, ctx->nfs_server.hostname, - &ctx->nfs_server.address, + &ctx->nfs_server._address, ctx->nfs_server.addrlen, ctx->client_address, ctx->nfs_server.protocol, @@ -1238,7 +1238,7 @@ struct nfs_server *nfs4_create_referral_server(struct fs_context *fc) rpc_set_port(&ctx->nfs_server.address, NFS_RDMA_PORT); error = nfs4_set_client(server, ctx->nfs_server.hostname, - &ctx->nfs_server.address, + &ctx->nfs_server._address, ctx->nfs_server.addrlen, parent_client->cl_ipaddr, XPRT_TRANSPORT_RDMA, @@ -1254,7 +1254,7 @@ struct nfs_server *nfs4_create_referral_server(struct fs_context *fc) rpc_set_port(&ctx->nfs_server.address, NFS_PORT); error = nfs4_set_client(server, ctx->nfs_server.hostname, - &ctx->nfs_server.address, + &ctx->nfs_server._address, ctx->nfs_server.addrlen, parent_client->cl_ipaddr, XPRT_TRANSPORT_TCP, @@ -1303,14 +1303,14 @@ struct nfs_server *nfs4_create_referral_server(struct fs_context *fc) * Returns zero on success, or a negative errno value. */ int nfs4_update_server(struct nfs_server *server, const char *hostname, - struct sockaddr *sap, size_t salen, struct net *net) + struct sockaddr_storage *sap, size_t salen, struct net *net) { struct nfs_client *clp = server->nfs_client; struct rpc_clnt *clnt = server->client; struct xprt_create xargs = { .ident = clp->cl_proto, .net = net, - .dstaddr = sap, + .dstaddr = (struct sockaddr *)sap, .addrlen = salen, .servername = hostname, }; diff --git a/fs/nfs/nfs4namespace.c b/fs/nfs/nfs4namespace.c index f2dbf904c598..9a98595bb160 100644 --- a/fs/nfs/nfs4namespace.c +++ b/fs/nfs/nfs4namespace.c @@ -164,16 +164,17 @@ static int nfs4_validate_fspath(struct dentry *dentry, return 0; } -size_t nfs_parse_server_name(char *string, size_t len, struct sockaddr *sa, +size_t nfs_parse_server_name(char *string, size_t len, struct sockaddr_storage *ss, size_t salen, struct net *net, int port) { + struct sockaddr *sa = (struct sockaddr *)ss; ssize_t ret; ret = rpc_pton(net, string, len, sa, salen); if (ret == 0) { ret = rpc_uaddr2sockaddr(net, string, len, sa, salen); if (ret == 0) { - ret = nfs_dns_resolve_name(net, string, len, sa, salen); + ret = nfs_dns_resolve_name(net, string, len, ss, salen); if (ret < 0) ret = 0; } @@ -331,7 +332,7 @@ static int try_location(struct fs_context *fc, ctx->nfs_server.addrlen = nfs_parse_server_name(buf->data, buf->len, - &ctx->nfs_server.address, + &ctx->nfs_server._address, sizeof(ctx->nfs_server._address), fc->net_ns, 0); if (ctx->nfs_server.addrlen == 0) @@ -483,14 +484,13 @@ static int nfs4_try_replacing_one_location(struct nfs_server *server, char *page, char *page2, const struct nfs4_fs_location *location) { - const size_t addr_bufsize = sizeof(struct sockaddr_storage); struct net *net = rpc_net_ns(server->client); - struct sockaddr *sap; + struct sockaddr_storage *sap; unsigned int s; size_t salen; int error; - sap = kmalloc(addr_bufsize, GFP_KERNEL); + sap = kmalloc(sizeof(*sap), GFP_KERNEL); if (sap == NULL) return -ENOMEM; @@ -506,10 +506,10 @@ static int nfs4_try_replacing_one_location(struct nfs_server *server, continue; salen = nfs_parse_server_name(buf->data, buf->len, - sap, addr_bufsize, net, 0); + sap, sizeof(*sap), net, 0); if (salen == 0) continue; - rpc_set_port(sap, NFS_PORT); + rpc_set_port((struct sockaddr *)sap, NFS_PORT); error = -ENOMEM; hostname = kmemdup_nul(buf->data, buf->len, GFP_KERNEL); diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index e2efcd26336c..4c4df7f213b6 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -3951,7 +3951,7 @@ static void test_fs_location_for_trunking(struct nfs4_fs_location *location, for (i = 0; i < location->nservers; i++) { struct nfs4_string *srv_loc = &location->servers[i]; - struct sockaddr addr; + struct sockaddr_storage addr; size_t addrlen; struct xprt_create xprt_args = { .ident = 0, @@ -3974,7 +3974,7 @@ static void test_fs_location_for_trunking(struct nfs4_fs_location *location, clp->cl_net, server->port); if (!addrlen) return; - xprt_args.dstaddr = &addr; + xprt_args.dstaddr = (struct sockaddr *)&addr; xprt_args.addrlen = addrlen; servername = kmalloc(srv_loc->len + 1, GFP_KERNEL); if (!servername) diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c index 987c88ddeaf0..5d035dd2d7bf 100644 --- a/fs/nfs/pnfs_nfs.c +++ b/fs/nfs/pnfs_nfs.c @@ -821,7 +821,7 @@ static void nfs4_clear_ds_conn_bit(struct nfs4_pnfs_ds *ds) static struct nfs_client *(*get_v3_ds_connect)( struct nfs_server *mds_srv, - const struct sockaddr *ds_addr, + const struct sockaddr_storage *ds_addr, int ds_addrlen, int ds_proto, unsigned int ds_timeo, @@ -882,7 +882,7 @@ static int _nfs4_pnfs_v3_ds_connect(struct nfs_server *mds_srv, continue; } clp = get_v3_ds_connect(mds_srv, - (struct sockaddr *)&da->da_addr, + &da->da_addr, da->da_addrlen, da->da_transport, timeo, retrans); if (IS_ERR(clp)) @@ -951,7 +951,7 @@ static int _nfs4_pnfs_v4_ds_connect(struct nfs_server *mds_srv, put_cred(xprtdata.cred); } else { clp = nfs4_set_ds_client(mds_srv, - (struct sockaddr *)&da->da_addr, + &da->da_addr, da->da_addrlen, da->da_transport, timeo, retrans, minor_version); diff --git a/fs/nfs/super.c b/fs/nfs/super.c index ee66ffdb985e..05ae23657527 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -822,8 +822,7 @@ static int nfs_request_mount(struct fs_context *fc, { struct nfs_fs_context *ctx = nfs_fc2context(fc); struct nfs_mount_request request = { - .sap = (struct sockaddr *) - &ctx->mount_server.address, + .sap = &ctx->mount_server._address, .dirpath = ctx->nfs_server.export_path, .protocol = ctx->mount_server.protocol, .fh = root_fh, @@ -854,7 +853,7 @@ static int nfs_request_mount(struct fs_context *fc, * Construct the mount server's address. */ if (ctx->mount_server.address.sa_family == AF_UNSPEC) { - memcpy(request.sap, &ctx->nfs_server.address, + memcpy(request.sap, &ctx->nfs_server._address, ctx->nfs_server.addrlen); ctx->mount_server.addrlen = ctx->nfs_server.addrlen; }
The 'nfs_server' and 'mount_server' structures include a union of 'struct sockaddr' (with the older 16 bytes max address size) and 'struct sockaddr_storage' which is large enough to hold all the supported sa_family types (128 bytes max size). The runtime memcpy() buffer overflow checker is seeing attempts to write beyond the 16 bytes as an overflow, but the actual expected size is that of 'struct sockaddr_storage'. Plumb the use of 'struct sockaddr_storage' more completely through-out NFS, which results in adjusting the memcpy() buffers to the correct union members. Avoids this false positive run-time warning under CONFIG_FORTIFY_SOURCE: memcpy: detected field-spanning write (size 28) of single field "&ctx->nfs_server.address" at fs/nfs/namespace.c:178 (size 16) Reported-by: kernel test robot <yujie.liu@intel.com> Link: https://lore.kernel.org/all/202210110948.26b43120-yujie.liu@intel.com Cc: Trond Myklebust <trond.myklebust@hammerspace.com> Cc: Anna Schumaker <anna@kernel.org> Cc: linux-nfs@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- v2: expand to include all sockaddr destintations... v1: https://lore.kernel.org/lkml/20221011065243.583650-1-keescook@chromium.org/ --- fs/nfs/client.c | 4 ++-- fs/nfs/dns_resolve.c | 7 ++++--- fs/nfs/dns_resolve.h | 2 +- fs/nfs/fs_context.c | 14 +++++++------- fs/nfs/internal.h | 14 +++++++------- fs/nfs/mount_clnt.c | 4 ++-- fs/nfs/namespace.c | 2 +- fs/nfs/nfs3client.c | 4 ++-- fs/nfs/nfs4_fs.h | 2 +- fs/nfs/nfs4client.c | 18 +++++++++--------- fs/nfs/nfs4namespace.c | 16 ++++++++-------- fs/nfs/nfs4proc.c | 4 ++-- fs/nfs/pnfs_nfs.c | 6 +++--- fs/nfs/super.c | 5 ++--- 14 files changed, 51 insertions(+), 51 deletions(-)