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(-) (limited to 'drivers') 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 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(-) (limited to 'drivers') 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 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(-) (limited to 'drivers') 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(-) (limited to 'drivers') 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(-) (limited to 'drivers') 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(-) (limited to 'drivers') 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(+) (limited to 'drivers') 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