* [PATCH for-next 01/25] scsi: ibmvscsi: Avoid multi-field memset() overflow by aiming at srp
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
@ 2021-08-22 7:50 ` Kees Cook
2021-08-22 7:50 ` [PATCH for-next 02/25] powerpc: Split memset() to avoid multi-field overflow Kees Cook
` (23 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:50 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Tyrel Datwyler, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, James E.J. Bottomley,
Martin K. Petersen, linux-scsi, linuxppc-dev, Rasmus Villemoes,
Daniel Micay, Francis Laniel, Bart Van Assche, David Gow,
linux-mm, clang-built-linux, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Instead of writing beyond the end of evt_struct->iu.srp.cmd, target the
upper union (evt_struct->iu.srp) instead, as that's what is being wiped.
Cc: Tyrel Datwyler <tyreld@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Martin K. Petersen <martin.petersen@oracle.com>
Link: https://lore.kernel.org/lkml/yq135rzp79c.fsf@ca-mkp.ca.oracle.com
Acked-by: Tyrel Datwyler <tyreld@linux.ibm.com>
Link: https://lore.kernel.org/lkml/6eae8434-e9a7-aa74-628b-b515b3695359@linux.ibm.com
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index e6a3eaaa57d9..3bd3a0124123 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -1055,8 +1055,9 @@ static int ibmvscsi_queuecommand_lck(struct scsi_cmnd *cmnd,
return SCSI_MLQUEUE_HOST_BUSY;
/* Set up the actual SRP IU */
+ BUILD_BUG_ON(sizeof(evt_struct->iu.srp) != SRP_MAX_IU_LEN);
+ memset(&evt_struct->iu.srp, 0x00, sizeof(evt_struct->iu.srp));
srp_cmd = &evt_struct->iu.srp.cmd;
- memset(srp_cmd, 0x00, SRP_MAX_IU_LEN);
srp_cmd->opcode = SRP_CMD;
memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb));
int_to_scsilun(lun, &srp_cmd->lun);
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 02/25] powerpc: Split memset() to avoid multi-field overflow
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
2021-08-22 7:50 ` [PATCH for-next 01/25] scsi: ibmvscsi: Avoid multi-field memset() overflow by aiming at srp Kees Cook
@ 2021-08-22 7:50 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 03/25] stddef: Fix kerndoc for sizeof_field() and offsetofend() Kees Cook
` (22 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:50 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Benjamin Herrenschmidt, Qinglang Miao,
Gustavo A. R. Silva, Hulk Robot, Wang Wensheng, linuxppc-dev,
Michael Ellerman, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Instead of writing across a field boundary with memset(), move the call
to just the array, and an explicit zeroing of the prior field.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Qinglang Miao <miaoqinglang@huawei.com>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Hulk Robot <hulkci@huawei.com>
Cc: Wang Wensheng <wangwensheng4@huawei.com>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Michael Ellerman <mpe@ellerman.id.au>
Link: https://lore.kernel.org/lkml/87czqsnmw9.fsf@mpe.ellerman.id.au
---
drivers/macintosh/smu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/macintosh/smu.c b/drivers/macintosh/smu.c
index 94fb63a7b357..3e2b25ea58a3 100644
--- a/drivers/macintosh/smu.c
+++ b/drivers/macintosh/smu.c
@@ -848,7 +848,8 @@ int smu_queue_i2c(struct smu_i2c_cmd *cmd)
cmd->read = cmd->info.devaddr & 0x01;
switch(cmd->info.type) {
case SMU_I2C_TRANSFER_SIMPLE:
- memset(&cmd->info.sublen, 0, 4);
+ cmd->info.sublen = 0;
+ memset(cmd->info.subaddr, 0, sizeof(cmd->info.subaddr));
break;
case SMU_I2C_TRANSFER_COMBINED:
cmd->info.devaddr &= 0xfe;
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 03/25] stddef: Fix kerndoc for sizeof_field() and offsetofend()
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
2021-08-22 7:50 ` [PATCH for-next 01/25] scsi: ibmvscsi: Avoid multi-field memset() overflow by aiming at srp Kees Cook
2021-08-22 7:50 ` [PATCH for-next 02/25] powerpc: Split memset() to avoid multi-field overflow Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 04/25] stddef: Introduce struct_group() helper macro Kees Cook
` (21 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
Adjust the comment styles so these are correctly identified as valid
kern-doc.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/stddef.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index 998a4ba28eba..8553b33143d1 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -20,7 +20,7 @@ enum {
#endif
/**
- * sizeof_field(TYPE, MEMBER)
+ * sizeof_field() - Report the size of a struct field in bytes
*
* @TYPE: The structure containing the field of interest
* @MEMBER: The field to return the size of
@@ -28,7 +28,7 @@ enum {
#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
/**
- * offsetofend(TYPE, MEMBER)
+ * offsetofend() - Report the offset of a struct field within the struct
*
* @TYPE: The type of the structure
* @MEMBER: The member within the structure to get the end offset of
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 04/25] stddef: Introduce struct_group() helper macro
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (2 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 03/25] stddef: Fix kerndoc for sizeof_field() and offsetofend() Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 05/25] cxl/core: Replace unions with struct_group() Kees Cook
` (20 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Keith Packard, Gustavo A . R . Silva,
Rasmus Villemoes, Dan Williams, Daniel Vetter, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
Kernel code has a regular need to describe groups of members within a
structure usually when they need to be copied or initialized separately
from the rest of the surrounding structure. The generally accepted design
pattern in C is to use a named sub-struct:
struct foo {
int one;
struct {
int two;
int three, four;
} thing;
int five;
};
This would allow for traditional references and sizing:
memcpy(&dst.thing, &src.thing, sizeof(dst.thing));
However, doing this would mean that referencing struct members enclosed
by such named structs would always require including the sub-struct name
in identifiers:
do_something(dst.thing.three);
This has tended to be quite inflexible, especially when such groupings
need to be added to established code which causes huge naming churn.
Three workarounds exist in the kernel for this problem, and each have
other negative properties.
To avoid the naming churn, there is a design pattern of adding macro
aliases for the named struct:
#define f_three thing.three
This ends up polluting the global namespace, and makes it difficult to
search for identifiers.
Another common work-around in kernel code avoids the pollution by avoiding
the named struct entirely, instead identifying the group's boundaries using
either a pair of empty anonymous structs of a pair of zero-element arrays:
struct foo {
int one;
struct { } start;
int two;
int three, four;
struct { } finish;
int five;
};
struct foo {
int one;
int start[0];
int two;
int three, four;
int finish[0];
int five;
};
This allows code to avoid needing to use a sub-struct named for member
references within the surrounding structure, but loses the benefits of
being able to actually use such a struct, making it rather fragile. Using
these requires open-coded calculation of sizes and offsets. The efforts
made to avoid common mistakes include lots of comments, or adding various
BUILD_BUG_ON()s. Such code is left with no way for the compiler to reason
about the boundaries (e.g. the "start" object looks like it's 0 bytes
in length), making bounds checking depend on open-coded calculations:
if (length > offsetof(struct foo, finish) -
offsetof(struct foo, start))
return -EINVAL;
memcpy(&dst.start, &src.start, offsetof(struct foo, finish) -
offsetof(struct foo, start));
However, the vast majority of places in the kernel that operate on
groups of members do so without any identification of the grouping,
relying either on comments or implicit knowledge of the struct contents,
which is even harder for the compiler to reason about, and results in
even more fragile manual sizing, usually depending on member locations
outside of the region (e.g. to copy "two" and "three", use the start of
"four" to find the size):
BUILD_BUG_ON((offsetof(struct foo, four) <
offsetof(struct foo, two)) ||
(offsetof(struct foo, four) <
offsetof(struct foo, three));
if (length > offsetof(struct foo, four) -
offsetof(struct foo, two))
return -EINVAL;
memcpy(&dst.two, &src.two, length);
In order to have a regular programmatic way to describe a struct
region that can be used for references and sizing, can be examined for
bounds checking, avoids forcing the use of intermediate identifiers,
and avoids polluting the global namespace, introduce the struct_group()
macro. This macro wraps the member declarations to create an anonymous
union of an anonymous struct (no intermediate name) and a named struct
(for references and sizing):
struct foo {
int one;
struct_group(thing,
int two;
int three, four;
);
int five;
};
if (length > sizeof(src.thing))
return -EINVAL;
memcpy(&dst.thing, &src.thing, length);
do_something(dst.three);
There are some rare cases where the resulting struct_group() needs
attributes added, so struct_group_attr() is also introduced to allow
for specifying struct attributes (e.g. __align(x) or __packed).
Additionally, there are places where such declarations would like to
have the struct be tagged, so struct_group_tagged() is added.
Given there is a need for a handful of UAPI uses too, the underlying
__struct_group() macro has been defined in UAPI so it can be used there
too.
To avoid confusing scripts/kernel-doc, hide the macro from its struct
parsing.
Co-developed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Link: https://lore.kernel.org/lkml/20210728023217.GC35706@embeddedor
Enhanced-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Link: https://lore.kernel.org/lkml/41183a98-bdb9-4ad6-7eab-5a7292a6df84@rasmusvillemoes.dk
Enhanced-by: Dan Williams <dan.j.williams@intel.com>
Link: https://lore.kernel.org/lkml/1d9a2e6df2a9a35b2cdd50a9a68cac5991e7e5f0.camel@intel.com
Enhanced-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://lore.kernel.org/lkml/YQKa76A6XuFqgM03@phenom.ffwll.local
Acked-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/stddef.h | 48 +++++++++++++++++++++++++++++++++++++
include/uapi/linux/stddef.h | 21 ++++++++++++++++
scripts/kernel-doc | 7 ++++++
3 files changed, 76 insertions(+)
diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index 8553b33143d1..8b103a53b000 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -36,4 +36,52 @@ enum {
#define offsetofend(TYPE, MEMBER) \
(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
+/**
+ * struct_group() - Wrap a set of declarations in a mirrored struct
+ *
+ * @NAME: The identifier name of the mirrored sub-struct
+ * @MEMBERS: The member declarations for the mirrored structs
+ *
+ * Used to create an anonymous union of two structs with identical
+ * layout and size: one anonymous and one named. The former can be
+ * used normally without sub-struct naming, and the latter can be
+ * used to reason about the start, end, and size of the group of
+ * struct members.
+ */
+#define struct_group(NAME, MEMBERS...) \
+ __struct_group(/* no tag */, NAME, /* no attrs */, MEMBERS)
+
+/**
+ * struct_group_attr() - Create a struct_group() with trailing attributes
+ *
+ * @NAME: The identifier name of the mirrored sub-struct
+ * @ATTRS: Any struct attributes to apply
+ * @MEMBERS: The member declarations for the mirrored structs
+ *
+ * Used to create an anonymous union of two structs with identical
+ * layout and size: one anonymous and one named. The former can be
+ * used normally without sub-struct naming, and the latter can be
+ * used to reason about the start, end, and size of the group of
+ * struct members. Includes structure attributes argument.
+ */
+#define struct_group_attr(NAME, ATTRS, MEMBERS...) \
+ __struct_group(/* no tag */, NAME, ATTRS, MEMBERS)
+
+/**
+ * struct_group_tagged() - Create a struct_group with a reusable tag
+ *
+ * @TAG: The tag name for the named sub-struct
+ * @NAME: The identifier name of the mirrored sub-struct
+ * @MEMBERS: The member declarations for the mirrored structs
+ *
+ * Used to create an anonymous union of two structs with identical
+ * layout and size: one anonymous and one named. The former can be
+ * used normally without sub-struct naming, and the latter can be
+ * used to reason about the start, end, and size of the group of
+ * struct members. Includes struct tag argument for the named copy,
+ * so the specified layout can be reused later.
+ */
+#define struct_group_tagged(TAG, NAME, MEMBERS...) \
+ __struct_group(TAG, NAME, /* no attrs */, MEMBERS)
+
#endif
diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h
index ee8220f8dcf5..610204f7c275 100644
--- a/include/uapi/linux/stddef.h
+++ b/include/uapi/linux/stddef.h
@@ -4,3 +4,24 @@
#ifndef __always_inline
#define __always_inline inline
#endif
+
+/**
+ * __struct_group() - Create a mirrored named and anonyomous struct
+ *
+ * @TAG: The tag name for the named sub-struct (usually empty)
+ * @NAME: The identifier name of the mirrored sub-struct
+ * @ATTRS: Any struct attributes (usually empty)
+ * @MEMBERS: The member declarations for the mirrored structs
+ *
+ * Used to create an anonymous union of two structs with identical layout
+ * and size: one anonymous and one named. The former's members can be used
+ * normally without sub-struct naming, and the latter can be used to
+ * reason about the start, end, and size of the group of struct members.
+ * The named struct can also be explicitly tagged for layer reuse, as well
+ * as both having struct attributes appended.
+ */
+#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \
+ union { \
+ struct { MEMBERS } ATTRS; \
+ struct TAG { MEMBERS } ATTRS NAME; \
+ }
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 7c4a6a507ac4..d9715efbe0b7 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1245,6 +1245,13 @@ sub dump_struct($$) {
$members =~ s/\s*CRYPTO_MINALIGN_ATTR/ /gos;
$members =~ s/\s*____cacheline_aligned_in_smp/ /gos;
$members =~ s/\s*____cacheline_aligned/ /gos;
+ # unwrap struct_group():
+ # - first eat non-declaration parameters and rewrite for final match
+ # - then remove macro, outer parens, and trailing semicolon
+ $members =~ s/\bstruct_group\s*\(([^,]*,)/STRUCT_GROUP(/gos;
+ $members =~ s/\bstruct_group_(attr|tagged)\s*\(([^,]*,){2}/STRUCT_GROUP(/gos;
+ $members =~ s/\b__struct_group\s*\(([^,]*,){3}/STRUCT_GROUP(/gos;
+ $members =~ s/\bSTRUCT_GROUP(\(((?:(?>[^)(]+)|(?1))*)\))[^;]*;/$2/gos;
my $args = qr{([^,)]+)};
# replace DECLARE_BITMAP
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 05/25] cxl/core: Replace unions with struct_group()
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (3 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 04/25] stddef: Introduce struct_group() helper macro Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 06/25] bnxt_en: Use struct_group_attr() for memcpy() region Kees Cook
` (19 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Alison Schofield, Vishal Verma, Ira Weiny,
Ben Widawsky, linux-cxl, Dan Williams, Rasmus Villemoes,
Daniel Micay, Francis Laniel, Bart Van Assche, David Gow,
linux-mm, clang-built-linux, linux-hardening
Use the newly introduced struct_group_typed() macro to clean up the
declaration of struct cxl_regs.
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Ben Widawsky <ben.widawsky@intel.com>
Cc: linux-cxl@vger.kernel.org
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Link: https://lore.kernel.org/lkml/1d9a2e6df2a9a35b2cdd50a9a68cac5991e7e5f0.camel@intel.com
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/cxl/cxl.h | 61 ++++++++++++++---------------------------------
1 file changed, 18 insertions(+), 43 deletions(-)
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index b6bda39a59e3..97a83ba4e783 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -75,52 +75,27 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr)
#define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18
#define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
-#define CXL_COMPONENT_REGS() \
- void __iomem *hdm_decoder
-
-#define CXL_DEVICE_REGS() \
- void __iomem *status; \
- void __iomem *mbox; \
- void __iomem *memdev
-
-/* See note for 'struct cxl_regs' for the rationale of this organization */
-/*
- * CXL_COMPONENT_REGS - Common set of CXL Component register block base pointers
- * @hdm_decoder: CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure
- */
-struct cxl_component_regs {
- CXL_COMPONENT_REGS();
-};
-
-/* See note for 'struct cxl_regs' for the rationale of this organization */
-/*
- * CXL_DEVICE_REGS - Common set of CXL Device register block base pointers
- * @status: CXL 2.0 8.2.8.3 Device Status Registers
- * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers
- * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers
- */
-struct cxl_device_regs {
- CXL_DEVICE_REGS();
-};
-
/*
- * Note, the anonymous union organization allows for per
- * register-block-type helper routines, without requiring block-type
- * agnostic code to include the prefix.
+ * Using struct_group() allows for per register-block-type helper routines,
+ * without requiring block-type agnostic code to include the prefix.
*/
struct cxl_regs {
- union {
- struct {
- CXL_COMPONENT_REGS();
- };
- struct cxl_component_regs component;
- };
- union {
- struct {
- CXL_DEVICE_REGS();
- };
- struct cxl_device_regs device_regs;
- };
+ /*
+ * Common set of CXL Component register block base pointers
+ * @hdm_decoder: CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure
+ */
+ struct_group_tagged(cxl_component_regs, component,
+ void __iomem *hdm_decoder;
+ );
+ /*
+ * Common set of CXL Device register block base pointers
+ * @status: CXL 2.0 8.2.8.3 Device Status Registers
+ * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers
+ * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers
+ */
+ struct_group_tagged(cxl_device_regs, device_regs,
+ void __iomem *status, *mbox, *memdev;
+ );
};
struct cxl_reg_map {
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 06/25] bnxt_en: Use struct_group_attr() for memcpy() region
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (4 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 05/25] cxl/core: Replace unions with struct_group() Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 07/25] iommu/amd: Use struct_group() " Kees Cook
` (18 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Michael Chan, Gustavo A . R . Silva, Rasmus Villemoes,
Daniel Micay, Francis Laniel, Bart Van Assche, David Gow,
linux-mm, clang-built-linux, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() around members queue_id, min_bw, max_bw, tsa, pri_lvl,
and bw_weight so they can be referenced together. This will allow memcpy()
and sizeof() to more easily reason about sizes, improve readability,
and avoid future warnings about writing beyond the end of queue_id.
"pahole" shows no size nor member offset changes to struct bnxt_cos2bw_cfg.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
Link: https://lore.kernel.org/lkml/CACKFLinDc6Y+P8eZ=450yA1nMC7swTURLtcdyiNR=9J6dfFyBg@mail.gmail.com
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Link: https://lore.kernel.org/lkml/20210728044517.GE35706@embeddedor
---
drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c | 4 ++--
drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h | 14 ++++++++------
2 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
index 8e90224c43a2..2ddebbfc9cc9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c
@@ -148,10 +148,10 @@ static int bnxt_hwrm_queue_cos2bw_qcfg(struct bnxt *bp, struct ieee_ets *ets)
}
data = &resp->queue_id0 + offsetof(struct bnxt_cos2bw_cfg, queue_id);
- for (i = 0; i < bp->max_tc; i++, data += sizeof(cos2bw) - 4) {
+ for (i = 0; i < bp->max_tc; i++, data += sizeof(cos2bw.cfg)) {
int tc;
- memcpy(&cos2bw.queue_id, data, sizeof(cos2bw) - 4);
+ memcpy(&cos2bw.cfg, data, sizeof(cos2bw.cfg));
if (i == 0)
cos2bw.queue_id = resp->queue_id0;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h
index 6eed231de565..716742522161 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h
@@ -23,13 +23,15 @@ struct bnxt_dcb {
struct bnxt_cos2bw_cfg {
u8 pad[3];
- u8 queue_id;
- __le32 min_bw;
- __le32 max_bw;
+ struct_group_attr(cfg, __packed,
+ u8 queue_id;
+ __le32 min_bw;
+ __le32 max_bw;
#define BW_VALUE_UNIT_PERCENT1_100 (0x1UL << 29)
- u8 tsa;
- u8 pri_lvl;
- u8 bw_weight;
+ u8 tsa;
+ u8 pri_lvl;
+ u8 bw_weight;
+ );
u8 unused;
};
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 07/25] iommu/amd: Use struct_group() for memcpy() region
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (5 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 06/25] bnxt_en: Use struct_group_attr() for memcpy() region Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 08/25] drm/mga/mga_ioc32: " Kees Cook
` (17 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Joerg Roedel, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct ivhd_entry around members ext and hidh, so
they can be referenced together. This will allow memcpy() and sizeof()
to more easily reason about sizes, improve readability, and avoid future
warnings about writing beyond the end of ext.
"pahole" shows no size nor member offset changes to struct ivhd_entry.
"objdump -d" shows no object code changes.
Acked-by: Joerg Roedel <jroedel@suse.de>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/iommu/amd/init.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 46280e6e1535..2df84737417b 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -121,8 +121,10 @@ struct ivhd_entry {
u8 type;
u16 devid;
u8 flags;
- u32 ext;
- u32 hidh;
+ struct_group(ext_hid,
+ u32 ext;
+ u32 hidh;
+ );
u64 cid;
u8 uidf;
u8 uidl;
@@ -1378,7 +1380,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu *iommu,
break;
}
- memcpy(hid, (u8 *)(&e->ext), ACPIHID_HID_LEN - 1);
+ BUILD_BUG_ON(sizeof(e->ext_hid) != ACPIHID_HID_LEN - 1);
+ memcpy(hid, &e->ext_hid, ACPIHID_HID_LEN - 1);
hid[ACPIHID_HID_LEN - 1] = '\0';
if (!(*hid)) {
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 08/25] drm/mga/mga_ioc32: Use struct_group() for memcpy() region
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (6 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 07/25] iommu/amd: Use struct_group() " Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 09/25] HID: cp2112: " Kees Cook
` (16 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Daniel Vetter, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct drm32_mga_init around members chipset, sgram,
maccess, fb_cpp, front_offset, front_pitch, back_offset, back_pitch,
depth_cpp, depth_offset, depth_pitch, texture_offset, and texture_size,
so they can be referenced together. This will allow memcpy() and sizeof()
to more easily reason about sizes, improve readability, and avoid future
warnings about writing beyond the end of chipset.
"pahole" shows no size nor member offset changes to struct drm32_mga_init.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).
Note that since this is a UAPI header, __struct_group() is used
directly.
Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Daniel Vetter <daniel@ffwll.ch>
Link: https://lore.kernel.org/lkml/YQKa76A6XuFqgM03@phenom.ffwll.local
---
drivers/gpu/drm/mga/mga_ioc32.c | 27 ++++++++++++++-------------
include/uapi/drm/mga_drm.h | 22 ++++++++++++----------
2 files changed, 26 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/mga/mga_ioc32.c b/drivers/gpu/drm/mga/mga_ioc32.c
index 4fd4de16cd32..894472921c30 100644
--- a/drivers/gpu/drm/mga/mga_ioc32.c
+++ b/drivers/gpu/drm/mga/mga_ioc32.c
@@ -38,16 +38,18 @@
typedef struct drm32_mga_init {
int func;
u32 sarea_priv_offset;
- int chipset;
- int sgram;
- unsigned int maccess;
- unsigned int fb_cpp;
- unsigned int front_offset, front_pitch;
- unsigned int back_offset, back_pitch;
- unsigned int depth_cpp;
- unsigned int depth_offset, depth_pitch;
- unsigned int texture_offset[MGA_NR_TEX_HEAPS];
- unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ struct_group(always32bit,
+ int chipset;
+ int sgram;
+ unsigned int maccess;
+ unsigned int fb_cpp;
+ unsigned int front_offset, front_pitch;
+ unsigned int back_offset, back_pitch;
+ unsigned int depth_cpp;
+ unsigned int depth_offset, depth_pitch;
+ unsigned int texture_offset[MGA_NR_TEX_HEAPS];
+ unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ );
u32 fb_offset;
u32 mmio_offset;
u32 status_offset;
@@ -67,9 +69,8 @@ static int compat_mga_init(struct file *file, unsigned int cmd,
init.func = init32.func;
init.sarea_priv_offset = init32.sarea_priv_offset;
- memcpy(&init.chipset, &init32.chipset,
- offsetof(drm_mga_init_t, fb_offset) -
- offsetof(drm_mga_init_t, chipset));
+ memcpy(&init.always32bit, &init32.always32bit,
+ sizeof(init32.always32bit));
init.fb_offset = init32.fb_offset;
init.mmio_offset = init32.mmio_offset;
init.status_offset = init32.status_offset;
diff --git a/include/uapi/drm/mga_drm.h b/include/uapi/drm/mga_drm.h
index 8c4337548ab5..bb31567e66c0 100644
--- a/include/uapi/drm/mga_drm.h
+++ b/include/uapi/drm/mga_drm.h
@@ -279,20 +279,22 @@ typedef struct drm_mga_init {
unsigned long sarea_priv_offset;
- int chipset;
- int sgram;
+ __struct_group(/* no tag */, always32bit, /* no attrs */,
+ int chipset;
+ int sgram;
- unsigned int maccess;
+ unsigned int maccess;
- unsigned int fb_cpp;
- unsigned int front_offset, front_pitch;
- unsigned int back_offset, back_pitch;
+ unsigned int fb_cpp;
+ unsigned int front_offset, front_pitch;
+ unsigned int back_offset, back_pitch;
- unsigned int depth_cpp;
- unsigned int depth_offset, depth_pitch;
+ unsigned int depth_cpp;
+ unsigned int depth_offset, depth_pitch;
- unsigned int texture_offset[MGA_NR_TEX_HEAPS];
- unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ unsigned int texture_offset[MGA_NR_TEX_HEAPS];
+ unsigned int texture_size[MGA_NR_TEX_HEAPS];
+ );
unsigned long fb_offset;
unsigned long mmio_offset;
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 09/25] HID: cp2112: Use struct_group() for memcpy() region
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (7 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 08/25] drm/mga/mga_ioc32: " Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 10/25] HID: roccat: Use struct_group() to zero kone_mouse_event Kees Cook
` (15 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Jiri Kosina, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in struct cp2112_string_report around members report,
length, type, and string, so they can be referenced together. This will
allow memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end of
report.
"pahole" shows no size nor member offset changes to struct
cp2112_string_report. "objdump -d" shows no meaningful object
code changes (i.e. only source line number induced differences.)
Acked-by: Jiri Kosina <jikos@kernel.org>
Link: https://lore.kernel.org/lkml/nycvar.YFH.7.76.2108201810560.15313@cbobk.fhfr.pm
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/hid/hid-cp2112.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 477baa30889c..ece147d1a278 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -129,10 +129,12 @@ struct cp2112_xfer_status_report {
struct cp2112_string_report {
u8 dummy; /* force .string to be aligned */
- u8 report; /* CP2112_*_STRING */
- u8 length; /* length in bytes of everyting after .report */
- u8 type; /* USB_DT_STRING */
- wchar_t string[30]; /* UTF16_LITTLE_ENDIAN string */
+ struct_group_attr(contents, __packed,
+ u8 report; /* CP2112_*_STRING */
+ u8 length; /* length in bytes of everything after .report */
+ u8 type; /* USB_DT_STRING */
+ wchar_t string[30]; /* UTF16_LITTLE_ENDIAN string */
+ );
} __packed;
/* Number of times to request transfer status before giving up waiting for a
@@ -986,8 +988,8 @@ static ssize_t pstr_show(struct device *kdev,
u8 length;
int ret;
- ret = cp2112_hid_get(hdev, attr->report, &report.report,
- sizeof(report) - 1, HID_FEATURE_REPORT);
+ ret = cp2112_hid_get(hdev, attr->report, (u8 *)&report.contents,
+ sizeof(report.contents), HID_FEATURE_REPORT);
if (ret < 3) {
hid_err(hdev, "error reading %s string: %d\n", kattr->attr.name,
ret);
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 10/25] HID: roccat: Use struct_group() to zero kone_mouse_event
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (8 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 09/25] HID: cp2112: " Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 11/25] can: flexcan: Use struct_group() to zero struct flexcan_regs regions Kees Cook
` (14 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Jiri Kosina, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct kone_mouse_event that should
be initialized to zero.
Acked-by: Jiri Kosina <jikos@kernel.org>
Link: https://lore.kernel.org/lkml/nycvar.YFH.7.76.2108201810560.15313@cbobk.fhfr.pm
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/hid/hid-roccat-kone.c | 2 +-
drivers/hid/hid-roccat-kone.h | 12 +++++++-----
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index 1ca64481145e..ea17abc7ad52 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -857,7 +857,7 @@ static int kone_raw_event(struct hid_device *hdev, struct hid_report *report,
memcpy(&kone->last_mouse_event, event,
sizeof(struct kone_mouse_event));
else
- memset(&event->tilt, 0, 5);
+ memset(&event->wipe, 0, sizeof(event->wipe));
kone_keep_values_up_to_date(kone, event);
diff --git a/drivers/hid/hid-roccat-kone.h b/drivers/hid/hid-roccat-kone.h
index 4a1a9cb76b08..65c800e3addc 100644
--- a/drivers/hid/hid-roccat-kone.h
+++ b/drivers/hid/hid-roccat-kone.h
@@ -152,11 +152,13 @@ struct kone_mouse_event {
uint16_t x;
uint16_t y;
uint8_t wheel; /* up = 1, down = -1 */
- uint8_t tilt; /* right = 1, left = -1 */
- uint8_t unknown;
- uint8_t event;
- uint8_t value; /* press = 0, release = 1 */
- uint8_t macro_key; /* 0 to 8 */
+ struct_group(wipe,
+ uint8_t tilt; /* right = 1, left = -1 */
+ uint8_t unknown;
+ uint8_t event;
+ uint8_t value; /* press = 0, release = 1 */
+ uint8_t macro_key; /* 0 to 8 */
+ );
} __attribute__ ((__packed__));
enum kone_mouse_events {
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 11/25] can: flexcan: Use struct_group() to zero struct flexcan_regs regions
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (9 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 10/25] HID: roccat: Use struct_group() to zero kone_mouse_event Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 12/25] cm4000_cs: Use struct_group() to zero struct cm4000_dev region Kees Cook
` (13 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Marc Kleine-Budde, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark both regions of struct flexcan_regs that get
initialized to zero. Avoid the future warnings:
In function 'fortify_memset_chk',
inlined from 'memset_io' at ./include/asm-generic/io.h:1169:2,
inlined from 'flexcan_ram_init' at drivers/net/can/flexcan.c:1403:2:
./include/linux/fortify-string.h:199:4: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
199 | __write_overflow_field(p_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'fortify_memset_chk',
inlined from 'memset_io' at ./include/asm-generic/io.h:1169:2,
inlined from 'flexcan_ram_init' at drivers/net/can/flexcan.c:1408:3:
./include/linux/fortify-string.h:199:4: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
199 | __write_overflow_field(p_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/net/can/flexcan.c | 68 +++++++++++++++++++--------------------
1 file changed, 34 insertions(+), 34 deletions(-)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 57f3635ad8d7..b1a261622b76 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -284,31 +284,33 @@ struct flexcan_regs {
u32 dbg1; /* 0x58 */
u32 dbg2; /* 0x5c */
u32 _reserved3[8]; /* 0x60 */
- u8 mb[2][512]; /* 0x80 - Not affected by Soft Reset */
- /* FIFO-mode:
- * MB
- * 0x080...0x08f 0 RX message buffer
- * 0x090...0x0df 1-5 reserved
- * 0x0e0...0x0ff 6-7 8 entry ID table
- * (mx25, mx28, mx35, mx53)
- * 0x0e0...0x2df 6-7..37 8..128 entry ID table
- * size conf'ed via ctrl2::RFFN
- * (mx6, vf610)
- */
- u32 _reserved4[256]; /* 0x480 */
- u32 rximr[64]; /* 0x880 - Not affected by Soft Reset */
- u32 _reserved5[24]; /* 0x980 */
- u32 gfwr_mx6; /* 0x9e0 - MX6 */
- u32 _reserved6[39]; /* 0x9e4 */
- u32 _rxfir[6]; /* 0xa80 */
- u32 _reserved8[2]; /* 0xa98 */
- u32 _rxmgmask; /* 0xaa0 */
- u32 _rxfgmask; /* 0xaa4 */
- u32 _rx14mask; /* 0xaa8 */
- u32 _rx15mask; /* 0xaac */
- u32 tx_smb[4]; /* 0xab0 */
- u32 rx_smb0[4]; /* 0xac0 */
- u32 rx_smb1[4]; /* 0xad0 */
+ struct_group(init,
+ u8 mb[2][512]; /* 0x80 - Not affected by Soft Reset */
+ /* FIFO-mode:
+ * MB
+ * 0x080...0x08f 0 RX message buffer
+ * 0x090...0x0df 1-5 reserved
+ * 0x0e0...0x0ff 6-7 8 entry ID table
+ * (mx25, mx28, mx35, mx53)
+ * 0x0e0...0x2df 6-7..37 8..128 entry ID table
+ * size conf'ed via ctrl2::RFFN
+ * (mx6, vf610)
+ */
+ u32 _reserved4[256]; /* 0x480 */
+ u32 rximr[64]; /* 0x880 - Not affected by Soft Reset */
+ u32 _reserved5[24]; /* 0x980 */
+ u32 gfwr_mx6; /* 0x9e0 - MX6 */
+ u32 _reserved6[39]; /* 0x9e4 */
+ u32 _rxfir[6]; /* 0xa80 */
+ u32 _reserved8[2]; /* 0xa98 */
+ u32 _rxmgmask; /* 0xaa0 */
+ u32 _rxfgmask; /* 0xaa4 */
+ u32 _rx14mask; /* 0xaa8 */
+ u32 _rx15mask; /* 0xaac */
+ u32 tx_smb[4]; /* 0xab0 */
+ u32 rx_smb0[4]; /* 0xac0 */
+ u32 rx_smb1[4]; /* 0xad0 */
+ );
u32 mecr; /* 0xae0 */
u32 erriar; /* 0xae4 */
u32 erridpr; /* 0xae8 */
@@ -322,9 +324,11 @@ struct flexcan_regs {
u32 fdcbt; /* 0xc04 - Not affected by Soft Reset */
u32 fdcrc; /* 0xc08 */
u32 _reserved9[199]; /* 0xc0c */
- u32 tx_smb_fd[18]; /* 0xf28 */
- u32 rx_smb0_fd[18]; /* 0xf70 */
- u32 rx_smb1_fd[18]; /* 0xfb8 */
+ struct_group(init_fd,
+ u32 tx_smb_fd[18]; /* 0xf28 */
+ u32 rx_smb0_fd[18]; /* 0xf70 */
+ u32 rx_smb1_fd[18]; /* 0xfb8 */
+ );
};
static_assert(sizeof(struct flexcan_regs) == 0x4 * 18 + 0xfb8);
@@ -1379,14 +1383,10 @@ static void flexcan_ram_init(struct net_device *dev)
reg_ctrl2 |= FLEXCAN_CTRL2_WRMFRZ;
priv->write(reg_ctrl2, ®s->ctrl2);
- memset_io(®s->mb[0][0], 0,
- offsetof(struct flexcan_regs, rx_smb1[3]) -
- offsetof(struct flexcan_regs, mb[0][0]) + 0x4);
+ memset_io(®s->init, 0, sizeof(regs->init));
if (priv->can.ctrlmode & CAN_CTRLMODE_FD)
- memset_io(®s->tx_smb_fd[0], 0,
- offsetof(struct flexcan_regs, rx_smb1_fd[17]) -
- offsetof(struct flexcan_regs, tx_smb_fd[0]) + 0x4);
+ memset_io(®s->init_fd, 0, sizeof(regs->init_fd));
reg_ctrl2 &= ~FLEXCAN_CTRL2_WRMFRZ;
priv->write(reg_ctrl2, ®s->ctrl2);
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 12/25] cm4000_cs: Use struct_group() to zero struct cm4000_dev region
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (10 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 11/25] can: flexcan: Use struct_group() to zero struct flexcan_regs regions Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 13/25] compiler_types.h: Remove __compiletime_object_size() Kees Cook
` (12 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Greg Kroah-Hartman, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct cm4000_dev that should be
initialized to zero.
Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Link: https://lore.kernel.org/lkml/YQDvxAofJlI1JoGZ@kroah.com
---
drivers/char/pcmcia/cm4000_cs.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
index 8f1bce0b4fe5..adaec8fd4b16 100644
--- a/drivers/char/pcmcia/cm4000_cs.c
+++ b/drivers/char/pcmcia/cm4000_cs.c
@@ -116,8 +116,9 @@ struct cm4000_dev {
wait_queue_head_t atrq; /* wait for ATR valid */
wait_queue_head_t readq; /* used by write to wake blk.read */
- /* warning: do not move this fields.
+ /* warning: do not move this struct group.
* initialising to zero depends on it - see ZERO_DEV below. */
+ struct_group(init,
unsigned char atr_csum;
unsigned char atr_len_retry;
unsigned short atr_len;
@@ -140,12 +141,10 @@ struct cm4000_dev {
struct timer_list timer; /* used to keep monitor running */
int monitor_running;
+ );
};
-#define ZERO_DEV(dev) \
- memset(&dev->atr_csum,0, \
- sizeof(struct cm4000_dev) - \
- offsetof(struct cm4000_dev, atr_csum))
+#define ZERO_DEV(dev) memset(&((dev)->init), 0, sizeof((dev)->init))
static struct pcmcia_device *dev_table[CM4000_MAX_DEV];
static struct class *cmm_class;
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 13/25] compiler_types.h: Remove __compiletime_object_size()
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (11 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 12/25] cm4000_cs: Use struct_group() to zero struct cm4000_dev region Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-23 6:43 ` Rasmus Villemoes
2021-08-22 7:51 ` [PATCH for-next 14/25] lib/string: Move helper functions out of string.c Kees Cook
` (11 subsequent siblings)
24 siblings, 1 reply; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Nick Desaulniers, Miguel Ojeda, Rasmus Villemoes,
Daniel Micay, Francis Laniel, Bart Van Assche, David Gow,
linux-mm, clang-built-linux, linux-hardening
Since all compilers support __builtin_object_size(), and there is only
one user of __compiletime_object_size, remove it to avoid the needless
indirection. This lets Clang reason about check_copy_size() correctly.
Link: https://github.com/ClangBuiltLinux/linux/issues/1179
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/compiler-gcc.h | 2 --
include/linux/compiler_types.h | 4 ----
include/linux/thread_info.h | 2 +-
3 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index cb9217fc60af..01985821944b 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -41,8 +41,6 @@
#define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
-#define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
-
#define __compiletime_warning(message) __attribute__((__warning__(message)))
#define __compiletime_error(message) __attribute__((__error__(message)))
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index e4ea86fc584d..c43308b0a9a9 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -290,10 +290,6 @@ struct ftrace_likely_data {
(sizeof(t) == sizeof(char) || sizeof(t) == sizeof(short) || \
sizeof(t) == sizeof(int) || sizeof(t) == sizeof(long))
-/* Compile time object size, -1 for unknown */
-#ifndef __compiletime_object_size
-# define __compiletime_object_size(obj) -1
-#endif
#ifndef __compiletime_warning
# define __compiletime_warning(message)
#endif
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 0999f6317978..ad0c4e041030 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -203,7 +203,7 @@ static inline void copy_overflow(int size, unsigned long count)
static __always_inline __must_check bool
check_copy_size(const void *addr, size_t bytes, bool is_source)
{
- int sz = __compiletime_object_size(addr);
+ int sz = __builtin_object_size(addr, 0);
if (unlikely(sz >= 0 && sz < bytes)) {
if (!__builtin_constant_p(bytes))
copy_overflow(sz, bytes);
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH for-next 13/25] compiler_types.h: Remove __compiletime_object_size()
2021-08-22 7:51 ` [PATCH for-next 13/25] compiler_types.h: Remove __compiletime_object_size() Kees Cook
@ 2021-08-23 6:43 ` Rasmus Villemoes
2021-08-25 19:43 ` Nick Desaulniers
0 siblings, 1 reply; 42+ messages in thread
From: Rasmus Villemoes @ 2021-08-23 6:43 UTC (permalink / raw)
To: Kees Cook, linux-kernel
Cc: Nick Desaulniers, Miguel Ojeda, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
On 22/08/2021 09.51, Kees Cook wrote:
> - int sz = __compiletime_object_size(addr);
> + int sz = __builtin_object_size(addr, 0);
Not directly related to this patch, but seeing this I wonder if there
would be some value in introducing names for those magic 0/1/2/3 that
are used with __b_o_s. Every time I stumble on code using that I have to
go to the gcc docs, and even then it takes me a while to grok what
TYPE is an integer constant from 0 to 3. If the least significant
bit is clear, objects are whole variables, if it is set, a closest
surrounding subobject is considered the object a pointer points to.
The second bit determines if maximum or minimum of remaining bytes
is computed.
means. The names don't need to be too verbose, just having a few
#defines in-tree with the above quoted above them makes it a lot easier
to figure out what they mean.
Rasmus
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH for-next 13/25] compiler_types.h: Remove __compiletime_object_size()
2021-08-23 6:43 ` Rasmus Villemoes
@ 2021-08-25 19:43 ` Nick Desaulniers
0 siblings, 0 replies; 42+ messages in thread
From: Nick Desaulniers @ 2021-08-25 19:43 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Kees Cook, linux-kernel, Miguel Ojeda, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
On Sun, Aug 22, 2021 at 11:43 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 22/08/2021 09.51, Kees Cook wrote:
>
> > - int sz = __compiletime_object_size(addr);
> > + int sz = __builtin_object_size(addr, 0);
>
> Not directly related to this patch, but seeing this I wonder if there
> would be some value in introducing names for those magic 0/1/2/3 that
> are used with __b_o_s. Every time I stumble on code using that I have to
> go to the gcc docs, and even then it takes me a while to grok what
>
> TYPE is an integer constant from 0 to 3. If the least significant
> bit is clear, objects are whole variables, if it is set, a closest
> surrounding subobject is considered the object a pointer points to.
> The second bit determines if maximum or minimum of remaining bytes
> is computed.
>
> means. The names don't need to be too verbose, just having a few
> #defines in-tree with the above quoted above them makes it a lot easier
> to figure out what they mean.
We share a similar experience.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH for-next 14/25] lib/string: Move helper functions out of string.c
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (12 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 13/25] compiler_types.h: Remove __compiletime_object_size() Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-25 21:48 ` Nick Desaulniers
2021-08-22 7:51 ` [PATCH for-next 15/25] fortify: Move remaining fortify helpers into fortify-string.h Kees Cook
` (10 subsequent siblings)
24 siblings, 1 reply; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Andy Shevchenko, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
The core functions of string.c are those that may be implemented by
per-architecture functions, or overloaded by FORTIFY_SOURCE. As a
result, it needs to be built with __NO_FORTIFY. Without this, macros
will collide with function declarations. This was accidentally working
due to -ffreestanding (on some architectures). Make this deterministic
by explicitly setting __NO_FORTIFY and move all the helper functions
into string_helpers.c so that they gain the fortification coverage they
had been missing.
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
arch/arm/boot/compressed/string.c | 1 +
arch/s390/lib/string.c | 3 +
arch/x86/boot/compressed/misc.h | 2 +
arch/x86/boot/compressed/pgtable_64.c | 2 +
arch/x86/lib/string_32.c | 1 +
lib/string.c | 210 +-------------------------
lib/string_helpers.c | 193 +++++++++++++++++++++++
7 files changed, 208 insertions(+), 204 deletions(-)
diff --git a/arch/arm/boot/compressed/string.c b/arch/arm/boot/compressed/string.c
index 8c0fa276d994..fcc678fce045 100644
--- a/arch/arm/boot/compressed/string.c
+++ b/arch/arm/boot/compressed/string.c
@@ -5,6 +5,7 @@
* Small subset of simple string routines
*/
+#define __NO_FORTIFY
#include <linux/string.h>
/*
diff --git a/arch/s390/lib/string.c b/arch/s390/lib/string.c
index cfcdf76d6a95..392fb9f4f4db 100644
--- a/arch/s390/lib/string.c
+++ b/arch/s390/lib/string.c
@@ -8,6 +8,9 @@
*/
#define IN_ARCH_STRING_C 1
+#ifndef __NO_FORTIFY
+# define __NO_FORTIFY
+#endif
#include <linux/types.h>
#include <linux/string.h>
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 31139256859f..49bde196da9b 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -14,6 +14,8 @@
#undef CONFIG_KASAN
#undef CONFIG_KASAN_GENERIC
+#define __NO_FORTIFY
+
/* cpu_feature_enabled() cannot be used this early */
#define USE_EARLY_PGTABLE_L5
diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c
index 2a78746f5a4c..a1733319a22a 100644
--- a/arch/x86/boot/compressed/pgtable_64.c
+++ b/arch/x86/boot/compressed/pgtable_64.c
@@ -1,3 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "misc.h"
#include <linux/efi.h>
#include <asm/e820/types.h>
#include <asm/processor.h>
diff --git a/arch/x86/lib/string_32.c b/arch/x86/lib/string_32.c
index d15fdae9656e..53b3f202267c 100644
--- a/arch/x86/lib/string_32.c
+++ b/arch/x86/lib/string_32.c
@@ -11,6 +11,7 @@
* strings.
*/
+#define __NO_FORTIFY
#include <linux/string.h>
#include <linux/export.h>
diff --git a/lib/string.c b/lib/string.c
index 77bd0b1d3296..1e6259f263b8 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -6,20 +6,15 @@
*/
/*
- * stupid library routines.. The optimized versions should generally be found
- * as inline code in <asm-xx/string.h>
+ * This file should be used only for "library" routines that may have
+ * alternative implementations on specific architectures (generally
+ * found in <asm-xx/string.h>), or get overloaded by FORTIFY_SOURCE.
+ * (Specifically, this file is built with __NO_FORTIFY.)
*
- * These are buggy as well..
- *
- * * Fri Jun 25 1999, Ingo Oeser <ioe@informatik.tu-chemnitz.de>
- * - Added strsep() which will replace strtok() soon (because strsep() is
- * reentrant and should be faster). Use only strsep() in new code, please.
- *
- * * Sat Feb 09 2002, Jason Thomas <jason@topic.com.au>,
- * Matthew Hawkins <matt@mh.dropbear.id.au>
- * - Kissed strtok() goodbye
+ * Other helper functions should live in string_helpers.c.
*/
+#define __NO_FORTIFY
#include <linux/types.h>
#include <linux/string.h>
#include <linux/ctype.h>
@@ -237,40 +232,6 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
EXPORT_SYMBOL(strscpy);
#endif
-/**
- * strscpy_pad() - Copy a C-string into a sized buffer
- * @dest: Where to copy the string to
- * @src: Where to copy the string from
- * @count: Size of destination buffer
- *
- * Copy the string, or as much of it as fits, into the dest buffer. The
- * behavior is undefined if the string buffers overlap. The destination
- * buffer is always %NUL terminated, unless it's zero-sized.
- *
- * If the source string is shorter than the destination buffer, zeros
- * the tail of the destination buffer.
- *
- * For full explanation of why you may want to consider using the
- * 'strscpy' functions please see the function docstring for strscpy().
- *
- * Returns:
- * * The number of characters copied (not including the trailing %NUL)
- * * -E2BIG if count is 0 or @src was truncated.
- */
-ssize_t strscpy_pad(char *dest, const char *src, size_t count)
-{
- ssize_t written;
-
- written = strscpy(dest, src, count);
- if (written < 0 || written == count - 1)
- return written;
-
- memset(dest + written + 1, 0, count - written - 1);
-
- return written;
-}
-EXPORT_SYMBOL(strscpy_pad);
-
/**
* stpcpy - copy a string from src to dest returning a pointer to the new end
* of dest, including src's %NUL-terminator. May overrun dest.
@@ -513,46 +474,6 @@ char *strnchr(const char *s, size_t count, int c)
EXPORT_SYMBOL(strnchr);
#endif
-/**
- * skip_spaces - Removes leading whitespace from @str.
- * @str: The string to be stripped.
- *
- * Returns a pointer to the first non-whitespace character in @str.
- */
-char *skip_spaces(const char *str)
-{
- while (isspace(*str))
- ++str;
- return (char *)str;
-}
-EXPORT_SYMBOL(skip_spaces);
-
-/**
- * strim - Removes leading and trailing whitespace from @s.
- * @s: The string to be stripped.
- *
- * Note that the first trailing whitespace is replaced with a %NUL-terminator
- * in the given string @s. Returns a pointer to the first non-whitespace
- * character in @s.
- */
-char *strim(char *s)
-{
- size_t size;
- char *end;
-
- size = strlen(s);
- if (!size)
- return s;
-
- end = s + size - 1;
- while (end >= s && isspace(*end))
- end--;
- *(end + 1) = '\0';
-
- return skip_spaces(s);
-}
-EXPORT_SYMBOL(strim);
-
#ifndef __HAVE_ARCH_STRLEN
/**
* strlen - Find the length of a string
@@ -687,101 +608,6 @@ char *strsep(char **s, const char *ct)
EXPORT_SYMBOL(strsep);
#endif
-/**
- * sysfs_streq - return true if strings are equal, modulo trailing newline
- * @s1: one string
- * @s2: another string
- *
- * This routine returns true iff two strings are equal, treating both
- * NUL and newline-then-NUL as equivalent string terminations. It's
- * geared for use with sysfs input strings, which generally terminate
- * with newlines but are compared against values without newlines.
- */
-bool sysfs_streq(const char *s1, const char *s2)
-{
- while (*s1 && *s1 == *s2) {
- s1++;
- s2++;
- }
-
- if (*s1 == *s2)
- return true;
- if (!*s1 && *s2 == '\n' && !s2[1])
- return true;
- if (*s1 == '\n' && !s1[1] && !*s2)
- return true;
- return false;
-}
-EXPORT_SYMBOL(sysfs_streq);
-
-/**
- * match_string - matches given string in an array
- * @array: array of strings
- * @n: number of strings in the array or -1 for NULL terminated arrays
- * @string: string to match with
- *
- * This routine will look for a string in an array of strings up to the
- * n-th element in the array or until the first NULL element.
- *
- * Historically the value of -1 for @n, was used to search in arrays that
- * are NULL terminated. However, the function does not make a distinction
- * when finishing the search: either @n elements have been compared OR
- * the first NULL element was found.
- *
- * Return:
- * index of a @string in the @array if matches, or %-EINVAL otherwise.
- */
-int match_string(const char * const *array, size_t n, const char *string)
-{
- int index;
- const char *item;
-
- for (index = 0; index < n; index++) {
- item = array[index];
- if (!item)
- break;
- if (!strcmp(item, string))
- return index;
- }
-
- return -EINVAL;
-}
-EXPORT_SYMBOL(match_string);
-
-/**
- * __sysfs_match_string - matches given string in an array
- * @array: array of strings
- * @n: number of strings in the array or -1 for NULL terminated arrays
- * @str: string to match with
- *
- * Returns index of @str in the @array or -EINVAL, just like match_string().
- * Uses sysfs_streq instead of strcmp for matching.
- *
- * This routine will look for a string in an array of strings up to the
- * n-th element in the array or until the first NULL element.
- *
- * Historically the value of -1 for @n, was used to search in arrays that
- * are NULL terminated. However, the function does not make a distinction
- * when finishing the search: either @n elements have been compared OR
- * the first NULL element was found.
- */
-int __sysfs_match_string(const char * const *array, size_t n, const char *str)
-{
- const char *item;
- int index;
-
- for (index = 0; index < n; index++) {
- item = array[index];
- if (!item)
- break;
- if (sysfs_streq(item, str))
- return index;
- }
-
- return -EINVAL;
-}
-EXPORT_SYMBOL(__sysfs_match_string);
-
#ifndef __HAVE_ARCH_MEMSET
/**
* memset - Fill a region of memory with the given value
@@ -1125,27 +951,3 @@ void *memchr_inv(const void *start, int c, size_t bytes)
return check_bytes8(start, value, bytes % 8);
}
EXPORT_SYMBOL(memchr_inv);
-
-/**
- * strreplace - Replace all occurrences of character in string.
- * @s: The string to operate on.
- * @old: The character being replaced.
- * @new: The character @old is replaced with.
- *
- * Returns pointer to the nul byte at the end of @s.
- */
-char *strreplace(char *s, char old, char new)
-{
- for (; *s; ++s)
- if (*s == old)
- *s = new;
- return s;
-}
-EXPORT_SYMBOL(strreplace);
-
-void fortify_panic(const char *name)
-{
- pr_emerg("detected buffer overflow in %s\n", name);
- BUG();
-}
-EXPORT_SYMBOL(fortify_panic);
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 5a35c7e16e96..e9433caab217 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -692,3 +692,196 @@ void kfree_strarray(char **array, size_t n)
kfree(array);
}
EXPORT_SYMBOL_GPL(kfree_strarray);
+
+/**
+ * strscpy_pad() - Copy a C-string into a sized buffer
+ * @dest: Where to copy the string to
+ * @src: Where to copy the string from
+ * @count: Size of destination buffer
+ *
+ * Copy the string, or as much of it as fits, into the dest buffer. The
+ * behavior is undefined if the string buffers overlap. The destination
+ * buffer is always %NUL terminated, unless it's zero-sized.
+ *
+ * If the source string is shorter than the destination buffer, zeros
+ * the tail of the destination buffer.
+ *
+ * For full explanation of why you may want to consider using the
+ * 'strscpy' functions please see the function docstring for strscpy().
+ *
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if count is 0 or @src was truncated.
+ */
+ssize_t strscpy_pad(char *dest, const char *src, size_t count)
+{
+ ssize_t written;
+
+ written = strscpy(dest, src, count);
+ if (written < 0 || written == count - 1)
+ return written;
+
+ memset(dest + written + 1, 0, count - written - 1);
+
+ return written;
+}
+EXPORT_SYMBOL(strscpy_pad);
+
+/**
+ * skip_spaces - Removes leading whitespace from @str.
+ * @str: The string to be stripped.
+ *
+ * Returns a pointer to the first non-whitespace character in @str.
+ */
+char *skip_spaces(const char *str)
+{
+ while (isspace(*str))
+ ++str;
+ return (char *)str;
+}
+EXPORT_SYMBOL(skip_spaces);
+
+/**
+ * strim - Removes leading and trailing whitespace from @s.
+ * @s: The string to be stripped.
+ *
+ * Note that the first trailing whitespace is replaced with a %NUL-terminator
+ * in the given string @s. Returns a pointer to the first non-whitespace
+ * character in @s.
+ */
+char *strim(char *s)
+{
+ size_t size;
+ char *end;
+
+ size = strlen(s);
+ if (!size)
+ return s;
+
+ end = s + size - 1;
+ while (end >= s && isspace(*end))
+ end--;
+ *(end + 1) = '\0';
+
+ return skip_spaces(s);
+}
+EXPORT_SYMBOL(strim);
+
+/**
+ * sysfs_streq - return true if strings are equal, modulo trailing newline
+ * @s1: one string
+ * @s2: another string
+ *
+ * This routine returns true iff two strings are equal, treating both
+ * NUL and newline-then-NUL as equivalent string terminations. It's
+ * geared for use with sysfs input strings, which generally terminate
+ * with newlines but are compared against values without newlines.
+ */
+bool sysfs_streq(const char *s1, const char *s2)
+{
+ while (*s1 && *s1 == *s2) {
+ s1++;
+ s2++;
+ }
+
+ if (*s1 == *s2)
+ return true;
+ if (!*s1 && *s2 == '\n' && !s2[1])
+ return true;
+ if (*s1 == '\n' && !s1[1] && !*s2)
+ return true;
+ return false;
+}
+EXPORT_SYMBOL(sysfs_streq);
+
+/**
+ * match_string - matches given string in an array
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @string: string to match with
+ *
+ * This routine will look for a string in an array of strings up to the
+ * n-th element in the array or until the first NULL element.
+ *
+ * Historically the value of -1 for @n, was used to search in arrays that
+ * are NULL terminated. However, the function does not make a distinction
+ * when finishing the search: either @n elements have been compared OR
+ * the first NULL element was found.
+ *
+ * Return:
+ * index of a @string in the @array if matches, or %-EINVAL otherwise.
+ */
+int match_string(const char * const *array, size_t n, const char *string)
+{
+ int index;
+ const char *item;
+
+ for (index = 0; index < n; index++) {
+ item = array[index];
+ if (!item)
+ break;
+ if (!strcmp(item, string))
+ return index;
+ }
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(match_string);
+
+/**
+ * __sysfs_match_string - matches given string in an array
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @str: string to match with
+ *
+ * Returns index of @str in the @array or -EINVAL, just like match_string().
+ * Uses sysfs_streq instead of strcmp for matching.
+ *
+ * This routine will look for a string in an array of strings up to the
+ * n-th element in the array or until the first NULL element.
+ *
+ * Historically the value of -1 for @n, was used to search in arrays that
+ * are NULL terminated. However, the function does not make a distinction
+ * when finishing the search: either @n elements have been compared OR
+ * the first NULL element was found.
+ */
+int __sysfs_match_string(const char * const *array, size_t n, const char *str)
+{
+ const char *item;
+ int index;
+
+ for (index = 0; index < n; index++) {
+ item = array[index];
+ if (!item)
+ break;
+ if (sysfs_streq(item, str))
+ return index;
+ }
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(__sysfs_match_string);
+
+/**
+ * strreplace - Replace all occurrences of character in string.
+ * @s: The string to operate on.
+ * @old: The character being replaced.
+ * @new: The character @old is replaced with.
+ *
+ * Returns pointer to the nul byte at the end of @s.
+ */
+char *strreplace(char *s, char old, char new)
+{
+ for (; *s; ++s)
+ if (*s == old)
+ *s = new;
+ return s;
+}
+EXPORT_SYMBOL(strreplace);
+
+void fortify_panic(const char *name)
+{
+ pr_emerg("detected buffer overflow in %s\n", name);
+ BUG();
+}
+EXPORT_SYMBOL(fortify_panic);
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH for-next 14/25] lib/string: Move helper functions out of string.c
2021-08-22 7:51 ` [PATCH for-next 14/25] lib/string: Move helper functions out of string.c Kees Cook
@ 2021-08-25 21:48 ` Nick Desaulniers
2021-08-26 2:47 ` Kees Cook
0 siblings, 1 reply; 42+ messages in thread
From: Nick Desaulniers @ 2021-08-25 21:48 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Andy Shevchenko, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
On Sun, Aug 22, 2021 at 12:51 AM Kees Cook <keescook@chromium.org> wrote:
>
> The core functions of string.c are those that may be implemented by
> per-architecture functions, or overloaded by FORTIFY_SOURCE. As a
> result, it needs to be built with __NO_FORTIFY. Without this, macros
Sorry, is there more info on what exactly __NO_FORTIFY is (and why we
"need" it)?
> will collide with function declarations. This was accidentally working
> due to -ffreestanding (on some architectures). Make this deterministic
> by explicitly setting __NO_FORTIFY and move all the helper functions
> into string_helpers.c so that they gain the fortification coverage they
> had been missing.
>
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> arch/arm/boot/compressed/string.c | 1 +
> arch/s390/lib/string.c | 3 +
> arch/x86/boot/compressed/misc.h | 2 +
> arch/x86/boot/compressed/pgtable_64.c | 2 +
> arch/x86/lib/string_32.c | 1 +
> lib/string.c | 210 +-------------------------
> lib/string_helpers.c | 193 +++++++++++++++++++++++
> 7 files changed, 208 insertions(+), 204 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/string.c b/arch/arm/boot/compressed/string.c
> index 8c0fa276d994..fcc678fce045 100644
> --- a/arch/arm/boot/compressed/string.c
> +++ b/arch/arm/boot/compressed/string.c
> @@ -5,6 +5,7 @@
> * Small subset of simple string routines
> */
>
> +#define __NO_FORTIFY
> #include <linux/string.h>
>
> /*
> diff --git a/arch/s390/lib/string.c b/arch/s390/lib/string.c
> index cfcdf76d6a95..392fb9f4f4db 100644
> --- a/arch/s390/lib/string.c
> +++ b/arch/s390/lib/string.c
> @@ -8,6 +8,9 @@
> */
>
> #define IN_ARCH_STRING_C 1
> +#ifndef __NO_FORTIFY
> +# define __NO_FORTIFY
> +#endif
>
> #include <linux/types.h>
> #include <linux/string.h>
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index 31139256859f..49bde196da9b 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -14,6 +14,8 @@
> #undef CONFIG_KASAN
> #undef CONFIG_KASAN_GENERIC
>
> +#define __NO_FORTIFY
> +
> /* cpu_feature_enabled() cannot be used this early */
> #define USE_EARLY_PGTABLE_L5
>
> diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c
> index 2a78746f5a4c..a1733319a22a 100644
> --- a/arch/x86/boot/compressed/pgtable_64.c
> +++ b/arch/x86/boot/compressed/pgtable_64.c
> @@ -1,3 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "misc.h"
^ should we just `#define __NO_FORTIFY` here in pgtable_64.c rather
than set that for a whole header?
> #include <linux/efi.h>
> #include <asm/e820/types.h>
> #include <asm/processor.h>
> diff --git a/arch/x86/lib/string_32.c b/arch/x86/lib/string_32.c
> index d15fdae9656e..53b3f202267c 100644
> --- a/arch/x86/lib/string_32.c
> +++ b/arch/x86/lib/string_32.c
> @@ -11,6 +11,7 @@
> * strings.
> */
>
> +#define __NO_FORTIFY
> #include <linux/string.h>
> #include <linux/export.h>
>
> diff --git a/lib/string.c b/lib/string.c
> index 77bd0b1d3296..1e6259f263b8 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -6,20 +6,15 @@
> */
>
> /*
> - * stupid library routines.. The optimized versions should generally be found
> - * as inline code in <asm-xx/string.h>
> + * This file should be used only for "library" routines that may have
> + * alternative implementations on specific architectures (generally
> + * found in <asm-xx/string.h>), or get overloaded by FORTIFY_SOURCE.
> + * (Specifically, this file is built with __NO_FORTIFY.)
> *
> - * These are buggy as well..
> - *
> - * * Fri Jun 25 1999, Ingo Oeser <ioe@informatik.tu-chemnitz.de>
> - * - Added strsep() which will replace strtok() soon (because strsep() is
> - * reentrant and should be faster). Use only strsep() in new code, please.
> - *
> - * * Sat Feb 09 2002, Jason Thomas <jason@topic.com.au>,
> - * Matthew Hawkins <matt@mh.dropbear.id.au>
> - * - Kissed strtok() goodbye
> + * Other helper functions should live in string_helpers.c.
> */
>
> +#define __NO_FORTIFY
> #include <linux/types.h>
> #include <linux/string.h>
> #include <linux/ctype.h>
> @@ -237,40 +232,6 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
> EXPORT_SYMBOL(strscpy);
> #endif
>
> -/**
> - * strscpy_pad() - Copy a C-string into a sized buffer
> - * @dest: Where to copy the string to
> - * @src: Where to copy the string from
> - * @count: Size of destination buffer
> - *
> - * Copy the string, or as much of it as fits, into the dest buffer. The
> - * behavior is undefined if the string buffers overlap. The destination
> - * buffer is always %NUL terminated, unless it's zero-sized.
> - *
> - * If the source string is shorter than the destination buffer, zeros
> - * the tail of the destination buffer.
> - *
> - * For full explanation of why you may want to consider using the
> - * 'strscpy' functions please see the function docstring for strscpy().
> - *
> - * Returns:
> - * * The number of characters copied (not including the trailing %NUL)
> - * * -E2BIG if count is 0 or @src was truncated.
> - */
> -ssize_t strscpy_pad(char *dest, const char *src, size_t count)
> -{
> - ssize_t written;
> -
> - written = strscpy(dest, src, count);
> - if (written < 0 || written == count - 1)
> - return written;
> -
> - memset(dest + written + 1, 0, count - written - 1);
> -
> - return written;
> -}
> -EXPORT_SYMBOL(strscpy_pad);
> -
> /**
> * stpcpy - copy a string from src to dest returning a pointer to the new end
> * of dest, including src's %NUL-terminator. May overrun dest.
> @@ -513,46 +474,6 @@ char *strnchr(const char *s, size_t count, int c)
> EXPORT_SYMBOL(strnchr);
> #endif
>
> -/**
> - * skip_spaces - Removes leading whitespace from @str.
> - * @str: The string to be stripped.
> - *
> - * Returns a pointer to the first non-whitespace character in @str.
> - */
> -char *skip_spaces(const char *str)
> -{
> - while (isspace(*str))
> - ++str;
> - return (char *)str;
> -}
> -EXPORT_SYMBOL(skip_spaces);
> -
> -/**
> - * strim - Removes leading and trailing whitespace from @s.
> - * @s: The string to be stripped.
> - *
> - * Note that the first trailing whitespace is replaced with a %NUL-terminator
> - * in the given string @s. Returns a pointer to the first non-whitespace
> - * character in @s.
> - */
> -char *strim(char *s)
> -{
> - size_t size;
> - char *end;
> -
> - size = strlen(s);
> - if (!size)
> - return s;
> -
> - end = s + size - 1;
> - while (end >= s && isspace(*end))
> - end--;
> - *(end + 1) = '\0';
> -
> - return skip_spaces(s);
> -}
> -EXPORT_SYMBOL(strim);
> -
> #ifndef __HAVE_ARCH_STRLEN
> /**
> * strlen - Find the length of a string
> @@ -687,101 +608,6 @@ char *strsep(char **s, const char *ct)
> EXPORT_SYMBOL(strsep);
> #endif
>
> -/**
> - * sysfs_streq - return true if strings are equal, modulo trailing newline
> - * @s1: one string
> - * @s2: another string
> - *
> - * This routine returns true iff two strings are equal, treating both
> - * NUL and newline-then-NUL as equivalent string terminations. It's
> - * geared for use with sysfs input strings, which generally terminate
> - * with newlines but are compared against values without newlines.
> - */
> -bool sysfs_streq(const char *s1, const char *s2)
> -{
> - while (*s1 && *s1 == *s2) {
> - s1++;
> - s2++;
> - }
> -
> - if (*s1 == *s2)
> - return true;
> - if (!*s1 && *s2 == '\n' && !s2[1])
> - return true;
> - if (*s1 == '\n' && !s1[1] && !*s2)
> - return true;
> - return false;
> -}
> -EXPORT_SYMBOL(sysfs_streq);
> -
> -/**
> - * match_string - matches given string in an array
> - * @array: array of strings
> - * @n: number of strings in the array or -1 for NULL terminated arrays
> - * @string: string to match with
> - *
> - * This routine will look for a string in an array of strings up to the
> - * n-th element in the array or until the first NULL element.
> - *
> - * Historically the value of -1 for @n, was used to search in arrays that
> - * are NULL terminated. However, the function does not make a distinction
> - * when finishing the search: either @n elements have been compared OR
> - * the first NULL element was found.
> - *
> - * Return:
> - * index of a @string in the @array if matches, or %-EINVAL otherwise.
> - */
> -int match_string(const char * const *array, size_t n, const char *string)
> -{
> - int index;
> - const char *item;
> -
> - for (index = 0; index < n; index++) {
> - item = array[index];
> - if (!item)
> - break;
> - if (!strcmp(item, string))
> - return index;
> - }
> -
> - return -EINVAL;
> -}
> -EXPORT_SYMBOL(match_string);
> -
> -/**
> - * __sysfs_match_string - matches given string in an array
> - * @array: array of strings
> - * @n: number of strings in the array or -1 for NULL terminated arrays
> - * @str: string to match with
> - *
> - * Returns index of @str in the @array or -EINVAL, just like match_string().
> - * Uses sysfs_streq instead of strcmp for matching.
> - *
> - * This routine will look for a string in an array of strings up to the
> - * n-th element in the array or until the first NULL element.
> - *
> - * Historically the value of -1 for @n, was used to search in arrays that
> - * are NULL terminated. However, the function does not make a distinction
> - * when finishing the search: either @n elements have been compared OR
> - * the first NULL element was found.
> - */
> -int __sysfs_match_string(const char * const *array, size_t n, const char *str)
> -{
> - const char *item;
> - int index;
> -
> - for (index = 0; index < n; index++) {
> - item = array[index];
> - if (!item)
> - break;
> - if (sysfs_streq(item, str))
> - return index;
> - }?
> -
> - return -EINVAL;
> -}
> -EXPORT_SYMBOL(__sysfs_match_string);
> -
are memset16, memset32, and memset64 worth moving as well? Also,
memscan(), check_bytes(), memchr_inv()?
> #ifndef __HAVE_ARCH_MEMSET
> /**
> * memset - Fill a region of memory with the given value
> @@ -1125,27 +951,3 @@ void *memchr_inv(const void *start, int c, size_t bytes)
> return check_bytes8(start, value, bytes % 8);
> }
> EXPORT_SYMBOL(memchr_inv);
> -
> -/**
> - * strreplace - Replace all occurrences of character in string.
> - * @s: The string to operate on.
> - * @old: The character being replaced.
> - * @new: The character @old is replaced with.
> - *
> - * Returns pointer to the nul byte at the end of @s.
> - */
> -char *strreplace(char *s, char old, char new)
> -{
> - for (; *s; ++s)
> - if (*s == old)
> - *s = new;
> - return s;
> -}
> -EXPORT_SYMBOL(strreplace);
> -
> -void fortify_panic(const char *name)
> -{
> - pr_emerg("detected buffer overflow in %s\n", name);
> - BUG();
> -}
> -EXPORT_SYMBOL(fortify_panic);
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index 5a35c7e16e96..e9433caab217 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -692,3 +692,196 @@ void kfree_strarray(char **array, size_t n)
> kfree(array);
> }
> EXPORT_SYMBOL_GPL(kfree_strarray);
> +
> +/**
> + * strscpy_pad() - Copy a C-string into a sized buffer
> + * @dest: Where to copy the string to
> + * @src: Where to copy the string from
> + * @count: Size of destination buffer
> + *
> + * Copy the string, or as much of it as fits, into the dest buffer. The
> + * behavior is undefined if the string buffers overlap. The destination
> + * buffer is always %NUL terminated, unless it's zero-sized.
> + *
> + * If the source string is shorter than the destination buffer, zeros
> + * the tail of the destination buffer.
> + *
> + * For full explanation of why you may want to consider using the
> + * 'strscpy' functions please see the function docstring for strscpy().
> + *
> + * Returns:
> + * * The number of characters copied (not including the trailing %NUL)
> + * * -E2BIG if count is 0 or @src was truncated.
> + */
> +ssize_t strscpy_pad(char *dest, const char *src, size_t count)
> +{
> + ssize_t written;
> +
> + written = strscpy(dest, src, count);
> + if (written < 0 || written == count - 1)
> + return written;
> +
> + memset(dest + written + 1, 0, count - written - 1);
> +
> + return written;
> +}
> +EXPORT_SYMBOL(strscpy_pad);
> +
> +/**
> + * skip_spaces - Removes leading whitespace from @str.
> + * @str: The string to be stripped.
> + *
> + * Returns a pointer to the first non-whitespace character in @str.
> + */
> +char *skip_spaces(const char *str)
> +{
> + while (isspace(*str))
> + ++str;
> + return (char *)str;
> +}
> +EXPORT_SYMBOL(skip_spaces);
> +
> +/**
> + * strim - Removes leading and trailing whitespace from @s.
> + * @s: The string to be stripped.
> + *
> + * Note that the first trailing whitespace is replaced with a %NUL-terminator
> + * in the given string @s. Returns a pointer to the first non-whitespace
> + * character in @s.
> + */
> +char *strim(char *s)
> +{
> + size_t size;
> + char *end;
> +
> + size = strlen(s);
> + if (!size)
> + return s;
> +
> + end = s + size - 1;
> + while (end >= s && isspace(*end))
> + end--;
> + *(end + 1) = '\0';
> +
> + return skip_spaces(s);
> +}
> +EXPORT_SYMBOL(strim);
> +
> +/**
> + * sysfs_streq - return true if strings are equal, modulo trailing newline
> + * @s1: one string
> + * @s2: another string
> + *
> + * This routine returns true iff two strings are equal, treating both
> + * NUL and newline-then-NUL as equivalent string terminations. It's
> + * geared for use with sysfs input strings, which generally terminate
> + * with newlines but are compared against values without newlines.
> + */
> +bool sysfs_streq(const char *s1, const char *s2)
> +{
> + while (*s1 && *s1 == *s2) {
> + s1++;
> + s2++;
> + }
> +
> + if (*s1 == *s2)
> + return true;
> + if (!*s1 && *s2 == '\n' && !s2[1])
> + return true;
> + if (*s1 == '\n' && !s1[1] && !*s2)
> + return true;
> + return false;
> +}
> +EXPORT_SYMBOL(sysfs_streq);
> +
> +/**
> + * match_string - matches given string in an array
> + * @array: array of strings
> + * @n: number of strings in the array or -1 for NULL terminated arrays
> + * @string: string to match with
> + *
> + * This routine will look for a string in an array of strings up to the
> + * n-th element in the array or until the first NULL element.
> + *
> + * Historically the value of -1 for @n, was used to search in arrays that
> + * are NULL terminated. However, the function does not make a distinction
> + * when finishing the search: either @n elements have been compared OR
> + * the first NULL element was found.
> + *
> + * Return:
> + * index of a @string in the @array if matches, or %-EINVAL otherwise.
> + */
> +int match_string(const char * const *array, size_t n, const char *string)
> +{
> + int index;
> + const char *item;
> +
> + for (index = 0; index < n; index++) {
> + item = array[index];
> + if (!item)
> + break;
> + if (!strcmp(item, string))
> + return index;
> + }
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(match_string);
> +
> +/**
> + * __sysfs_match_string - matches given string in an array
> + * @array: array of strings
> + * @n: number of strings in the array or -1 for NULL terminated arrays
> + * @str: string to match with
> + *
> + * Returns index of @str in the @array or -EINVAL, just like match_string().
> + * Uses sysfs_streq instead of strcmp for matching.
> + *
> + * This routine will look for a string in an array of strings up to the
> + * n-th element in the array or until the first NULL element.
> + *
> + * Historically the value of -1 for @n, was used to search in arrays that
> + * are NULL terminated. However, the function does not make a distinction
> + * when finishing the search: either @n elements have been compared OR
> + * the first NULL element was found.
> + */
> +int __sysfs_match_string(const char * const *array, size_t n, const char *str)
> +{
> + const char *item;
> + int index;
> +
> + for (index = 0; index < n; index++) {
> + item = array[index];
> + if (!item)
> + break;
> + if (sysfs_streq(item, str))
> + return index;
> + }
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(__sysfs_match_string);
> +
> +/**
> + * strreplace - Replace all occurrences of character in string.
> + * @s: The string to operate on.
> + * @old: The character being replaced.
> + * @new: The character @old is replaced with.
> + *
> + * Returns pointer to the nul byte at the end of @s.
> + */
> +char *strreplace(char *s, char old, char new)
> +{
> + for (; *s; ++s)
> + if (*s == old)
> + *s = new;
> + return s;
> +}
> +EXPORT_SYMBOL(strreplace);
> +
> +void fortify_panic(const char *name)
> +{
> + pr_emerg("detected buffer overflow in %s\n", name);
> + BUG();
> +}
> +EXPORT_SYMBOL(fortify_panic);
> --
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH for-next 14/25] lib/string: Move helper functions out of string.c
2021-08-25 21:48 ` Nick Desaulniers
@ 2021-08-26 2:47 ` Kees Cook
2021-08-26 18:08 ` Nick Desaulniers
0 siblings, 1 reply; 42+ messages in thread
From: Kees Cook @ 2021-08-26 2:47 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-kernel, Andy Shevchenko, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
On Wed, Aug 25, 2021 at 02:48:30PM -0700, Nick Desaulniers wrote:
> On Sun, Aug 22, 2021 at 12:51 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > The core functions of string.c are those that may be implemented by
> > per-architecture functions, or overloaded by FORTIFY_SOURCE. As a
> > result, it needs to be built with __NO_FORTIFY. Without this, macros
>
> Sorry, is there more info on what exactly __NO_FORTIFY is (and why we
> "need" it)?
include/linux/string.h has:
#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
#include <linux/fortify-string.h>
#endif
It's needed in cases where fortification won't actually operate (i.e.
early boot without the full kernel running), or in similar situations
(code meant to be executed under different "runtimes" (hypervisors), or
for the fortified string implementations themselves (i.e. what these
couple of patches are reorganizing to correctly do here).
>
> > will collide with function declarations. This was accidentally working
> > due to -ffreestanding (on some architectures). Make this deterministic
> > by explicitly setting __NO_FORTIFY and move all the helper functions
> > into string_helpers.c so that they gain the fortification coverage they
> > had been missing.
> >
> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > arch/arm/boot/compressed/string.c | 1 +
> > arch/s390/lib/string.c | 3 +
> > arch/x86/boot/compressed/misc.h | 2 +
> > arch/x86/boot/compressed/pgtable_64.c | 2 +
> > arch/x86/lib/string_32.c | 1 +
> > lib/string.c | 210 +-------------------------
> > lib/string_helpers.c | 193 +++++++++++++++++++++++
> > 7 files changed, 208 insertions(+), 204 deletions(-)
> >
> > diff --git a/arch/arm/boot/compressed/string.c b/arch/arm/boot/compressed/string.c
> > index 8c0fa276d994..fcc678fce045 100644
> > --- a/arch/arm/boot/compressed/string.c
> > +++ b/arch/arm/boot/compressed/string.c
> > @@ -5,6 +5,7 @@
> > * Small subset of simple string routines
> > */
> >
> > +#define __NO_FORTIFY
> > #include <linux/string.h>
> >
> > /*
> > diff --git a/arch/s390/lib/string.c b/arch/s390/lib/string.c
> > index cfcdf76d6a95..392fb9f4f4db 100644
> > --- a/arch/s390/lib/string.c
> > +++ b/arch/s390/lib/string.c
> > @@ -8,6 +8,9 @@
> > */
> >
> > #define IN_ARCH_STRING_C 1
> > +#ifndef __NO_FORTIFY
> > +# define __NO_FORTIFY
> > +#endif
> >
> > #include <linux/types.h>
> > #include <linux/string.h>
> > diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> > index 31139256859f..49bde196da9b 100644
> > --- a/arch/x86/boot/compressed/misc.h
> > +++ b/arch/x86/boot/compressed/misc.h
> > @@ -14,6 +14,8 @@
> > #undef CONFIG_KASAN
> > #undef CONFIG_KASAN_GENERIC
> >
> > +#define __NO_FORTIFY
> > +
> > /* cpu_feature_enabled() cannot be used this early */
> > #define USE_EARLY_PGTABLE_L5
> >
> > diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c
> > index 2a78746f5a4c..a1733319a22a 100644
> > --- a/arch/x86/boot/compressed/pgtable_64.c
> > +++ b/arch/x86/boot/compressed/pgtable_64.c
> > @@ -1,3 +1,5 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "misc.h"
>
> ^ should we just `#define __NO_FORTIFY` here in pgtable_64.c rather
> than set that for a whole header?
No -- anything using the string functions in x86 early boot needs to use
misc.h, since it correctly handles to various dances of includes and
defines to get the right combinations.
>
> > #include <linux/efi.h>
> > #include <asm/e820/types.h>
> > #include <asm/processor.h>
> > diff --git a/arch/x86/lib/string_32.c b/arch/x86/lib/string_32.c
> > index d15fdae9656e..53b3f202267c 100644
> > --- a/arch/x86/lib/string_32.c
> > +++ b/arch/x86/lib/string_32.c
> > @@ -11,6 +11,7 @@
> > * strings.
> > */
> >
> > +#define __NO_FORTIFY
> > #include <linux/string.h>
> > #include <linux/export.h>
> >
> > diff --git a/lib/string.c b/lib/string.c
> > index 77bd0b1d3296..1e6259f263b8 100644
> > --- a/lib/string.c
> > +++ b/lib/string.c
> > @@ -6,20 +6,15 @@
> > */
> >
> > /*
> > - * stupid library routines.. The optimized versions should generally be found
> > - * as inline code in <asm-xx/string.h>
> > + * This file should be used only for "library" routines that may have
> > + * alternative implementations on specific architectures (generally
> > + * found in <asm-xx/string.h>), or get overloaded by FORTIFY_SOURCE.
> > + * (Specifically, this file is built with __NO_FORTIFY.)
> > *
> > - * These are buggy as well..
> > - *
> > - * * Fri Jun 25 1999, Ingo Oeser <ioe@informatik.tu-chemnitz.de>
> > - * - Added strsep() which will replace strtok() soon (because strsep() is
> > - * reentrant and should be faster). Use only strsep() in new code, please.
> > - *
> > - * * Sat Feb 09 2002, Jason Thomas <jason@topic.com.au>,
> > - * Matthew Hawkins <matt@mh.dropbear.id.au>
> > - * - Kissed strtok() goodbye
> > + * Other helper functions should live in string_helpers.c.
> > */
> >
> > +#define __NO_FORTIFY
> > #include <linux/types.h>
> > #include <linux/string.h>
> > #include <linux/ctype.h>
> > @@ -237,40 +232,6 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
> > EXPORT_SYMBOL(strscpy);
> > #endif
> >
> > -/**
> > - * strscpy_pad() - Copy a C-string into a sized buffer
> > - * @dest: Where to copy the string to
> > - * @src: Where to copy the string from
> > - * @count: Size of destination buffer
> > - *
> > - * Copy the string, or as much of it as fits, into the dest buffer. The
> > - * behavior is undefined if the string buffers overlap. The destination
> > - * buffer is always %NUL terminated, unless it's zero-sized.
> > - *
> > - * If the source string is shorter than the destination buffer, zeros
> > - * the tail of the destination buffer.
> > - *
> > - * For full explanation of why you may want to consider using the
> > - * 'strscpy' functions please see the function docstring for strscpy().
> > - *
> > - * Returns:
> > - * * The number of characters copied (not including the trailing %NUL)
> > - * * -E2BIG if count is 0 or @src was truncated.
> > - */
> > -ssize_t strscpy_pad(char *dest, const char *src, size_t count)
> > -{
> > - ssize_t written;
> > -
> > - written = strscpy(dest, src, count);
> > - if (written < 0 || written == count - 1)
> > - return written;
> > -
> > - memset(dest + written + 1, 0, count - written - 1);
> > -
> > - return written;
> > -}
> > -EXPORT_SYMBOL(strscpy_pad);
> > -
> > /**
> > * stpcpy - copy a string from src to dest returning a pointer to the new end
> > * of dest, including src's %NUL-terminator. May overrun dest.
> > @@ -513,46 +474,6 @@ char *strnchr(const char *s, size_t count, int c)
> > EXPORT_SYMBOL(strnchr);
> > #endif
> >
> > -/**
> > - * skip_spaces - Removes leading whitespace from @str.
> > - * @str: The string to be stripped.
> > - *
> > - * Returns a pointer to the first non-whitespace character in @str.
> > - */
> > -char *skip_spaces(const char *str)
> > -{
> > - while (isspace(*str))
> > - ++str;
> > - return (char *)str;
> > -}
> > -EXPORT_SYMBOL(skip_spaces);
> > -
> > -/**
> > - * strim - Removes leading and trailing whitespace from @s.
> > - * @s: The string to be stripped.
> > - *
> > - * Note that the first trailing whitespace is replaced with a %NUL-terminator
> > - * in the given string @s. Returns a pointer to the first non-whitespace
> > - * character in @s.
> > - */
> > -char *strim(char *s)
> > -{
> > - size_t size;
> > - char *end;
> > -
> > - size = strlen(s);
> > - if (!size)
> > - return s;
> > -
> > - end = s + size - 1;
> > - while (end >= s && isspace(*end))
> > - end--;
> > - *(end + 1) = '\0';
> > -
> > - return skip_spaces(s);
> > -}
> > -EXPORT_SYMBOL(strim);
> > -
> > #ifndef __HAVE_ARCH_STRLEN
> > /**
> > * strlen - Find the length of a string
> > @@ -687,101 +608,6 @@ char *strsep(char **s, const char *ct)
> > EXPORT_SYMBOL(strsep);
> > #endif
> >
> > -/**
> > - * sysfs_streq - return true if strings are equal, modulo trailing newline
> > - * @s1: one string
> > - * @s2: another string
> > - *
> > - * This routine returns true iff two strings are equal, treating both
> > - * NUL and newline-then-NUL as equivalent string terminations. It's
> > - * geared for use with sysfs input strings, which generally terminate
> > - * with newlines but are compared against values without newlines.
> > - */
> > -bool sysfs_streq(const char *s1, const char *s2)
> > -{
> > - while (*s1 && *s1 == *s2) {
> > - s1++;
> > - s2++;
> > - }
> > -
> > - if (*s1 == *s2)
> > - return true;
> > - if (!*s1 && *s2 == '\n' && !s2[1])
> > - return true;
> > - if (*s1 == '\n' && !s1[1] && !*s2)
> > - return true;
> > - return false;
> > -}
> > -EXPORT_SYMBOL(sysfs_streq);
> > -
> > -/**
> > - * match_string - matches given string in an array
> > - * @array: array of strings
> > - * @n: number of strings in the array or -1 for NULL terminated arrays
> > - * @string: string to match with
> > - *
> > - * This routine will look for a string in an array of strings up to the
> > - * n-th element in the array or until the first NULL element.
> > - *
> > - * Historically the value of -1 for @n, was used to search in arrays that
> > - * are NULL terminated. However, the function does not make a distinction
> > - * when finishing the search: either @n elements have been compared OR
> > - * the first NULL element was found.
> > - *
> > - * Return:
> > - * index of a @string in the @array if matches, or %-EINVAL otherwise.
> > - */
> > -int match_string(const char * const *array, size_t n, const char *string)
> > -{
> > - int index;
> > - const char *item;
> > -
> > - for (index = 0; index < n; index++) {
> > - item = array[index];
> > - if (!item)
> > - break;
> > - if (!strcmp(item, string))
> > - return index;
> > - }
> > -
> > - return -EINVAL;
> > -}
> > -EXPORT_SYMBOL(match_string);
> > -
> > -/**
> > - * __sysfs_match_string - matches given string in an array
> > - * @array: array of strings
> > - * @n: number of strings in the array or -1 for NULL terminated arrays
> > - * @str: string to match with
> > - *
> > - * Returns index of @str in the @array or -EINVAL, just like match_string().
> > - * Uses sysfs_streq instead of strcmp for matching.
> > - *
> > - * This routine will look for a string in an array of strings up to the
> > - * n-th element in the array or until the first NULL element.
> > - *
> > - * Historically the value of -1 for @n, was used to search in arrays that
> > - * are NULL terminated. However, the function does not make a distinction
> > - * when finishing the search: either @n elements have been compared OR
> > - * the first NULL element was found.
> > - */
> > -int __sysfs_match_string(const char * const *array, size_t n, const char *str)
> > -{
> > - const char *item;
> > - int index;
> > -
> > - for (index = 0; index < n; index++) {
> > - item = array[index];
> > - if (!item)
> > - break;
> > - if (sysfs_streq(item, str))
> > - return index;
> > - }?
> > -
> > - return -EINVAL;
> > -}
> > -EXPORT_SYMBOL(__sysfs_match_string);
> > -
>
> are memset16, memset32, and memset64 worth moving as well? Also,
> memscan(), check_bytes(), memchr_inv()?
All of these are implementations, so they should stay put.
>
> > #ifndef __HAVE_ARCH_MEMSET
> > /**
> > * memset - Fill a region of memory with the given value
> > @@ -1125,27 +951,3 @@ void *memchr_inv(const void *start, int c, size_t bytes)
> > return check_bytes8(start, value, bytes % 8);
> > }
> > EXPORT_SYMBOL(memchr_inv);
> > -
> > -/**
> > - * strreplace - Replace all occurrences of character in string.
> > - * @s: The string to operate on.
> > - * @old: The character being replaced.
> > - * @new: The character @old is replaced with.
> > - *
> > - * Returns pointer to the nul byte at the end of @s.
> > - */
> > -char *strreplace(char *s, char old, char new)
> > -{
> > - for (; *s; ++s)
> > - if (*s == old)
> > - *s = new;
> > - return s;
> > -}
> > -EXPORT_SYMBOL(strreplace);
> > -
> > -void fortify_panic(const char *name)
> > -{
> > - pr_emerg("detected buffer overflow in %s\n", name);
> > - BUG();
> > -}
> > -EXPORT_SYMBOL(fortify_panic);
> > diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> > index 5a35c7e16e96..e9433caab217 100644
> > --- a/lib/string_helpers.c
> > +++ b/lib/string_helpers.c
> > @@ -692,3 +692,196 @@ void kfree_strarray(char **array, size_t n)
> > kfree(array);
> > }
> > EXPORT_SYMBOL_GPL(kfree_strarray);
> > +
> > +/**
> > + * strscpy_pad() - Copy a C-string into a sized buffer
> > + * @dest: Where to copy the string to
> > + * @src: Where to copy the string from
> > + * @count: Size of destination buffer
> > + *
> > + * Copy the string, or as much of it as fits, into the dest buffer. The
> > + * behavior is undefined if the string buffers overlap. The destination
> > + * buffer is always %NUL terminated, unless it's zero-sized.
> > + *
> > + * If the source string is shorter than the destination buffer, zeros
> > + * the tail of the destination buffer.
> > + *
> > + * For full explanation of why you may want to consider using the
> > + * 'strscpy' functions please see the function docstring for strscpy().
> > + *
> > + * Returns:
> > + * * The number of characters copied (not including the trailing %NUL)
> > + * * -E2BIG if count is 0 or @src was truncated.
> > + */
> > +ssize_t strscpy_pad(char *dest, const char *src, size_t count)
> > +{
> > + ssize_t written;
> > +
> > + written = strscpy(dest, src, count);
> > + if (written < 0 || written == count - 1)
> > + return written;
> > +
> > + memset(dest + written + 1, 0, count - written - 1);
> > +
> > + return written;
> > +}
> > +EXPORT_SYMBOL(strscpy_pad);
> > +
> > +/**
> > + * skip_spaces - Removes leading whitespace from @str.
> > + * @str: The string to be stripped.
> > + *
> > + * Returns a pointer to the first non-whitespace character in @str.
> > + */
> > +char *skip_spaces(const char *str)
> > +{
> > + while (isspace(*str))
> > + ++str;
> > + return (char *)str;
> > +}
> > +EXPORT_SYMBOL(skip_spaces);
> > +
> > +/**
> > + * strim - Removes leading and trailing whitespace from @s.
> > + * @s: The string to be stripped.
> > + *
> > + * Note that the first trailing whitespace is replaced with a %NUL-terminator
> > + * in the given string @s. Returns a pointer to the first non-whitespace
> > + * character in @s.
> > + */
> > +char *strim(char *s)
> > +{
> > + size_t size;
> > + char *end;
> > +
> > + size = strlen(s);
> > + if (!size)
> > + return s;
> > +
> > + end = s + size - 1;
> > + while (end >= s && isspace(*end))
> > + end--;
> > + *(end + 1) = '\0';
> > +
> > + return skip_spaces(s);
> > +}
> > +EXPORT_SYMBOL(strim);
> > +
> > +/**
> > + * sysfs_streq - return true if strings are equal, modulo trailing newline
> > + * @s1: one string
> > + * @s2: another string
> > + *
> > + * This routine returns true iff two strings are equal, treating both
> > + * NUL and newline-then-NUL as equivalent string terminations. It's
> > + * geared for use with sysfs input strings, which generally terminate
> > + * with newlines but are compared against values without newlines.
> > + */
> > +bool sysfs_streq(const char *s1, const char *s2)
> > +{
> > + while (*s1 && *s1 == *s2) {
> > + s1++;
> > + s2++;
> > + }
> > +
> > + if (*s1 == *s2)
> > + return true;
> > + if (!*s1 && *s2 == '\n' && !s2[1])
> > + return true;
> > + if (*s1 == '\n' && !s1[1] && !*s2)
> > + return true;
> > + return false;
> > +}
> > +EXPORT_SYMBOL(sysfs_streq);
> > +
> > +/**
> > + * match_string - matches given string in an array
> > + * @array: array of strings
> > + * @n: number of strings in the array or -1 for NULL terminated arrays
> > + * @string: string to match with
> > + *
> > + * This routine will look for a string in an array of strings up to the
> > + * n-th element in the array or until the first NULL element.
> > + *
> > + * Historically the value of -1 for @n, was used to search in arrays that
> > + * are NULL terminated. However, the function does not make a distinction
> > + * when finishing the search: either @n elements have been compared OR
> > + * the first NULL element was found.
> > + *
> > + * Return:
> > + * index of a @string in the @array if matches, or %-EINVAL otherwise.
> > + */
> > +int match_string(const char * const *array, size_t n, const char *string)
> > +{
> > + int index;
> > + const char *item;
> > +
> > + for (index = 0; index < n; index++) {
> > + item = array[index];
> > + if (!item)
> > + break;
> > + if (!strcmp(item, string))
> > + return index;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +EXPORT_SYMBOL(match_string);
> > +
> > +/**
> > + * __sysfs_match_string - matches given string in an array
> > + * @array: array of strings
> > + * @n: number of strings in the array or -1 for NULL terminated arrays
> > + * @str: string to match with
> > + *
> > + * Returns index of @str in the @array or -EINVAL, just like match_string().
> > + * Uses sysfs_streq instead of strcmp for matching.
> > + *
> > + * This routine will look for a string in an array of strings up to the
> > + * n-th element in the array or until the first NULL element.
> > + *
> > + * Historically the value of -1 for @n, was used to search in arrays that
> > + * are NULL terminated. However, the function does not make a distinction
> > + * when finishing the search: either @n elements have been compared OR
> > + * the first NULL element was found.
> > + */
> > +int __sysfs_match_string(const char * const *array, size_t n, const char *str)
> > +{
> > + const char *item;
> > + int index;
> > +
> > + for (index = 0; index < n; index++) {
> > + item = array[index];
> > + if (!item)
> > + break;
> > + if (sysfs_streq(item, str))
> > + return index;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +EXPORT_SYMBOL(__sysfs_match_string);
> > +
> > +/**
> > + * strreplace - Replace all occurrences of character in string.
> > + * @s: The string to operate on.
> > + * @old: The character being replaced.
> > + * @new: The character @old is replaced with.
> > + *
> > + * Returns pointer to the nul byte at the end of @s.
> > + */
> > +char *strreplace(char *s, char old, char new)
> > +{
> > + for (; *s; ++s)
> > + if (*s == old)
> > + *s = new;
> > + return s;
> > +}
> > +EXPORT_SYMBOL(strreplace);
> > +
> > +void fortify_panic(const char *name)
> > +{
> > + pr_emerg("detected buffer overflow in %s\n", name);
> > + BUG();
> > +}
> > +EXPORT_SYMBOL(fortify_panic);
> > --
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Kees Cook
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH for-next 14/25] lib/string: Move helper functions out of string.c
2021-08-26 2:47 ` Kees Cook
@ 2021-08-26 18:08 ` Nick Desaulniers
0 siblings, 0 replies; 42+ messages in thread
From: Nick Desaulniers @ 2021-08-26 18:08 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Andy Shevchenko, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
On Wed, Aug 25, 2021 at 7:47 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Aug 25, 2021 at 02:48:30PM -0700, Nick Desaulniers wrote:
> > are memset16, memset32, and memset64 worth moving as well? Also,
> > memscan(), check_bytes(), memchr_inv()?
>
> All of these are implementations, so they should stay put.
All of the functions being moved here are definitions. So what's the
difference between moving the definitions of functions like
strrreplace, fortify_panic, etc., but not memscan(), check_bytes(),
memchr_inv(), etc? ie. it looks to me like a few more functions can
or should be moved as well. If the point of this patch is to "move
all the helper functions into string_helpers.c so that they gain the
fortification coverage they had been missing" then it looks like you
missed a few. I don't think the compiler will recognize those
non-libc identifiers for any fortification related transforms (unlike
memcpy and friends which are left in place).
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH for-next 15/25] fortify: Move remaining fortify helpers into fortify-string.h
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (13 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 14/25] lib/string: Move helper functions out of string.c Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-25 21:59 ` Nick Desaulniers
2021-08-22 7:51 ` [PATCH for-next 16/25] fortify: Explicitly disable Clang support Kees Cook
` (9 subsequent siblings)
24 siblings, 1 reply; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Francis Laniel, Rasmus Villemoes, Daniel Micay,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
When commit a28a6e860c6c ("string.h: move fortified functions definitions
in a dedicated header.") moved the fortify-specific code, some helpers
were left behind. Move the remaining fortify-specific helpers into
fortify-string.h so they're together where they're used. This requires
that any FORTIFY helper function prototypes be conditionally built to
avoid "no prototype" warnings. Additionally removes unused helpers.
Acked-by: Francis Laniel <laniel_francis@privacyrequired.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/fortify-string.h | 7 +++++++
include/linux/string.h | 9 ---------
lib/string_helpers.c | 2 ++
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index c1be37437e77..7e67d02764db 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -2,6 +2,13 @@
#ifndef _LINUX_FORTIFY_STRING_H_
#define _LINUX_FORTIFY_STRING_H_
+#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
+#define __RENAME(x) __asm__(#x)
+
+void fortify_panic(const char *name) __noreturn __cold;
+void __read_overflow(void) __compiletime_error("detected read beyond size of object (1st parameter)");
+void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)");
+void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
extern void *__underlying_memchr(const void *p, int c, __kernel_size_t size) __RENAME(memchr);
diff --git a/include/linux/string.h b/include/linux/string.h
index b48d2d28e0b1..9473f81b9db2 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -249,15 +249,6 @@ static inline const char *kbasename(const char *path)
return tail ? tail + 1 : path;
}
-#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
-#define __RENAME(x) __asm__(#x)
-
-void fortify_panic(const char *name) __noreturn __cold;
-void __read_overflow(void) __compiletime_error("detected read beyond size of object passed as 1st parameter");
-void __read_overflow2(void) __compiletime_error("detected read beyond size of object passed as 2nd parameter");
-void __read_overflow3(void) __compiletime_error("detected read beyond size of object passed as 3rd parameter");
-void __write_overflow(void) __compiletime_error("detected write beyond size of object passed as 1st parameter");
-
#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
#include <linux/fortify-string.h>
#endif
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index e9433caab217..1274f45ffaf4 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -879,9 +879,11 @@ char *strreplace(char *s, char old, char new)
}
EXPORT_SYMBOL(strreplace);
+#ifdef CONFIG_FORTIFY_SOURCE
void fortify_panic(const char *name)
{
pr_emerg("detected buffer overflow in %s\n", name);
BUG();
}
EXPORT_SYMBOL(fortify_panic);
+#endif /* CONFIG_FORTIFY_SOURCE */
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH for-next 15/25] fortify: Move remaining fortify helpers into fortify-string.h
2021-08-22 7:51 ` [PATCH for-next 15/25] fortify: Move remaining fortify helpers into fortify-string.h Kees Cook
@ 2021-08-25 21:59 ` Nick Desaulniers
0 siblings, 0 replies; 42+ messages in thread
From: Nick Desaulniers @ 2021-08-25 21:59 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Francis Laniel, Rasmus Villemoes, Daniel Micay,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
On Sun, Aug 22, 2021 at 12:57 AM Kees Cook <keescook@chromium.org> wrote:
>
> When commit a28a6e860c6c ("string.h: move fortified functions definitions
> in a dedicated header.") moved the fortify-specific code, some helpers
> were left behind. Move the remaining fortify-specific helpers into
> fortify-string.h so they're together where they're used. This requires
> that any FORTIFY helper function prototypes be conditionally built to
> avoid "no prototype" warnings. Additionally removes unused helpers.
>
> Acked-by: Francis Laniel <laniel_francis@privacyrequired.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> include/linux/fortify-string.h | 7 +++++++
> include/linux/string.h | 9 ---------
> lib/string_helpers.c | 2 ++
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index c1be37437e77..7e67d02764db 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -2,6 +2,13 @@
> #ifndef _LINUX_FORTIFY_STRING_H_
> #define _LINUX_FORTIFY_STRING_H_
>
> +#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
> +#define __RENAME(x) __asm__(#x)
> +
> +void fortify_panic(const char *name) __noreturn __cold;
> +void __read_overflow(void) __compiletime_error("detected read beyond size of object (1st parameter)");
> +void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)");
> +void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
>
> #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
> extern void *__underlying_memchr(const void *p, int c, __kernel_size_t size) __RENAME(memchr);
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b48d2d28e0b1..9473f81b9db2 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -249,15 +249,6 @@ static inline const char *kbasename(const char *path)
> return tail ? tail + 1 : path;
> }
>
> -#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
> -#define __RENAME(x) __asm__(#x)
> -
> -void fortify_panic(const char *name) __noreturn __cold;
> -void __read_overflow(void) __compiletime_error("detected read beyond size of object passed as 1st parameter");
> -void __read_overflow2(void) __compiletime_error("detected read beyond size of object passed as 2nd parameter");
> -void __read_overflow3(void) __compiletime_error("detected read beyond size of object passed as 3rd parameter");
> -void __write_overflow(void) __compiletime_error("detected write beyond size of object passed as 1st parameter");
> -
> #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE)
> #include <linux/fortify-string.h>
> #endif
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index e9433caab217..1274f45ffaf4 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -879,9 +879,11 @@ char *strreplace(char *s, char old, char new)
> }
> EXPORT_SYMBOL(strreplace);
>
> +#ifdef CONFIG_FORTIFY_SOURCE
> void fortify_panic(const char *name)
> {
> pr_emerg("detected buffer overflow in %s\n", name);
> BUG();
> }
> EXPORT_SYMBOL(fortify_panic);
> +#endif /* CONFIG_FORTIFY_SOURCE */
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210822075122.864511-16-keescook%40chromium.org.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH for-next 16/25] fortify: Explicitly disable Clang support
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (14 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 15/25] fortify: Move remaining fortify helpers into fortify-string.h Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-25 19:41 ` Nick Desaulniers
2021-08-22 7:51 ` [PATCH for-next 17/25] fortify: Fix dropped strcpy() compile-time write overflow check Kees Cook
` (8 subsequent siblings)
24 siblings, 1 reply; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
Clang has never correctly compiled the FORTIFY_SOURCE defenses due to
a couple bugs:
Eliding inlines with matching __builtin_* names
https://bugs.llvm.org/show_bug.cgi?id=50322
Incorrect __builtin_constant_p() of some globals
https://bugs.llvm.org/show_bug.cgi?id=41459
In the process of making improvements to the FORTIFY_SOURCE defenses, the
first (silent) bug (coincidentally) becomes worked around, but exposes
the latter which breaks the build. As such, Clang must not be used with
CONFIG_FORTIFY_SOURCE until at least latter bug is fixed (in Clang 13),
and the fortify routines have been rearranged.
Update the Kconfig to reflect the reality of the current situation.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
security/Kconfig | 3 +++
1 file changed, 3 insertions(+)
diff --git a/security/Kconfig b/security/Kconfig
index 0ced7fd33e4d..fe6c0395fa02 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -191,6 +191,9 @@ config HARDENED_USERCOPY_PAGESPAN
config FORTIFY_SOURCE
bool "Harden common str/mem functions against buffer overflows"
depends on ARCH_HAS_FORTIFY_SOURCE
+ # https://bugs.llvm.org/show_bug.cgi?id=50322
+ # https://bugs.llvm.org/show_bug.cgi?id=41459
+ depends on !CC_IS_CLANG
help
Detect overflows of buffers in common string and memory functions
where the compiler can determine and validate the buffer sizes.
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH for-next 16/25] fortify: Explicitly disable Clang support
2021-08-22 7:51 ` [PATCH for-next 16/25] fortify: Explicitly disable Clang support Kees Cook
@ 2021-08-25 19:41 ` Nick Desaulniers
0 siblings, 0 replies; 42+ messages in thread
From: Nick Desaulniers @ 2021-08-25 19:41 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
On Sun, Aug 22, 2021 at 12:57 AM Kees Cook <keescook@chromium.org> wrote:
>
> Clang has never correctly compiled the FORTIFY_SOURCE defenses due to
> a couple bugs:
>
> Eliding inlines with matching __builtin_* names
> https://bugs.llvm.org/show_bug.cgi?id=50322
>
> Incorrect __builtin_constant_p() of some globals
> https://bugs.llvm.org/show_bug.cgi?id=41459
>
> In the process of making improvements to the FORTIFY_SOURCE defenses, the
> first (silent) bug (coincidentally) becomes worked around, but exposes
> the latter which breaks the build. As such, Clang must not be used with
> CONFIG_FORTIFY_SOURCE until at least latter bug is fixed (in Clang 13),
> and the fortify routines have been rearranged.
>
> Update the Kconfig to reflect the reality of the current situation.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> security/Kconfig | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/security/Kconfig b/security/Kconfig
> index 0ced7fd33e4d..fe6c0395fa02 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -191,6 +191,9 @@ config HARDENED_USERCOPY_PAGESPAN
> config FORTIFY_SOURCE
> bool "Harden common str/mem functions against buffer overflows"
> depends on ARCH_HAS_FORTIFY_SOURCE
> + # https://bugs.llvm.org/show_bug.cgi?id=50322
> + # https://bugs.llvm.org/show_bug.cgi?id=41459
> + depends on !CC_IS_CLANG
> help
> Detect overflows of buffers in common string and memory functions
> where the compiler can determine and validate the buffer sizes.
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210822075122.864511-17-keescook%40chromium.org.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH for-next 17/25] fortify: Fix dropped strcpy() compile-time write overflow check
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (15 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 16/25] fortify: Explicitly disable Clang support Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-25 21:55 ` Nick Desaulniers
2021-08-22 7:51 ` [PATCH for-next 18/25] fortify: Prepare to improve strnlen() and strlen() warnings Kees Cook
` (7 subsequent siblings)
24 siblings, 1 reply; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Daniel Axtens, Francis Laniel, Rasmus Villemoes,
Daniel Micay, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
The implementation for intra-object overflow in str*-family functions
accidentally dropped compile-time write overflow checking in strcpy(),
leaving it entirely to run-time. Add back the intended check.
Fixes: 6a39e62abbaf ("lib: string.h: detect intra-object overflow in fortified string functions")
Cc: Daniel Axtens <dja@axtens.net>
Cc: Francis Laniel <laniel_francis@privacyrequired.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/fortify-string.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 7e67d02764db..68bc5978d916 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -287,7 +287,10 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
if (p_size == (size_t)-1 && q_size == (size_t)-1)
return __underlying_strcpy(p, q);
size = strlen(q) + 1;
- /* test here to use the more stringent object size */
+ /* Compile-time check for const size overflow. */
+ if (__builtin_constant_p(size) && p_size < size)
+ __write_overflow();
+ /* Run-time check for dynamic size overflow. */
if (p_size < size)
fortify_panic(__func__);
memcpy(p, q, size);
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH for-next 17/25] fortify: Fix dropped strcpy() compile-time write overflow check
2021-08-22 7:51 ` [PATCH for-next 17/25] fortify: Fix dropped strcpy() compile-time write overflow check Kees Cook
@ 2021-08-25 21:55 ` Nick Desaulniers
0 siblings, 0 replies; 42+ messages in thread
From: Nick Desaulniers @ 2021-08-25 21:55 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Daniel Axtens, Francis Laniel, Rasmus Villemoes,
Daniel Micay, Bart Van Assche, David Gow, linux-mm,
clang-built-linux, linux-hardening
On Sun, Aug 22, 2021 at 12:57 AM Kees Cook <keescook@chromium.org> wrote:
>
> The implementation for intra-object overflow in str*-family functions
> accidentally dropped compile-time write overflow checking in strcpy(),
> leaving it entirely to run-time. Add back the intended check.
>
> Fixes: 6a39e62abbaf ("lib: string.h: detect intra-object overflow in fortified string functions")
> Cc: Daniel Axtens <dja@axtens.net>
> Cc: Francis Laniel <laniel_francis@privacyrequired.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> include/linux/fortify-string.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index 7e67d02764db..68bc5978d916 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -287,7 +287,10 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
> if (p_size == (size_t)-1 && q_size == (size_t)-1)
> return __underlying_strcpy(p, q);
> size = strlen(q) + 1;
> - /* test here to use the more stringent object size */
> + /* Compile-time check for const size overflow. */
> + if (__builtin_constant_p(size) && p_size < size)
> + __write_overflow();
> + /* Run-time check for dynamic size overflow. */
> if (p_size < size)
> fortify_panic(__func__);
> memcpy(p, q, size);
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210822075122.864511-18-keescook%40chromium.org.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH for-next 18/25] fortify: Prepare to improve strnlen() and strlen() warnings
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (16 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 17/25] fortify: Fix dropped strcpy() compile-time write overflow check Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-25 22:01 ` Nick Desaulniers
2021-08-22 7:51 ` [PATCH for-next 19/25] fortify: Allow strlen() and strnlen() to pass compile-time known lengths Kees Cook
` (6 subsequent siblings)
24 siblings, 1 reply; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Francis Laniel, Rasmus Villemoes, Daniel Micay,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
In order to have strlen() use fortified strnlen() internally, swap their
positions in the source. Doing this as part of later changes makes
review difficult, so reoroder it here; no code changes.
Cc: Francis Laniel <laniel_francis@privacyrequired.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/fortify-string.h | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 68bc5978d916..a3cb1d9aacce 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -56,6 +56,17 @@ __FORTIFY_INLINE char *strcat(char *p, const char *q)
return p;
}
+extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
+__FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
+{
+ size_t p_size = __builtin_object_size(p, 1);
+ __kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
+
+ if (p_size <= ret && maxlen != ret)
+ fortify_panic(__func__);
+ return ret;
+}
+
__FORTIFY_INLINE __kernel_size_t strlen(const char *p)
{
__kernel_size_t ret;
@@ -71,17 +82,6 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
return ret;
}
-extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
-__FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
-{
- size_t p_size = __builtin_object_size(p, 1);
- __kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
-
- if (p_size <= ret && maxlen != ret)
- fortify_panic(__func__);
- return ret;
-}
-
/* defined after fortified strlen to reuse it */
extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy);
__FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH for-next 18/25] fortify: Prepare to improve strnlen() and strlen() warnings
2021-08-22 7:51 ` [PATCH for-next 18/25] fortify: Prepare to improve strnlen() and strlen() warnings Kees Cook
@ 2021-08-25 22:01 ` Nick Desaulniers
0 siblings, 0 replies; 42+ messages in thread
From: Nick Desaulniers @ 2021-08-25 22:01 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Francis Laniel, Rasmus Villemoes, Daniel Micay,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
On Sun, Aug 22, 2021 at 12:57 AM Kees Cook <keescook@chromium.org> wrote:
>
> In order to have strlen() use fortified strnlen() internally, swap their
> positions in the source. Doing this as part of later changes makes
> review difficult, so reoroder it here; no code changes.
>
> Cc: Francis Laniel <laniel_francis@privacyrequired.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> include/linux/fortify-string.h | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index 68bc5978d916..a3cb1d9aacce 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -56,6 +56,17 @@ __FORTIFY_INLINE char *strcat(char *p, const char *q)
> return p;
> }
>
> +extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
> +__FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
> +{
> + size_t p_size = __builtin_object_size(p, 1);
> + __kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
> +
> + if (p_size <= ret && maxlen != ret)
> + fortify_panic(__func__);
> + return ret;
> +}
> +
> __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> {
> __kernel_size_t ret;
> @@ -71,17 +82,6 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> return ret;
> }
>
> -extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
> -__FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
> -{
> - size_t p_size = __builtin_object_size(p, 1);
> - __kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
> -
> - if (p_size <= ret && maxlen != ret)
> - fortify_panic(__func__);
> - return ret;
> -}
> -
> /* defined after fortified strlen to reuse it */
> extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy);
> __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210822075122.864511-19-keescook%40chromium.org.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH for-next 19/25] fortify: Allow strlen() and strnlen() to pass compile-time known lengths
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (17 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 18/25] fortify: Prepare to improve strnlen() and strlen() warnings Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-25 22:05 ` Nick Desaulniers
2021-08-22 7:51 ` [PATCH for-next 20/25] fortify: Add compile-time FORTIFY_SOURCE tests Kees Cook
` (5 subsequent siblings)
24 siblings, 1 reply; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
Under CONFIG_FORTIFY_SOURCE, it is possible for the compiler to perform
strlen() and strnlen() at compile-time when the string size is known.
This is required to support compile-time overflow checking in strlcpy().
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/fortify-string.h | 47 ++++++++++++++++++++++++++--------
1 file changed, 36 insertions(+), 11 deletions(-)
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index a3cb1d9aacce..e232a63fd826 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -10,6 +10,18 @@ void __read_overflow(void) __compiletime_error("detected read beyond size of obj
void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)");
void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
+#define __compiletime_strlen(p) ({ \
+ size_t ret = (size_t)-1; \
+ size_t p_size = __builtin_object_size(p, 1); \
+ if (p_size != (size_t)-1) { \
+ size_t p_len = p_size - 1; \
+ if (__builtin_constant_p(p[p_len]) && \
+ p[p_len] == '\0') \
+ ret = __builtin_strlen(p); \
+ } \
+ ret; \
+})
+
#if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
extern void *__underlying_memchr(const void *p, int c, __kernel_size_t size) __RENAME(memchr);
extern int __underlying_memcmp(const void *p, const void *q, __kernel_size_t size) __RENAME(memcmp);
@@ -60,21 +72,31 @@ extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(st
__FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
{
size_t p_size = __builtin_object_size(p, 1);
- __kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
+ size_t p_len = __compiletime_strlen(p);
+ size_t ret;
+
+ /* We can take compile-time actions when maxlen is const. */
+ if (__builtin_constant_p(maxlen) && p_len != (size_t)-1) {
+ /* If p is const, we can use its compile-time-known len. */
+ if (maxlen >= p_size)
+ return p_len;
+ }
+ /* Do no check characters beyond the end of p. */
+ ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
if (p_size <= ret && maxlen != ret)
fortify_panic(__func__);
return ret;
}
+/* defined after fortified strnlen to reuse it. */
__FORTIFY_INLINE __kernel_size_t strlen(const char *p)
{
__kernel_size_t ret;
size_t p_size = __builtin_object_size(p, 1);
- /* Work around gcc excess stack consumption issue */
- if (p_size == (size_t)-1 ||
- (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
+ /* Give up if we don't know how large p is. */
+ if (p_size == (size_t)-1)
return __underlying_strlen(p);
ret = strnlen(p, p_size);
if (p_size <= ret)
@@ -86,24 +108,27 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy);
__FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
{
- size_t ret;
size_t p_size = __builtin_object_size(p, 1);
size_t q_size = __builtin_object_size(q, 1);
+ size_t q_len; /* Full count of source string length. */
+ size_t len; /* Count of characters going into destination. */
if (p_size == (size_t)-1 && q_size == (size_t)-1)
return __real_strlcpy(p, q, size);
- ret = strlen(q);
- if (size) {
- size_t len = (ret >= size) ? size - 1 : ret;
-
- if (__builtin_constant_p(len) && len >= p_size)
+ q_len = strlen(q);
+ len = (q_len >= size) ? size - 1 : q_len;
+ if (__builtin_constant_p(size) && __builtin_constant_p(q_len) && size) {
+ /* Write size is always larger than destintation. */
+ if (len >= p_size)
__write_overflow();
+ }
+ if (size) {
if (len >= p_size)
fortify_panic(__func__);
__underlying_memcpy(p, q, len);
p[len] = '\0';
}
- return ret;
+ return q_len;
}
/* defined after fortified strnlen to reuse it */
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH for-next 19/25] fortify: Allow strlen() and strnlen() to pass compile-time known lengths
2021-08-22 7:51 ` [PATCH for-next 19/25] fortify: Allow strlen() and strnlen() to pass compile-time known lengths Kees Cook
@ 2021-08-25 22:05 ` Nick Desaulniers
2021-08-26 2:56 ` Kees Cook
0 siblings, 1 reply; 42+ messages in thread
From: Nick Desaulniers @ 2021-08-25 22:05 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
On Sun, Aug 22, 2021 at 12:57 AM Kees Cook <keescook@chromium.org> wrote:
>
> Under CONFIG_FORTIFY_SOURCE, it is possible for the compiler to perform
> strlen() and strnlen() at compile-time when the string size is known.
> This is required to support compile-time overflow checking in strlcpy().
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> include/linux/fortify-string.h | 47 ++++++++++++++++++++++++++--------
> 1 file changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index a3cb1d9aacce..e232a63fd826 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -10,6 +10,18 @@ void __read_overflow(void) __compiletime_error("detected read beyond size of obj
> void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)");
> void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
>
> +#define __compiletime_strlen(p) ({ \
> + size_t ret = (size_t)-1; \
> + size_t p_size = __builtin_object_size(p, 1); \
> + if (p_size != (size_t)-1) { \
> + size_t p_len = p_size - 1; \
> + if (__builtin_constant_p(p[p_len]) && \
> + p[p_len] == '\0') \
> + ret = __builtin_strlen(p); \
> + } \
> + ret; \
> +})
Can this be a `static inline` function that accepts a `const char *`
and returns a `size_t` rather than a statement expression?
> +
> #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
> extern void *__underlying_memchr(const void *p, int c, __kernel_size_t size) __RENAME(memchr);
> extern int __underlying_memcmp(const void *p, const void *q, __kernel_size_t size) __RENAME(memcmp);
> @@ -60,21 +72,31 @@ extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(st
> __FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
> {
> size_t p_size = __builtin_object_size(p, 1);
> - __kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
> + size_t p_len = __compiletime_strlen(p);
> + size_t ret;
> +
> + /* We can take compile-time actions when maxlen is const. */
> + if (__builtin_constant_p(maxlen) && p_len != (size_t)-1) {
> + /* If p is const, we can use its compile-time-known len. */
> + if (maxlen >= p_size)
> + return p_len;
> + }
>
> + /* Do no check characters beyond the end of p. */
s/no/not/
> + ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
> if (p_size <= ret && maxlen != ret)
> fortify_panic(__func__);
> return ret;
> }
>
> +/* defined after fortified strnlen to reuse it. */
> __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> {
> __kernel_size_t ret;
> size_t p_size = __builtin_object_size(p, 1);
>
> - /* Work around gcc excess stack consumption issue */
> - if (p_size == (size_t)-1 ||
> - (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
> + /* Give up if we don't know how large p is. */
> + if (p_size == (size_t)-1)
> return __underlying_strlen(p);
> ret = strnlen(p, p_size);
> if (p_size <= ret)
> @@ -86,24 +108,27 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy);
> __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
> {
> - size_t ret;
> size_t p_size = __builtin_object_size(p, 1);
> size_t q_size = __builtin_object_size(q, 1);
> + size_t q_len; /* Full count of source string length. */
> + size_t len; /* Count of characters going into destination. */
>
> if (p_size == (size_t)-1 && q_size == (size_t)-1)
> return __real_strlcpy(p, q, size);
> - ret = strlen(q);
> - if (size) {
> - size_t len = (ret >= size) ? size - 1 : ret;
> -
> - if (__builtin_constant_p(len) && len >= p_size)
> + q_len = strlen(q);
> + len = (q_len >= size) ? size - 1 : q_len;
> + if (__builtin_constant_p(size) && __builtin_constant_p(q_len) && size) {
> + /* Write size is always larger than destintation. */
s/destintation/destination/
> + if (len >= p_size)
> __write_overflow();
> + }
> + if (size) {
> if (len >= p_size)
> fortify_panic(__func__);
> __underlying_memcpy(p, q, len);
> p[len] = '\0';
> }
> - return ret;
> + return q_len;
> }
>
> /* defined after fortified strnlen to reuse it */
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210822075122.864511-20-keescook%40chromium.org.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH for-next 19/25] fortify: Allow strlen() and strnlen() to pass compile-time known lengths
2021-08-25 22:05 ` Nick Desaulniers
@ 2021-08-26 2:56 ` Kees Cook
2021-08-26 18:02 ` Nick Desaulniers
0 siblings, 1 reply; 42+ messages in thread
From: Kees Cook @ 2021-08-26 2:56 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-kernel, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
On Wed, Aug 25, 2021 at 03:05:56PM -0700, Nick Desaulniers wrote:
> On Sun, Aug 22, 2021 at 12:57 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > Under CONFIG_FORTIFY_SOURCE, it is possible for the compiler to perform
> > strlen() and strnlen() at compile-time when the string size is known.
> > This is required to support compile-time overflow checking in strlcpy().
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > include/linux/fortify-string.h | 47 ++++++++++++++++++++++++++--------
> > 1 file changed, 36 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> > index a3cb1d9aacce..e232a63fd826 100644
> > --- a/include/linux/fortify-string.h
> > +++ b/include/linux/fortify-string.h
> > @@ -10,6 +10,18 @@ void __read_overflow(void) __compiletime_error("detected read beyond size of obj
> > void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)");
> > void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
> >
> > +#define __compiletime_strlen(p) ({ \
> > + size_t ret = (size_t)-1; \
> > + size_t p_size = __builtin_object_size(p, 1); \
> > + if (p_size != (size_t)-1) { \
> > + size_t p_len = p_size - 1; \
> > + if (__builtin_constant_p(p[p_len]) && \
> > + p[p_len] == '\0') \
> > + ret = __builtin_strlen(p); \
> > + } \
> > + ret; \
> > +})
>
> Can this be a `static inline` function that accepts a `const char *`
> and returns a `size_t` rather than a statement expression?
No because both __builtin_object_size() and __builtin_strlen() may not
work. See:
https://lore.kernel.org/lkml/20210818060533.3569517-64-keescook@chromium.org/
Regardless, it will always collapse to a const value of either -1 or
the length of the string.
>
> > +
> > #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
> > extern void *__underlying_memchr(const void *p, int c, __kernel_size_t size) __RENAME(memchr);
> > extern int __underlying_memcmp(const void *p, const void *q, __kernel_size_t size) __RENAME(memcmp);
> > @@ -60,21 +72,31 @@ extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(st
> > __FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
> > {
> > size_t p_size = __builtin_object_size(p, 1);
> > - __kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
> > + size_t p_len = __compiletime_strlen(p);
> > + size_t ret;
> > +
> > + /* We can take compile-time actions when maxlen is const. */
> > + if (__builtin_constant_p(maxlen) && p_len != (size_t)-1) {
> > + /* If p is const, we can use its compile-time-known len. */
> > + if (maxlen >= p_size)
> > + return p_len;
> > + }
> >
> > + /* Do no check characters beyond the end of p. */
>
> s/no/not/
Thanks!
>
> > + ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
> > if (p_size <= ret && maxlen != ret)
> > fortify_panic(__func__);
> > return ret;
> > }
> >
> > +/* defined after fortified strnlen to reuse it. */
> > __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> > {
> > __kernel_size_t ret;
> > size_t p_size = __builtin_object_size(p, 1);
> >
> > - /* Work around gcc excess stack consumption issue */
> > - if (p_size == (size_t)-1 ||
> > - (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0'))
> > + /* Give up if we don't know how large p is. */
> > + if (p_size == (size_t)-1)
> > return __underlying_strlen(p);
> > ret = strnlen(p, p_size);
> > if (p_size <= ret)
> > @@ -86,24 +108,27 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> > extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy);
> > __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
> > {
> > - size_t ret;
> > size_t p_size = __builtin_object_size(p, 1);
> > size_t q_size = __builtin_object_size(q, 1);
> > + size_t q_len; /* Full count of source string length. */
> > + size_t len; /* Count of characters going into destination. */
> >
> > if (p_size == (size_t)-1 && q_size == (size_t)-1)
> > return __real_strlcpy(p, q, size);
> > - ret = strlen(q);
> > - if (size) {
> > - size_t len = (ret >= size) ? size - 1 : ret;
> > -
> > - if (__builtin_constant_p(len) && len >= p_size)
> > + q_len = strlen(q);
> > + len = (q_len >= size) ? size - 1 : q_len;
> > + if (__builtin_constant_p(size) && __builtin_constant_p(q_len) && size) {
> > + /* Write size is always larger than destintation. */
>
> s/destintation/destination/
I can't type. :)
Fixed now.
-Kees
>
> > + if (len >= p_size)
> > __write_overflow();
> > + }
> > + if (size) {
> > if (len >= p_size)
> > fortify_panic(__func__);
> > __underlying_memcpy(p, q, len);
> > p[len] = '\0';
> > }
> > - return ret;
> > + return q_len;
> > }
> >
> > /* defined after fortified strnlen to reuse it */
> > --
> > 2.30.2
> >
> > --
> > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210822075122.864511-20-keescook%40chromium.org.
>
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Kees Cook
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH for-next 19/25] fortify: Allow strlen() and strnlen() to pass compile-time known lengths
2021-08-26 2:56 ` Kees Cook
@ 2021-08-26 18:02 ` Nick Desaulniers
0 siblings, 0 replies; 42+ messages in thread
From: Nick Desaulniers @ 2021-08-26 18:02 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
On Wed, Aug 25, 2021 at 7:56 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Aug 25, 2021 at 03:05:56PM -0700, Nick Desaulniers wrote:
> > On Sun, Aug 22, 2021 at 12:57 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> > > index a3cb1d9aacce..e232a63fd826 100644
> > > --- a/include/linux/fortify-string.h
> > > +++ b/include/linux/fortify-string.h
> > > @@ -10,6 +10,18 @@ void __read_overflow(void) __compiletime_error("detected read beyond size of obj
> > > void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)");
> > > void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
> > >
> > > +#define __compiletime_strlen(p) ({ \
> > > + size_t ret = (size_t)-1; \
> > > + size_t p_size = __builtin_object_size(p, 1); \
> > > + if (p_size != (size_t)-1) { \
> > > + size_t p_len = p_size - 1; \
> > > + if (__builtin_constant_p(p[p_len]) && \
> > > + p[p_len] == '\0') \
> > > + ret = __builtin_strlen(p); \
> > > + } \
> > > + ret; \
> > > +})
> >
> > Can this be a `static inline` function that accepts a `const char *`
> > and returns a `size_t` rather than a statement expression?
>
> No because both __builtin_object_size() and __builtin_strlen() may not
> work. See:
> https://lore.kernel.org/lkml/20210818060533.3569517-64-keescook@chromium.org/
Ah right, then consider adding a comment to encourage others not to
rewrite it as such.
>
> Regardless, it will always collapse to a const value of either -1 or
> the length of the string.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH for-next 20/25] fortify: Add compile-time FORTIFY_SOURCE tests
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (18 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 19/25] fortify: Allow strlen() and strnlen() to pass compile-time known lengths Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 21/25] lib: Introduce CONFIG_TEST_MEMCPY Kees Cook
` (4 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
While the run-time testing of FORTIFY_SOURCE is already present in
LKDTM, there is no testing of the expected compile-time detections. In
preparation for correctly supporting FORTIFY_SOURCE under Clang, adding
additional FORTIFY_SOURCE defenses, and making sure FORTIFY_SOURCE
doesn't silently regress with GCC, introduce a build-time test suite that
checks each expected compile-time failure condition.
As this is relatively backwards from standard build rules in the
sense that a successful test is actually a compile _failure_, create
a wrapper script to check for the correct errors, and wire it up as
a dummy dependency to lib/string.o, collecting the results into a log
file artifact.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
lib/.gitignore | 2 +
lib/Makefile | 33 +++++++++++
lib/test_fortify/read_overflow-memchr.c | 5 ++
lib/test_fortify/read_overflow-memchr_inv.c | 5 ++
lib/test_fortify/read_overflow-memcmp.c | 5 ++
lib/test_fortify/read_overflow-memscan.c | 5 ++
lib/test_fortify/read_overflow2-memcmp.c | 5 ++
lib/test_fortify/read_overflow2-memcpy.c | 5 ++
lib/test_fortify/read_overflow2-memmove.c | 5 ++
lib/test_fortify/test_fortify.h | 35 +++++++++++
lib/test_fortify/write_overflow-memcpy.c | 5 ++
lib/test_fortify/write_overflow-memmove.c | 5 ++
lib/test_fortify/write_overflow-memset.c | 5 ++
lib/test_fortify/write_overflow-strcpy-lit.c | 5 ++
lib/test_fortify/write_overflow-strcpy.c | 5 ++
lib/test_fortify/write_overflow-strlcpy-src.c | 5 ++
lib/test_fortify/write_overflow-strlcpy.c | 5 ++
lib/test_fortify/write_overflow-strncpy-src.c | 5 ++
lib/test_fortify/write_overflow-strncpy.c | 5 ++
lib/test_fortify/write_overflow-strscpy.c | 5 ++
scripts/test_fortify.sh | 59 +++++++++++++++++++
21 files changed, 214 insertions(+)
create mode 100644 lib/test_fortify/read_overflow-memchr.c
create mode 100644 lib/test_fortify/read_overflow-memchr_inv.c
create mode 100644 lib/test_fortify/read_overflow-memcmp.c
create mode 100644 lib/test_fortify/read_overflow-memscan.c
create mode 100644 lib/test_fortify/read_overflow2-memcmp.c
create mode 100644 lib/test_fortify/read_overflow2-memcpy.c
create mode 100644 lib/test_fortify/read_overflow2-memmove.c
create mode 100644 lib/test_fortify/test_fortify.h
create mode 100644 lib/test_fortify/write_overflow-memcpy.c
create mode 100644 lib/test_fortify/write_overflow-memmove.c
create mode 100644 lib/test_fortify/write_overflow-memset.c
create mode 100644 lib/test_fortify/write_overflow-strcpy-lit.c
create mode 100644 lib/test_fortify/write_overflow-strcpy.c
create mode 100644 lib/test_fortify/write_overflow-strlcpy-src.c
create mode 100644 lib/test_fortify/write_overflow-strlcpy.c
create mode 100644 lib/test_fortify/write_overflow-strncpy-src.c
create mode 100644 lib/test_fortify/write_overflow-strncpy.c
create mode 100644 lib/test_fortify/write_overflow-strscpy.c
create mode 100644 scripts/test_fortify.sh
diff --git a/lib/.gitignore b/lib/.gitignore
index 5e7fa54c4536..e5e217b8307b 100644
--- a/lib/.gitignore
+++ b/lib/.gitignore
@@ -4,3 +4,5 @@
/gen_crc32table
/gen_crc64table
/oid_registry_data.c
+/test_fortify.log
+/test_fortify/*.log
diff --git a/lib/Makefile b/lib/Makefile
index 5efd1b435a37..bd17c2bf43e1 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -360,3 +360,36 @@ obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o
obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
+
+# FORTIFY_SOURCE compile-time behavior tests
+TEST_FORTIFY_SRCS = $(wildcard $(srctree)/$(src)/test_fortify/*-*.c)
+TEST_FORTIFY_LOGS = $(patsubst $(srctree)/$(src)/%.c, %.log, $(TEST_FORTIFY_SRCS))
+TEST_FORTIFY_LOG = test_fortify.log
+
+quiet_cmd_test_fortify = TEST $@
+ cmd_test_fortify = $(CONFIG_SHELL) $(srctree)/scripts/test_fortify.sh \
+ $< $@ "$(NM)" $(CC) $(c_flags) \
+ $(call cc-disable-warning,fortify-source)
+
+targets += $(TEST_FORTIFY_LOGS)
+clean-files += $(TEST_FORTIFY_LOGS)
+clean-files += $(addsuffix .o, $(TEST_FORTIFY_LOGS))
+$(obj)/test_fortify/%.log: $(src)/test_fortify/%.c \
+ $(src)/test_fortify/test_fortify.h \
+ $(srctree)/include/linux/fortify-string.h \
+ $(srctree)/scripts/test_fortify.sh \
+ FORCE
+ $(call if_changed,test_fortify)
+
+quiet_cmd_gen_fortify_log = GEN $@
+ cmd_gen_fortify_log = cat </dev/null $(filter-out FORCE,$^) 2>/dev/null > $@ || true
+
+targets += $(TEST_FORTIFY_LOG)
+clean-files += $(TEST_FORTIFY_LOG)
+$(obj)/$(TEST_FORTIFY_LOG): $(addprefix $(obj)/, $(TEST_FORTIFY_LOGS)) FORCE
+ $(call if_changed,gen_fortify_log)
+
+# Fake dependency to trigger the fortify tests.
+ifeq ($(CONFIG_FORTIFY_SOURCE),y)
+$(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG)
+endif
diff --git a/lib/test_fortify/read_overflow-memchr.c b/lib/test_fortify/read_overflow-memchr.c
new file mode 100644
index 000000000000..2743084b32af
--- /dev/null
+++ b/lib/test_fortify/read_overflow-memchr.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memchr(small, 0x7A, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow-memchr_inv.c b/lib/test_fortify/read_overflow-memchr_inv.c
new file mode 100644
index 000000000000..b26e1f1bc217
--- /dev/null
+++ b/lib/test_fortify/read_overflow-memchr_inv.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memchr_inv(small, 0x7A, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow-memcmp.c b/lib/test_fortify/read_overflow-memcmp.c
new file mode 100644
index 000000000000..d5d301ff64ef
--- /dev/null
+++ b/lib/test_fortify/read_overflow-memcmp.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memcmp(small, large, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow-memscan.c b/lib/test_fortify/read_overflow-memscan.c
new file mode 100644
index 000000000000..c1a97f2df0f0
--- /dev/null
+++ b/lib/test_fortify/read_overflow-memscan.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memscan(small, 0x7A, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow2-memcmp.c b/lib/test_fortify/read_overflow2-memcmp.c
new file mode 100644
index 000000000000..c6091e640f76
--- /dev/null
+++ b/lib/test_fortify/read_overflow2-memcmp.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memcmp(large, small, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow2-memcpy.c b/lib/test_fortify/read_overflow2-memcpy.c
new file mode 100644
index 000000000000..07b62e56cf16
--- /dev/null
+++ b/lib/test_fortify/read_overflow2-memcpy.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memcpy(large, instance.buf, sizeof(large))
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/read_overflow2-memmove.c b/lib/test_fortify/read_overflow2-memmove.c
new file mode 100644
index 000000000000..34edfab040a3
--- /dev/null
+++ b/lib/test_fortify/read_overflow2-memmove.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memmove(large, instance.buf, sizeof(large))
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/test_fortify.h b/lib/test_fortify/test_fortify.h
new file mode 100644
index 000000000000..d22664fff197
--- /dev/null
+++ b/lib/test_fortify/test_fortify.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#include <linux/kernel.h>
+#include <linux/printk.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+void do_fortify_tests(void);
+
+#define __BUF_SMALL 16
+#define __BUF_LARGE 32
+struct fortify_object {
+ int a;
+ char buf[__BUF_SMALL];
+ int c;
+};
+
+#define LITERAL_SMALL "AAAAAAAAAAAAAAA"
+#define LITERAL_LARGE "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+const char small_src[__BUF_SMALL] = LITERAL_SMALL;
+const char large_src[__BUF_LARGE] = LITERAL_LARGE;
+
+char small[__BUF_SMALL];
+char large[__BUF_LARGE];
+struct fortify_object instance;
+size_t size;
+
+void do_fortify_tests(void)
+{
+ /* Normal initializations. */
+ memset(&instance, 0x32, sizeof(instance));
+ memset(small, 0xA5, sizeof(small));
+ memset(large, 0x5A, sizeof(large));
+
+ TEST;
+}
diff --git a/lib/test_fortify/write_overflow-memcpy.c b/lib/test_fortify/write_overflow-memcpy.c
new file mode 100644
index 000000000000..3b3984e428fb
--- /dev/null
+++ b/lib/test_fortify/write_overflow-memcpy.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memcpy(instance.buf, large_src, sizeof(large_src))
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-memmove.c b/lib/test_fortify/write_overflow-memmove.c
new file mode 100644
index 000000000000..640437c3b3e0
--- /dev/null
+++ b/lib/test_fortify/write_overflow-memmove.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memmove(instance.buf, large_src, sizeof(large_src))
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-memset.c b/lib/test_fortify/write_overflow-memset.c
new file mode 100644
index 000000000000..36e34908cfb3
--- /dev/null
+++ b/lib/test_fortify/write_overflow-memset.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memset(instance.buf, 0x5A, sizeof(large_src))
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-strcpy-lit.c b/lib/test_fortify/write_overflow-strcpy-lit.c
new file mode 100644
index 000000000000..51effb3e50f9
--- /dev/null
+++ b/lib/test_fortify/write_overflow-strcpy-lit.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ strcpy(small, LITERAL_LARGE)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-strcpy.c b/lib/test_fortify/write_overflow-strcpy.c
new file mode 100644
index 000000000000..84f1c56a64c8
--- /dev/null
+++ b/lib/test_fortify/write_overflow-strcpy.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ strcpy(small, large_src)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-strlcpy-src.c b/lib/test_fortify/write_overflow-strlcpy-src.c
new file mode 100644
index 000000000000..91bf83ebd34a
--- /dev/null
+++ b/lib/test_fortify/write_overflow-strlcpy-src.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ strlcpy(small, large_src, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-strlcpy.c b/lib/test_fortify/write_overflow-strlcpy.c
new file mode 100644
index 000000000000..1883db7c0cd6
--- /dev/null
+++ b/lib/test_fortify/write_overflow-strlcpy.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ strlcpy(instance.buf, large_src, sizeof(instance.buf) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-strncpy-src.c b/lib/test_fortify/write_overflow-strncpy-src.c
new file mode 100644
index 000000000000..8dcfb8c788dd
--- /dev/null
+++ b/lib/test_fortify/write_overflow-strncpy-src.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ strncpy(small, large_src, sizeof(small) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-strncpy.c b/lib/test_fortify/write_overflow-strncpy.c
new file mode 100644
index 000000000000..b85f079c815d
--- /dev/null
+++ b/lib/test_fortify/write_overflow-strncpy.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ strncpy(instance.buf, large_src, sizeof(instance.buf) + 1)
+
+#include "test_fortify.h"
diff --git a/lib/test_fortify/write_overflow-strscpy.c b/lib/test_fortify/write_overflow-strscpy.c
new file mode 100644
index 000000000000..38feddf377dc
--- /dev/null
+++ b/lib/test_fortify/write_overflow-strscpy.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ strscpy(instance.buf, large_src, sizeof(instance.buf) + 1)
+
+#include "test_fortify.h"
diff --git a/scripts/test_fortify.sh b/scripts/test_fortify.sh
new file mode 100644
index 000000000000..622c7a0b15e5
--- /dev/null
+++ b/scripts/test_fortify.sh
@@ -0,0 +1,59 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0-only
+set -e
+
+# Argument 1: Source file to build.
+IN="$1"
+shift
+# Extract just the filename for error messages below.
+FILE="${IN##*/}"
+# Extract the function name for error messages below.
+FUNC="${FILE#*-}"
+FUNC="${FUNC%%-*}"
+FUNC="${FUNC%%.*}"
+# Extract the symbol to test for in build/symbol test below.
+WANT="__${FILE%%-*}"
+
+# Argument 2: Where to write the build log.
+OUT="$1"
+shift
+TMP="${OUT}.tmp"
+
+# Argument 3: Path to "nm" tool.
+NM="$1"
+shift
+
+# Remaining arguments are: $(CC) $(c_flags)
+
+# Clean up temporary file at exit.
+__cleanup() {
+ rm -f "$TMP"
+}
+trap __cleanup EXIT
+
+status=
+# Attempt to build a source that is expected to fail with a specific warning.
+if "$@" -Werror -c "$IN" -o "$OUT".o 2> "$TMP" ; then
+ # If the build succeeds, either the test has failed or the
+ # warning may only happen at link time (Clang). In that case,
+ # make sure the expected symbol is unresolved in the symbol list.
+ # If so, FORTIFY is working for this case.
+ if ! $NM -A "$OUT".o | grep -m1 "\bU ${WANT}$" >>"$TMP" ; then
+ status="warning: unsafe ${FUNC}() usage lacked '$WANT' symbol in $IN"
+ fi
+else
+ # If the build failed, check for the warning in the stderr (gcc).
+ if ! grep -q -m1 "error:.*\b${WANT}'" "$TMP" ; then
+ status="warning: unsafe ${FUNC}() usage lacked '$WANT' warning in $IN"
+ fi
+fi
+
+if [ -n "$status" ]; then
+ # Report on failure results, including compilation warnings.
+ echo "$status" | tee "$OUT" >&2
+ cat "$TMP" | tee -a "$OUT" >&2
+else
+ # Report on good results, and save any compilation output to log.
+ echo "ok: unsafe ${FUNC}() usage correctly detected with '$WANT' in $IN" >"$OUT"
+ cat "$TMP" >>"$OUT"
+fi
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 21/25] lib: Introduce CONFIG_TEST_MEMCPY
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (19 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 20/25] fortify: Add compile-time FORTIFY_SOURCE tests Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-24 7:00 ` David Gow
2021-10-18 15:46 ` Arnd Bergmann
2021-08-22 7:51 ` [PATCH for-next 22/25] string.h: Introduce memset_after() for wiping trailing members/padding Kees Cook
` (3 subsequent siblings)
24 siblings, 2 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
Before changing anything about memcpy(), memmove(), and memset(), add
run-time tests to check basic behaviors for any regressions.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
MAINTAINERS | 9 ++
lib/Kconfig.debug | 11 ++
lib/Makefile | 1 +
lib/test_memcpy.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 286 insertions(+)
create mode 100644 lib/test_memcpy.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 6c8be735cc91..e3ffd4bdc24f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7248,6 +7248,15 @@ L: netdev@vger.kernel.org
S: Maintained
F: drivers/net/ethernet/nvidia/*
+FORTIFY_SOURCE
+M: Kees Cook <keescook@chomium.org>
+L: linux-hardening@vger.kernel.org
+S: Supported
+F: include/linux/fortify-string.h
+F: lib/test_fortify/*
+F: scripts/test_fortify.sh
+K: \b__NO_FORTIFY\b
+
FPGA DFL DRIVERS
M: Wu Hao <hao.wu@intel.com>
R: Tom Rix <trix@redhat.com>
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 831212722924..9199be57ba2a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2467,6 +2467,17 @@ config RATIONAL_KUNIT_TEST
If unsure, say N.
+config MEMCPY_KUNIT_TEST
+ tristate "Test memcpy(), memmove(), and memset() functions at runtime" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ Builds unit tests for memcpy(), memmove(), and memset() functions.
+ For more information on KUnit and unit tests in general please refer
+ to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+ If unsure, say N.
+
config TEST_UDELAY
tristate "udelay test driver"
help
diff --git a/lib/Makefile b/lib/Makefile
index bd17c2bf43e1..8a4c8bdb38a2 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_TEST_MIN_HEAP) += test_min_heap.o
obj-$(CONFIG_TEST_LKM) += test_module.o
obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
+obj-$(CONFIG_TEST_MEMCPY) += test_memcpy.o
obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
obj-$(CONFIG_TEST_SORT) += test_sort.o
obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
diff --git a/lib/test_memcpy.c b/lib/test_memcpy.c
new file mode 100644
index 000000000000..ec546cec883e
--- /dev/null
+++ b/lib/test_memcpy.c
@@ -0,0 +1,265 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test cases for memcpy(), memmove(), and memset().
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <kunit/test.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/overflow.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/vmalloc.h>
+
+struct some_bytes {
+ union {
+ u8 data[32];
+ struct {
+ u32 one;
+ u16 two;
+ u8 three;
+ /* 1 byte hole */
+ u32 four[4];
+ };
+ };
+};
+
+#define check(instance, v) do { \
+ int i; \
+ BUILD_BUG_ON(sizeof(instance.data) != 32); \
+ for (i = 0; i < sizeof(instance.data); i++) { \
+ KUNIT_ASSERT_EQ_MSG(test, instance.data[i], v, \
+ "line %d: '%s' not initialized to 0x%02x @ %d (saw 0x%02x)\n", \
+ __LINE__, #instance, v, i, instance.data[i]); \
+ } \
+} while (0)
+
+#define compare(name, one, two) do { \
+ int i; \
+ BUILD_BUG_ON(sizeof(one) != sizeof(two)); \
+ for (i = 0; i < sizeof(one); i++) { \
+ KUNIT_EXPECT_EQ_MSG(test, one.data[i], two.data[i], \
+ "line %d: %s.data[%d] (0x%02x) != %s.data[%d] (0x%02x)\n", \
+ __LINE__, #one, i, one.data[i], #two, i, two.data[i]); \
+ } \
+ kunit_info(test, "ok: " TEST_OP "() " name "\n"); \
+} while (0)
+
+static void memcpy_test(struct kunit *test)
+{
+#define TEST_OP "memcpy"
+ struct some_bytes control = {
+ .data = { 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ },
+ };
+ struct some_bytes zero = { };
+ struct some_bytes middle = {
+ .data = { 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ },
+ };
+ struct some_bytes three = {
+ .data = { 0x00, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x00, 0x00, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+ },
+ };
+ struct some_bytes dest = { };
+ int count;
+ u8 *ptr;
+
+ /* Verify static initializers. */
+ check(control, 0x20);
+ check(zero, 0);
+ compare("static initializers", dest, zero);
+
+ /* Verify assignment. */
+ dest = control;
+ compare("direct assignment", dest, control);
+
+ /* Verify complete overwrite. */
+ memcpy(dest.data, zero.data, sizeof(dest.data));
+ compare("complete overwrite", dest, zero);
+
+ /* Verify middle overwrite. */
+ dest = control;
+ memcpy(dest.data + 12, zero.data, 7);
+ compare("middle overwrite", dest, middle);
+
+ /* Verify argument side-effects aren't repeated. */
+ dest = control;
+ ptr = dest.data;
+ count = 1;
+ memcpy(ptr++, zero.data, count++);
+ ptr += 8;
+ memcpy(ptr++, zero.data, count++);
+ compare("argument side-effects", dest, three);
+#undef TEST_OP
+}
+
+static void memmove_test(struct kunit *test)
+{
+#define TEST_OP "memmove"
+ struct some_bytes control = {
+ .data = { 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ },
+ };
+ struct some_bytes zero = { };
+ struct some_bytes middle = {
+ .data = { 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ },
+ };
+ struct some_bytes five = {
+ .data = { 0x00, 0x00, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x00, 0x00, 0x00, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ },
+ };
+ struct some_bytes overlap = {
+ .data = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+ 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ },
+ };
+ struct some_bytes overlap_expected = {
+ .data = { 0x00, 0x01, 0x00, 0x01, 0x02, 0x03, 0x04, 0x07,
+ 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
+ },
+ };
+ struct some_bytes dest = { };
+ int count;
+ u8 *ptr;
+
+ /* Verify static initializers. */
+ check(control, 0x99);
+ check(zero, 0);
+ compare("static initializers", zero, dest);
+
+ /* Verify assignment. */
+ dest = control;
+ compare("direct assignment", dest, control);
+
+ /* Verify complete overwrite. */
+ memmove(dest.data, zero.data, sizeof(dest.data));
+ compare("complete overwrite", dest, zero);
+
+ /* Verify middle overwrite. */
+ dest = control;
+ memmove(dest.data + 12, zero.data, 7);
+ compare("middle overwrite", dest, middle);
+
+ /* Verify argument side-effects aren't repeated. */
+ dest = control;
+ ptr = dest.data;
+ count = 2;
+ memmove(ptr++, zero.data, count++);
+ ptr += 9;
+ memmove(ptr++, zero.data, count++);
+ compare("argument side-effects", dest, five);
+
+ /* Verify overlapping overwrite is correct. */
+ ptr = &overlap.data[2];
+ memmove(ptr, overlap.data, 5);
+ compare("overlapping write", overlap, overlap_expected);
+#undef TEST_OP
+}
+
+static void memset_test(struct kunit *test)
+{
+#define TEST_OP "memset"
+ struct some_bytes control = {
+ .data = { 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ },
+ };
+ struct some_bytes complete = {
+ .data = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+ },
+ };
+ struct some_bytes middle = {
+ .data = { 0x30, 0x30, 0x30, 0x30, 0x31, 0x31, 0x31, 0x31,
+ 0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31,
+ 0x31, 0x31, 0x31, 0x31, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ },
+ };
+ struct some_bytes three = {
+ .data = { 0x60, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x61, 0x61, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ },
+ };
+ struct some_bytes dest = { };
+ int count, value;
+ u8 *ptr;
+
+ /* Verify static initializers. */
+ check(control, 0x30);
+ check(dest, 0);
+
+ /* Verify assignment. */
+ dest = control;
+ compare("direct assignment", dest, control);
+
+ /* Verify complete overwrite. */
+ memset(dest.data, 0xff, sizeof(dest.data));
+ compare("complete overwrite", dest, complete);
+
+ /* Verify middle overwrite. */
+ dest = control;
+ memset(dest.data + 4, 0x31, 16);
+ compare("middle overwrite", dest, middle);
+
+ /* Verify argument side-effects aren't repeated. */
+ dest = control;
+ ptr = dest.data;
+ value = 0x60;
+ count = 1;
+ memset(ptr++, value++, count++);
+ ptr += 8;
+ memset(ptr++, value++, count++);
+ compare("argument side-effects", dest, three);
+#undef TEST_OP
+}
+
+static struct kunit_case memcpy_test_cases[] = {
+ KUNIT_CASE(memset_test),
+ KUNIT_CASE(memcpy_test),
+ KUNIT_CASE(memmove_test),
+ {}
+};
+
+static struct kunit_suite memcpy_test_suite = {
+ .name = "memcpy-test",
+ .test_cases = memcpy_test_cases,
+};
+
+kunit_test_suite(memcpy_test_suite);
+
+MODULE_LICENSE("GPL");
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH for-next 21/25] lib: Introduce CONFIG_TEST_MEMCPY
2021-08-22 7:51 ` [PATCH for-next 21/25] lib: Introduce CONFIG_TEST_MEMCPY Kees Cook
@ 2021-08-24 7:00 ` David Gow
2021-08-25 2:32 ` Kees Cook
2021-10-18 15:46 ` Arnd Bergmann
1 sibling, 1 reply; 42+ messages in thread
From: David Gow @ 2021-08-24 7:00 UTC (permalink / raw)
To: Kees Cook
Cc: Linux Kernel Mailing List, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, Linux Memory Management List,
clang-built-linux, linux-hardening
On Sun, Aug 22, 2021 at 3:56 PM Kees Cook <keescook@chromium.org> wrote:
>
> Before changing anything about memcpy(), memmove(), and memset(), add
> run-time tests to check basic behaviors for any regressions.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Thanks for adding a KUnit test here: it's great to have better
coverage of some of these basic functions!
There's a name mismatch with the Kconfig entry and the Makefile,
otherwise this looks good and works fine on my machine (under both UML
and qemu/x86_64).
It would be possible to split these tests up further if you wanted,
which could be useful if there's a desire to track the individual
assertion results independently. That's probably what I'd've done, but
It's a matter of personal preference either way, though: the tests
aren't absurdly huge or over-complicated as-is.
Cheers,
-- David
> MAINTAINERS | 9 ++
> lib/Kconfig.debug | 11 ++
> lib/Makefile | 1 +
> lib/test_memcpy.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 286 insertions(+)
> create mode 100644 lib/test_memcpy.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6c8be735cc91..e3ffd4bdc24f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7248,6 +7248,15 @@ L: netdev@vger.kernel.org
> S: Maintained
> F: drivers/net/ethernet/nvidia/*
>
> +FORTIFY_SOURCE
> +M: Kees Cook <keescook@chomium.org>
> +L: linux-hardening@vger.kernel.org
> +S: Supported
> +F: include/linux/fortify-string.h
> +F: lib/test_fortify/*
> +F: scripts/test_fortify.sh
> +K: \b__NO_FORTIFY\b
> +
Do you want this to be part of the memcpy() KUnit test commit, or is
it better suited in one of the changes to the actual fortify stuff?
> FPGA DFL DRIVERS
> M: Wu Hao <hao.wu@intel.com>
> R: Tom Rix <trix@redhat.com>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 831212722924..9199be57ba2a 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2467,6 +2467,17 @@ config RATIONAL_KUNIT_TEST
>
> If unsure, say N.
>
> +config MEMCPY_KUNIT_TEST
> + tristate "Test memcpy(), memmove(), and memset() functions at runtime" if !KUNIT_ALL_TESTS
> + depends on KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + Builds unit tests for memcpy(), memmove(), and memset() functions.
> + For more information on KUnit and unit tests in general please refer
> + to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> + If unsure, say N.
> +
> config TEST_UDELAY
> tristate "udelay test driver"
> help
> diff --git a/lib/Makefile b/lib/Makefile
> index bd17c2bf43e1..8a4c8bdb38a2 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_TEST_MIN_HEAP) += test_min_heap.o
> obj-$(CONFIG_TEST_LKM) += test_module.o
> obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
> obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
> +obj-$(CONFIG_TEST_MEMCPY) += test_memcpy.o
This doesn't match CONFIG_MEMCPY_KUNIT_TEST above, so the test is
never compiled in.
> obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
> obj-$(CONFIG_TEST_SORT) += test_sort.o
> obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> diff --git a/lib/test_memcpy.c b/lib/test_memcpy.c
> new file mode 100644
> index 000000000000..ec546cec883e
> --- /dev/null
> +++ b/lib/test_memcpy.c
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test cases for memcpy(), memmove(), and memset().
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <kunit/test.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/overflow.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/vmalloc.h>
> +
> +struct some_bytes {
> + union {
> + u8 data[32];
> + struct {
> + u32 one;
> + u16 two;
> + u8 three;
> + /* 1 byte hole */
> + u32 four[4];
> + };
> + };
> +};
> +
> +#define check(instance, v) do { \
> + int i; \
> + BUILD_BUG_ON(sizeof(instance.data) != 32); \
> + for (i = 0; i < sizeof(instance.data); i++) { \
> + KUNIT_ASSERT_EQ_MSG(test, instance.data[i], v, \
> + "line %d: '%s' not initialized to 0x%02x @ %d (saw 0x%02x)\n", \
> + __LINE__, #instance, v, i, instance.data[i]); \
> + } \
> +} while (0)
> +
> +#define compare(name, one, two) do { \
> + int i; \
> + BUILD_BUG_ON(sizeof(one) != sizeof(two)); \
> + for (i = 0; i < sizeof(one); i++) { \
> + KUNIT_EXPECT_EQ_MSG(test, one.data[i], two.data[i], \
> + "line %d: %s.data[%d] (0x%02x) != %s.data[%d] (0x%02x)\n", \
> + __LINE__, #one, i, one.data[i], #two, i, two.data[i]); \
> + } \
> + kunit_info(test, "ok: " TEST_OP "() " name "\n"); \
> +} while (0)
> +
> +static void memcpy_test(struct kunit *test)
> +{
> +#define TEST_OP "memcpy"
> + struct some_bytes control = {
> + .data = { 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> + },
> + };
> + struct some_bytes zero = { };
> + struct some_bytes middle = {
> + .data = { 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> + 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x20, 0x20, 0x20, 0x20, 0x20,
> + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> + },
> + };
> + struct some_bytes three = {
> + .data = { 0x00, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> + 0x20, 0x00, 0x00, 0x20, 0x20, 0x20, 0x20, 0x20,
> + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> + },
> + };
> + struct some_bytes dest = { };
> + int count;
> + u8 *ptr;
> +
> + /* Verify static initializers. */
> + check(control, 0x20);
> + check(zero, 0);
> + compare("static initializers", dest, zero);
> +
> + /* Verify assignment. */
> + dest = control;
> + compare("direct assignment", dest, control);
> +
> + /* Verify complete overwrite. */
> + memcpy(dest.data, zero.data, sizeof(dest.data));
> + compare("complete overwrite", dest, zero);
> +
> + /* Verify middle overwrite. */
> + dest = control;
> + memcpy(dest.data + 12, zero.data, 7);
> + compare("middle overwrite", dest, middle);
> +
> + /* Verify argument side-effects aren't repeated. */
> + dest = control;
> + ptr = dest.data;
> + count = 1;
> + memcpy(ptr++, zero.data, count++);
> + ptr += 8;
> + memcpy(ptr++, zero.data, count++);
> + compare("argument side-effects", dest, three);
> +#undef TEST_OP
> +}
> +
> +static void memmove_test(struct kunit *test)
> +{
> +#define TEST_OP "memmove"
> + struct some_bytes control = {
> + .data = { 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + },
> + };
> + struct some_bytes zero = { };
> + struct some_bytes middle = {
> + .data = { 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + 0x99, 0x99, 0x99, 0x99, 0x00, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x99, 0x99, 0x99, 0x99, 0x99,
> + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + },
> + };
> + struct some_bytes five = {
> + .data = { 0x00, 0x00, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + 0x99, 0x99, 0x00, 0x00, 0x00, 0x99, 0x99, 0x99,
> + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + },
> + };
> + struct some_bytes overlap = {
> + .data = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
> + 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
> + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + },
> + };
> + struct some_bytes overlap_expected = {
> + .data = { 0x00, 0x01, 0x00, 0x01, 0x02, 0x03, 0x04, 0x07,
> + 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
> + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> + },
> + };
> + struct some_bytes dest = { };
> + int count;
> + u8 *ptr;
> +
> + /* Verify static initializers. */
> + check(control, 0x99);
> + check(zero, 0);
> + compare("static initializers", zero, dest);
> +
> + /* Verify assignment. */
> + dest = control;
> + compare("direct assignment", dest, control);
> +
> + /* Verify complete overwrite. */
> + memmove(dest.data, zero.data, sizeof(dest.data));
> + compare("complete overwrite", dest, zero);
> +
> + /* Verify middle overwrite. */
> + dest = control;
> + memmove(dest.data + 12, zero.data, 7);
> + compare("middle overwrite", dest, middle);
> +
> + /* Verify argument side-effects aren't repeated. */
> + dest = control;
> + ptr = dest.data;
> + count = 2;
> + memmove(ptr++, zero.data, count++);
> + ptr += 9;
> + memmove(ptr++, zero.data, count++);
> + compare("argument side-effects", dest, five);
> +
> + /* Verify overlapping overwrite is correct. */
> + ptr = &overlap.data[2];
> + memmove(ptr, overlap.data, 5);
> + compare("overlapping write", overlap, overlap_expected);
> +#undef TEST_OP
> +}
> +
> +static void memset_test(struct kunit *test)
> +{
> +#define TEST_OP "memset"
> + struct some_bytes control = {
> + .data = { 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> + },
> + };
> + struct some_bytes complete = {
> + .data = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> + },
> + };
> + struct some_bytes middle = {
> + .data = { 0x30, 0x30, 0x30, 0x30, 0x31, 0x31, 0x31, 0x31,
> + 0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31,
> + 0x31, 0x31, 0x31, 0x31, 0x30, 0x30, 0x30, 0x30,
> + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> + },
> + };
> + struct some_bytes three = {
> + .data = { 0x60, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> + 0x30, 0x61, 0x61, 0x30, 0x30, 0x30, 0x30, 0x30,
> + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> + },
> + };
> + struct some_bytes dest = { };
> + int count, value;
> + u8 *ptr;
> +
> + /* Verify static initializers. */
> + check(control, 0x30);
> + check(dest, 0);
> +
> + /* Verify assignment. */
> + dest = control;
> + compare("direct assignment", dest, control);
> +
> + /* Verify complete overwrite. */
> + memset(dest.data, 0xff, sizeof(dest.data));
> + compare("complete overwrite", dest, complete);
> +
> + /* Verify middle overwrite. */
> + dest = control;
> + memset(dest.data + 4, 0x31, 16);
> + compare("middle overwrite", dest, middle);
> +
> + /* Verify argument side-effects aren't repeated. */
> + dest = control;
> + ptr = dest.data;
> + value = 0x60;
> + count = 1;
> + memset(ptr++, value++, count++);
> + ptr += 8;
> + memset(ptr++, value++, count++);
> + compare("argument side-effects", dest, three);
> +#undef TEST_OP
> +}
> +
> +static struct kunit_case memcpy_test_cases[] = {
> + KUNIT_CASE(memset_test),
> + KUNIT_CASE(memcpy_test),
> + KUNIT_CASE(memmove_test),
> + {}
> +};
> +
> +static struct kunit_suite memcpy_test_suite = {
> + .name = "memcpy-test",
It may be better to just name the suite "memcpy", since -- by
definition -- it's a test if it's a KUnit test suite.
> + .test_cases = memcpy_test_cases,
> +};
> +
> +kunit_test_suite(memcpy_test_suite);
> +
> +MODULE_LICENSE("GPL");
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH for-next 21/25] lib: Introduce CONFIG_TEST_MEMCPY
2021-08-24 7:00 ` David Gow
@ 2021-08-25 2:32 ` Kees Cook
0 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-25 2:32 UTC (permalink / raw)
To: David Gow
Cc: Linux Kernel Mailing List, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, Linux Memory Management List,
clang-built-linux, linux-hardening
On Tue, Aug 24, 2021 at 03:00:19PM +0800, David Gow wrote:
> On Sun, Aug 22, 2021 at 3:56 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > Before changing anything about memcpy(), memmove(), and memset(), add
> > run-time tests to check basic behaviors for any regressions.
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
>
> Thanks for adding a KUnit test here: it's great to have better
> coverage of some of these basic functions!
>
> There's a name mismatch with the Kconfig entry and the Makefile,
> otherwise this looks good and works fine on my machine (under both UML
> and qemu/x86_64).
Hah! Whoops. Thanks for noticing this -- I think I didn't use a clean
tree and never noticed because I had the old module.
> It would be possible to split these tests up further if you wanted,
> which could be useful if there's a desire to track the individual
> assertion results independently. That's probably what I'd've done, but
> It's a matter of personal preference either way, though: the tests
> aren't absurdly huge or over-complicated as-is.
Noted. Yeah, for me, I think it's "does memcpy work or not?" and each of
the EXPECTs are required, so it felt like 1 test with lots of EXPECTs.
> > +FORTIFY_SOURCE
> > +M: Kees Cook <keescook@chomium.org>
> > +L: linux-hardening@vger.kernel.org
> > +S: Supported
> > +F: include/linux/fortify-string.h
> > +F: lib/test_fortify/*
> > +F: scripts/test_fortify.sh
> > +K: \b__NO_FORTIFY\b
> > +
>
> Do you want this to be part of the memcpy() KUnit test commit, or is
> it better suited in one of the changes to the actual fortify stuff?
Whoops, thanks. This got --fixup'ed into the wrong patch. I've moved it
now.
> > FPGA DFL DRIVERS
> > M: Wu Hao <hao.wu@intel.com>
> > R: Tom Rix <trix@redhat.com>
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 831212722924..9199be57ba2a 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -2467,6 +2467,17 @@ config RATIONAL_KUNIT_TEST
> >
> > If unsure, say N.
> >
> > +config MEMCPY_KUNIT_TEST
> > + tristate "Test memcpy(), memmove(), and memset() functions at runtime" if !KUNIT_ALL_TESTS
> > + depends on KUNIT
> > + default KUNIT_ALL_TESTS
> > + help
> > + Builds unit tests for memcpy(), memmove(), and memset() functions.
> > + For more information on KUnit and unit tests in general please refer
> > + to the KUnit documentation in Documentation/dev-tools/kunit/.
> > +
> > + If unsure, say N.
> > +
> > config TEST_UDELAY
> > tristate "udelay test driver"
> > help
> > diff --git a/lib/Makefile b/lib/Makefile
> > index bd17c2bf43e1..8a4c8bdb38a2 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -77,6 +77,7 @@ obj-$(CONFIG_TEST_MIN_HEAP) += test_min_heap.o
> > obj-$(CONFIG_TEST_LKM) += test_module.o
> > obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
> > obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
> > +obj-$(CONFIG_TEST_MEMCPY) += test_memcpy.o
>
> This doesn't match CONFIG_MEMCPY_KUNIT_TEST above, so the test is
> never compiled in.
Now fixed to be CONFIG_MEMCPY_KUNIT_TEST.
>
> > obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
> > obj-$(CONFIG_TEST_SORT) += test_sort.o
> > obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> > diff --git a/lib/test_memcpy.c b/lib/test_memcpy.c
> > new file mode 100644
> > index 000000000000..ec546cec883e
> > --- /dev/null
> > +++ b/lib/test_memcpy.c
> > @@ -0,0 +1,265 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Test cases for memcpy(), memmove(), and memset().
> > + */
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <kunit/test.h>
> > +#include <linux/device.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mm.h>
> > +#include <linux/module.h>
> > +#include <linux/overflow.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +#include <linux/vmalloc.h>
> > +
> > +struct some_bytes {
> > + union {
> > + u8 data[32];
> > + struct {
> > + u32 one;
> > + u16 two;
> > + u8 three;
> > + /* 1 byte hole */
> > + u32 four[4];
> > + };
> > + };
> > +};
> > +
> > +#define check(instance, v) do { \
> > + int i; \
> > + BUILD_BUG_ON(sizeof(instance.data) != 32); \
> > + for (i = 0; i < sizeof(instance.data); i++) { \
> > + KUNIT_ASSERT_EQ_MSG(test, instance.data[i], v, \
> > + "line %d: '%s' not initialized to 0x%02x @ %d (saw 0x%02x)\n", \
> > + __LINE__, #instance, v, i, instance.data[i]); \
> > + } \
> > +} while (0)
> > +
> > +#define compare(name, one, two) do { \
> > + int i; \
> > + BUILD_BUG_ON(sizeof(one) != sizeof(two)); \
> > + for (i = 0; i < sizeof(one); i++) { \
> > + KUNIT_EXPECT_EQ_MSG(test, one.data[i], two.data[i], \
> > + "line %d: %s.data[%d] (0x%02x) != %s.data[%d] (0x%02x)\n", \
> > + __LINE__, #one, i, one.data[i], #two, i, two.data[i]); \
> > + } \
> > + kunit_info(test, "ok: " TEST_OP "() " name "\n"); \
> > +} while (0)
> > +
> > +static void memcpy_test(struct kunit *test)
> > +{
> > +#define TEST_OP "memcpy"
> > + struct some_bytes control = {
> > + .data = { 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> > + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> > + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> > + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> > + },
> > + };
> > + struct some_bytes zero = { };
> > + struct some_bytes middle = {
> > + .data = { 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> > + 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0x00,
> > + 0x00, 0x00, 0x00, 0x20, 0x20, 0x20, 0x20, 0x20,
> > + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> > + },
> > + };
> > + struct some_bytes three = {
> > + .data = { 0x00, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> > + 0x20, 0x00, 0x00, 0x20, 0x20, 0x20, 0x20, 0x20,
> > + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> > + 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> > + },
> > + };
> > + struct some_bytes dest = { };
> > + int count;
> > + u8 *ptr;
> > +
> > + /* Verify static initializers. */
> > + check(control, 0x20);
> > + check(zero, 0);
> > + compare("static initializers", dest, zero);
> > +
> > + /* Verify assignment. */
> > + dest = control;
> > + compare("direct assignment", dest, control);
> > +
> > + /* Verify complete overwrite. */
> > + memcpy(dest.data, zero.data, sizeof(dest.data));
> > + compare("complete overwrite", dest, zero);
> > +
> > + /* Verify middle overwrite. */
> > + dest = control;
> > + memcpy(dest.data + 12, zero.data, 7);
> > + compare("middle overwrite", dest, middle);
> > +
> > + /* Verify argument side-effects aren't repeated. */
> > + dest = control;
> > + ptr = dest.data;
> > + count = 1;
> > + memcpy(ptr++, zero.data, count++);
> > + ptr += 8;
> > + memcpy(ptr++, zero.data, count++);
> > + compare("argument side-effects", dest, three);
> > +#undef TEST_OP
> > +}
> > +
> > +static void memmove_test(struct kunit *test)
> > +{
> > +#define TEST_OP "memmove"
> > + struct some_bytes control = {
> > + .data = { 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + },
> > + };
> > + struct some_bytes zero = { };
> > + struct some_bytes middle = {
> > + .data = { 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + 0x99, 0x99, 0x99, 0x99, 0x00, 0x00, 0x00, 0x00,
> > + 0x00, 0x00, 0x00, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + },
> > + };
> > + struct some_bytes five = {
> > + .data = { 0x00, 0x00, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + 0x99, 0x99, 0x00, 0x00, 0x00, 0x99, 0x99, 0x99,
> > + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + },
> > + };
> > + struct some_bytes overlap = {
> > + .data = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
> > + 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
> > + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + },
> > + };
> > + struct some_bytes overlap_expected = {
> > + .data = { 0x00, 0x01, 0x00, 0x01, 0x02, 0x03, 0x04, 0x07,
> > + 0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
> > + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99, 0x99,
> > + },
> > + };
> > + struct some_bytes dest = { };
> > + int count;
> > + u8 *ptr;
> > +
> > + /* Verify static initializers. */
> > + check(control, 0x99);
> > + check(zero, 0);
> > + compare("static initializers", zero, dest);
> > +
> > + /* Verify assignment. */
> > + dest = control;
> > + compare("direct assignment", dest, control);
> > +
> > + /* Verify complete overwrite. */
> > + memmove(dest.data, zero.data, sizeof(dest.data));
> > + compare("complete overwrite", dest, zero);
> > +
> > + /* Verify middle overwrite. */
> > + dest = control;
> > + memmove(dest.data + 12, zero.data, 7);
> > + compare("middle overwrite", dest, middle);
> > +
> > + /* Verify argument side-effects aren't repeated. */
> > + dest = control;
> > + ptr = dest.data;
> > + count = 2;
> > + memmove(ptr++, zero.data, count++);
> > + ptr += 9;
> > + memmove(ptr++, zero.data, count++);
> > + compare("argument side-effects", dest, five);
> > +
> > + /* Verify overlapping overwrite is correct. */
> > + ptr = &overlap.data[2];
> > + memmove(ptr, overlap.data, 5);
> > + compare("overlapping write", overlap, overlap_expected);
> > +#undef TEST_OP
> > +}
> > +
> > +static void memset_test(struct kunit *test)
> > +{
> > +#define TEST_OP "memset"
> > + struct some_bytes control = {
> > + .data = { 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> > + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> > + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> > + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> > + },
> > + };
> > + struct some_bytes complete = {
> > + .data = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > + },
> > + };
> > + struct some_bytes middle = {
> > + .data = { 0x30, 0x30, 0x30, 0x30, 0x31, 0x31, 0x31, 0x31,
> > + 0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31, 0x31,
> > + 0x31, 0x31, 0x31, 0x31, 0x30, 0x30, 0x30, 0x30,
> > + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> > + },
> > + };
> > + struct some_bytes three = {
> > + .data = { 0x60, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> > + 0x30, 0x61, 0x61, 0x30, 0x30, 0x30, 0x30, 0x30,
> > + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> > + 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
> > + },
> > + };
> > + struct some_bytes dest = { };
> > + int count, value;
> > + u8 *ptr;
> > +
> > + /* Verify static initializers. */
> > + check(control, 0x30);
> > + check(dest, 0);
> > +
> > + /* Verify assignment. */
> > + dest = control;
> > + compare("direct assignment", dest, control);
> > +
> > + /* Verify complete overwrite. */
> > + memset(dest.data, 0xff, sizeof(dest.data));
> > + compare("complete overwrite", dest, complete);
> > +
> > + /* Verify middle overwrite. */
> > + dest = control;
> > + memset(dest.data + 4, 0x31, 16);
> > + compare("middle overwrite", dest, middle);
> > +
> > + /* Verify argument side-effects aren't repeated. */
> > + dest = control;
> > + ptr = dest.data;
> > + value = 0x60;
> > + count = 1;
> > + memset(ptr++, value++, count++);
> > + ptr += 8;
> > + memset(ptr++, value++, count++);
> > + compare("argument side-effects", dest, three);
> > +#undef TEST_OP
> > +}
> > +
> > +static struct kunit_case memcpy_test_cases[] = {
> > + KUNIT_CASE(memset_test),
> > + KUNIT_CASE(memcpy_test),
> > + KUNIT_CASE(memmove_test),
> > + {}
> > +};
> > +
> > +static struct kunit_suite memcpy_test_suite = {
> > + .name = "memcpy-test",
>
> It may be better to just name the suite "memcpy", since -- by
> definition -- it's a test if it's a KUnit test suite.
Sounds good. I will adjust this.
>
> > + .test_cases = memcpy_test_cases,
> > +};
> > +
> > +kunit_test_suite(memcpy_test_suite);
> > +
> > +MODULE_LICENSE("GPL");
> > --
> > 2.30.2
> >
Thanks!
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH for-next 21/25] lib: Introduce CONFIG_TEST_MEMCPY
2021-08-22 7:51 ` [PATCH for-next 21/25] lib: Introduce CONFIG_TEST_MEMCPY Kees Cook
2021-08-24 7:00 ` David Gow
@ 2021-10-18 15:46 ` Arnd Bergmann
2021-10-18 19:28 ` Kees Cook
1 sibling, 1 reply; 42+ messages in thread
From: Arnd Bergmann @ 2021-10-18 15:46 UTC (permalink / raw)
To: Kees Cook
Cc: Linux Kernel Mailing List, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, Linux-MM,
clang-built-linux, linux-hardening
On Sun, Aug 22, 2021 at 9:56 AM Kees Cook <keescook@chromium.org> wrote:
> +FORTIFY_SOURCE
> +M: Kees Cook <keescook@chomium.org>
> +L: linux-hardening@vger.kernel.org
> +S: Supported
I had added you to Cc on a patch because of this entry, and the email
bounced. It looks like you need an extra 'r' in the address.
Arnd
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH for-next 21/25] lib: Introduce CONFIG_TEST_MEMCPY
2021-10-18 15:46 ` Arnd Bergmann
@ 2021-10-18 19:28 ` Kees Cook
0 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-10-18 19:28 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linux Kernel Mailing List, Rasmus Villemoes, Daniel Micay,
Francis Laniel, Bart Van Assche, David Gow, Linux-MM,
clang-built-linux, linux-hardening
On Mon, Oct 18, 2021 at 05:46:09PM +0200, Arnd Bergmann wrote:
> On Sun, Aug 22, 2021 at 9:56 AM Kees Cook <keescook@chromium.org> wrote:
> > +FORTIFY_SOURCE
> > +M: Kees Cook <keescook@chomium.org>
> > +L: linux-hardening@vger.kernel.org
> > +S: Supported
>
> I had added you to Cc on a patch because of this entry, and the email
> bounced. It looks like you need an extra 'r' in the address.
*facepalm*
Thank you, fixing...
--
Kees Cook
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH for-next 22/25] string.h: Introduce memset_after() for wiping trailing members/padding
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (20 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 21/25] lib: Introduce CONFIG_TEST_MEMCPY Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 23/25] xfrm: Use memset_after() to clear padding Kees Cook
` (2 subsequent siblings)
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
A common idiom in kernel code is to wipe the contents of a structure
after a given member. This is especially useful in places where there is
trailing padding. These open-coded cases are usually difficult to read
and very sensitive to struct layout changes. Introduce a new helper,
memset_after() that takes the target struct instance, the byte to write,
and the member name after which the zeroing should start.
Additionally adds memset_startat() for wiping trailing members _starting_
at a specific member instead of after a member, which is more readable
in certain circumstances, but doesn't include any preceding padding.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/string.h | 17 +++++++++++++++++
lib/test_memcpy.c | 13 +++++++++++++
2 files changed, 30 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h
index 9473f81b9db2..d593de2635ba 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -271,6 +271,23 @@ static inline void memcpy_and_pad(void *dest, size_t dest_len,
memcpy(dest, src, dest_len);
}
+/**
+ * memset_after - Set a value after a struct member to the end of a struct
+ *
+ * @obj: Address of target struct instance
+ * @v: Byte value to repeatedly write
+ * @member: after which struct member to start writing bytes
+ *
+ * This is good for clearing padding following the given member.
+ */
+#define memset_after(obj, v, member) \
+({ \
+ u8 *__ptr = (u8 *)(obj); \
+ typeof(v) __val = (v); \
+ memset(__ptr + offsetofend(typeof(*(obj)), member), __val, \
+ sizeof(*(obj)) - offsetofend(typeof(*(obj)), member)); \
+})
+
/**
* str_has_prefix - Test if a string has a given prefix
* @str: The string to test
diff --git a/lib/test_memcpy.c b/lib/test_memcpy.c
index ec546cec883e..3b485de8c885 100644
--- a/lib/test_memcpy.c
+++ b/lib/test_memcpy.c
@@ -215,6 +215,13 @@ static void memset_test(struct kunit *test)
0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
},
};
+ struct some_bytes after = {
+ .data = { 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x72,
+ 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72,
+ 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72,
+ 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72,
+ },
+ };
struct some_bytes dest = { };
int count, value;
u8 *ptr;
@@ -245,6 +252,12 @@ static void memset_test(struct kunit *test)
ptr += 8;
memset(ptr++, value++, count++);
compare("argument side-effects", dest, three);
+
+ /* Verify memset_after() */
+ dest = control;
+ memset_after(&dest, 0x72, three);
+ compare("memset_after()", dest, after);
+
#undef TEST_OP
}
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 23/25] xfrm: Use memset_after() to clear padding
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (21 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 22/25] string.h: Introduce memset_after() for wiping trailing members/padding Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 24/25] string.h: Introduce memset_startat() for wiping trailing members and padding Kees Cook
2021-08-22 7:51 ` [PATCH for-next 25/25] btrfs: Use memset_startat() to clear end of struct Kees Cook
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Clear trailing padding bytes using the new helper so that memset()
doesn't get confused about writing "past the end" of the last struct
member. There is no change to the resulting machine code.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
net/xfrm/xfrm_policy.c | 4 +---
net/xfrm/xfrm_user.c | 2 +-
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 827d84255021..a21af241a2bb 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2494,9 +2494,7 @@ static inline struct xfrm_dst *xfrm_alloc_dst(struct net *net, int family)
xdst = dst_alloc(dst_ops, NULL, 1, DST_OBSOLETE_NONE, 0);
if (likely(xdst)) {
- struct dst_entry *dst = &xdst->u.dst;
-
- memset(dst + 1, 0, sizeof(*xdst) - sizeof(*dst));
+ memset_after(xdst, 0, u.dst);
} else
xdst = ERR_PTR(-ENOBUFS);
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index b47d613409b7..880d260541c9 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2845,7 +2845,7 @@ static int build_expire(struct sk_buff *skb, struct xfrm_state *x, const struct
copy_to_user_state(x, &ue->state);
ue->hard = (c->data.hard != 0) ? 1 : 0;
/* clear the padding bytes */
- memset(&ue->hard + 1, 0, sizeof(*ue) - offsetofend(typeof(*ue), hard));
+ memset_after(ue, 0, hard);
err = xfrm_mark_put(skb, &x->mark);
if (err)
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 24/25] string.h: Introduce memset_startat() for wiping trailing members and padding
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (22 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 23/25] xfrm: Use memset_after() to clear padding Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
2021-08-22 7:51 ` [PATCH for-next 25/25] btrfs: Use memset_startat() to clear end of struct Kees Cook
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Rasmus Villemoes, Daniel Micay, Francis Laniel,
Bart Van Assche, David Gow, linux-mm, clang-built-linux,
linux-hardening
A common idiom in kernel code is to wipe the contents of a structure
starting from a given member. These open-coded cases are usually difficult
to read and very sensitive to struct layout changes. Like memset_after(),
introduce a new helper, memset_startat() that takes the target struct
instance, the byte to write, and the member name where zeroing should
start.
Note that this doesn't zero padding preceding the target member. For
those cases, memset_after() should be used on the preceding member.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/string.h | 18 ++++++++++++++++++
lib/test_memcpy.c | 11 +++++++++++
2 files changed, 29 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h
index d593de2635ba..38acc436dba2 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -288,6 +288,24 @@ static inline void memcpy_and_pad(void *dest, size_t dest_len,
sizeof(*(obj)) - offsetofend(typeof(*(obj)), member)); \
})
+/**
+ * memset_startat - Set a value starting at a member to the end of a struct
+ *
+ * @obj: Address of target struct instance
+ * @v: Byte value to repeatedly write
+ * @member: struct member to start writing at
+ *
+ * Note that if there is padding between the prior member and the target
+ * member, memset_after() should be used to clear the prior padding.
+ */
+#define memset_startat(obj, v, member) \
+({ \
+ u8 *__ptr = (u8 *)(obj); \
+ typeof(v) __val = (v); \
+ memset(__ptr + offsetof(typeof(*(obj)), member), __val, \
+ sizeof(*(obj)) - offsetof(typeof(*(obj)), member)); \
+})
+
/**
* str_has_prefix - Test if a string has a given prefix
* @str: The string to test
diff --git a/lib/test_memcpy.c b/lib/test_memcpy.c
index 3b485de8c885..fb5deaf04418 100644
--- a/lib/test_memcpy.c
+++ b/lib/test_memcpy.c
@@ -222,6 +222,13 @@ static void memset_test(struct kunit *test)
0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72, 0x72,
},
};
+ struct some_bytes startat = {
+ .data = { 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30,
+ 0x79, 0x79, 0x79, 0x79, 0x79, 0x79, 0x79, 0x79,
+ 0x79, 0x79, 0x79, 0x79, 0x79, 0x79, 0x79, 0x79,
+ 0x79, 0x79, 0x79, 0x79, 0x79, 0x79, 0x79, 0x79,
+ },
+ };
struct some_bytes dest = { };
int count, value;
u8 *ptr;
@@ -258,6 +265,10 @@ static void memset_test(struct kunit *test)
memset_after(&dest, 0x72, three);
compare("memset_after()", dest, after);
+ /* Verify memset_startat() */
+ dest = control;
+ memset_startat(&dest, 0x79, four);
+ compare("memset_startat()", dest, startat);
#undef TEST_OP
}
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH for-next 25/25] btrfs: Use memset_startat() to clear end of struct
2021-08-22 7:50 [PATCH for-next 00/25] Prepare for better FORTIFY_SOURCE Kees Cook
` (23 preceding siblings ...)
2021-08-22 7:51 ` [PATCH for-next 24/25] string.h: Introduce memset_startat() for wiping trailing members and padding Kees Cook
@ 2021-08-22 7:51 ` Kees Cook
24 siblings, 0 replies; 42+ messages in thread
From: Kees Cook @ 2021-08-22 7:51 UTC (permalink / raw)
To: linux-kernel
Cc: Kees Cook, Nikolay Borisov, David Sterba, Rasmus Villemoes,
Daniel Micay, Francis Laniel, Bart Van Assche, David Gow,
linux-mm, clang-built-linux, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_startat() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Acked-by: David Sterba <dsterba@suse.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/btrfs/root-tree.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 702dc5441f03..12ceb14a1141 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -39,10 +39,8 @@ static void btrfs_read_root_item(struct extent_buffer *eb, int slot,
need_reset = 1;
}
if (need_reset) {
- memset(&item->generation_v2, 0,
- sizeof(*item) - offsetof(struct btrfs_root_item,
- generation_v2));
-
+ /* Clear all members from generation_v2 onwards. */
+ memset_startat(item, 0, generation_v2);
generate_random_guid(item->uuid);
}
}
--
2.30.2
^ permalink raw reply related [flat|nested] 42+ messages in thread