From cb6d2fd30dddd00499333e9475f8b11bbd84f37c Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 14 Nov 2023 09:54:18 -0800 Subject: SUNRPC: Replace strlcpy() with strscpy() strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated[1]. Additionally, it returns the size of the source string, not the resulting size of the destination string. In an effort to remove strlcpy() completely[2], replace strlcpy() here with strscpy(). Explicitly handle the truncation case by returning the size of the resulting string. If "nodename" was ever longer than sizeof(clnt->cl_nodename) - 1, this change will fix a bug where clnt->cl_nodelen would end up thinking there were more characters in clnt->cl_nodename than there actually were, which might have lead to kernel memory content exposures. Cc: Trond Myklebust Cc: Anna Schumaker Cc: Chuck Lever Cc: Jeff Layton Cc: Neil Brown Cc: Olga Kornievskaia Cc: Dai Ngo Cc: Tom Talpey Cc: "David S. Miller" Cc: Eric Dumazet Cc: Jakub Kicinski Cc: Paolo Abeni Cc: linux-nfs@vger.kernel.org Cc: netdev@vger.kernel.org Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [1] Link: https://github.com/KSPP/linux/issues/89 [2] Co-developed-by: Azeem Shaikh Signed-off-by: Azeem Shaikh Reviewed-by: NeilBrown Link: https://lore.kernel.org/r/20231114175407.work.410-kees@kernel.org Signed-off-by: Kees Cook --- net/sunrpc/clnt.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c index daa9582ec861..7afe02bdea4a 100644 --- a/net/sunrpc/clnt.c +++ b/net/sunrpc/clnt.c @@ -287,8 +287,14 @@ static struct rpc_xprt *rpc_clnt_set_transport(struct rpc_clnt *clnt, static void rpc_clnt_set_nodename(struct rpc_clnt *clnt, const char *nodename) { - clnt->cl_nodelen = strlcpy(clnt->cl_nodename, - nodename, sizeof(clnt->cl_nodename)); + ssize_t copied; + + copied = strscpy(clnt->cl_nodename, + nodename, sizeof(clnt->cl_nodename)); + + clnt->cl_nodelen = copied < 0 + ? sizeof(clnt->cl_nodename) - 1 + : copied; } static int rpc_client_register(struct rpc_clnt *clnt, -- cgit v1.2.3 From 40b2519d7566266d7eafd3c5232c73a497640bca Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 16 Nov 2023 11:15:10 -0800 Subject: samples: Replace strlcpy() with strscpy() strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated[1]. Additionally, it returns the size of the source string, not the resulting size of the destination string. In an effort to remove strlcpy() completely[2], replace strlcpy() here with strscpy(). Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [1] Link: https://github.com/KSPP/linux/issues/89 [2] Cc: Masami Hiramatsu Cc: Valentin Schneider Cc: "Steven Rostedt (Google)" Cc: Chuck Lever Cc: Geliang Tang Cc: Greg Kroah-Hartman Cc: Christophe JAILLET Cc: Thomas Gleixner Cc: Arnd Bergmann Acked-by: "Steven Rostedt (Google)" Link: https://lore.kernel.org/r/20231116191510.work.550-kees@kernel.org Signed-off-by: Kees Cook --- samples/trace_events/trace-events-sample.h | 2 +- samples/v4l/v4l2-pci-skeleton.c | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/samples/trace_events/trace-events-sample.h b/samples/trace_events/trace-events-sample.h index 1c6b843b8c4e..23f923ccd529 100644 --- a/samples/trace_events/trace-events-sample.h +++ b/samples/trace_events/trace-events-sample.h @@ -305,7 +305,7 @@ TRACE_EVENT(foo_bar, ), TP_fast_assign( - strlcpy(__entry->foo, foo, 10); + strscpy(__entry->foo, foo, 10); __entry->bar = bar; memcpy(__get_dynamic_array(list), lst, __length_of(lst) * sizeof(int)); diff --git a/samples/v4l/v4l2-pci-skeleton.c b/samples/v4l/v4l2-pci-skeleton.c index a61f94db18d9..69ef788d9e3b 100644 --- a/samples/v4l/v4l2-pci-skeleton.c +++ b/samples/v4l/v4l2-pci-skeleton.c @@ -291,8 +291,8 @@ static int skeleton_querycap(struct file *file, void *priv, { struct skeleton *skel = video_drvdata(file); - strlcpy(cap->driver, KBUILD_MODNAME, sizeof(cap->driver)); - strlcpy(cap->card, "V4L2 PCI Skeleton", sizeof(cap->card)); + strscpy(cap->driver, KBUILD_MODNAME, sizeof(cap->driver)); + strscpy(cap->card, "V4L2 PCI Skeleton", sizeof(cap->card)); snprintf(cap->bus_info, sizeof(cap->bus_info), "PCI:%s", pci_name(skel->pdev)); return 0; @@ -597,11 +597,11 @@ static int skeleton_enum_input(struct file *file, void *priv, i->type = V4L2_INPUT_TYPE_CAMERA; if (i->index == 0) { i->std = SKEL_TVNORMS; - strlcpy(i->name, "S-Video", sizeof(i->name)); + strscpy(i->name, "S-Video", sizeof(i->name)); i->capabilities = V4L2_IN_CAP_STD; } else { i->std = 0; - strlcpy(i->name, "HDMI", sizeof(i->name)); + strscpy(i->name, "HDMI", sizeof(i->name)); i->capabilities = V4L2_IN_CAP_DV_TIMINGS; } return 0; @@ -845,7 +845,7 @@ static int skeleton_probe(struct pci_dev *pdev, const struct pci_device_id *ent) /* Initialize the video_device structure */ vdev = &skel->vdev; - strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name)); + strscpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name)); /* * There is nothing to clean up, so release is set to an empty release * function. The release callback must be non-NULL. -- cgit v1.2.3 From d4011f6817ae85e42874af705fec866fec7c4ecf Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Tue, 3 Oct 2023 21:01:58 +0000 Subject: HID: uhid: replace deprecated strncpy with strscpy `strncpy` is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Furthermore, let's make sure `hid->xyz` and `ev->u.create2.xyz` are the same size at compile time to prevent silent truncation. With these changes, it is abundantly clear what the intent and behavior of the code is -- We are getting a string to string copy with NUL-termination and no truncation. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Cc: Kees Cook Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20231003-strncpy-drivers-hid-uhid-c-v2-1-6a501402581e@google.com Signed-off-by: Kees Cook --- drivers/hid/uhid.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c index 4588d2cd4ea4..a54c7995b9be 100644 --- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -490,7 +490,7 @@ static int uhid_dev_create2(struct uhid_device *uhid, const struct uhid_event *ev) { struct hid_device *hid; - size_t rd_size, len; + size_t rd_size; void *rd_data; int ret; @@ -514,13 +514,12 @@ static int uhid_dev_create2(struct uhid_device *uhid, goto err_free; } - /* @hid is zero-initialized, strncpy() is correct, strlcpy() not */ - len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1; - strncpy(hid->name, ev->u.create2.name, len); - len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1; - strncpy(hid->phys, ev->u.create2.phys, len); - len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1; - strncpy(hid->uniq, ev->u.create2.uniq, len); + BUILD_BUG_ON(sizeof(hid->name) != sizeof(ev->u.create2.name)); + strscpy(hid->name, ev->u.create2.name, sizeof(hid->name)); + BUILD_BUG_ON(sizeof(hid->phys) != sizeof(ev->u.create2.phys)); + strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys)); + BUILD_BUG_ON(sizeof(hid->uniq) != sizeof(ev->u.create2.uniq)); + strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq)); hid->ll_driver = &uhid_hid_driver; hid->bus = ev->u.create2.bus; -- cgit v1.2.3 From da2e08d4630ab04ee5b61515fe423c582b5c3be2 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 3 Oct 2023 16:18:38 -0700 Subject: i40e: Annotate struct i40e_qvlist_info with __counted_by Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). As found with Coccinelle[1], add __counted_by for struct i40e_qvlist_info. Cc: Tony Nguyen Cc: Shiraz Saleem Cc: Jakub Kicinski Cc: Jesse Brandeburg Cc: Gurucharan G Cc: "Gustavo A. R. Silva" Link: https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci [1] Reviewed-by: "Gustavo A. R. Silva" Link: https://lore.kernel.org/r/20231003231838.work.510-kees@kernel.org Signed-off-by: Kees Cook --- include/linux/net/intel/i40e_client.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/net/intel/i40e_client.h b/include/linux/net/intel/i40e_client.h index ed42bd5f639f..0aa4411528fc 100644 --- a/include/linux/net/intel/i40e_client.h +++ b/include/linux/net/intel/i40e_client.h @@ -45,7 +45,7 @@ struct i40e_qv_info { struct i40e_qvlist_info { u32 num_vectors; - struct i40e_qv_info qv_info[]; + struct i40e_qv_info qv_info[] __counted_by(num_vectors); }; -- cgit v1.2.3 From 97f3880a33cd4a0c916242fd296aed975ad512d3 Mon Sep 17 00:00:00 2001 From: Christophe JAILLET Date: Sat, 7 Oct 2023 16:32:34 +0200 Subject: VMCI: Annotate struct vmci_handle_arr with __counted_by Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). Signed-off-by: Christophe JAILLET Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/56bef519d982218176b59bbba64a3a308d8733d5.1696689091.git.christophe.jaillet@wanadoo.fr Signed-off-by: Kees Cook --- drivers/misc/vmw_vmci/vmci_handle_array.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/misc/vmw_vmci/vmci_handle_array.h b/drivers/misc/vmw_vmci/vmci_handle_array.h index 96193f85be5b..b0e6b1956014 100644 --- a/drivers/misc/vmw_vmci/vmci_handle_array.h +++ b/drivers/misc/vmw_vmci/vmci_handle_array.h @@ -17,7 +17,7 @@ struct vmci_handle_arr { u32 max_capacity; u32 size; u32 pad; - struct vmci_handle entries[]; + struct vmci_handle entries[] __counted_by(capacity); }; #define VMCI_HANDLE_ARRAY_HEADER_SIZE \ -- cgit v1.2.3 From 446425648c5d19ff7564923863538e9fae93e916 Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Tue, 10 Oct 2023 06:59:44 -0600 Subject: afs: Add __counted_by for struct afs_acl and use struct_size() Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member. This code was found with the help of Coccinelle, and audited and fixed manually. Signed-off-by: "Gustavo A. R. Silva" Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/ZSVKwBmxQ1amv47E@work Signed-off-by: Kees Cook --- fs/afs/internal.h | 2 +- fs/afs/xattr.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/afs/internal.h b/fs/afs/internal.h index c9cef3782b4a..9d6f1aa65776 100644 --- a/fs/afs/internal.h +++ b/fs/afs/internal.h @@ -1116,7 +1116,7 @@ extern void afs_fs_inline_bulk_status(struct afs_operation *); struct afs_acl { u32 size; - u8 data[]; + u8 data[] __counted_by(size); }; extern void afs_fs_fetch_acl(struct afs_operation *); diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c index 64b2c0224f62..e19f396aa370 100644 --- a/fs/afs/xattr.c +++ b/fs/afs/xattr.c @@ -75,7 +75,7 @@ static bool afs_make_acl(struct afs_operation *op, { struct afs_acl *acl; - acl = kmalloc(sizeof(*acl) + size, GFP_KERNEL); + acl = kmalloc(struct_size(acl, data, size), GFP_KERNEL); if (!acl) { afs_op_nomem(op); return false; -- cgit v1.2.3 From 3b2894c967377a49be084b9b39b21b2315bd9b2c Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Mon, 16 Oct 2023 22:38:20 +0000 Subject: drm/modes: replace deprecated strncpy with strscpy_pad `strncpy` is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. We should NUL-pad as there are full struct copies happening in places: | struct drm_mode_modeinfo umode; | | ... | struct drm_property_blob *blob; | | drm_mode_convert_to_umode(&umode, mode); | blob = drm_property_create_blob(crtc->dev, | sizeof(umode), &umode); A suitable replacement is `strscpy_pad` due to the fact that it guarantees both NUL-termination and NUL-padding on the destination buffer. Additionally, replace size macro `DRM_DISPLAY_MODE_LEN` with sizeof() to more directly tie the maximum buffer size to the destination buffer: | struct drm_display_mode { | ... | char name[DRM_DISPLAY_MODE_LEN]; Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Cc: Xu Panda Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20231016-strncpy-drivers-gpu-drm-drm_modes-c-v2-1-d0b60686e1c6@google.com Signed-off-by: Kees Cook --- drivers/gpu/drm/drm_modes.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index ac9a406250c5..893f52ee4926 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -2617,8 +2617,7 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out, break; } - strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); - out->name[DRM_DISPLAY_MODE_LEN-1] = 0; + strscpy_pad(out->name, in->name, sizeof(out->name)); } /** @@ -2659,8 +2658,7 @@ int drm_mode_convert_umode(struct drm_device *dev, * useful for the kernel->userspace direction anyway. */ out->type = in->type & DRM_MODE_TYPE_ALL; - strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); - out->name[DRM_DISPLAY_MODE_LEN-1] = 0; + strscpy_pad(out->name, in->name, sizeof(out->name)); /* Clearing picture aspect ratio bits from out flags, * as the aspect-ratio information is not stored in -- cgit v1.2.3 From 576b75f93b3d3c408235808f689453f1ed891486 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Wed, 18 Oct 2023 22:48:49 +0000 Subject: nvme-fabrics: replace deprecated strncpy with strscpy strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. We expect both data->subsysnqn and data->hostnqn to be NUL-terminated based on their usage with format specifier ("%s"): fabrics.c: 322: dev_err(ctrl->device, 323: "%s, subsysnqn \"%s\"\n", 324: inv_data, data->subsysnqn); ... 349: dev_err(ctrl->device, 350: "Connect for subsystem %s is not allowed, hostnqn: %s\n", 351: data->subsysnqn, data->hostnqn); Moreover, there's no need to NUL-pad since `data` is zero-allocated already in fabrics.c: 383: data = kzalloc(sizeof(*data), GFP_KERNEL); ... therefore any further NUL-padding is rendered useless. Considering the above, a suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. I opted not to switch NVMF_NQN_SIZE to sizeof(data->xyz) because the size is defined as: | /* NQN names in commands fields specified one size */ | #define NVMF_NQN_FIELD_LEN 256 ... while NVMF_NQN_SIZE is defined as: | /* However the max length of a qualified name is another size */ | #define NVMF_NQN_SIZE 223 Since 223 seems pretty magic, I'm not going to touch it. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20231018-strncpy-drivers-nvme-host-fabrics-c-v1-1-b6677df40a35@google.com Signed-off-by: Kees Cook --- drivers/nvme/host/fabrics.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c index 4673ead69c5f..830bf01df4b8 100644 --- a/drivers/nvme/host/fabrics.c +++ b/drivers/nvme/host/fabrics.c @@ -387,8 +387,8 @@ static struct nvmf_connect_data *nvmf_connect_data_prep(struct nvme_ctrl *ctrl, uuid_copy(&data->hostid, &ctrl->opts->host->id); data->cntlid = cpu_to_le16(cntlid); - strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE); - strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE); + strscpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE); + strscpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE); return data; } -- cgit v1.2.3 From ab7e8bb6e077a55ae5ac1a4bb4ebba85470d47e5 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Thu, 19 Oct 2023 17:54:15 +0000 Subject: nvdimm/btt: replace deprecated strncpy with strscpy Found with grep. strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. We expect super->signature to be NUL-terminated based on its usage with memcmp against a NUL-term'd buffer: btt_devs.c: 253 | if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0) btt.h: 13 | #define BTT_SIG "BTT_ARENA_INFO\0" NUL-padding is not required as `super` is already zero-allocated: btt.c: 985 | super = kzalloc(sizeof(struct btt_sb), GFP_NOIO); ... rendering any additional NUL-padding superfluous. Considering the above, a suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Let's also use the more idiomatic strscpy usage of (dest, src, sizeof(dest)) instead of (dest, src, XYZ_LEN) for buffers that the compiler can determine the size of. This more tightly correlates the destination buffer to the amount of bytes copied. Side note, this pattern of memcmp() on two NUL-terminated strings should really be changed to just a strncmp(), if i'm not mistaken? I see multiple instances of this pattern in this system: | if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0) | return false; where BIT_SIG is defined (weirdly) as a double NUL-terminated string: | #define BTT_SIG "BTT_ARENA_INFO\0" Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20231019-strncpy-drivers-nvdimm-btt-c-v2-1-366993878cf0@google.com Signed-off-by: Kees Cook --- drivers/nvdimm/btt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c index d5593b0dc700..9372c36e8f76 100644 --- a/drivers/nvdimm/btt.c +++ b/drivers/nvdimm/btt.c @@ -986,7 +986,7 @@ static int btt_arena_write_layout(struct arena_info *arena) if (!super) return -ENOMEM; - strncpy(super->signature, BTT_SIG, BTT_SIG_LEN); + strscpy(super->signature, BTT_SIG, sizeof(super->signature)); export_uuid(super->uuid, nd_btt->uuid); export_uuid(super->parent_uuid, parent_uuid); super->flags = cpu_to_le32(arena->flags); -- cgit v1.2.3 From e5a4975ca463e91c2009f5950e0156f0b857eb10 Mon Sep 17 00:00:00 2001 From: Justin Stitt Date: Thu, 19 Oct 2023 21:34:35 +0000 Subject: nvme-fc: replace deprecated strncpy with strscpy strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. Let's instead use strscpy() [2] as it guarantees NUL-termination on the destination buffer. Moreover, there is no need to use: | min(FCNVME_ASSOC_HOSTNQN_LEN, NVMF_NQN_SIZE)); I imagine this was originally done to make sure the destination buffer is NUL-terminated by ensuring we copy a number of bytes less than the size of our destination, thus leaving some NUL-bytes at the end. However, with strscpy(), we no longer need to do this and we can instead opt for the more idiomatic strscpy() usage of: | strscpy(dest, src, sizeof(dest)) Also, no NUL-padding is required as lsop is zero-allocated: | lsop = kzalloc((sizeof(*lsop) + | sizeof(*assoc_rqst) + sizeof(*assoc_acc) + | ctrl->lport->ops->lsrqst_priv_sz), GFP_KERNEL); ... and assoc_rqst points to a field in lsop: | assoc_rqst = (struct fcnvme_ls_cr_assoc_rqst *)&lsop[1]; Therefore, any additional NUL-byte assignments (like the ones that strncpy() makes) are redundant. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt Similar-to: https://lore.kernel.org/all/20231018-strncpy-drivers-nvme-host-fabrics-c-v1-1-b6677df40a35@google.com/ Reviewed-by: Kees Cook Link: https://lore.kernel.org/r/20231019-strncpy-drivers-nvme-host-fc-c-v1-1-5805c15e4b49@google.com Signed-off-by: Kees Cook --- drivers/nvme/host/fc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c index 49c3e46eaa1e..a719ca2cd406 100644 --- a/drivers/nvme/host/fc.c +++ b/drivers/nvme/host/fc.c @@ -1218,10 +1218,10 @@ nvme_fc_connect_admin_queue(struct nvme_fc_ctrl *ctrl, /* Linux supports only Dynamic controllers */ assoc_rqst->assoc_cmd.cntlid = cpu_to_be16(0xffff); uuid_copy(&assoc_rqst->assoc_cmd.hostid, &ctrl->ctrl.opts->host->id); - strncpy(assoc_rqst->assoc_cmd.hostnqn, ctrl->ctrl.opts->host->nqn, - min(FCNVME_ASSOC_HOSTNQN_LEN, NVMF_NQN_SIZE)); - strncpy(assoc_rqst->assoc_cmd.subnqn, ctrl->ctrl.opts->subsysnqn, - min(FCNVME_ASSOC_SUBNQN_LEN, NVMF_NQN_SIZE)); + strscpy(assoc_rqst->assoc_cmd.hostnqn, ctrl->ctrl.opts->host->nqn, + sizeof(assoc_rqst->assoc_cmd.hostnqn)); + strscpy(assoc_rqst->assoc_cmd.subnqn, ctrl->ctrl.opts->subsysnqn, + sizeof(assoc_rqst->assoc_cmd.subnqn)); lsop->queue = queue; lsreq->rqstaddr = assoc_rqst; -- cgit v1.2.3 From aabf7c37dfbce3e5fe24f0c86a34bc8f2f63cee8 Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Wed, 29 Nov 2023 13:44:04 -0800 Subject: lkdtm: Add kfence read after free crash type Add the ability to allocate memory from kfence and trigger a read after free on that memory to validate that kfence is working properly. This is used by ChromeOS integration tests to validate that kfence errors can be collected on user devices and parsed properly. Cc: Alexander Potapenko Acked-by: Marco Elver Cc: Dmitry Vyukov Cc: Andrew Morton Cc: kasan-dev@googlegroups.com Signed-off-by: Stephen Boyd Link: https://lore.kernel.org/r/20231129214413.3156334-1-swboyd@chromium.org Signed-off-by: Kees Cook --- drivers/misc/lkdtm/heap.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/kfence.h | 2 ++ 2 files changed, 62 insertions(+) diff --git a/drivers/misc/lkdtm/heap.c b/drivers/misc/lkdtm/heap.c index 0ce4cbf6abda..4f467d3972a6 100644 --- a/drivers/misc/lkdtm/heap.c +++ b/drivers/misc/lkdtm/heap.c @@ -4,6 +4,7 @@ * page allocation and slab allocations. */ #include "lkdtm.h" +#include #include #include #include @@ -132,6 +133,64 @@ static void lkdtm_READ_AFTER_FREE(void) kfree(val); } +static void lkdtm_KFENCE_READ_AFTER_FREE(void) +{ + int *base, val, saw; + unsigned long timeout, resched_after; + size_t len = 1024; + /* + * The slub allocator will use the either the first word or + * the middle of the allocation to store the free pointer, + * depending on configurations. Store in the second word to + * avoid running into the freelist. + */ + size_t offset = sizeof(*base); + + /* + * 100x the sample interval should be more than enough to ensure we get + * a KFENCE allocation eventually. + */ + timeout = jiffies + msecs_to_jiffies(100 * kfence_sample_interval); + /* + * Especially for non-preemption kernels, ensure the allocation-gate + * timer can catch up: after @resched_after, every failed allocation + * attempt yields, to ensure the allocation-gate timer is scheduled. + */ + resched_after = jiffies + msecs_to_jiffies(kfence_sample_interval); + do { + base = kmalloc(len, GFP_KERNEL); + if (!base) { + pr_err("FAIL: Unable to allocate kfence memory!\n"); + return; + } + + if (is_kfence_address(base)) { + val = 0x12345678; + base[offset] = val; + pr_info("Value in memory before free: %x\n", base[offset]); + + kfree(base); + + pr_info("Attempting bad read from freed memory\n"); + saw = base[offset]; + if (saw != val) { + /* Good! Poisoning happened, so declare a win. */ + pr_info("Memory correctly poisoned (%x)\n", saw); + } else { + pr_err("FAIL: Memory was not poisoned!\n"); + pr_expected_config_param(CONFIG_INIT_ON_FREE_DEFAULT_ON, "init_on_free"); + } + return; + } + + kfree(base); + if (time_after(jiffies, resched_after)) + cond_resched(); + } while (time_before(jiffies, timeout)); + + pr_err("FAIL: kfence memory never allocated!\n"); +} + static void lkdtm_WRITE_BUDDY_AFTER_FREE(void) { unsigned long p = __get_free_page(GFP_KERNEL); @@ -327,6 +386,7 @@ static struct crashtype crashtypes[] = { CRASHTYPE(VMALLOC_LINEAR_OVERFLOW), CRASHTYPE(WRITE_AFTER_FREE), CRASHTYPE(READ_AFTER_FREE), + CRASHTYPE(KFENCE_READ_AFTER_FREE), CRASHTYPE(WRITE_BUDDY_AFTER_FREE), CRASHTYPE(READ_BUDDY_AFTER_FREE), CRASHTYPE(SLAB_INIT_ON_ALLOC), diff --git a/include/linux/kfence.h b/include/linux/kfence.h index 401af4757514..88100cc9caba 100644 --- a/include/linux/kfence.h +++ b/include/linux/kfence.h @@ -223,6 +223,8 @@ bool __kfence_obj_info(struct kmem_obj_info *kpp, void *object, struct slab *sla #else /* CONFIG_KFENCE */ +#define kfence_sample_interval (0) + static inline bool is_kfence_address(const void *addr) { return false; } static inline void kfence_alloc_pool_and_metadata(void) { } static inline void kfence_init(void) { } -- cgit v1.2.3 From 12cd3cd8c797e07afcc47bc4afa760e4ec75e9d7 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Mon, 20 Nov 2023 17:11:42 +0200 Subject: params: Introduce the param_unknown_fn type Introduce a new type for the callback to parse an unknown argument. This unifies function prototypes which takes that as a parameter. Reviewed-by: Luis Chamberlain Reviewed-by: Kees Cook Signed-off-by: Andy Shevchenko Link: https://lore.kernel.org/r/20231120151419.1661807-2-andriy.shevchenko@linux.intel.com Signed-off-by: Kees Cook --- include/linux/moduleparam.h | 6 +++--- kernel/params.c | 8 ++------ 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h index 4fa9726bc328..bfb85fd13e1f 100644 --- a/include/linux/moduleparam.h +++ b/include/linux/moduleparam.h @@ -385,6 +385,8 @@ extern bool parameq(const char *name1, const char *name2); */ extern bool parameqn(const char *name1, const char *name2, size_t n); +typedef int (*parse_unknown_fn)(char *param, char *val, const char *doing, void *arg); + /* Called on module insert or kernel boot */ extern char *parse_args(const char *name, char *args, @@ -392,9 +394,7 @@ extern char *parse_args(const char *name, unsigned num, s16 level_min, s16 level_max, - void *arg, - int (*unknown)(char *param, char *val, - const char *doing, void *arg)); + void *arg, parse_unknown_fn unknown); /* Called by module remove. */ #ifdef CONFIG_SYSFS diff --git a/kernel/params.c b/kernel/params.c index 2d4a0564697e..626fa8265932 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -120,9 +120,7 @@ static int parse_one(char *param, unsigned num_params, s16 min_level, s16 max_level, - void *arg, - int (*handle_unknown)(char *param, char *val, - const char *doing, void *arg)) + void *arg, parse_unknown_fn handle_unknown) { unsigned int i; int err; @@ -165,9 +163,7 @@ char *parse_args(const char *doing, unsigned num, s16 min_level, s16 max_level, - void *arg, - int (*unknown)(char *param, char *val, - const char *doing, void *arg)) + void *arg, parse_unknown_fn unknown) { char *param, *val, *err = NULL; -- cgit v1.2.3 From fd0cd057a1b7351604daa6ffc91dfe28adf7225d Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Mon, 20 Nov 2023 17:11:43 +0200 Subject: params: Do not go over the limit when getting the string length We can use strnlen() even on early stages and it prevents from going over the string boundaries in case it's already too long. Reviewed-by: Luis Chamberlain Reviewed-by: Kees Cook Signed-off-by: Andy Shevchenko Link: https://lore.kernel.org/r/20231120151419.1661807-3-andriy.shevchenko@linux.intel.com Signed-off-by: Kees Cook --- kernel/params.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/kernel/params.c b/kernel/params.c index 626fa8265932..f8e3c4139854 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -260,7 +260,10 @@ EXPORT_SYMBOL_GPL(param_set_uint_minmax); int param_set_charp(const char *val, const struct kernel_param *kp) { - if (strlen(val) > 1024) { + size_t len, maxlen = 1024; + + len = strnlen(val, maxlen + 1); + if (len == maxlen + 1) { pr_err("%s: string parameter too long\n", kp->name); return -ENOSPC; } @@ -270,7 +273,7 @@ int param_set_charp(const char *val, const struct kernel_param *kp) /* This is a hack. We can't kmalloc in early boot, and we * don't need to; this mangled commandline is preserved. */ if (slab_is_available()) { - *(char **)kp->arg = kmalloc_parameter(strlen(val)+1); + *(char **)kp->arg = kmalloc_parameter(len + 1); if (!*(char **)kp->arg) return -ENOMEM; strcpy(*(char **)kp->arg, val); @@ -508,7 +511,7 @@ int param_set_copystring(const char *val, const struct kernel_param *kp) { const struct kparam_string *kps = kp->str; - if (strlen(val)+1 > kps->maxlen) { + if (strnlen(val, kps->maxlen) == kps->maxlen) { pr_err("%s: string doesn't fit in %u chars.\n", kp->name, kps->maxlen-1); return -ENOSPC; -- cgit v1.2.3 From 0fc79cbc937f2a754a302a710a94b68c61d0a89a Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Mon, 20 Nov 2023 17:11:44 +0200 Subject: params: Use size_add() for kmalloc() Prevent allocations from integer overflow by using size_add(). Reviewed-by: Luis Chamberlain Reviewed-by: Kees Cook Signed-off-by: Andy Shevchenko Link: https://lore.kernel.org/r/20231120151419.1661807-4-andriy.shevchenko@linux.intel.com Signed-off-by: Kees Cook --- kernel/params.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kernel/params.c b/kernel/params.c index f8e3c4139854..c3a029fe183d 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -48,7 +49,7 @@ static void *kmalloc_parameter(unsigned int size) { struct kmalloced_param *p; - p = kmalloc(sizeof(*p) + size, GFP_KERNEL); + p = kmalloc(size_add(sizeof(*p), size), GFP_KERNEL); if (!p) return NULL; -- cgit v1.2.3 From a05f096c2c0ca52e8fd34740c7d4b53ab3e7123e Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Mon, 20 Nov 2023 17:11:45 +0200 Subject: params: Sort headers Sort the headers in alphabetic order in order to ease the maintenance for this part. Reviewed-by: Luis Chamberlain Reviewed-by: Kees Cook Signed-off-by: Andy Shevchenko Link: https://lore.kernel.org/r/20231120151419.1661807-5-andriy.shevchenko@linux.intel.com Signed-off-by: Kees Cook --- kernel/params.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/kernel/params.c b/kernel/params.c index c3a029fe183d..eb55b32399b4 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -3,18 +3,18 @@ Copyright (C) 2001 Rusty Russell. */ +#include +#include +#include +#include #include #include -#include -#include #include #include -#include -#include #include -#include -#include #include +#include +#include #ifdef CONFIG_SYSFS /* Protects all built-in parameters, modules use their own param_lock */ -- cgit v1.2.3 From b5e3f86a47d34f7b8af899f8cc70520f6daf8b53 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Mon, 20 Nov 2023 17:11:46 +0200 Subject: params: Fix multi-line comment style The multi-line comment style in the file is rather arbitrary. Make it follow the standard one. Reviewed-by: Luis Chamberlain Reviewed-by: Kees Cook Signed-off-by: Andy Shevchenko Link: https://lore.kernel.org/r/20231120151419.1661807-6-andriy.shevchenko@linux.intel.com Signed-off-by: Kees Cook --- kernel/params.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/kernel/params.c b/kernel/params.c index eb55b32399b4..2e447f8ae183 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -1,8 +1,8 @@ // SPDX-License-Identifier: GPL-2.0-or-later -/* Helpers for initial module or kernel cmdline parsing - Copyright (C) 2001 Rusty Russell. - -*/ +/* + * Helpers for initial module or kernel cmdline parsing + * Copyright (C) 2001 Rusty Russell. + */ #include #include #include @@ -271,8 +271,10 @@ int param_set_charp(const char *val, const struct kernel_param *kp) maybe_kfree_parameter(*(char **)kp->arg); - /* This is a hack. We can't kmalloc in early boot, and we - * don't need to; this mangled commandline is preserved. */ + /* + * This is a hack. We can't kmalloc() in early boot, and we + * don't need to; this mangled commandline is preserved. + */ if (slab_is_available()) { *(char **)kp->arg = kmalloc_parameter(len + 1); if (!*(char **)kp->arg) @@ -743,8 +745,10 @@ void module_param_sysfs_remove(struct module *mod) { if (mod->mkobj.mp) { sysfs_remove_group(&mod->mkobj.kobj, &mod->mkobj.mp->grp); - /* We are positive that no one is using any param - * attrs at this point. Deallocate immediately. */ + /* + * We are positive that no one is using any param + * attrs at this point. Deallocate immediately. + */ free_module_param_attrs(&mod->mkobj); } } -- cgit v1.2.3 From 8a3750ecf8104de55c569ffbe844a85aa9d5deaa Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 30 Nov 2023 12:56:08 -0800 Subject: tracing/uprobe: Replace strlcpy() with strscpy() strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated[1]. Additionally, it returns the size of the source string, not the resulting size of the destination string. In an effort to remove strlcpy() completely[2], replace strlcpy() here with strscpy(). The negative return value is already handled by this code so no new handling is needed here. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [1] Link: https://github.com/KSPP/linux/issues/89 [2] Cc: Steven Rostedt Cc: Masami Hiramatsu Cc: linux-trace-kernel@vger.kernel.org Acked-by: "Masami Hiramatsu (Google)" Link: https://lore.kernel.org/r/20231130205607.work.463-kees@kernel.org Signed-off-by: Kees Cook --- kernel/trace/trace_uprobe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index 99c051de412a..a84b85d8aac1 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -151,7 +151,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base) return -ENOMEM; if (addr == FETCH_TOKEN_COMM) - ret = strlcpy(dst, current->comm, maxlen); + ret = strscpy(dst, current->comm, maxlen); else ret = strncpy_from_user(dst, src, maxlen); if (ret >= 0) { -- cgit v1.2.3 From ac7110d883ff2a25d2b0ae45c909c02d598c33af Mon Sep 17 00:00:00 2001 From: "Gustavo A. R. Silva" Date: Tue, 10 Oct 2023 06:46:50 -0600 Subject: atags_proc: Add __counted_by for struct buffer and use struct_size() Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). While there, use struct_size() helper, instead of the open-coded version, to calculate the size for the allocation of the whole flexible structure, including of course, the flexible-array member. This code was found with the help of Coccinelle, and audited and fixed manually. Signed-off-by: Gustavo A. R. Silva Reviewed-by: Kees Cook Reviewed-by: Justin Stitt Link: https://lore.kernel.org/r/ZSVHurzo/4aFQcT3@work Signed-off-by: Kees Cook --- arch/arm/kernel/atags_proc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/arm/kernel/atags_proc.c b/arch/arm/kernel/atags_proc.c index 3ec2afe78423..cd09f8ab93e3 100644 --- a/arch/arm/kernel/atags_proc.c +++ b/arch/arm/kernel/atags_proc.c @@ -7,7 +7,7 @@ struct buffer { size_t size; - char data[]; + char data[] __counted_by(size); }; static ssize_t atags_read(struct file *file, char __user *buf, @@ -54,7 +54,7 @@ static int __init init_atags_procfs(void) WARN_ON(tag->hdr.tag != ATAG_NONE); - b = kmalloc(sizeof(*b) + size, GFP_KERNEL); + b = kmalloc(struct_size(b, data, size), GFP_KERNEL); if (!b) goto nomem; -- cgit v1.2.3 From 53853995c6652e12b0aa0d15aecda4cbba5183ec Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 30 Nov 2023 12:51:18 -0800 Subject: qnx4: Extract dir entry filename processing into helper Both dir.c and namei.c need to perform the same work to figure out a directory entry's name and size. Extract this into a helper for use in the next patch. Acked-by: Anders Larsen Link: https://lore.kernel.org/r/20231130205120.3642477-1-keescook@chromium.org Signed-off-by: Kees Cook --- fs/qnx4/dir.c | 52 +++++++------------------------------------------- fs/qnx4/qnx4.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 45 deletions(-) diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c index 66645a5a35f3..42a529e26bd6 100644 --- a/fs/qnx4/dir.c +++ b/fs/qnx4/dir.c @@ -15,43 +15,6 @@ #include #include "qnx4.h" -/* - * A qnx4 directory entry is an inode entry or link info - * depending on the status field in the last byte. The - * first byte is where the name start either way, and a - * zero means it's empty. - * - * Also, due to a bug in gcc, we don't want to use the - * real (differently sized) name arrays in the inode and - * link entries, but always the 'de_name[]' one in the - * fake struct entry. - * - * See - * - * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578#c6 - * - * for details, but basically gcc will take the size of the - * 'name' array from one of the used union entries randomly. - * - * This use of 'de_name[]' (48 bytes) avoids the false positive - * warnings that would happen if gcc decides to use 'inode.di_name' - * (16 bytes) even when the pointer and size were to come from - * 'link.dl_name' (48 bytes). - * - * In all cases the actual name pointer itself is the same, it's - * only the gcc internal 'what is the size of this field' logic - * that can get confused. - */ -union qnx4_directory_entry { - struct { - const char de_name[48]; - u8 de_pad[15]; - u8 de_status; - }; - struct qnx4_inode_entry inode; - struct qnx4_link_info link; -}; - static int qnx4_readdir(struct file *file, struct dir_context *ctx) { struct inode *inode = file_inode(file); @@ -74,26 +37,25 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx) ix = (ctx->pos >> QNX4_DIR_ENTRY_SIZE_BITS) % QNX4_INODES_PER_BLOCK; for (; ix < QNX4_INODES_PER_BLOCK; ix++, ctx->pos += QNX4_DIR_ENTRY_SIZE) { union qnx4_directory_entry *de; + const char *fname; offset = ix * QNX4_DIR_ENTRY_SIZE; de = (union qnx4_directory_entry *) (bh->b_data + offset); - if (!de->de_name[0]) - continue; - if (!(de->de_status & (QNX4_FILE_USED|QNX4_FILE_LINK))) + fname = get_entry_fname(de, &size); + if (!fname) continue; + if (!(de->de_status & QNX4_FILE_LINK)) { - size = sizeof(de->inode.di_fname); ino = blknum * QNX4_INODES_PER_BLOCK + ix - 1; } else { - size = sizeof(de->link.dl_fname); ino = ( le32_to_cpu(de->link.dl_inode_blk) - 1 ) * QNX4_INODES_PER_BLOCK + de->link.dl_inode_ndx; } - size = strnlen(de->de_name, size); - QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size, name)); - if (!dir_emit(ctx, de->de_name, size, ino, DT_UNKNOWN)) { + + QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size, fname)); + if (!dir_emit(ctx, fname, size, ino, DT_UNKNOWN)) { brelse(bh); return 0; } diff --git a/fs/qnx4/qnx4.h b/fs/qnx4/qnx4.h index 6283705466a4..5c2b1fb6b952 100644 --- a/fs/qnx4/qnx4.h +++ b/fs/qnx4/qnx4.h @@ -44,3 +44,63 @@ static inline struct qnx4_inode_entry *qnx4_raw_inode(struct inode *inode) { return &qnx4_i(inode)->raw; } + +/* + * A qnx4 directory entry is an inode entry or link info + * depending on the status field in the last byte. The + * first byte is where the name start either way, and a + * zero means it's empty. + * + * Also, due to a bug in gcc, we don't want to use the + * real (differently sized) name arrays in the inode and + * link entries, but always the 'de_name[]' one in the + * fake struct entry. + * + * See + * + * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578#c6 + * + * for details, but basically gcc will take the size of the + * 'name' array from one of the used union entries randomly. + * + * This use of 'de_name[]' (48 bytes) avoids the false positive + * warnings that would happen if gcc decides to use 'inode.di_name' + * (16 bytes) even when the pointer and size were to come from + * 'link.dl_name' (48 bytes). + * + * In all cases the actual name pointer itself is the same, it's + * only the gcc internal 'what is the size of this field' logic + * that can get confused. + */ +union qnx4_directory_entry { + struct { + const char de_name[48]; + u8 de_pad[15]; + u8 de_status; + }; + struct qnx4_inode_entry inode; + struct qnx4_link_info link; +}; + +static inline const char *get_entry_fname(union qnx4_directory_entry *de, + int *size) +{ + /* Make sure the status byte is in the same place for all structs. */ + BUILD_BUG_ON(offsetof(struct qnx4_inode_entry, di_status) != + offsetof(struct qnx4_link_info, dl_status)); + BUILD_BUG_ON(offsetof(struct qnx4_inode_entry, di_status) != + offsetof(union qnx4_directory_entry, de_status)); + + if (!de->de_name[0]) + return NULL; + if (!(de->de_status & (QNX4_FILE_USED|QNX4_FILE_LINK))) + return NULL; + if (!(de->de_status & QNX4_FILE_LINK)) + *size = sizeof(de->inode.di_fname); + else + *size = sizeof(de->link.dl_fname); + + *size = strnlen(de->de_name, *size); + + return de->de_name; +} -- cgit v1.2.3 From a75b3809dce2ad006ebf7fa641f49881fa0d79d7 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 30 Nov 2023 12:51:19 -0800 Subject: qnx4: Use get_directory_fname() in qnx4_match() Use the new common directory entry name accessor helper to avoid confusing the compiler about over-running the file name buffer. Avoids false positive buffer overflow warning: [ 4849.636861] detected buffer overflow in strlen [ 4849.636897] ------------[ cut here ]------------ [ 4849.636902] kernel BUG at lib/string.c:1165! ... [ 4849.637047] Call Trace: ... [ 4849.637251] qnx4_find_entry.cold+0xc/0x18 [qnx4] [ 4849.637264] qnx4_lookup+0x3c/0xa0 [qnx4] Reported-by: Ronald Monthero Closes: https://lore.kernel.org/lkml/20231112095353.579855-1-debug.penguin32@gmail.com/ Acked-by: Anders Larsen Link: https://lore.kernel.org/r/20231130205120.3642477-2-keescook@chromium.org Signed-off-by: Kees Cook --- fs/qnx4/namei.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c index 8d72221735d7..bb8db6550ca5 100644 --- a/fs/qnx4/namei.c +++ b/fs/qnx4/namei.c @@ -26,31 +26,24 @@ static int qnx4_match(int len, const char *name, struct buffer_head *bh, unsigned long *offset) { - struct qnx4_inode_entry *de; - int namelen, thislen; + union qnx4_directory_entry *de; + const char *fname; + int fnamelen; if (bh == NULL) { printk(KERN_WARNING "qnx4: matching unassigned buffer !\n"); return 0; } - de = (struct qnx4_inode_entry *) (bh->b_data + *offset); + de = (union qnx4_directory_entry *) (bh->b_data + *offset); *offset += QNX4_DIR_ENTRY_SIZE; - if ((de->di_status & QNX4_FILE_LINK) != 0) { - namelen = QNX4_NAME_MAX; - } else { - namelen = QNX4_SHORT_NAME_MAX; - } - thislen = strlen( de->di_fname ); - if ( thislen > namelen ) - thislen = namelen; - if (len != thislen) { + + fname = get_entry_fname(de, &fnamelen); + if (!fname || len != fnamelen) return 0; - } - if (strncmp(name, de->di_fname, len) == 0) { - if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) != 0) { - return 1; - } - } + + if (strncmp(name, fname, len) == 0) + return 1; + return 0; } -- cgit v1.2.3