Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Sun, May 12, 2024 at 09:32:40PM +0200, Joel Granados wrote: > On Sat, May 11, 2024 at 11:51:18AM +0200, Thomas Weißschuh wrote: > > Hi Kees, > > > > On 2024-05-08 10:11:35+, Kees Cook wrote: > > > On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote: > > > > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote: > > > > > The series was split from my larger series sysctl-const series [0]. > > > > > It only focusses on the proc_handlers but is an important step to be > > > > > able to move all static definitions of ctl_table into .rodata. > > > > > > > > Split this per subsystem, please. > > > > > > I've done a few painful API transitions before, and I don't think the > > > complexity of these changes needs a per-subsystem constification pass. I > > > think this series is the right approach, but that patch 11 will need > > > coordination with Linus. We regularly do system-wide prototype changes > > > like this right at the end of the merge window before -rc1 comes out. > > > > That sounds good. > > > > > The requirements are pretty simple: it needs to be a obvious changes > > > (this certainly is) and as close to 100% mechanical as possible. I think > > > patch 11 easily qualifies. Linus should be able to run the same Coccinelle > > > script and get nearly the same results, etc. And all the other changes > > > need to have landed. This change also has no "silent failure" conditions: > > > anything mismatched will immediately stand out. > > > > Unfortunately coccinelle alone is not sufficient, as some helpers with > > different prototypes are called by handlers and themselves are calling > > handler and therefore need to change in the same commit. > > But if I add a diff for those on top of the coccinelle script to the > > changelog it should be obvious. > Judging by Kees' comment on "100% mechanical", it might be better just > having the diff and have Linus apply than rather than two step process? > Have not these types of PRs, so am interested in what folks think. I tried to soften it a little with my "*close* to 100%" modifier, and I think that patch basically matched that requirement, and where it had manual changes it was detailed in the commit log. I only split out the seccomp part because it could actually stand alone. So yeah, let's get the last of the subsystem specific stuff landed after -rc1, and it should be possible to finish it all up for 6.11. Yay! :) -Kees -- Kees Cook
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote: > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote: > > The series was split from my larger series sysctl-const series [0]. > > It only focusses on the proc_handlers but is an important step to be > > able to move all static definitions of ctl_table into .rodata. > > Split this per subsystem, please. I've done a few painful API transitions before, and I don't think the complexity of these changes needs a per-subsystem constification pass. I think this series is the right approach, but that patch 11 will need coordination with Linus. We regularly do system-wide prototype changes like this right at the end of the merge window before -rc1 comes out. The requirements are pretty simple: it needs to be a obvious changes (this certainly is) and as close to 100% mechanical as possible. I think patch 11 easily qualifies. Linus should be able to run the same Coccinelle script and get nearly the same results, etc. And all the other changes need to have landed. This change also has no "silent failure" conditions: anything mismatched will immediately stand out. So, have patches 1-10 go via their respective subsystems, and once all of those are in Linus's tree, send patch 11 as a stand-alone PR. (From patch 11, it looks like the seccomp read/write function changes could be split out? I'll do that now...) -Kees -- Kees Cook
Re: [PATCH v2 1/1] x86/elf: Add a new .note section containing Xfeatures information to x86 core files
continue;
> + if (supported_features & (1U << sub_leaf))
> + xfeatures_count++;
> + }
> +
> + return xfeatures_count * (sizeof(struct xfeat_component));
> +}
> +
> +int elf_coredump_extra_notes_write(struct coredump_params *cprm)
> +{
> + int num_records = 0;
> + struct elf_note en;
> +
> + en.n_namesz = sizeof(owner_name);
> + en.n_descsz = get_xsave_desc_size();
> + en.n_type = NT_X86_XSAVE_LAYOUT;
> +
> + if (!dump_emit(cprm, , sizeof(en)))
> + return 1;
> + if (!dump_emit(cprm, owner_name, en.n_namesz))
> + return 1;
> + if (!dump_align(cprm, 4))
> + return 1;
> +
> + num_records = dump_xsave_layout_desc(cprm);
> + if (!num_records) {
> + pr_warn_ratelimited("Error adding XSTATE layout ELF note.
> XSTATE buffer in the core file will be unparseable.");
Missing trailing newline.
> + return 1;
> + }
> +
> + /* Total size should be equal to the number of records */
> + if ((sizeof(struct xfeat_component) * num_records) != en.n_descsz) {
> + pr_warn_ratelimited("Error adding XSTATE layout ELF note. The
> size of the .note section does not match with the total size of the
> records.");
Same.
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Return the size of new note.
> + */
> +int elf_coredump_extra_notes_size(void)
> +{
> + int size = 0;
> +
> + /* NOTE Header */
> + size += sizeof(struct elf_note);
> + /* name + align */
> + size += roundup(sizeof(owner_name), 4);
> + size += get_xsave_desc_size();
> +
> + return size;
> +}
> +#endif
Since it's a long if/endif, add: /* CONFIG_COREDUMP */ after the endif
here.
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 5397b552fbeb..833bcb7e957b 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -2000,7 +2000,7 @@ static int elf_core_dump(struct coredump_params *cprm)
> {
> size_t sz = info.size;
>
> - /* For cell spufs */
> + /* For cell spufs and x86 xstate */
> sz += elf_coredump_extra_notes_size();
>
> phdr4note = kmalloc(sizeof(*phdr4note), GFP_KERNEL);
> @@ -2064,7 +2064,7 @@ static int elf_core_dump(struct coredump_params *cprm)
> if (!write_note_info(, cprm))
> goto end_coredump;
>
> - /* For cell spufs */
> + /* For cell spufs and x86 xstate */
> if (elf_coredump_extra_notes_write(cprm))
> goto end_coredump;
>
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index b54b313bcf07..e30a9b47dc87 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -411,6 +411,7 @@ typedef struct elf64_shdr {
> #define NT_X86_XSTATE0x202 /* x86 extended state using
> xsave */
> /* Old binutils treats 0x203 as a CET state */
> #define NT_X86_SHSTK 0x204 /* x86 SHSTK state */
> +#define NT_X86_XSAVE_LAYOUT 0x205 /* XSAVE layout description */
> #define NT_S390_HIGH_GPRS0x300 /* s390 upper register halves */
> #define NT_S390_TIMER0x301 /* s390 timer register */
> #define NT_S390_TODCMP 0x302 /* s390 TOD clock comparator
> register */
> --
> 2.34.1
>
Otherwise looks good. I'd like to see feedback from Intel folks too.
Thanks for working on this!
-Kees
--
Kees Cook
Re: [PATCH][next] crypto/nx: Avoid potential -Wflex-array-member-not-at-end warning
x-842.c
@@ -62,10 +62,7 @@
*/
#define NX842_CRYPTO_MAGIC (0xf842)
#define NX842_CRYPTO_HEADER_SIZE(g)\
- (sizeof(struct nx842_crypto_header) + \
-sizeof(struct nx842_crypto_header_group) * (g))
-#define NX842_CRYPTO_HEADER_MAX_SIZE \
- NX842_CRYPTO_HEADER_SIZE(NX842_CRYPTO_GROUP_MAX)
+ struct_size_t(nx842_crypto_header, group, g)
/* bounce buffer size */
#define BOUNCE_BUFFER_ORDER(2)
diff --git a/drivers/crypto/nx/nx-842.h b/drivers/crypto/nx/nx-842.h
index 7590bfb24d79..70d9f99a4595 100644
--- a/drivers/crypto/nx/nx-842.h
+++ b/drivers/crypto/nx/nx-842.h
@@ -160,7 +160,7 @@ struct nx842_crypto_header {
__be16 magic; /* NX842_CRYPTO_MAGIC */
__be16 ignore; /* decompressed end bytes to ignore */
u8 groups; /* total groups in this header */
- struct nx842_crypto_header_group group[];
+ struct nx842_crypto_header_group group[] __counted_by(groups);
} __packed;
#define NX842_CRYPTO_GROUP_MAX (0x20)
@@ -171,10 +171,9 @@ struct nx842_crypto_ctx {
u8 *wmem;
u8 *sbounce, *dbounce;
- struct nx842_crypto_header header;
- struct nx842_crypto_header_group group[NX842_CRYPTO_GROUP_MAX];
-
struct nx842_driver *driver;
+
+ struct nx842_crypto_header header;
};
int nx842_crypto_init(struct crypto_tfm *tfm, struct nx842_driver *driver);
diff --git a/drivers/crypto/nx/nx-common-powernv.c
b/drivers/crypto/nx/nx-common-powernv.c
index 8c859872c183..22ab4a5885f2 100644
--- a/drivers/crypto/nx/nx-common-powernv.c
+++ b/drivers/crypto/nx/nx-common-powernv.c
@@ -1041,7 +1041,8 @@ static struct crypto_alg nx842_powernv_alg = {
.cra_driver_name= "842-nx",
.cra_priority = 300,
.cra_flags = CRYPTO_ALG_TYPE_COMPRESS,
- .cra_ctxsize= sizeof(struct nx842_crypto_ctx),
+ .cra_ctxsize= struct_size_t(struct nx842_crypto_ctx,
header.group,
+ NX842_CRYPTO_GROUP_MAX),
.cra_module = THIS_MODULE,
.cra_init = nx842_powernv_crypto_init,
.cra_exit = nx842_crypto_exit,
diff --git a/drivers/crypto/nx/nx-common-pseries.c
b/drivers/crypto/nx/nx-common-pseries.c
index 35f2d0d8507e..fdf328eab6fc 100644
--- a/drivers/crypto/nx/nx-common-pseries.c
+++ b/drivers/crypto/nx/nx-common-pseries.c
@@ -1018,7 +1018,8 @@ static struct crypto_alg nx842_pseries_alg = {
.cra_driver_name= "842-nx",
.cra_priority = 300,
.cra_flags = CRYPTO_ALG_TYPE_COMPRESS,
- .cra_ctxsize= sizeof(struct nx842_crypto_ctx),
+ .cra_ctxsize= struct_size_t(struct nx842_crypto_ctx,
header.group,
+ NX842_CRYPTO_GROUP_MAX),
.cra_module = THIS_MODULE,
.cra_init = nx842_pseries_crypto_init,
.cra_exit = nx842_crypto_exit,
--
Kees Cook
Re: [PATCH 0/1] Replace the macro "ARCH_HAVE_EXTRA_ELF_NOTES" with kconfig
On Fri, 12 Apr 2024 11:51:37 +0530, Vignesh Balasubramanian wrote: > This patch replaces the macro "ARCH_HAVE_EXTRA_ELF_NOTES" with kconfig > as discussed here > https://lore.kernel.org/lkml/ca+55afxdk9_cmo4spymgg_wq+_g5e_v6o-hetq_nts-q1zj...@mail.gmail.com/ > It is a pre-requisite patch for the review > https://lore.kernel.org/lkml/20240314112359.50713-1-vigba...@amd.com/ > I have split this patch as suggested in the review comment > https://lore.kernel.org/lkml/87o7bg31jd.fsf@mail.lhotse/ > > [...] Applied to for-next/execve, thanks! [1/1] Replace macro "ARCH_HAVE_EXTRA_ELF_NOTES" with kconfig https://git.kernel.org/kees/c/a9c3475dd67b Take care, -- Kees Cook
Re: [PATCH v3 00/15] Add support for suppressing warning backtraces
ing noise at end of dev_addr_lists tests > x86: Add support for suppressing warning backtraces > arm64: Add support for suppressing warning backtraces > loongarch: Add support for suppressing warning backtraces > parisc: Add support for suppressing warning backtraces > s390: Add support for suppressing warning backtraces > sh: Add support for suppressing warning backtraces > sh: Move defines needed for suppressing warning backtraces > riscv: Add support for suppressing warning backtraces > powerpc: Add support for suppressing warning backtraces Tested-by: Kees Cook (for x86 and um) I was planning to add warning suppression for the "overflow" KUnit tests, but it seems the vmalloc routines aren't calling warn_alloc() any more for impossible sizes. So, I think, no patches needed for lib/overflow_kunit.c, but at the end of the day, I've tested this series is working for me. :P -- Kees Cook
Re: [PATCH] vdso: use CONFIG_PAGE_SHIFT in vdso/datapage.h
On Wed, Mar 20, 2024 at 07:02:15PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Both the vdso rework and the CONFIG_PAGE_SHIFT changes were merged during
> the v6.9 merge window, so it is now possible to use CONFIG_PAGE_SHIFT
> instead of including asm/page.h in the vdso.
>
> This avoids the workaround for arm64 and addresses a build warning
> for powerpc64:
>
> In file included from :4:
> In file included from /home/arnd/arm-soc/arm-soc/lib/vdso/gettimeofday.c:5:
> In file included from ../include/vdso/datapage.h:25:
> arch/powerpc/include/asm/page.h:230:9: error: result of comparison of
> constant 13835058055282163712 with expression of type 'unsigned long' is
> always true [-Werror,-Wtautological-constant-out-of-range-compare]
> 230 | return __pa(kaddr) >> PAGE_SHIFT;
> |^~~
> arch/powerpc/include/asm/page.h:217:37: note: expanded from macro '__pa'
> 217 | VIRTUAL_WARN_ON((unsigned long)(x) < PAGE_OFFSET);
> \
> | ~~~^~
> arch/powerpc/include/asm/page.h:202:73: note: expanded from macro
> 'VIRTUAL_WARN_ON'
> 202 | #define VIRTUAL_WARN_ON(x)
> WARN_ON(IS_ENABLED(CONFIG_DEBUG_VIRTUAL) && (x))
> |
> ~^~~
> arch/powerpc/include/asm/bug.h:88:25: note: expanded from macro 'WARN_ON'
>88 | int __ret_warn_on = !!(x); \
> |^
>
> Cc: Michael Ellerman
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Andy Lutomirski
> Cc: Thomas Gleixner
> Cc: Vincenzo Frascino
> Cc: Anna-Maria Behnsen
> See-also: 8b3843ae3634 ("vdso/datapage: Quick fix - use asm/page-def.h for
> ARM64")
> Signed-off-by: Arnd Bergmann
Thanks for tracking this!
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH 1/1] x86/elf: Add a new .note section containing Xfeatures information to x86 core files
gacy states. */
> + for (i = 0; i < FIRST_EXTENDED_XFEATURE; i++) {
> + xc.xfeat_type = i;
> + xc.xfeat_sz = xstate_sizes[i];
> + xc.xfeat_off = xstate_offsets[i];
> + xc.xfeat_flags = xstate_flags[i];
> +
> + if (!dump_emit(cprm, , sizeof(struct xfeat_component)))
> + return 0;
> + num_records++;
> + }
> +
> + for_each_extended_xfeature(i, fpu_user_cfg.max_features) {
> + xc.xfeat_type = i;
> + xc.xfeat_sz = xstate_sizes[i];
> + xc.xfeat_off = xstate_offsets[i];
> + xc.xfeat_flags = xstate_flags[i];
> +
> + if (!dump_emit(cprm, , sizeof(struct xfeat_component)))
> + return 0;
> + num_records++;
> + }
> +
> + return num_records;
> +}
> +
> +static int get_xsave_desc_size(void)
> +{
> + /* XFEATURE_FP and XFEATURE_SSE, both are fixed legacy states */
> + int xfeatures_count = 2;
> + int i;
> +
> + for_each_extended_xfeature(i, fpu_user_cfg.max_features)
> + xfeatures_count++;
> +
> + return xfeatures_count * (sizeof(struct xfeat_component));
> +}
> +
> +int elf_coredump_extra_notes_write(struct coredump_params *cprm)
> +{
> + const char *owner_name = "LINUX";
If you use an array instead of a pointer, there's no need for strlen(),
and you can make it a static outside of the function to refer to it
later.
static const char owner_name[] = "LINUX";
> + int num_records = 0;
> + struct elf_note en;
> +
> + en.n_namesz = strlen(owner_name) + 1;
en.n_namesz = sizeof(owner_name);
> + en.n_descsz = get_xsave_desc_size();
> + en.n_type = NT_X86_XSAVE_LAYOUT;
> +
> + if (!dump_emit(cprm, , sizeof(en)))
> + return 1;
> + if (!dump_emit(cprm, owner_name, en.n_namesz))
> + return 1;
> + if (!dump_align(cprm, 4))
> + return 1;
> +
> + num_records = dump_xsave_layout_desc(cprm);
> + if (!num_records) {
> + pr_warn("Error adding XSTATE layout ELF note. XSTATE buffer in
> the core file will be unparseable.");
Can you make this pr_warn_ratelimited() (and below)?
> + return 1;
> + }
> +
> + /* Total size should be equal to the number of records */
> + if ((sizeof(struct xfeat_component) * num_records) != en.n_descsz) {
> + pr_warn("Error adding XSTATE layout ELF note. The size of the
> .note section does not match with the total size of the records.");
> + return 1;
> + }
> +
> + if (!dump_align(cprm, 4))
> + return 1;
I don't think this call is needed?
> +
> + return 0;
> +}
> +
> +/*
> + * Return the size of new note.
> + */
> +int elf_coredump_extra_notes_size(void)
> +{
> + const char *fullname = "LINUX";
Now this can be dropped.
> + int size = 0;
> +
> + /* NOTE Header */
> + size += sizeof(struct elf_note);
> + /* name + align */
> + size += roundup(strlen(fullname) + 1, 4);
size += roundup(sizeof(owner_name), 4);
> + size += get_xsave_desc_size();
> +
> + return size;
> +}
> diff --git a/include/linux/elf.h b/include/linux/elf.h
> index c9a46c4e183b..5c402788da19 100644
> --- a/include/linux/elf.h
> +++ b/include/linux/elf.h
> @@ -65,7 +65,7 @@ extern Elf64_Dyn _DYNAMIC [];
> struct file;
> struct coredump_params;
>
> -#ifndef ARCH_HAVE_EXTRA_ELF_NOTES
> +#ifndef CONFIG_ARCH_HAVE_EXTRA_ELF_NOTES
> static inline int elf_coredump_extra_notes_size(void) { return 0; }
> static inline int elf_coredump_extra_notes_write(struct coredump_params
> *cprm) { return 0; }
> #else
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index 9417309b7230..3325488cb39b 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -411,6 +411,7 @@ typedef struct elf64_shdr {
> #define NT_X86_XSTATE0x202 /* x86 extended state using
> xsave */
> /* Old binutils treats 0x203 as a CET state */
> #define NT_X86_SHSTK 0x204 /* x86 SHSTK state */
> +#define NT_X86_XSAVE_LAYOUT 0x205 /* XSAVE layout description */
> #define NT_S390_HIGH_GPRS0x300 /* s390 upper register halves */
> #define NT_S390_TIMER0x301 /* s390 timer register */
> #define NT_S390_TODCMP 0x302 /* s390 TOD clock comparator
> register */
> --
> 2.43.0
>
Otherwise looks reasonable, though I see Dave has feedback to address
too. :)
Thanks for working on this!
-Kees
--
Kees Cook
Re: [PATCH 04/14] kunit: Add documentation for warning backtrace suppression API
On Tue, Mar 12, 2024 at 10:02:59AM -0700, Guenter Roeck wrote: > Document API functions for suppressing warning backtraces. > > Signed-off-by: Guenter Roeck Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH 03/14] kunit: Add test cases for backtrace warning suppression
On Tue, Mar 12, 2024 at 10:02:58AM -0700, Guenter Roeck wrote: > Add unit tests to verify that warning backtrace suppression works. > > If backtrace suppression does _not_ work, the unit tests will likely > trigger unsuppressed backtraces, which should actually help to get > the affected architectures / platforms fixed. > > Signed-off-by: Guenter Roeck Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH 02/14] kunit: bug: Count suppressed warning backtraces
On Tue, Mar 12, 2024 at 10:02:57AM -0700, Guenter Roeck wrote:
> Count suppressed warning backtraces to enable code which suppresses
> warning backtraces to check if the expected backtrace(s) have been
> observed.
>
> Using atomics for the backtrace count resulted in build errors on some
> architectures due to include file recursion, so use a plain integer
> for now.
>
> Signed-off-by: Guenter Roeck
> ---
> include/kunit/bug.h | 7 ++-
> lib/kunit/bug.c | 4 +++-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/kunit/bug.h b/include/kunit/bug.h
> index 1e34da961599..2097a854ac8c 100644
> --- a/include/kunit/bug.h
> +++ b/include/kunit/bug.h
> @@ -20,6 +20,7 @@
> struct __suppressed_warning {
> struct list_head node;
> const char *function;
> + int counter;
Thanks for adding a counter!
> };
>
> void __start_suppress_warning(struct __suppressed_warning *warning);
> @@ -28,7 +29,7 @@ bool __is_suppressed_warning(const char *function);
>
> #define DEFINE_SUPPRESSED_WARNING(func) \
> struct __suppressed_warning __kunit_suppress_##func = \
> - { .function = __stringify(func) }
> + { .function = __stringify(func), .counter = 0 }
>
> #define START_SUPPRESSED_WARNING(func) \
> __start_suppress_warning(&__kunit_suppress_##func)
> @@ -39,12 +40,16 @@ bool __is_suppressed_warning(const char *function);
> #define IS_SUPPRESSED_WARNING(func) \
> __is_suppressed_warning(func)
>
> +#define SUPPRESSED_WARNING_COUNT(func) \
> + (__kunit_suppress_##func.counter)
> +
> #else /* CONFIG_KUNIT */
>
> #define DEFINE_SUPPRESSED_WARNING(func)
> #define START_SUPPRESSED_WARNING(func)
> #define END_SUPPRESSED_WARNING(func)
> #define IS_SUPPRESSED_WARNING(func) (false)
> +#define SUPPRESSED_WARNING_COUNT(func) (0)
>
> #endif /* CONFIG_KUNIT */
> #endif /* __ASSEMBLY__ */
> diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c
> index f93544d7a9d1..13b3d896c114 100644
> --- a/lib/kunit/bug.c
> +++ b/lib/kunit/bug.c
> @@ -32,8 +32,10 @@ bool __is_suppressed_warning(const char *function)
> return false;
>
> list_for_each_entry(warning, _warnings, node) {
> - if (!strcmp(function, warning->function))
> + if (!strcmp(function, warning->function)) {
> + warning->counter++;
> return true;
> + }
> }
> return false;
> }
> --
> 2.39.2
>
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH 01/14] bug/kunit: Core support for suppressing warning backtraces
On Tue, Mar 12, 2024 at 10:02:56AM -0700, Guenter Roeck wrote: > Some unit tests intentionally trigger warning backtraces by passing > bad parameters to API functions. Such unit tests typically check the > return value from those calls, not the existence of the warning backtrace. > > Such intentionally generated warning backtraces are neither desirable > nor useful for a number of reasons. > - They can result in overlooked real problems. > - A warning that suddenly starts to show up in unit tests needs to be > investigated and has to be marked to be ignored, for example by > adjusting filter scripts. Such filters are ad-hoc because there is > no real standard format for warnings. On top of that, such filter > scripts would require constant maintenance. > > One option to address problem would be to add messages such as "expected > warning backtraces start / end here" to the kernel log. However, that > would again require filter scripts, it might result in missing real > problematic warning backtraces triggered while the test is running, and > the irrelevant backtrace(s) would still clog the kernel log. > > Solve the problem by providing a means to identify and suppress specific > warning backtraces while executing test code. > > Cc: Dan Carpenter > Cc: Daniel Diaz > Cc: Naresh Kamboju > Cc: Kees Cook > Signed-off-by: Guenter Roeck Yup, this looks fine to me. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] selftests/powerpc: Fix load_unaligned_zeropad build failure
On Tue, 05 Mar 2024 23:56:44 +1100, Michael Ellerman wrote: > This test is userspace code, but uses some kernel headers via symlinks, > and mocks other headers, in order to test load_unaligned_zeropad(). > > Currently the test fails to build with: > > In file included from load_unaligned_zeropad.c:26: > word-at-a-time.h:7:10: fatal error: linux/bitops.h: No such file or > directory > 7 | #include > > [...] Applied to for-next/hardening, thanks! [1/1] selftests/powerpc: Fix load_unaligned_zeropad build failure https://git.kernel.org/kees/c/3fe1eb4dd2e4 Take care, -- Kees Cook
Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area_info
On Sat, Mar 02, 2024 at 12:47:08AM +, Edgecombe, Rick P wrote:
> On Wed, 2024-02-28 at 09:21 -0800, Kees Cook wrote:
> > I totally understand. If the "uninitialized" warnings were actually
> > reliable, I would agree. I look at it this way:
> >
> > - initializations can be missed either in static initializers or via
> > run time initializers. (So the risk of mistake here is matched --
> > though I'd argue it's easier to *find* static initializers when
> > adding
> > new struct members.)
> > - uninitialized warnings are inconsistent (this becomes an unknown
> > risk)
> > - when a run time initializer is missed, the contents are whatever
> > was
> > on the stack (high risk)
> > - what a static initializer is missed, the content is 0 (low risk)
> >
> > I think unambiguous state (always 0) is significantly more important
> > for
> > the safety of the system as a whole. Yes, individual cases maybe bad
> > ("what uid should this be? root?!") but from a general memory safety
> > perspective the value doesn't become potentially influenced by order
> > of
> > operations, leftover stack memory, etc.
> >
> > I'd agree, lifting everything into a static initializer does seem
> > cleanest of all the choices.
>
> Hi Kees,
>
> Well, I just gave this a try. It is giving me flashbacks of when I last
> had to do a tree wide change that I couldn't fully test and the
> breakage was caught by Linus.
Yeah, testing isn't fun for these kinds of things. This is traditionally
why the "obviously correct" changes tend to have an easier time landing
(i.e. adding "= {}" to all of them).
> Could you let me know if you think this is additionally worthwhile
> cleanup outside of the guard gap improvements of this series? Because I
> was thinking a more cowardly approach could be a new vm_unmapped_area()
> variant that takes the new start gap member as a separate argument
> outside of struct vm_unmapped_area_info. It would be kind of strange to
> keep them separate, but it would be less likely to bump something.
I think you want a new member -- AIUI, that's what that struct is for.
Looking at this resulting set of patches, I do kinda think just adding
the "= {}" in a single patch is more sensible. Having to split things
that are know at the top of the function from the stuff known at the
existing initialization time is rather awkward.
Personally, I think a single patch that sets "= {}" for all of them and
drop the all the "= 0" or "= NULL" assignments would be the cleanest way
to go.
-Kees
--
Kees Cook
Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area_info
On Wed, Feb 28, 2024 at 01:22:09PM +, Christophe Leroy wrote:
> [...]
> My worry with initialisation at declaration is it often hides missing
> assignments. Let's take following simple exemple:
>
> char *colour(int num)
> {
> char *name;
>
> if (num == 0) {
> name = "black";
> } else if (num == 1) {
> name = "white";
> } else if (num == 2) {
> } else {
> name = "no colour";
> }
>
> return name;
> }
>
> Here, GCC warns about a missing initialisation of variable 'name'.
Sometimes. :( We build with -Wno-maybe-uninitialized because GCC gets
this wrong too often. Also, like with large structs like this, all
uninit warnings get suppressed if anything takes it by reference. So, if
before your "return name" statement above, you had something like:
do_something();
it won't warn with any option enabled.
> But if I declare it as
>
> char *name = "no colour";
>
> Then GCC won't warn anymore that we are missing a value for when num is 2.
>
> During my life I have so many times spent huge amount of time
> investigating issues and bugs due to missing assignments that were going
> undetected due to default initialisation at declaration.
I totally understand. If the "uninitialized" warnings were actually
reliable, I would agree. I look at it this way:
- initializations can be missed either in static initializers or via
run time initializers. (So the risk of mistake here is matched --
though I'd argue it's easier to *find* static initializers when adding
new struct members.)
- uninitialized warnings are inconsistent (this becomes an unknown risk)
- when a run time initializer is missed, the contents are whatever was
on the stack (high risk)
- what a static initializer is missed, the content is 0 (low risk)
I think unambiguous state (always 0) is significantly more important for
the safety of the system as a whole. Yes, individual cases maybe bad
("what uid should this be? root?!") but from a general memory safety
perspective the value doesn't become potentially influenced by order of
operations, leftover stack memory, etc.
I'd agree, lifting everything into a static initializer does seem
cleanest of all the choices.
-Kees
--
Kees Cook
Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area_info
On Tue, Feb 27, 2024 at 07:02:59AM +, Christophe Leroy wrote: > > > Le 26/02/2024 à 20:09, Rick Edgecombe a écrit : > > Future changes will need to add a field to struct vm_unmapped_area_info. > > This would cause trouble for any archs that don't initialize the > > struct. Currently every user sets each field, so if new fields are > > added, the core code parsing the struct will see garbage in the new > > field. > > > > It could be possible to initialize the new field for each arch to 0, but > > instead simply inialize the field with a C99 struct inializing syntax. > > Why doing a full init of the struct when all fields are re-written a few > lines after ? It's a nice change for robustness and makes future changes easier. It's not actually wasteful since the compiler will throw away all redundant stores. > If I take the exemple of powerpc function slice_find_area_bottomup(): > > struct vm_unmapped_area_info info; > > info.flags = 0; > info.length = len; > info.align_mask = PAGE_MASK & ((1ul << pshift) - 1); > info.align_offset = 0; But one cleanup that is possible from explicitly zero-initializing the whole structure would be dropping all the individual "= 0" assignments. :) -- Kees Cook
Re: [PATCH 00/11] Bump the minimum supported version of LLVM to 13.0.1
| 6 -- > include/linux/compiler-clang.h| 8 ++-- > lib/Kconfig.debug | 2 +- > scripts/min-tool-version.sh | 2 +- > scripts/recordmcount.pl | 2 +- > security/Kconfig | 2 -- > 14 files changed, 15 insertions(+), 57 deletions(-) > --- > base-commit: 979741ebd48f75ed6d101c7290e3325340d361ff > change-id: 20240124-bump-min-llvm-ver-to-13-0-1-39f84dd36b19 > > Best regards, > -- > Nathan Chancellor > Yes, please. :) This looks reasonable -- I appreciate the review of default Clang versions across distros! Reviewed-by: Kees Cook -- Kees Cook
[PATCH 60/82] powerpc: Refactor intentional wrap-around test
In an effort to separate intentional arithmetic wrap-around from
unexpected wrap-around, we need to refactor places that depend on this
kind of math. One of the most common code patterns of this is:
VAR + value < VAR
Notably, this is considered "undefined behavior" for signed and pointer
types, which the kernel works around by using the -fno-strict-overflow
option in the build[1] (which used to just be -fwrapv). Regardless, we
want to get the kernel source to the position where we can meaningfully
instrument arithmetic wrap-around conditions and catch them when they
are unexpected, regardless of whether they are signed[2], unsigned[3],
or pointer[4] types.
Refactor open-coded wrap-around addition test to use add_would_overflow().
This paves the way to enabling the wrap-around sanitizers in the future.
Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 [1]
Link: https://github.com/KSPP/linux/issues/26 [2]
Link: https://github.com/KSPP/linux/issues/27 [3]
Link: https://github.com/KSPP/linux/issues/344 [4]
Cc: Michael Ellerman
Cc: Nicholas Piggin
Cc: Christophe Leroy
Cc: "Aneesh Kumar K.V"
Cc: "Naveen N. Rao"
Cc: Mahesh Salgaonkar
Cc: Vasant Hegde
Cc: dingsenjie
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Aneesh Kumar K.V
Cc: Naveen N. Rao
Signed-off-by: Kees Cook
---
arch/powerpc/platforms/powernv/opal-prd.c | 2 +-
arch/powerpc/xmon/xmon.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/opal-prd.c
b/arch/powerpc/platforms/powernv/opal-prd.c
index b66b06efcef1..eaf95dc82925 100644
--- a/arch/powerpc/platforms/powernv/opal-prd.c
+++ b/arch/powerpc/platforms/powernv/opal-prd.c
@@ -51,7 +51,7 @@ static bool opal_prd_range_is_valid(uint64_t addr, uint64_t
size)
struct device_node *parent, *node;
bool found;
- if (addr + size < addr)
+ if (add_would_overflow(addr, size))
return false;
parent = of_find_node_by_path("/reserved-memory");
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index b3b94cd37713..b91fdda49434 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3252,7 +3252,7 @@ memzcan(void)
} else if (!ok && ook)
printf("%.8lx\n", a - mskip);
ook = ok;
- if (a + mskip < a)
+ if (add_would_overflow(a, mskip))
break;
}
if (ook)
--
2.34.1
Re: [PATCH 0/3] Update LLVM Phabricator and Bugzilla links
On Tue, Jan 09, 2024 at 03:16:28PM -0700, Nathan Chancellor wrote: > This series updates all instances of LLVM Phabricator and Bugzilla links > to point to GitHub commits directly and LLVM's Bugzilla to GitHub issue > shortlinks respectively. > > I split up the Phabricator patch into BPF selftests and the rest of the > kernel in case the BPF folks want to take it separately from the rest of > the series, there are obviously no dependency issues in that case. The > Bugzilla change was mechanical enough and should have no conflicts. > > I am aiming this at Andrew and CC'ing other lists, in case maintainers > want to chime in, but I think this is pretty uncontroversial (famous > last words...). > > --- > Nathan Chancellor (3): > selftests/bpf: Update LLVM Phabricator links > arch and include: Update LLVM Phabricator links > treewide: Update LLVM Bugzilla links > > arch/arm64/Kconfig | 4 +-- > arch/powerpc/Makefile | 4 +-- > arch/powerpc/kvm/book3s_hv_nested.c| 2 +- > arch/riscv/Kconfig | 2 +- > arch/riscv/include/asm/ftrace.h| 2 +- > arch/s390/include/asm/ftrace.h | 2 +- > arch/x86/power/Makefile| 2 +- > crypto/blake2b_generic.c | 2 +- > drivers/firmware/efi/libstub/Makefile | 2 +- > drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 2 +- > drivers/media/test-drivers/vicodec/codec-fwht.c| 2 +- > drivers/regulator/Kconfig | 2 +- > include/asm-generic/vmlinux.lds.h | 2 +- > include/linux/compiler-clang.h | 2 +- > lib/Kconfig.kasan | 2 +- > lib/raid6/Makefile | 2 +- > lib/stackinit_kunit.c | 2 +- > mm/slab_common.c | 2 +- > net/bridge/br_multicast.c | 2 +- > security/Kconfig | 2 +- > tools/testing/selftests/bpf/README.rst | 32 > +++--- > tools/testing/selftests/bpf/prog_tests/xdpwall.c | 2 +- > .../selftests/bpf/progs/test_core_reloc_type_id.c | 2 +- > 23 files changed, 40 insertions(+), 40 deletions(-) > --- > base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a > change-id: 20240109-update-llvm-links-d03f9d649e1e > > Best regards, > -- > Nathan Chancellor > Excellent! Thanks for doing this. I spot checked a handful I was familiar with and everything looks good to me. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH 1/3] init: Declare rodata_enabled and mark_rodata_ro() at all time
On December 21, 2023 4:16:56 AM PST, Michael Ellerman
wrote:
>Cc +Kees
>
>Christophe Leroy writes:
>> Declaring rodata_enabled and mark_rodata_ro() at all time
>> helps removing related #ifdefery in C files.
>>
>> Signed-off-by: Christophe Leroy
>> ---
>> include/linux/init.h | 4
>> init/main.c | 21 +++--
>> 2 files changed, 7 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/linux/init.h b/include/linux/init.h
>> index 01b52c9c7526..d2b47be38a07 100644
>> --- a/include/linux/init.h
>> +++ b/include/linux/init.h
>> @@ -168,12 +168,8 @@ extern initcall_entry_t __initcall_end[];
>>
>> extern struct file_system_type rootfs_fs_type;
>>
>> -#if defined(CONFIG_STRICT_KERNEL_RWX) || defined(CONFIG_STRICT_MODULE_RWX)
>> extern bool rodata_enabled;
>> -#endif
>> -#ifdef CONFIG_STRICT_KERNEL_RWX
>> void mark_rodata_ro(void);
>> -#endif
>>
>> extern void (*late_time_init)(void);
>>
>> diff --git a/init/main.c b/init/main.c
>> index e24b0780fdff..807df08c501f 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -1396,10 +1396,9 @@ static int __init set_debug_rodata(char *str)
>> early_param("rodata", set_debug_rodata);
>> #endif
>>
>> -#ifdef CONFIG_STRICT_KERNEL_RWX
>> static void mark_readonly(void)
>> {
>> -if (rodata_enabled) {
>> +if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX) && rodata_enabled) {
I think this will break without rodata_enabled actual existing on other
architectures. (Only declaration was made visible, not the definition, which is
above here and still behind ifdefs?)
-Kees
>> /*
>> * load_module() results in W+X mappings, which are cleaned
>> * up with call_rcu(). Let's make sure that queued work is
>> @@ -1409,20 +1408,14 @@ static void mark_readonly(void)
>> rcu_barrier();
>> mark_rodata_ro();
>> rodata_test();
>> -} else
>> +} else if (IS_ENABLED(CONFIG_STRICT_KERNEL_RWX)) {
>> pr_info("Kernel memory protection disabled.\n");
>> +} else if (IS_ENABLED(CONFIG_ARCH_HAS_STRICT_KERNEL_RWX)) {
>> +pr_warn("Kernel memory protection not selected by kernel
>> config.\n");
>> +} else {
>> +pr_warn("This architecture does not have kernel memory
>> protection.\n");
>> +}
>> }
>> -#elif defined(CONFIG_ARCH_HAS_STRICT_KERNEL_RWX)
>> -static inline void mark_readonly(void)
>> -{
>> -pr_warn("Kernel memory protection not selected by kernel config.\n");
>> -}
>> -#else
>> -static inline void mark_readonly(void)
>> -{
>> -pr_warn("This architecture does not have kernel memory protection.\n");
>> -}
>> -#endif
>>
>> void __weak free_initmem(void)
>> {
>> --
>> 2.41.0
--
Kees Cook
Re: [PATCH] scsi: ibmvscsi: replace deprecated strncpy with strscpy
On Mon, Oct 30, 2023 at 08:40:48PM +, Justin Stitt wrote: > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > We expect partition_name to be NUL-terminated based on its usage with > format strings: > | dev_info(hostdata->dev, "host srp version: %s, " > |"host partition %s (%d), OS %d, max io %u\n", > |hostdata->madapter_info.srp_version, > |hostdata->madapter_info.partition_name, > |be32_to_cpu(hostdata->madapter_info.partition_number), > |be32_to_cpu(hostdata->madapter_info.os_type), > |be32_to_cpu(hostdata->madapter_info.port_max_txu[0])); > ... > | len = snprintf(buf, PAGE_SIZE, "%s\n", > |hostdata->madapter_info.partition_name); > > Moreover, NUL-padding is not required as madapter_info is explicitly > memset to 0: > | memset(>madapter_info, 0x00, > | sizeof(hostdata->madapter_info)); > > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt Agreed; this conversion looks correct to me too. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] scsi: ibmvfc: replace deprecated strncpy with strscpy
On Mon, Oct 30, 2023 at 07:04:33PM +, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
>
> We expect these fields to be NUL-terminated as the property names from
> which they are derived are also NUL-terminated.
>
> Moreover, NUL-padding is not required as our destination buffers are
> already NUL-allocated and any future NUL-byte assignments are redundant
> (like the ones that strncpy() does).
> ibmvfc_probe() ->
> | struct ibmvfc_host *vhost;
> | struct Scsi_Host *shost;
> ...
> | shost = scsi_host_alloc(_template, sizeof(*vhost));
> ... **side note: is this a bug? Looks like a type to me ^**
I think this is the "privsize", so *vhost is correct, IIUC.
> ...
> | vhost = shost_priv(shost);
I.e. vhost is a part of the shost allocation
>
> ... where shost_priv() is:
> | static inline void *shost_priv(struct Scsi_Host *shost)
> | {
> | return (void *)shost->hostdata;
> | }
>
> .. and:
> scsi_host_alloc() ->
> | shost = kzalloc(sizeof(struct Scsi_Host) + privsize, GFP_KERNEL);
As seen here. :)
>
> And for login_info->..., NUL-padding is also not required as it is
> explicitly memset to 0:
> | memset(login_info, 0, sizeof(*login_info));
>
> Considering the above, a suitable replacement is `strscpy` [2] due to
> the fact that it guarantees NUL-termination on the destination buffer
> without unnecessarily NUL-padding.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Signed-off-by: Justin Stitt
Yeah, this conversion looks correct to me too.
Reviewed-by: Kees Cook
-Kees
> ---
> Note: build-tested only.
>
> Found with: $ rg "strncpy\("
> ---
> drivers/scsi/ibmvscsi/ibmvfc.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index ce9eb00e2ca0..e73a39b1c832 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -1455,7 +1455,7 @@ static void ibmvfc_gather_partition_info(struct
> ibmvfc_host *vhost)
>
> name = of_get_property(rootdn, "ibm,partition-name", NULL);
> if (name)
> - strncpy(vhost->partition_name, name,
> sizeof(vhost->partition_name));
> + strscpy(vhost->partition_name, name,
> sizeof(vhost->partition_name));
> num = of_get_property(rootdn, "ibm,partition-no", NULL);
> if (num)
> vhost->partition_number = *num;
> @@ -1498,13 +1498,15 @@ static void ibmvfc_set_login_info(struct ibmvfc_host
> *vhost)
> login_info->async.va = cpu_to_be64(vhost->async_crq.msg_token);
> login_info->async.len = cpu_to_be32(async_crq->size *
> sizeof(*async_crq->msgs.async));
> - strncpy(login_info->partition_name, vhost->partition_name,
> IBMVFC_MAX_NAME);
> - strncpy(login_info->device_name,
> - dev_name(>host->shost_gendev), IBMVFC_MAX_NAME);
> + strscpy(login_info->partition_name, vhost->partition_name,
> + sizeof(login_info->partition_name));
> +
> + strscpy(login_info->device_name,
> + dev_name(>host->shost_gendev),
> sizeof(login_info->device_name));
>
> location = of_get_property(of_node, "ibm,loc-code", NULL);
> location = location ? location : dev_name(vhost->dev);
> - strncpy(login_info->drc_name, location, IBMVFC_MAX_NAME);
> + strscpy(login_info->drc_name, location, sizeof(login_info->drc_name));
> }
>
> /**
>
> ---
> base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
> change-id: 20231030-strncpy-drivers-scsi-ibmvscsi-ibmvfc-c-ccfce3255d58
>
> Best regards,
> --
> Justin Stitt
>
--
Kees Cook
Re: [Bisected] [1b4fa28a8b07] Build failure "net/core/gso_test.c"
On Thu, Oct 12, 2023 at 11:57:46AM +0200, Florian Westphal wrote:
> Tasmiya Nalatwad wrote:
> > Greetings,
> >
> > [net-next] [6.6-rc4] Build failure "net/core/gso_test.c"
> >
> > --- Traces ---
> >
> > make -j 33 -s && make modules_install && make install
> > net/core/gso_test.c:58:48: error: initializer element is not constant
> > 58 | .segs = (const unsigned int[]) { gso_size },
> > | ^
>
> Ouch, I can reproduce this with: gcc --version
> gcc (Debian 12.2.0-14) 12.2.0
> Copyright (C) 2022 Free Software Foundation, Inc.
>
> gcc 13.2.1 and clang-16.0.6 are ok.
>
> Whats the preference here? We could use simple preprocessor constant
> or we could require much more recent compiler version for the net
> kunit tests via kconfig.
>
> gcc-12.2.0 can compile it after this simple s//g "fix":
>
> diff --git a/net/core/gso_test.c b/net/core/gso_test.c
> --- a/net/core/gso_test.c
> +++ b/net/core/gso_test.c
> @@ -4,7 +4,7 @@
> #include
>
> static const char hdr[] = "abcdefgh";
> -static const int gso_size = 1000;
> +#define GSO_TEST_SIZE 1000
This fixes the build for me too.
Tested-by: Kees Cook
Reviewed-by: Kees Cook
-Kees
>
> static void __init_skb(struct sk_buff *skb)
> {
> @@ -18,7 +18,7 @@ static void __init_skb(struct sk_buff *skb)
>
> /* proto is arbitrary, as long as not ETH_P_TEB or vlan */
> skb->protocol = htons(ETH_P_ATALK);
> - skb_shinfo(skb)->gso_size = gso_size;
> + skb_shinfo(skb)->gso_size = GSO_TEST_SIZE;
> }
>
> enum gso_test_nr {
> @@ -53,70 +53,70 @@ static struct gso_test_case cases[] = {
> {
> .id = GSO_TEST_NO_GSO,
> .name = "no_gso",
> - .linear_len = gso_size,
> + .linear_len = GSO_TEST_SIZE,
> .nr_segs = 1,
> - .segs = (const unsigned int[]) { gso_size },
> + .segs = (const unsigned int[]) { GSO_TEST_SIZE },
> },
> {
> .id = GSO_TEST_LINEAR,
> .name = "linear",
> - .linear_len = gso_size + gso_size + 1,
> + .linear_len = GSO_TEST_SIZE + GSO_TEST_SIZE + 1,
> .nr_segs = 3,
> - .segs = (const unsigned int[]) { gso_size, gso_size, 1 },
> + .segs = (const unsigned int[]) { GSO_TEST_SIZE, GSO_TEST_SIZE,
> 1 },
> },
> {
> .id = GSO_TEST_FRAGS,
> .name = "frags",
> - .linear_len = gso_size,
> + .linear_len = GSO_TEST_SIZE,
> .nr_frags = 2,
> - .frags = (const unsigned int[]) { gso_size, 1 },
> + .frags = (const unsigned int[]) { GSO_TEST_SIZE, 1 },
> .nr_segs = 3,
> - .segs = (const unsigned int[]) { gso_size, gso_size, 1 },
> + .segs = (const unsigned int[]) { GSO_TEST_SIZE, GSO_TEST_SIZE,
> 1 },
> },
> {
> .id = GSO_TEST_FRAGS_PURE,
> .name = "frags_pure",
> .nr_frags = 3,
> - .frags = (const unsigned int[]) { gso_size, gso_size, 2 },
> + .frags = (const unsigned int[]) { GSO_TEST_SIZE, GSO_TEST_SIZE,
> 2 },
> .nr_segs = 3,
> - .segs = (const unsigned int[]) { gso_size, gso_size, 2 },
> + .segs = (const unsigned int[]) { GSO_TEST_SIZE, GSO_TEST_SIZE,
> 2 },
> },
> {
> .id = GSO_TEST_GSO_PARTIAL,
> .name = "gso_partial",
> - .linear_len = gso_size,
> + .linear_len = GSO_TEST_SIZE,
> .nr_frags = 2,
> - .frags = (const unsigned int[]) { gso_size, 3 },
> + .frags = (const unsigned int[]) { GSO_TEST_SIZE, 3 },
> .nr_segs = 2,
> - .segs = (const unsigned int[]) { 2 * gso_size, 3 },
> + .segs = (const unsigned int[]) { 2 * GSO_TEST_SIZE, 3 },
> },
> {
> /* commit 89319d3801d1: frag_list on mss boundaries */
> .id = GSO_TEST_FRAG_LIST,
> .name = "frag_list",
> - .linear_len = gso_size,
> + .linear_len = GSO_TEST_SIZE,
> .nr_frag_skbs = 2,
> - .frag_skbs = (const unsigned int[]) { gso_size, gso_size },
> + .frag_skbs = (const unsigned int[]) { GSO_TEST_SIZE,
> GSO_TEST_SIZE },
> .nr_segs = 3,
> - .segs = (const unsigned int[]) { gso_size, gso_size, gso_size },
> +
Re: [PATCH] ibmvnic: replace deprecated strncpy with strscpy
On Mon, Oct 09, 2023 at 11:19:57PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > NUL-padding is not required as the buffer is already memset to 0: > | memset(adapter->fw_version, 0, 32); > > Note that another usage of strscpy exists on the same buffer: > | strscpy((char *)adapter->fw_version, "N/A", > sizeof(adapter->fw_version)); > > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt Thanks, this looks right to me. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v2] hwmon: (ibmpowernv) refactor deprecated strncpy
On Tue, 19 Sep 2023 05:22:51 +, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> A suitable replacement is `memcpy` as we've already precisely calculated
> the number of bytes to copy while `buf` has been explicitly
> zero-initialized:
> | char buf[8] = { 0 };
>
> [...]
Applied to for-next/hardening, thanks! (I've updated the Subject here
and with the older "refactor" subjects...)
Take care,
--
Kees Cook
Re: [PATCH] selftests/powerpc: Fix emit_tests to work with run_kselftest.sh
On Thu, Sep 21, 2023 at 05:26:10PM +1000, Michael Ellerman wrote:
> In order to use run_kselftest.sh the list of tests must be emitted to
> populate kselftest-list.txt.
>
> The powerpc Makefile is written to use EMIT_TESTS. But support for
> EMIT_TESTS was dropped in commit d4e59a536f50 ("selftests: Use runner.sh
> for emit targets"). Although prior to that commit a548de0fe8e1
> ("selftests: lib.mk: add test execute bit check to EMIT_TESTS") had
> already broken run_kselftest.sh for powerpc due to the executable check
> using the wrong path.
>
> It can be fixed by replacing the EMIT_TESTS definitions with actual
> emit_tests rules in the powerpc Makefiles. This makes run_kselftest.sh
> able to run powerpc tests:
>
> $ cd linux
> $ export ARCH=powerpc
> $ export CROSS_COMPILE=powerpc64le-linux-gnu-
> $ make headers
> $ make -j -C tools/testing/selftests install
> $ grep -c "^powerpc"
> tools/testing/selftests/kselftest_install/kselftest-list.txt
> 182
>
> Fixes: d4e59a536f50 ("selftests: Use runner.sh for emit targets")
> Signed-off-by: Michael Ellerman
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH] i2c: replace deprecated strncpy
On Wed, Sep 20, 2023 at 11:07:35AM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > `info.type` is expected to be NUL-terminated judging by its use in > `i2c_new_client_device()` wherein it is used to populate `client->name`: > | strscpy(client->name, info->type, sizeof(client->name)); > > NUL-padding is not required and even if it was, `client` is already > zero-initialized. > > Considering the two points from above, a suitable replacement is > `strscpy` [2] due to the fact that it guarantees NUL-termination on the > destination buffer without unnecessarily NUL-padding. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt Looks like a straight replacement. Thanks! Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH][next] net: spider_net: Use size_add() in call to struct_size()
On Fri, Sep 15, 2023 at 01:25:36PM -0600, Gustavo A. R. Silva wrote:
> If, for any reason, the open-coded arithmetic causes a wraparound,
> the protection that `struct_size()` adds against potential integer
> overflows is defeated. Fix this by hardening call to `struct_size()`
> with `size_add()`.
>
> Fixes: 3f1071ec39f7 ("net: spider_net: Use struct_size() helper")
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH] hwmon: (ibmpowernv) refactor deprecated strncpy
On Thu, Sep 14, 2023 at 10:40:37PM -0700, Guenter Roeck wrote: > It is really sad that the submitters of such "cleanup" patches can't be > bothered > to check what they are doing. They can't even be bothered to write a > coccinelle > script that would avoid pitfalls like this one, and they expect others to do > their > homework for them. Well I'm not sure that's entirely fair to Justin's efforts (I know he's been studying these changes and everyone makes mistakes), but that's why I'm helping review his findings -- some code behaviors are more obvious than others. :) -- Kees Cook
Re: [PATCH] hwmon: (ibmpowernv) refactor deprecated strncpy
On Thu, Sep 14, 2023 at 11:21:06PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We should prefer more robust and less ambiguous string interfaces. > > A suitable replacement is `strscpy` [2] due to the fact that it > guarantees NUL-termination on the destination buffer without > unnecessarily NUL-padding since `buf` is already zero-initialized. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt > --- > drivers/hwmon/ibmpowernv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c > index 594254d6a72d..57d829dbcda6 100644 > --- a/drivers/hwmon/ibmpowernv.c > +++ b/drivers/hwmon/ibmpowernv.c > @@ -234,7 +234,7 @@ static int get_sensor_index_attr(const char *name, u32 > *index, char *attr) > if (copy_len >= sizeof(buf)) > return -EINVAL; > > - strncpy(buf, hash_pos + 1, copy_len); > + strscpy(buf, hash_pos + 1, copy_len); This is another case of precise byte copying -- this just needs to be memcpy. Otherwise this truncates the trailing character. Imagine a name input of "fan#2-data". "buf" wants to get "2". copy_len is 1, and strscpy would eat it. :) -Kees > > err = kstrtou32(buf, 10, index); > if (err) > > --- > base-commit: 3669558bdf354cd352be955ef2764cde6a9bf5ec > change-id: 20230914-strncpy-drivers-hwmon-ibmpowernv-c-80a03f16d93a > > Best regards, > -- > Justin Stitt > -- Kees Cook
Re: [PATCH v3] kbuild: Show marked Kconfig fragments in "help"
On Fri, Sep 01, 2023 at 04:58:37PM +0900, Masahiro Yamada wrote: > On Fri, Sep 1, 2023 at 4:13 AM Kees Cook wrote: > > > > Currently the Kconfig fragments in kernel/configs and arch/*/configs > > that aren't used internally aren't discoverable through "make help", > > which consists of hard-coded lists of config fragments. Instead, list > > all the fragment targets that have a "# Help: " comment prefix so the > > targets can be generated dynamically. > > > > Add logic to the Makefile to search for and display the fragment and > > comment. Add comments to fragments that are intended to be direct targets. > > > > Cc: Nicolas Schier > > Cc: Michael Ellerman > > Cc: Christophe Leroy > > Cc: Randy Dunlap > > Cc: linux-ker...@vger.kernel.org > > Cc: x...@kernel.org > > Cc: linux-arm-ker...@lists.infradead.org > > Cc: linuxppc-dev@lists.ozlabs.org > > Cc: linux-ri...@lists.infradead.org > > Cc: linux-s...@vger.kernel.org > > Cc: linux-kbu...@vger.kernel.org > > Cc: linux-harden...@vger.kernel.org > > Signed-off-by: Kees Cook > > Co-developed-by: Masahiro Yamada > > --- > > v3: > > - Use Makefile logic from Masahiro Yamada > > - Use "# Help: " prefix, but only on desired fragment targets > > v2: https://lore.kernel.org/all/20230825194329.gonna.911-k...@kernel.org > > v1: https://lore.kernel.org/all/20230824223606.never.762-k...@kernel.org > > --- > > Makefile | 1 - > > arch/arm/configs/dram_0x.config| 1 + > > arch/arm/configs/dram_0xc000.config| 1 + > > arch/arm/configs/dram_0xd000.config| 1 + > > arch/arm/configs/lpae.config | 1 + > > arch/arm64/configs/virt.config | 1 + > > arch/powerpc/configs/disable-werror.config | 1 + > > arch/powerpc/configs/security.config | 4 +++- > > arch/riscv/configs/32-bit.config | 1 + > > arch/riscv/configs/64-bit.config | 1 + > > arch/s390/configs/btf.config | 1 + > > arch/s390/configs/kasan.config | 1 + > > arch/x86/Makefile | 4 > > kernel/configs/debug.config| 2 ++ > > kernel/configs/kvm_guest.config| 1 + > > kernel/configs/nopm.config | 2 ++ > > kernel/configs/rust.config | 1 + > > kernel/configs/tiny.config | 2 ++ > > kernel/configs/x86_debug.config| 1 + > > kernel/configs/xen.config | 2 ++ > > scripts/kconfig/Makefile | 15 --- > > 21 files changed, 36 insertions(+), 9 deletions(-) > > > > > Just one thing. > > > > > > > diff --git a/kernel/configs/tiny.config b/kernel/configs/tiny.config > > index 9f7d0835..60a4b6d80b36 100644 > > --- a/kernel/configs/tiny.config > > +++ b/kernel/configs/tiny.config > > @@ -1,3 +1,5 @@ > > +# Help: Size-optimized kernel image > > > I will drop this. > > > We already have a hard-coded help message. > > tinyconfig - Configure the tiniest possible kernel > > > > > Then, some lines below, again. > > tiny.config - Size-optimized kernel image > > > > tiny.config is for internal use for tinyconfig. Shall I send a v4, or did you fix this up already? -- Kees Cook
[PATCH v3] kbuild: Show marked Kconfig fragments in "help"
Currently the Kconfig fragments in kernel/configs and arch/*/configs that aren't used internally aren't discoverable through "make help", which consists of hard-coded lists of config fragments. Instead, list all the fragment targets that have a "# Help: " comment prefix so the targets can be generated dynamically. Add logic to the Makefile to search for and display the fragment and comment. Add comments to fragments that are intended to be direct targets. Cc: Nicolas Schier Cc: Michael Ellerman Cc: Christophe Leroy Cc: Randy Dunlap Cc: linux-ker...@vger.kernel.org Cc: x...@kernel.org Cc: linux-arm-ker...@lists.infradead.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-ri...@lists.infradead.org Cc: linux-s...@vger.kernel.org Cc: linux-kbu...@vger.kernel.org Cc: linux-harden...@vger.kernel.org Signed-off-by: Kees Cook Co-developed-by: Masahiro Yamada --- v3: - Use Makefile logic from Masahiro Yamada - Use "# Help: " prefix, but only on desired fragment targets v2: https://lore.kernel.org/all/20230825194329.gonna.911-k...@kernel.org v1: https://lore.kernel.org/all/20230824223606.never.762-k...@kernel.org --- Makefile | 1 - arch/arm/configs/dram_0x.config| 1 + arch/arm/configs/dram_0xc000.config| 1 + arch/arm/configs/dram_0xd000.config| 1 + arch/arm/configs/lpae.config | 1 + arch/arm64/configs/virt.config | 1 + arch/powerpc/configs/disable-werror.config | 1 + arch/powerpc/configs/security.config | 4 +++- arch/riscv/configs/32-bit.config | 1 + arch/riscv/configs/64-bit.config | 1 + arch/s390/configs/btf.config | 1 + arch/s390/configs/kasan.config | 1 + arch/x86/Makefile | 4 kernel/configs/debug.config| 2 ++ kernel/configs/kvm_guest.config| 1 + kernel/configs/nopm.config | 2 ++ kernel/configs/rust.config | 1 + kernel/configs/tiny.config | 2 ++ kernel/configs/x86_debug.config| 1 + kernel/configs/xen.config | 2 ++ scripts/kconfig/Makefile | 15 --- 21 files changed, 36 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 4739c21a63e2..91c90ce8e0e3 100644 --- a/Makefile +++ b/Makefile @@ -1674,7 +1674,6 @@ help: @echo ' mrproper- Remove all generated files + config + various backup files' @echo ' distclean - mrproper + remove editor backup and patch files' @echo '' - @echo 'Configuration targets:' @$(MAKE) -f $(srctree)/scripts/kconfig/Makefile help @echo '' @echo 'Other generic targets:' diff --git a/arch/arm/configs/dram_0x.config b/arch/arm/configs/dram_0x.config index db96dcb420ce..8803a0f58343 100644 --- a/arch/arm/configs/dram_0x.config +++ b/arch/arm/configs/dram_0x.config @@ -1 +1,2 @@ +# Help: DRAM base at 0x CONFIG_DRAM_BASE=0x diff --git a/arch/arm/configs/dram_0xc000.config b/arch/arm/configs/dram_0xc000.config index 343d5333d973..aab8f864686b 100644 --- a/arch/arm/configs/dram_0xc000.config +++ b/arch/arm/configs/dram_0xc000.config @@ -1 +1,2 @@ +# Help: DRAM base at 0xc000 CONFIG_DRAM_BASE=0xc000 diff --git a/arch/arm/configs/dram_0xd000.config b/arch/arm/configs/dram_0xd000.config index 61ba7045f8a1..4aabce4ea3d4 100644 --- a/arch/arm/configs/dram_0xd000.config +++ b/arch/arm/configs/dram_0xd000.config @@ -1 +1,2 @@ +# Help: DRAM base at 0xd000 CONFIG_DRAM_BASE=0xd000 diff --git a/arch/arm/configs/lpae.config b/arch/arm/configs/lpae.config index a6d6f7ab3c01..1ab94da8345d 100644 --- a/arch/arm/configs/lpae.config +++ b/arch/arm/configs/lpae.config @@ -1,2 +1,3 @@ +# Help: Enable Large Physical Address Extension mode CONFIG_ARM_LPAE=y CONFIG_VMSPLIT_2G=y diff --git a/arch/arm64/configs/virt.config b/arch/arm64/configs/virt.config index 6865d54e68f8..c47c36f8f67b 100644 --- a/arch/arm64/configs/virt.config +++ b/arch/arm64/configs/virt.config @@ -1,3 +1,4 @@ +# Help: Virtualization guest # # Base options for platforms # diff --git a/arch/powerpc/configs/disable-werror.config b/arch/powerpc/configs/disable-werror.config index 6ea12a12432c..7776b91da37f 100644 --- a/arch/powerpc/configs/disable-werror.config +++ b/arch/powerpc/configs/disable-werror.config @@ -1 +1,2 @@ +# Help: Disable -Werror CONFIG_PPC_DISABLE_WERROR=y diff --git a/arch/powerpc/configs/security.config b/arch/powerpc/configs/security.config index 1c91a35c6a73..0d54e29e2cdf 100644 --- a/arch/powerpc/configs/security.config +++ b/arch/powerpc/configs/security.config @@ -1,3 +1,5 @@ +# Help: Common security options for PowerPC builds + # This is the equivalent of booting with lockdown=integrity CONFIG_SECURITY=y CONFIG_SECURITYFS=y @@ -12,4 +14,4 @@ CON
Re: [PATCH v2 0/2] kbuild: Show Kconfig fragments in "help"
On Tue, Aug 29, 2023 at 11:57:19PM +0900, Masahiro Yamada wrote: > The attached patch will work too. > > I dropped the "in the first line" restriction > because SPDX might be placed in the first line > of config fragments. Good call. Yes, this looks excellent; thank you! Do you want to send a formal patch? Please consider it: Reviewed-by: Kees Cook -- Kees Cook
[PATCH v2 2/2] kbuild: Split internal config targets from .config into .fragment
Many Kconfig fragments are being used internally to construct hard-coded
targets and shouldn't be reachable directly through the build system.
Splitting these out also means that the "help" target can display only
the "complete" .config targets intended for general use. This is
especially useful for powerpc where most of the arch fragments aren't
intended to be consumed individually.
Cc: Masahiro Yamada
Cc: x...@kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kbu...@vger.kernel.org
Suggested-by: Michael Ellerman
Signed-off-by: Kees Cook
---
arch/powerpc/Makefile | 26 +--
.../{32-bit.config => 32-bit.fragment}| 0
.../{64-bit.config => 64-bit.fragment}| 0
...{85xx-32bit.config => 85xx-32bit.fragment} | 0
...{85xx-64bit.config => 85xx-64bit.fragment} | 0
.../{85xx-hw.config => 85xx-hw.fragment} | 0
.../{85xx-smp.config => 85xx-smp.fragment}| 0
.../{86xx-hw.config => 86xx-hw.fragment} | 0
.../{86xx-smp.config => 86xx-smp.fragment}| 0
.../{altivec.config => altivec.fragment} | 0
.../configs/{be.config => be.fragment}| 0
.../{book3s_32.config => book3s_32.fragment} | 0
...enet_base.config => corenet_base.fragment} | 0
.../configs/{dpaa.config => dpaa.fragment}| 0
...mb-nonhw.config => fsl-emb-nonhw.fragment} | 0
.../configs/{guest.config => guest.fragment} | 0
.../configs/{le.config => le.fragment}| 0
...85xx_base.config => mpc85xx_base.fragment} | 0
...86xx_base.config => mpc86xx_base.fragment} | 0
.../{ppc64le.config => ppc64le.fragment} | 0
{kernel => arch/x86}/configs/x86_debug.config | 0
.../{tiny-base.config => tiny-base.fragment} | 0
scripts/Makefile.defconf | 12 ++---
scripts/kconfig/Makefile | 2 +-
24 files changed, 22 insertions(+), 18 deletions(-)
rename arch/powerpc/configs/{32-bit.config => 32-bit.fragment} (100%)
rename arch/powerpc/configs/{64-bit.config => 64-bit.fragment} (100%)
rename arch/powerpc/configs/{85xx-32bit.config => 85xx-32bit.fragment} (100%)
rename arch/powerpc/configs/{85xx-64bit.config => 85xx-64bit.fragment} (100%)
rename arch/powerpc/configs/{85xx-hw.config => 85xx-hw.fragment} (100%)
rename arch/powerpc/configs/{85xx-smp.config => 85xx-smp.fragment} (100%)
rename arch/powerpc/configs/{86xx-hw.config => 86xx-hw.fragment} (100%)
rename arch/powerpc/configs/{86xx-smp.config => 86xx-smp.fragment} (100%)
rename arch/powerpc/configs/{altivec.config => altivec.fragment} (100%)
rename arch/powerpc/configs/{be.config => be.fragment} (100%)
rename arch/powerpc/configs/{book3s_32.config => book3s_32.fragment} (100%)
rename arch/powerpc/configs/{corenet_base.config => corenet_base.fragment}
(100%)
rename arch/powerpc/configs/{dpaa.config => dpaa.fragment} (100%)
rename arch/powerpc/configs/{fsl-emb-nonhw.config => fsl-emb-nonhw.fragment}
(100%)
rename arch/powerpc/configs/{guest.config => guest.fragment} (100%)
rename arch/powerpc/configs/{le.config => le.fragment} (100%)
rename arch/powerpc/configs/{mpc85xx_base.config => mpc85xx_base.fragment}
(100%)
rename arch/powerpc/configs/{mpc86xx_base.config => mpc86xx_base.fragment}
(100%)
rename arch/powerpc/configs/{ppc64le.config => ppc64le.fragment} (100%)
rename {kernel => arch/x86}/configs/x86_debug.config (100%)
rename kernel/configs/{tiny-base.config => tiny-base.fragment} (100%)
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index dac7ca153886..b73f2b40a0bc 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -267,66 +267,66 @@ powernv_be_defconfig:
generated_configs += mpc85xx_defconfig
mpc85xx_defconfig:
- $(call merge_into_defconfig,mpc85xx_base.config,\
+ $(call merge_into_defconfig,mpc85xx_base.fragment,\
85xx-32bit 85xx-hw fsl-emb-nonhw)
generated_configs += mpc85xx_smp_defconfig
mpc85xx_smp_defconfig:
- $(call merge_into_defconfig,mpc85xx_base.config,\
+ $(call merge_into_defconfig,mpc85xx_base.fragment,\
85xx-32bit 85xx-smp 85xx-hw fsl-emb-nonhw)
generated_configs += corenet32_smp_defconfig
corenet32_smp_defconfig:
- $(call merge_into_defconfig,corenet_base.config,\
+ $(call merge_into_defconfig,corenet_base.fragment,\
85xx-32bit 85xx-smp 85xx-hw fsl-emb-nonhw dpaa)
generated_configs += corenet64_smp_defconfig
corenet64_smp_defconfig:
- $(call merge_into_defconfig,corenet_base.config,\
+ $(call merge_into_defconfig,corenet_base.fragment,\
85xx-64bit 85xx-smp altivec 85xx-hw fsl-emb-nonhw dpaa)
generated_configs += mpc86xx_defconfig
mpc86xx_defconfig:
- $(call merge_into_defconfig,mpc86xx_base.config,\
+ $(call merge_into_defconfig,mpc86xx_base.fragment,\
86
[PATCH v2 1/2] kbuild: Show Kconfig fragments in "help"
Doing a "make help" would show only hard-coded Kconfig targets and depended on the archhelp target to include ".config" targets. There was nothing showing global kernel/configs/ targets. Solve this by walking the wildcard list and include them in the output, using the first comment line as the help text. Additionally avoid showing duplicate targets when they exist in both general and arch-specific locations. Update all Kconfig fragments to include help text and adjust archhelp targets to avoid redundancy. Adds the following section to "help" target output: Configuration fragment targets (for enabling various Kconfig items): debug.config - Debugging for CI systems and finding regressions kvm_guest.config - Bootable as a KVM guest nopm.config - Disable Power Management rust.config - Enable Rust tiny-base.config - Minimal options for tiny systems tiny.config - Size-optimized kernel image x86_debug.config - Debugging options for tip tree testing xen.config - Bootable as a Xen guest tiny.config - x86-specific options for a small kernel image xen.config - x86-specific options for a Xen virtualization guest Cc: Masahiro Yamada Cc: x...@kernel.org Cc: linux-arm-ker...@lists.infradead.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-ri...@lists.infradead.org Cc: linux-s...@vger.kernel.org Signed-off-by: Kees Cook --- Makefile | 1 - arch/arm/configs/dram_0x.config| 1 + arch/arm/configs/dram_0xc000.config| 1 + arch/arm/configs/dram_0xd000.config| 1 + arch/arm/configs/lpae.config | 1 + arch/arm64/configs/virt.config | 1 + arch/powerpc/configs/32-bit.config | 1 + arch/powerpc/configs/64-bit.config | 1 + arch/powerpc/configs/85xx-32bit.config | 1 + arch/powerpc/configs/85xx-64bit.config | 1 + arch/powerpc/configs/85xx-hw.config| 1 + arch/powerpc/configs/85xx-smp.config | 1 + arch/powerpc/configs/86xx-hw.config| 1 + arch/powerpc/configs/86xx-smp.config | 1 + arch/powerpc/configs/altivec.config| 1 + arch/powerpc/configs/be.config | 1 + arch/powerpc/configs/book3s_32.config | 1 + arch/powerpc/configs/corenet_base.config | 1 + arch/powerpc/configs/debug.config | 1 + arch/powerpc/configs/disable-werror.config | 1 + arch/powerpc/configs/dpaa.config | 1 + arch/powerpc/configs/fsl-emb-nonhw.config | 1 + arch/powerpc/configs/guest.config | 1 + arch/powerpc/configs/le.config | 1 + arch/powerpc/configs/mpc85xx_base.config | 1 + arch/powerpc/configs/mpc86xx_base.config | 1 + arch/powerpc/configs/ppc64le.config| 1 + arch/powerpc/configs/security.config | 4 +++- arch/riscv/configs/32-bit.config | 1 + arch/riscv/configs/64-bit.config | 1 + arch/s390/configs/btf.config | 1 + arch/s390/configs/kasan.config | 1 + arch/x86/Makefile | 4 arch/x86/configs/tiny.config | 2 ++ arch/x86/configs/xen.config| 2 ++ kernel/configs/debug.config| 2 ++ kernel/configs/kvm_guest.config| 1 + kernel/configs/nopm.config | 2 ++ kernel/configs/rust.config | 1 + kernel/configs/tiny-base.config| 1 + kernel/configs/tiny.config | 2 ++ kernel/configs/x86_debug.config| 1 + kernel/configs/xen.config | 2 ++ scripts/kconfig/Makefile | 14 +++--- 44 files changed, 60 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 4739c21a63e2..91c90ce8e0e3 100644 --- a/Makefile +++ b/Makefile @@ -1674,7 +1674,6 @@ help: @echo ' mrproper- Remove all generated files + config + various backup files' @echo ' distclean - mrproper + remove editor backup and patch files' @echo '' - @echo 'Configuration targets:' @$(MAKE) -f $(srctree)/scripts/kconfig/Makefile help @echo '' @echo 'Other generic targets:' diff --git a/arch/arm/configs/dram_0x.config b/arch/arm/configs/dram_0x.config index db96dcb420ce..4de3fde0de9a 100644 --- a/arch/arm/configs/dram_0x.config +++ b/arch/arm/configs/dram_0x.config @@ -1 +1,2 @@ +# DRAM base at 0x CONFIG_DRAM_BASE=0x diff --git a/arch/arm/configs/dram_0xc000.config b/arch/arm/configs/dram_0xc000.config index 343d5333d973..fdd4c7b1461e 100644 --- a/arch/arm/configs/dram_0xc000.config +++ b/arch/arm/configs/dram_0xc000.config @@ -1 +1,2 @@ +# DRAM base at 0xc000 CONFIG_DRAM_BASE=0xc000 diff --git a/arch/arm/configs/dram_0xd000.config b/arch/arm/configs/dram_0xd000.config index 61ba7045f8a1..54defdc8d24c
[PATCH v2 0/2] kbuild: Show Kconfig fragments in "help"
Hi,
This is my series to show *.config targets in the "help" target so these
various topics can be more easily discoverd.
v2:
- split .fragment from .config to hide "internal" fragments
- fix various typos
- avoid duplicate entries
v1: https://lore.kernel.org/all/20230824223606.never.762-k...@kernel.org
Thanks!
-Kees
Kees Cook (2):
kbuild: Show Kconfig fragments in "help"
kbuild: Split internal config targets from .config into .fragment
Makefile | 1 -
arch/arm/configs/dram_0x.config | 1 +
arch/arm/configs/dram_0xc000.config | 1 +
arch/arm/configs/dram_0xd000.config | 1 +
arch/arm/configs/lpae.config | 1 +
arch/arm64/configs/virt.config| 1 +
arch/powerpc/Makefile | 26 +--
.../{32-bit.config => 32-bit.fragment}| 1 +
arch/powerpc/configs/64-bit.config| 1 -
arch/powerpc/configs/64-bit.fragment | 2 ++
...{85xx-32bit.config => 85xx-32bit.fragment} | 1 +
...{85xx-64bit.config => 85xx-64bit.fragment} | 1 +
.../{85xx-hw.config => 85xx-hw.fragment} | 1 +
.../{85xx-smp.config => 85xx-smp.fragment}| 1 +
.../{86xx-hw.config => 86xx-hw.fragment} | 1 +
.../{86xx-smp.config => 86xx-smp.fragment}| 1 +
arch/powerpc/configs/altivec.config | 1 -
arch/powerpc/configs/altivec.fragment | 2 ++
arch/powerpc/configs/be.config| 1 -
arch/powerpc/configs/be.fragment | 2 ++
.../{book3s_32.config => book3s_32.fragment} | 1 +
...enet_base.config => corenet_base.fragment} | 1 +
arch/powerpc/configs/debug.config | 1 +
arch/powerpc/configs/disable-werror.config| 1 +
.../configs/{dpaa.config => dpaa.fragment}| 1 +
...mb-nonhw.config => fsl-emb-nonhw.fragment} | 1 +
.../configs/{guest.config => guest.fragment} | 1 +
arch/powerpc/configs/le.config| 1 -
arch/powerpc/configs/le.fragment | 2 ++
...85xx_base.config => mpc85xx_base.fragment} | 1 +
...86xx_base.config => mpc86xx_base.fragment} | 1 +
.../{ppc64le.config => ppc64le.fragment} | 1 +
arch/powerpc/configs/security.config | 4 ++-
arch/riscv/configs/32-bit.config | 1 +
arch/riscv/configs/64-bit.config | 1 +
arch/s390/configs/btf.config | 1 +
arch/s390/configs/kasan.config| 1 +
arch/x86/Makefile | 4 ---
arch/x86/configs/tiny.config | 2 ++
{kernel => arch/x86}/configs/x86_debug.config | 1 +
arch/x86/configs/xen.config | 2 ++
kernel/configs/debug.config | 2 ++
kernel/configs/kvm_guest.config | 1 +
kernel/configs/nopm.config| 2 ++
kernel/configs/rust.config| 1 +
kernel/configs/tiny-base.config | 1 -
kernel/configs/tiny-base.fragment | 2 ++
kernel/configs/tiny.config| 2 ++
kernel/configs/xen.config | 2 ++
scripts/Makefile.defconf | 12 ++---
scripts/kconfig/Makefile | 16 +---
51 files changed, 87 insertions(+), 32 deletions(-)
rename arch/powerpc/configs/{32-bit.config => 32-bit.fragment} (53%)
delete mode 100644 arch/powerpc/configs/64-bit.config
create mode 100644 arch/powerpc/configs/64-bit.fragment
rename arch/powerpc/configs/{85xx-32bit.config => 85xx-32bit.fragment} (76%)
rename arch/powerpc/configs/{85xx-64bit.config => 85xx-64bit.fragment} (78%)
rename arch/powerpc/configs/{85xx-hw.config => 85xx-hw.fragment} (98%)
rename arch/powerpc/configs/{85xx-smp.config => 85xx-smp.fragment} (59%)
rename arch/powerpc/configs/{86xx-hw.config => 86xx-hw.fragment} (98%)
rename arch/powerpc/configs/{86xx-smp.config => 86xx-smp.fragment} (58%)
delete mode 100644 arch/powerpc/configs/altivec.config
create mode 100644 arch/powerpc/configs/altivec.fragment
delete mode 100644 arch/powerpc/configs/be.config
create mode 100644 arch/powerpc/configs/be.fragment
rename arch/powerpc/configs/{book3s_32.config => book3s_32.fragment} (52%)
rename arch/powerpc/configs/{corenet_base.config => corenet_base.fragment}
(64%)
rename arch/powerpc/configs/{dpaa.config => dpaa.fragment} (80%)
rename arch/powerpc/configs/{fsl-emb-nonhw.config => fsl-emb-nonhw.fragment}
(98%)
rename arch/powerpc/configs/{guest.config => guest.fragment} (85%)
delete mode 100644 arch/powerpc/configs/le.config
create mode 100644 arch/powerpc/configs/le.fragment
rename arch/powerpc/configs/{mpc85xx_base.config => mpc85xx_base.fragment}
(94%)
rename arch/powerpc/configs/{mpc86xx_base.config => mpc86xx_base.fragment}
(86%)
rename arch/powerpc/configs/{ppc64le.config => ppc64le.frag
Re: [PATCH] kbuild: Show Kconfig fragments in "help"
On Fri, Aug 25, 2023 at 04:11:58PM +1000, Michael Ellerman wrote: > Kees Cook writes: > > Doing a "make help" would show only hard-coded Kconfig targets and > > depended on the archhelp target to include ".config" targets. There was > > nothing showing global kernel/configs/ targets. Solve this by walking > > the wildcard list and include them in the output, using the first comment > > line as the help text. > > > > Update all Kconfig fragments to include help text and adjust archhelp > > targets to avoid redundancy. > > > > Adds the following section to "help" target output: > > > > Configuration fragment targets (for enabling various Kconfig items): > > debug.config - Debugging for CI systems and finding regressions > > kvm_guest.config - Bootable as a KVM guest > > nopm.config - Disable Power Management > > rust.config - Enable Rust > > tiny-base.config - Minimal options for tiny systems > > tiny.config - Smallest possible kernel image > > x86_debug.config - Debugging options for tip tree testing > > xen.config - Bootable as a Xen guest > > tiny.config - x86-specific options for a small kernel image > > xen.config - x86-specific options for a Xen virtualization guest > > I think we need a way to opt some files out. > > It's a bit ugly on powerpc because there are so many fragments: > > Configuration fragment targets (for enabling various Kconfig items): > debug.config - Debugging for CI systems and finding regressions > kvm_guest.config - Bootable as a KVM guest > nopm.config - Disable Power Management > rust.config - Enable Rust > tiny-base.config - Minimal options for tiny systems > tiny.config - Smallest possible kernel image > x86_debug.config - Debugging options for tip tree testing > xen.config - Bootable as a Xen guest > 32-bit.config- Build a 32-bit image > 64-bit.config- Build a 64-bit image > 85xx-32bit.config- Build a 32-bit 85xx image > 85xx-64bit.config- Build a 64-bit 85xx image > 85xx-hw.config - Base hardware support for 86xx > 85xx-smp.config - Enable SMP on 85xx > 86xx-hw.config - Base hardware support for 86xx > 86xx-smp.config - Enable SMP on 86xx > altivec.config - Enable Altivec support > be.config- Enable Big Endian mode > book3s_32.config - Base support for Book3s > corenet_base.config - Base support for corenet > debug.config - Enable PowerPC specific debug options > disable-werror.config - Disable -Werror > dpaa.config - Base suppot for DPPA > fsl-emb-nonhw.config - Non-hardware options common to 85xx and corenet > guest.config - PowerPC specific virtualization guest options > kvm_guest.config - Bootable as a KVM guest > le.config- Enable Little Endian mode > mpc85xx_base.config - Base mpc85xxx support > mpc86xx_base.config - Base mpc85xxx support > ppc64le.config - Enable ppc64le mode > security.config - Common security options for PowerPC builds > > And some of those are not intended for use with "make foo.config", > they're used internally for generating our "normal" defconfigs and they > don't necessarily work on their own. > > Also I'd like to add more fragments in future, to the point where nearly > all our configs are generated by them. > > Can we have some way to differentiate fragments that are intended to be > used with "make foo.config" vs just being used internally to generate > other configs. > > The obvious thing would be to use a different suffix, eg. "foo.fragment" > for internal use fragments. That would require changing > merge_into_defconfig and merge_into_defconfig_override to not assume the > .config suffix, and update the users in arm, arm64 and powerpc. > > The other option would be to ignore .config files starting with eg. "_". > > + @$(foreach c, $(filter-out $(call configfiles,_*.config),$(call > configfiles,*.config)), \ > + printf " %-20s - %s\\n" \ > + $(shell basename $(c)) \ > + "$(subst # ,,$(shell grep -m1 '^# ' $(c)))";) > > Not sure which is preferable. Yeah, I wasn't too happy about some of these results. There seems to be three cases a fragment: - user-visible make target - used internally - arch-specific settings for a user-visible make target (redundant) Only the first should be visible. The trouble is that some user-visible targets are arch-specific. I think I like your idea of having both .config and .fragment... I'll give that a try and see how it looks. -- Kees Cook
Re: [PATCH] kbuild: Show Kconfig fragments in "help"
On Fri, Aug 25, 2023 at 07:44:06AM +0200, Nicolas Schier wrote: > On Thu, Aug 24, 2023 at 03:36:10PM -0700, Kees Cook wrote: > > Doing a "make help" would show only hard-coded Kconfig targets and > > depended on the archhelp target to include ".config" targets. There was > > nothing showing global kernel/configs/ targets. Solve this by walking > > the wildcard list and include them in the output, using the first comment > > line as the help text. > > [...] > > Thanks for that patch! Several times I found myself searching the tree > to find a specific kconfig fragment; I think you found a nice solution. > Two minor things below. > > [...] > > diff --git a/kernel/configs/tiny-base.config > > b/kernel/configs/tiny-base.config > > index 2f0e6bf6db2c..ac4d254abc3f 100644 > > --- a/kernel/configs/tiny-base.config > > +++ b/kernel/configs/tiny-base.config > > @@ -1 +1,2 @@ > > +# Minimal options for tiny systems > > CONFIG_EMBEDDED=y > > (just a note: Randy prepared a patch for removing CONFIG_EMBEDDED: > https://lore.kernel.org/linux-kbuild/20230816055010.31534-1-rdun...@infradead.org/) Ah yeah, I'll rebase this after the merge window, I guess... > > diff --git a/kernel/configs/tiny.config b/kernel/configs/tiny.config > > index 9f7d0835..ea643e8f7f14 100644 > > --- a/kernel/configs/tiny.config > > +++ b/kernel/configs/tiny.config > > @@ -1,3 +1,5 @@ > > +# Smallest possible kernel image > > For this fragment alone (not within 'tinyconfig'), "Size-optimize kernel > image" possibly fits better? Sounds good to me! > > diff --git a/scripts/kconfig/Makefile b/scripts/kconfig/Makefile > > index af1c96198f49..c523f24b504a 100644 > > --- a/scripts/kconfig/Makefile > > +++ b/scripts/kconfig/Makefile > > @@ -93,11 +93,11 @@ endif > > %_defconfig: $(obj)/conf > > $(Q)$< $(silent) --defconfig=arch/$(SRCARCH)/configs/$@ $(Kconfig) > > > > -configfiles=$(wildcard $(srctree)/kernel/configs/$@ > > $(srctree)/arch/$(SRCARCH)/configs/$@) > > +configfiles=$(wildcard $(srctree)/kernel/configs/$(1) > > $(srctree)/arch/$(SRCARCH)/configs/$(1)) > > > > %.config: $(obj)/conf > > - $(if $(call configfiles),, $(error No configuration exists for this > > target on this architecture)) > > - $(Q)$(CONFIG_SHELL) $(srctree)/scripts/kconfig/merge_config.sh -m > > .config $(configfiles) > > + $(if $(call configfiles,$@),, $(error No configuration exists for this > > target on this architecture)) > > + $(Q)$(CONFIG_SHELL) $(srctree)/scripts/kconfig/merge_config.sh -m > > .config $(call configfiles,$@) > > $(Q)$(MAKE) -f $(srctree)/Makefile olddefconfig > > > > PHONY += tinyconfig > > @@ -115,6 +115,7 @@ clean-files += tests/.cache > > > > # Help text used by make help > > help: > > + @echo 'Configuration targets:' > > @echo ' config - Update current config utilising a > > line-oriented program' > > @echo ' nconfig - Update current config utilising a ncurses > > menu based program' > > @echo ' menuconfig - Update current config utilising a menu > > based program' > > @@ -141,6 +142,12 @@ help: > > @echo 'default value without prompting' > > @echo ' tinyconfig - Configure the tiniest possible kernel' > > @echo ' testconfig - Run Kconfig unit tests (requires python3 > > and pytest)' > > + @echo '' > > + @echo 'Configuration fragment targets (for enabling various Kconfig > > items):' > > + @$(foreach c, $(call configfiles,*.config), \ > > + printf " %-20s - %s\\n" \ > > + $(shell basename $(c)) \ > > + "$(subst # ,,$(shell grep -m1 '^# ' $(c)))";) > > Better use '$(notdir $(c))` instead of forking a shell with > '$(shell basename $(c))'. Ah! Thank you. I *knew* there was a function for this but couldn't find it for some reason. :) -- Kees Cook
Re: [PATCH] kbuild: Show Kconfig fragments in "help"
On Fri, Aug 25, 2023 at 04:56:54AM +, Christophe Leroy wrote: > Le 25/08/2023 à 00:36, Kees Cook a écrit : > > +# Base hardware support for 86xx > > s/86xx/85xx > [...] Thanks for the typo fixes! I'll get these all fixed up. :) -- Kees Cook
Re: [PATCH] kbuild: Show Kconfig fragments in "help"
On Thu, Aug 24, 2023 at 05:04:02PM -0700, Randy Dunlap wrote: > Hi Kees, > > On 8/24/23 15:36, Kees Cook wrote: > > Doing a "make help" would show only hard-coded Kconfig targets and > > depended on the archhelp target to include ".config" targets. There was > > nothing showing global kernel/configs/ targets. Solve this by walking > > the wildcard list and include them in the output, using the first comment > > line as the help text. > > > > Update all Kconfig fragments to include help text and adjust archhelp > > targets to avoid redundancy. > > > > Adds the following section to "help" target output: > > > > Configuration fragment targets (for enabling various Kconfig items): > > debug.config - Debugging for CI systems and finding regressions > > kvm_guest.config - Bootable as a KVM guest > > nopm.config - Disable Power Management > > rust.config - Enable Rust > > tiny-base.config - Minimal options for tiny systems > > tiny.config - Smallest possible kernel image > > x86_debug.config - Debugging options for tip tree testing > > xen.config - Bootable as a Xen guest > > tiny.config - x86-specific options for a small kernel image > > xen.config - x86-specific options for a Xen virtualization guest > > ISTM that you are missing the "why" part of this change in the commit > description. I want to see what fragments are available without needing to know the source tree layout for their locations. :) > "make tinyconfig" is the real target here. The other (tiny.) files are just > implementation details. > We can't put all implementation details into help messages and it's not > difficult to find that the (tiny.) config files are merged to make the > final .config file. Yeah, this seems true for much of the ppc stuff to, as pointed out by mpe. I'll go answer there... -- Kees Cook
[PATCH] kbuild: Show Kconfig fragments in "help"
Doing a "make help" would show only hard-coded Kconfig targets and depended on the archhelp target to include ".config" targets. There was nothing showing global kernel/configs/ targets. Solve this by walking the wildcard list and include them in the output, using the first comment line as the help text. Update all Kconfig fragments to include help text and adjust archhelp targets to avoid redundancy. Adds the following section to "help" target output: Configuration fragment targets (for enabling various Kconfig items): debug.config - Debugging for CI systems and finding regressions kvm_guest.config - Bootable as a KVM guest nopm.config - Disable Power Management rust.config - Enable Rust tiny-base.config - Minimal options for tiny systems tiny.config - Smallest possible kernel image x86_debug.config - Debugging options for tip tree testing xen.config - Bootable as a Xen guest tiny.config - x86-specific options for a small kernel image xen.config - x86-specific options for a Xen virtualization guest Cc: Masahiro Yamada Cc: x...@kernel.org Cc: linux-arm-ker...@lists.infradead.org Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-ri...@lists.infradead.org Cc: linux-s...@vger.kernel.org Signed-off-by: Kees Cook --- Makefile | 1 - arch/arm/configs/dram_0x.config| 1 + arch/arm/configs/dram_0xc000.config| 1 + arch/arm/configs/dram_0xd000.config| 1 + arch/arm/configs/lpae.config | 1 + arch/arm64/configs/virt.config | 1 + arch/powerpc/configs/32-bit.config | 1 + arch/powerpc/configs/64-bit.config | 1 + arch/powerpc/configs/85xx-32bit.config | 1 + arch/powerpc/configs/85xx-64bit.config | 1 + arch/powerpc/configs/85xx-hw.config| 1 + arch/powerpc/configs/85xx-smp.config | 1 + arch/powerpc/configs/86xx-hw.config| 1 + arch/powerpc/configs/86xx-smp.config | 1 + arch/powerpc/configs/altivec.config| 1 + arch/powerpc/configs/be.config | 1 + arch/powerpc/configs/book3s_32.config | 1 + arch/powerpc/configs/corenet_base.config | 1 + arch/powerpc/configs/debug.config | 1 + arch/powerpc/configs/disable-werror.config | 1 + arch/powerpc/configs/dpaa.config | 1 + arch/powerpc/configs/fsl-emb-nonhw.config | 1 + arch/powerpc/configs/guest.config | 1 + arch/powerpc/configs/le.config | 1 + arch/powerpc/configs/mpc85xx_base.config | 1 + arch/powerpc/configs/mpc86xx_base.config | 1 + arch/powerpc/configs/ppc64le.config| 1 + arch/powerpc/configs/security.config | 4 +++- arch/riscv/configs/32-bit.config | 1 + arch/riscv/configs/64-bit.config | 1 + arch/s390/configs/btf.config | 1 + arch/s390/configs/kasan.config | 1 + arch/x86/Makefile | 4 arch/x86/configs/tiny.config | 2 ++ arch/x86/configs/xen.config| 2 ++ kernel/configs/debug.config| 2 ++ kernel/configs/kvm_guest.config| 1 + kernel/configs/nopm.config | 2 ++ kernel/configs/rust.config | 1 + kernel/configs/tiny-base.config| 1 + kernel/configs/tiny.config | 2 ++ kernel/configs/x86_debug.config| 1 + kernel/configs/xen.config | 2 ++ scripts/kconfig/Makefile | 13 ++--- 44 files changed, 59 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 4739c21a63e2..91c90ce8e0e3 100644 --- a/Makefile +++ b/Makefile @@ -1674,7 +1674,6 @@ help: @echo ' mrproper- Remove all generated files + config + various backup files' @echo ' distclean - mrproper + remove editor backup and patch files' @echo '' - @echo 'Configuration targets:' @$(MAKE) -f $(srctree)/scripts/kconfig/Makefile help @echo '' @echo 'Other generic targets:' diff --git a/arch/arm/configs/dram_0x.config b/arch/arm/configs/dram_0x.config index db96dcb420ce..4de3fde0de9a 100644 --- a/arch/arm/configs/dram_0x.config +++ b/arch/arm/configs/dram_0x.config @@ -1 +1,2 @@ +# DRAM base at 0x CONFIG_DRAM_BASE=0x diff --git a/arch/arm/configs/dram_0xc000.config b/arch/arm/configs/dram_0xc000.config index 343d5333d973..fdd4c7b1461e 100644 --- a/arch/arm/configs/dram_0xc000.config +++ b/arch/arm/configs/dram_0xc000.config @@ -1 +1,2 @@ +# DRAM base at 0xc000 CONFIG_DRAM_BASE=0xc000 diff --git a/arch/arm/configs/dram_0xd000.config b/arch/arm/configs/dram_0xd000.config index 61ba7045f8a1..54defdc8d24c 100644 --- a/arch/arm/configs/dram_0xd000.config +++ b/arch/arm/configs/dram_0xd000.con
Re: [PATCH] powerpc/ps3: refactor strncpy usage
On Wed, Aug 16, 2023 at 09:39:24PM +, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > `make_first_field()` should use similar implementation to `make_field()` > due to memcpy having more obvious behavior here. The end result yields > the same behavior as the previous `strncpy`-based implementation > including the NUL-padding. > > Link: > www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings[1] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-harden...@vger.kernel.org > Signed-off-by: Justin Stitt Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v2] powerpc/rtas_flash: allow user copy to flash block cache objects
On Thu, Aug 10, 2023 at 10:37:55PM -0500, Nathan Lynch via B4 Relay wrote:
> From: Nathan Lynch
>
> With hardened usercopy enabled (CONFIG_HARDENED_USERCOPY=y), using the
> /proc/powerpc/rtas/firmware_update interface to prepare a system
> firmware update yields a BUG():
>
> kernel BUG at mm/usercopy.c:102!
> Oops: Exception in kernel mode, sig: 5 [#1]
> LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
> Modules linked in:
> CPU: 0 PID: 2232 Comm: dd Not tainted 6.5.0-rc3+ #2
> Hardware name: IBM,8408-E8E POWER8E (raw) 0x4b0201 0xf04 of:IBM,FW860.50
> (SV860_146) hv:phyp pSeries
> NIP: c05991d0 LR: c05991cc CTR:
> REGS: c000148c76a0 TRAP: 0700 Not tainted (6.5.0-rc3+)
> MSR: 80029033 CR: 24002242 XER: 000c
> CFAR: c01fbd34 IRQMASK: 0
> [ ... GPRs omitted ... ]
> NIP [c05991d0] usercopy_abort+0xa0/0xb0
> LR [c05991cc] usercopy_abort+0x9c/0xb0
> Call Trace:
> [c000148c7940] [c05991cc] usercopy_abort+0x9c/0xb0 (unreliable)
> [c000148c79b0] [c0536814] __check_heap_object+0x1b4/0x1d0
> [c000148c79f0] [c0599080] __check_object_size+0x2d0/0x380
> [c000148c7a30] [c0045ed4] rtas_flash_write+0xe4/0x250
> [c000148c7a80] [c068a0fc] proc_reg_write+0xfc/0x160
> [c000148c7ab0] [c05a381c] vfs_write+0xfc/0x4e0
> [c000148c7b70] [c05a3e10] ksys_write+0x90/0x160
> [c000148c7bc0] [c002f2c8] system_call_exception+0x178/0x320
> [c000148c7e50] [c000d520] system_call_common+0x160/0x2c4
> --- interrupt: c00 at 0x7fff9f17e5e4
>
> The blocks of the firmware image are copied directly from user memory
> to objects allocated from flash_block_cache, so flash_block_cache must
> be created using kmem_cache_create_usercopy() to mark it safe for user
> access.
>
> Fixes: 6d07d1cd300f ("usercopy: Restrict non-usercopy caches to size 0")
> Signed-off-by: Nathan Lynch
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH RFC 2/3] powerpc/ps3: refactor strncpy usage attempt 2
On Fri, Aug 11, 2023 at 09:19:20PM +, Justin Stitt wrote: > This approach tries to use `make_field` inside of `make_first_field`. > This comes with some weird implementation as to get the same result we > need to first subtract `index` from the `make_field` result whilst being > careful with order of operations. We then have to add index back. I think for readability, it's better to avoid the function composition. The index subtraction undoes the earlier addition -- I say just leave it separate. i.e. I like option 1 of 3 the best. -Kees -- Kees Cook
Re: [PATCH] soc: fsl: qe: Replace all non-returning strlcpy with strscpy
On Tue, 23 May 2023 02:14:25 +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > No return values were used, so direct replacement is safe. > > [...] Applied, thanks! [1/1] soc: fsl: qe: Replace all non-returning strlcpy with strscpy (no commit info) Best regards, -- Kees Cook
Re: [PATCH] soc: fsl: qe: Replace all non-returning strlcpy with strscpy
On Mon, Jul 10, 2023 at 04:46:50PM +, Leo Li wrote: > > > > -Original Message- > > From: Azeem Shaikh > > Sent: Sunday, July 9, 2023 9:36 PM > > To: Kees Cook > > Cc: Qiang Zhao ; linux-harden...@vger.kernel.org; > > linuxppc-dev@lists.ozlabs.org; linux-ker...@vger.kernel.org; Leo Li > > ; linux-arm-ker...@lists.infradead.org > > Subject: Re: [PATCH] soc: fsl: qe: Replace all non-returning strlcpy with > > strscpy > > > > On Tue, May 23, 2023 at 1:20 PM Kees Cook > > wrote: > > > > > > On Tue, May 23, 2023 at 02:14:25AM +, Azeem Shaikh wrote: > > > > strlcpy() reads the entire source buffer first. > > > > This read may exceed the destination size limit. > > > > This is both inefficient and can lead to linear read overflows if a > > > > source string is not NUL-terminated [1]. > > > > In an effort to remove strlcpy() completely [2], replace > > > > strlcpy() here with strscpy(). > > > > No return values were used, so direct replacement is safe. > > > > > > > > [1] > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww > > > > > > w.kernel.org%2Fdoc%2Fhtml%2Flatest%2Fprocess%2Fdeprecated.html%23s > > tr > > > > > > lcpy=05%7C01%7Cleoyang.li%40nxp.com%7C11f9df1df1b5440e4ec108 > > db8 > > > > > > 0ee64de%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63824553360 > > 3780 > > > > > > 889%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu > > MzIiLCJB > > > > > > TiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=jcTy3IF37wqC1 > > MWsSuF > > > > %2F51Z1trQEMaow7BHkPSh3hzI%3D=0 > > > > [2] > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi > > > > > > thub.com%2FKSPP%2Flinux%2Fissues%2F89=05%7C01%7Cleoyang.li% > > 40nx > > > > > > p.com%7C11f9df1df1b5440e4ec108db80ee64de%7C686ea1d3bc2b4c6fa92cd > > 99c5 > > > > > > c301635%7C0%7C0%7C638245533603780889%7CUnknown%7CTWFpbGZsb3d > > 8eyJWIjo > > > > > > iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C30 > > 00%7 > > > > > > C%7C%7C=Blr0W1oYPIw5uDu7HqlEkU7xOuAo4bQNkk%2Bt%2BAuFqc > > s%3D > > > > erved=0 > > > > > > > > Signed-off-by: Azeem Shaikh > > > > > > Reviewed-by: Kees Cook > > > > > > > Friendly ping on this. > > Sorry for the late response. But I found some old discussions with the > conclusion to be not converting old users. Has this been changed later on? > https://lwn.net/Articles/659214/ The objection was with _mass_ conversions due to the risk of regressions being introduced in a way that makes it hard to revert/bisect, etc. We've being long on the road to doing these conversions to eliminate strlcpy(), which continues to get introduced into the kernel, even though it is risky. -- Kees Cook
Re: [PATCH v1 00/21] refactor Kconfig to consolidate KEXEC and CRASH options
On Mon, Jun 12, 2023 at 01:27:52PM -0400, Eric DeVolder wrote: > The Kconfig is refactored to consolidate KEXEC and CRASH options from > various arch//Kconfig files into new file kernel/Kconfig.kexec. This looks very nice! > [...] > - The boolean ARCH_HAS_ in effect allows the arch to determine > when the feature is allowed. Archs which don't have the feature > simply do not provide the corresponding ARCH_HAS_. > For each arch, where there previously were KEXEC and/or CRASH > options, these have been replaced with the corresponding boolean > ARCH_HAS_, and an appropriate def_bool statement. > > For example, if the arch supports KEXEC_FILE, then the > ARCH_HAS_KEXEC_FILE simply has a 'def_bool y'. This permits the > KEXEC_FILE option to be available. > > If the arch has a 'depends on' statement in its original coding > of the option, then that expression becomes part of the def_bool > expression. For example, arm64 had: > > config KEXEC > depends on PM_SLEEP_SMP > > and in this solution, this converts to: > > config ARCH_HAS_KEXEC > def_bool PM_SLEEP_SMP > > > - In order to account for the differences in the config coding for > the three common options, the ARCH_SUPPORTS_ is used. > This options has a 'depends on ' statement to couple it > to the main option, and from there can insert the differences > from the common option and the arch original coding of that option. > > For example, a few archs enable CRYPTO and CRYTPO_SHA256 for > KEXEC_FILE. These require a ARCH_SUPPORTS_KEXEC_FILE and > 'select CRYPTO' and 'select CRYPTO_SHA256' statements. Naming nit: "HAS" and "SUPPORTS" feel very similar, and looking at existing configs, "ARCH_SUPPORTS_..." is already used for doing this kind of bare "bool" management. e.g. see ARCH_SUPPORTS_INT128 It looks like you need to split "depends" and "select" so the options can be chosen separately from the "selectable" configs. How about naming this ARCH_SELECTS_, since that's what it's there for? -Kees -- Kees Cook
Re: [PATCH 1/1] arch:hexagon/powerpc: use KSYM_NAME_LEN in array size
On Mon, May 29, 2023 at 04:50:45PM +0200, Miguel Ojeda wrote: > Kees: what is the current stance on `[static N]` parameters? Something like: > > const char *kallsyms_lookup(unsigned long addr, > unsigned long *symbolsize, > unsigned long *offset, > - char **modname, char *namebuf); > + char **modname, char namebuf[static > KSYM_NAME_LEN]); > > makes the compiler complain about cases like these (even if trivial): > > arch/powerpc/xmon/xmon.c:1711:10: error: array argument is too small; > contains 128 elements, callee requires at least 512 > [-Werror,-Warray-bounds] > name = kallsyms_lookup(pc, , , NULL, tmpstr); > ^ ~~ > ./include/linux/kallsyms.h:86:29: note: callee declares array > parameter as static here > char **modname, char namebuf[static KSYM_NAME_LEN]); > ^ ~~ Wouldn't that be a good thing? (I.e. complain about the size mismatch?) > But I only see 2 files in the kernel using `[static N]` (from 2020 and > 2021). Should something else be used instead (e.g. `__counted_by`), > even if constexpr-sized?. Yeah, it seems pretty uncommon. I'd say traditionally arrays aren't based too often, rather structs containing them. But ultimately, yeah, everything could gain __counted_by and friends in the future. -- Kees Cook
Re: [PATCH] soc: fsl: qe: Replace all non-returning strlcpy with strscpy
On Tue, May 23, 2023 at 02:14:25AM +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > No return values were used, so direct replacement is safe. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] scsi: ibmvscsi: Replace all non-returning strlcpy with strscpy
On Wed, 17 May 2023 14:34:09 +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > No return values were used, so direct replacement is safe. > > [...] Applied to for-next/hardening, thanks! [1/1] scsi: ibmvscsi: Replace all non-returning strlcpy with strscpy https://git.kernel.org/kees/c/015f6618194e -- Kees Cook
Re: [PATCH] scsi: ibmvscsi: Replace all non-returning strlcpy with strscpy
On Wed, May 17, 2023 at 02:34:09PM +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > No return values were used, so direct replacement is safe. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v6 13/41] mm: Make pte_mkwrite() take a VMA
On Sat, Feb 18, 2023 at 01:14:05PM -0800, Rick Edgecombe wrote: > The x86 Control-flow Enforcement Technology (CET) feature includes a new > type of memory called shadow stack. This shadow stack memory has some > unusual properties, which requires some core mm changes to function > properly. > > One of these unusual properties is that shadow stack memory is writable, > but only in limited ways. These limits are applied via a specific PTE > bit combination. Nevertheless, the memory is writable, and core mm code > will need to apply the writable permissions in the typical paths that > call pte_mkwrite(). > > In addition to VM_WRITE, the shadow stack VMA's will have a flag denoting > that they are special shadow stack flavor of writable memory. So make > pte_mkwrite() take a VMA, so that the x86 implementation of it can know to > create regular writable memory or shadow stack memory. > > Apply the same changes for pmd_mkwrite() and huge_pte_mkwrite(). > > No functional change. > > Cc: linux-...@vger.kernel.org > Cc: linux-ker...@vger.kernel.org > Cc: linux-al...@vger.kernel.org > Cc: linux-snps-...@lists.infradead.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-c...@vger.kernel.org > Cc: linux-hexa...@vger.kernel.org > Cc: linux-i...@vger.kernel.org > Cc: loonga...@lists.linux.dev > Cc: linux-m...@lists.linux-m68k.org > Cc: Michal Simek > Cc: Dinh Nguyen > Cc: linux-m...@vger.kernel.org > Cc: openr...@lists.librecores.org > Cc: linux-par...@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Cc: linux-ri...@lists.infradead.org > Cc: linux-s...@vger.kernel.org > Cc: linux...@vger.kernel.org > Cc: sparcli...@vger.kernel.org > Cc: linux...@lists.infradead.org > Cc: xen-de...@lists.xenproject.org > Cc: linux-a...@vger.kernel.org > Cc: linux...@kvack.org > Tested-by: Pengfei Xu > Suggested-by: David Hildenbrand > Signed-off-by: Rick Edgecombe I'm not an arch maintainer, but it looks like a correct tree-wide refactor. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] powerpc/rtas: Replace one-element arrays with flexible arrays
On Fri, Jan 27, 2023 at 07:10:28AM -0600, Nathan Lynch wrote:
> Andrew Donnellan writes:
> > Using a one-element array as a fake flexible array is deprecated.
> >
> > Replace the one-element flexible arrays in rtas-types.h with C99 standard
> > flexible array members instead.
> >
> > This helps us move towards enabling -fstrict-flex-arrays=3 in future.
> >
> > Found using scripts/coccinelle/misc/flexible_array.cocci.
> >
> > Cc: Nathan Lynch
> > Cc: Leonardo Bras
> > Cc: linux-harden...@vger.kernel.org
> > Link: https://github.com/KSPP/linux/issues/21
> > Link: https://github.com/KSPP/linux/issues/79
> > Signed-off-by: Andrew Donnellan
> > ---
> > arch/powerpc/include/asm/rtas-types.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/rtas-types.h
> > b/arch/powerpc/include/asm/rtas-types.h
> > index 8df6235d64d1..40ec03a05c0b 100644
> > --- a/arch/powerpc/include/asm/rtas-types.h
> > +++ b/arch/powerpc/include/asm/rtas-types.h
> > @@ -44,7 +44,7 @@ struct rtas_error_log {
> > */
> > u8 byte3; /* General event or error*/
> > __be32 extended_log_length;/* length in bytes */
> > - unsigned char buffer[1]; /* Start of extended log */
> > + unsigned char buffer[]; /* Start of extended log */
> > /* Variable length. */
> > };
> >
> > @@ -85,7 +85,7 @@ struct rtas_ext_event_log_v6 {
> > /* that defines the format for */
> > /* the vendor specific log type */
> > /* Byte 16-end of log */
> > - u8 vendor_log[1]; /* Start of vendor specific log */
> > + u8 vendor_log[];/* Start of vendor specific log */
> > /* Variable length. */
> > };
>
> I see at least one place that consults the size of one of these structs,
> in get_pseries_errorlog():
>
> /* Check that we understand the format */
> if (ext_log_length < sizeof(struct rtas_ext_event_log_v6) || ...
>
> Don't all such sites need to be audited/adjusted for changes like this?
Yeah, I'd expect a binary comparison[1] before/after to catch things like
this. E.g. the following C files mention those structs:
arch/powerpc/platforms/pseries/io_event_irq.c
arch/powerpc/platforms/pseries/ras.c
arch/powerpc/kernel/rtasd.c
arch/powerpc/kernel/rtas.c
-Kees
[1] https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/
--
Kees Cook
Re: [PATCH mm-unstable v1 20/20] mm: rename FOLL_FORCE to FOLL_PTRACE
On Fri, Nov 18, 2022 at 12:09:02PM +0100, Peter Zijlstra wrote:
> On Wed, Nov 16, 2022 at 10:16:34AM -0800, Linus Torvalds wrote:
> > Following the history of it is a big of a mess, because there's a
> > number of renamings and re-organizations, but it seems to go back to
> > 2007 and commit b6a2fea39318 ("mm: variable length argument support").
>
> I went back and read parts of the discussions with Ollie, and the
> .force=1 thing just magically appeared one day when we were sending
> work-in-progress patches back and forth without mention of where it came
> from :-/
>
> And I certainly can't remember now..
>
> Looking at it now, I have the same reaction as both you and Kees had, it
> seems entirely superflous. So I'm all for trying to remove it.
Thanks for digging through the history! I've pushed the change to -next:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/execve=cd57e443831d8eeb083c7165bce195d886e216d4
--
Kees Cook
Re: [PATCH mm-unstable v1 20/20] mm: rename FOLL_FORCE to FOLL_PTRACE
On Thu, Nov 17, 2022 at 03:20:01PM -0800, Linus Torvalds wrote: > On Thu, Nov 17, 2022 at 2:58 PM Kees Cook wrote: > > > > Oh, er, why does get_arg_page() even need FOLL_FORCE? This is writing the > > new stack contents to the nascent brpm->vma, which was newly allocated > > with VM_STACK_FLAGS, which an arch can override, but they all appear to > > include > > VM_WRITE | VM_MAYWRITE. > > Yeah, it does seem entirely superfluous. > > It's been there since the very beginning (although in that original > commit b6a2fea39318 it was there as a '1' to the 'force' argument to > get_user_pages()). > > I *think* it can be just removed. But as long as it exists, it should > most definitely not be renamed to FOLL_PTRACE. > > There's a slight worry that it currently hides some other setup issue > that makes it matter, since it's been that way so long, but I can't > see what it is. My test system boots happily with it removed. I'll throw it into -next and see if anything melts... -- Kees Cook
Re: [PATCH mm-unstable v1 20/20] mm: rename FOLL_FORCE to FOLL_PTRACE
On Wed, Nov 16, 2022 at 10:16:34AM -0800, Linus Torvalds wrote: > There _are_ also small random cases too, like get_cmdline(). Maybe > that counts as ptrace, but the execve() case most definitely does not. Oh, er, why does get_arg_page() even need FOLL_FORCE? This is writing the new stack contents to the nascent brpm->vma, which was newly allocated with VM_STACK_FLAGS, which an arch can override, but they all appear to include VM_WRITE | VM_MAYWRITE. -- Kees Cook
Re: [PATCH v3 1/3] treewide: use get_random_u32_below() instead of deprecated function
On Thu, Nov 17, 2022 at 09:29:04PM +0100, Jason A. Donenfeld wrote: > This is a simple mechanical transformation done by: > > @@ > expression E; > @@ > - prandom_u32_max > + get_random_u32_below > (E) > > Reviewed-by: Greg Kroah-Hartman > Acked-by: Darrick J. Wong # for xfs > Reviewed-by: SeongJae Park # for damon > Reviewed-by: Jason Gunthorpe # for infiniband > Reviewed-by: Russell King (Oracle) # for arm > Acked-by: Ulf Hansson # for mmc > Signed-off-by: Jason A. Donenfeld Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v3 2/3] treewide: use get_random_u32_{above,below}() instead of manual loop
On Thu, Nov 17, 2022 at 09:29:05PM +0100, Jason A. Donenfeld wrote:
> These cases were done with this Coccinelle:
>
> @@
> expression E;
> identifier I;
> @@
> - do {
> ... when != I
> - I = get_random_u32();
> ... when != I
> - } while (I > E);
> + I = get_random_u32_below(E + 1);
>
> @@
> expression E;
> identifier I;
> @@
> - do {
> ... when != I
> - I = get_random_u32();
> ... when != I
> - } while (I >= E);
> + I = get_random_u32_below(E);
>
> @@
> expression E;
> identifier I;
> @@
> - do {
> ... when != I
> - I = get_random_u32();
> ... when != I
> - } while (I < E);
> + I = get_random_u32_above(E - 1);
>
> @@
> expression E;
> identifier I;
> @@
> - do {
> ... when != I
> - I = get_random_u32();
> ... when != I
> - } while (I <= E);
> + I = get_random_u32_above(E);
>
> @@
> identifier I;
> @@
> - do {
> ... when != I
> - I = get_random_u32();
> ... when != I
> - } while (!I);
> + I = get_random_u32_above(0);
>
> @@
> identifier I;
> @@
> - do {
> ... when != I
> - I = get_random_u32();
> ... when != I
> - } while (I == 0);
> + I = get_random_u32_above(0);
>
> @@
> expression E;
> @@
> - E + 1 + get_random_u32_below(U32_MAX - E)
> + get_random_u32_above(E)
>
> Reviewed-by: Greg Kroah-Hartman
> Signed-off-by: Jason A. Donenfeld
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH v3 3/3] treewide: use get_random_u32_inclusive() when possible
On Thu, Nov 17, 2022 at 09:29:06PM +0100, Jason A. Donenfeld wrote: > These cases were done with this Coccinelle: > > @@ > expression H; > expression L; > @@ > - (get_random_u32_below(H) + L) > + get_random_u32_inclusive(L, H + L - 1) > > @@ > expression H; > expression L; > expression E; > @@ > get_random_u32_inclusive(L, > H > - + E > - - E > ) > > @@ > expression H; > expression L; > expression E; > @@ > get_random_u32_inclusive(L, > H > - - E > - + E > ) > > @@ > expression H; > expression L; > expression E; > expression F; > @@ > get_random_u32_inclusive(L, > H > - - E > + F > - + E > ) > > @@ > expression H; > expression L; > expression E; > expression F; > @@ > get_random_u32_inclusive(L, > H > - + E > + F > - - E > ) > > And then subsequently cleaned up by hand, with several automatic cases > rejected if it didn't make sense contextually. > > Reviewed-by: Greg Kroah-Hartman > Reviewed-by: Jason Gunthorpe # for infiniband > Signed-off-by: Jason A. Donenfeld > --- > arch/x86/kernel/module.c | 2 +- > crypto/rsa-pkcs1pad.c | 2 +- > crypto/testmgr.c | 10 > drivers/bus/mhi/host/internal.h | 2 +- > drivers/dma-buf/st-dma-fence-chain.c | 2 +- > drivers/infiniband/core/cma.c | 2 +- > drivers/infiniband/hw/hns/hns_roce_ah.c | 5 ++-- > drivers/mtd/nand/raw/nandsim.c| 2 +- > drivers/net/wireguard/selftest/allowedips.c | 8 +++--- > .../broadcom/brcm80211/brcmfmac/p2p.c | 2 +- > .../net/wireless/intel/iwlwifi/mvm/mac-ctxt.c | 2 +- > fs/f2fs/segment.c | 6 ++--- > kernel/kcsan/selftest.c | 2 +- > lib/test_hexdump.c| 10 > lib/test_printf.c | 2 +- > lib/test_vmalloc.c| 6 ++--- > mm/kasan/kasan_test.c | 6 ++--- > mm/kfence/kfence_test.c | 2 +- > mm/swapfile.c | 5 ++-- > net/bluetooth/mgmt.c | 5 ++-- > net/core/pktgen.c | 25 --- > net/ipv4/tcp_input.c | 2 +- > net/ipv6/addrconf.c | 6 ++--- > net/netfilter/nf_nat_helper.c | 2 +- > net/xfrm/xfrm_state.c | 2 +- > 25 files changed, 56 insertions(+), 64 deletions(-) Even the diffstat agrees this is a nice clean-up. :) Reviewed-by: Kees Cook The only comment I have is that maybe these cases can just be left as-is with _below()? > - size_t len = get_random_u32_below(rs) + gs; > + size_t len = get_random_u32_inclusive(gs, rs + gs - 1); It seems like writing it in the form of base plus [0, limit) is clearer? size_t len = gs + get_random_u32_below(rs); But there is only a handful, so *shrug* All the others are much cleaner rewritten as _inclusive(). -- Kees Cook
Re: [PATCH v2 3/3] treewide: use get_random_u32_between() when possible
On November 16, 2022 4:43:54 PM PST, "Jason A. Donenfeld" wrote: >On Wed, Nov 16, 2022 at 04:31:18PM -0800, Kees Cook wrote: >> On Thu, Nov 17, 2022 at 01:03:14AM +0100, Jason A. Donenfeld wrote: >> > On Thu, Nov 17, 2022 at 12:55:47AM +0100, Jason A. Donenfeld wrote: >> > > 2) What to call it: >> > >- between I still like, because it mirrors "I'm thinking of a number >> > > between 1 and 10 and..." that everybody knows, >> > >- inclusive I guess works, but it's not a preposition, >> > >- bikeshed color #3? >> > >> > - between >> > - ranged >> > - spanning >> > >> > https://www.thefreedictionary.com/List-of-prepositions.htm >> > - amid >> > >> > Sigh, names. >> >> I think "inclusive" is best. > >I find it not very descriptive of what the function does. Is there one >you like second best? Or are you convinced they're all much much much >worse than "inclusive" that they shouldn't be considered? Right, I don't think any are sufficiently descriptive. "Incluisve" with two arguments seems unambiguous and complete to me. :) -- Kees Cook
Re: [PATCH v2 3/3] treewide: use get_random_u32_between() when possible
On Thu, Nov 17, 2022 at 01:03:14AM +0100, Jason A. Donenfeld wrote: > On Thu, Nov 17, 2022 at 12:55:47AM +0100, Jason A. Donenfeld wrote: > > 2) What to call it: > >- between I still like, because it mirrors "I'm thinking of a number > > between 1 and 10 and..." that everybody knows, > >- inclusive I guess works, but it's not a preposition, > >- bikeshed color #3? > > - between > - ranged > - spanning > > https://www.thefreedictionary.com/List-of-prepositions.htm > - amid > > Sigh, names. I think "inclusive" is best. The other words still don't provide unambiguous language. It's the language used in formal math, e.g. sigma-notation, etc. It's an adjective for "get random" (verb, noun). -- Kees Cook
Re: [PATCH v2 3/3] treewide: use get_random_u32_between() when possible
On Mon, Nov 14, 2022 at 05:45:58PM +0100, Jason A. Donenfeld wrote:
> - (get_random_u32_below(1024) + 1) * PAGE_SIZE;
> + get_random_u32_between(1, 1024 + 1) * PAGE_SIZE;
I really don't like "between". Can't this be named "inclusive" (and
avoid adding 1 everywhere, which seems ugly), or at least named
something less ambiguous?
> - n = get_random_u32_below(100) + 1;
> + n = get_random_u32_between(1, 101);
Because I find this much less readable. "Below 100" is clear: 0-99
inclusive, plus 1, so 1-100 inclusive. "Between 1 and 101" is not obvious
to me to mean: 1-100 inclusive.
These seem so much nicer:
get_random_u32_inclusive(1, 1024)
get_random_u32_inclusive(1, 100)
--
Kees Cook
Re: [PATCH v5 0/7] treewide cleanup of random integer usage
On Fri, Oct 07, 2022 at 11:53:52PM -0600, Jason A. Donenfeld wrote:
> This is a five part treewide cleanup of random integer handling. The
> rules for random integers are:
Reviewing the delta between of my .cocci rules and your v5, everything
matches, except for get_random_int() conversions for files not in
your tree:
diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c
b/drivers/gpu/drm/tests/drm_buddy_test.c
index 7a2b2d6bc3fe..62f69589a72d 100644
--- a/drivers/gpu/drm/tests/drm_buddy_test.c
+++ b/drivers/gpu/drm/tests/drm_buddy_test.c
@@ -729,7 +729,7 @@ static void drm_test_buddy_alloc_limit(struct kunit *test)
static int drm_buddy_init_test(struct kunit *test)
{
while (!random_seed)
- random_seed = get_random_int();
+ random_seed = get_random_u32();
return 0;
}
diff --git a/drivers/gpu/drm/tests/drm_mm_test.c
b/drivers/gpu/drm/tests/drm_mm_test.c
index 659d1af4dca7..c4b66eeae203 100644
--- a/drivers/gpu/drm/tests/drm_mm_test.c
+++ b/drivers/gpu/drm/tests/drm_mm_test.c
@@ -2212,7 +2212,7 @@ static void drm_test_mm_color_evict_range(struct kunit
*test)
static int drm_mm_init_test(struct kunit *test)
{
while (!random_seed)
- random_seed = get_random_int();
+ random_seed = get_random_u32();
return 0;
}
So, I guess I mean to say that "prandom: remove unused functions" is
going to cause some pain. :) Perhaps don't push that to -next, and do a
final pass next merge window to catch any new stuff, and then send those
updates and the removal before -rc1 closes?
--
Kees Cook
Re: [PATCH v4 2/6] treewide: use prandom_u32_max() when possible
[resending because I failed to CC]
On October 7, 2022 7:21:28 PM PDT, "Jason A. Donenfeld" wrote:
>On Fri, Oct 07, 2022 at 03:47:44PM -0700, Kees Cook wrote:
>> On Fri, Oct 07, 2022 at 12:01:03PM -0600, Jason A. Donenfeld wrote:
>> > Rather than incurring a division or requesting too many random bytes for
>> > the given range, use the prandom_u32_max() function, which only takes
>> > the minimum required bytes from the RNG and avoids divisions.
>>
>> I actually meant splitting the by-hand stuff by subsystem, but nearly
>> all of these can be done mechanically too, so it shouldn't be bad. Notes
>> below...
>
>Oh, cool, more coccinelle. You're basically giving me a class on these
>recipes. Much appreciated.
You're welcome! This was a fun exercise. :)
>
>> > [...]
>> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> > index 92bcc1768f0b..87203429f802 100644
>> > --- a/arch/arm64/kernel/process.c
>> > +++ b/arch/arm64/kernel/process.c
>> > @@ -595,7 +595,7 @@ unsigned long __get_wchan(struct task_struct *p)
>> > unsigned long arch_align_stack(unsigned long sp)
>> > {
>> >if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
>> > - sp -= get_random_int() & ~PAGE_MASK;
>> > + sp -= prandom_u32_max(PAGE_SIZE);
>> >return sp & ~0xf;
>> > }
>> >
>>
>> @mask@
>> expression MASK;
>> @@
>>
>> - (get_random_int() & ~(MASK))
>> + prandom_u32_max(MASK)
>
>Not quite! PAGE_MASK != PAGE_SIZE. In this case, things get a litle
>more complicated where you can do:
>
>get_random_int() & MASK == prandom_u32_max(MASK + 1)
>*only if all the top bits of MASK are set* That is, if MASK one less
Oh whoops! Yes, right, I totally misread SIZE as MASK.
>than a power of two. Or if MASK & (MASK + 1) == 0.
>
>(If those top bits aren't set, you can technically do
>prandom_u32_max(MASK >> n + 1) << n. That'd be a nice thing to work out.
>But yeesh, maybe a bit much for the time being and probably a bit beyond
>coccinelle.)
>
>This case here, though, is a bit more special, where we can just rely on
>an obvious given kernel identity. Namely, PAGE_MASK == ~(PAGE_SIZE - 1).
>So ~PAGE_MASK == PAGE_SIZE - 1.
>So get_random_int() & ~PAGE_MASK == prandom_u32_max(PAGE_SIZE - 1 + 1).
>So get_random_int() & ~PAGE_MASK == prandom_u32_max(PAGE_SIZE).
>
>And most importantly, this makes the code more readable, since everybody
>knows what bounding by PAGE_SIZE means, where as what on earth is
>happening with the &~PAGE_MASK thing. So it's a good change. I'll try to
>teach coccinelle about that special case.
Yeah, it should be possible to just check for the literal.
>
>
>
>> > diff --git a/arch/loongarch/kernel/vdso.c b/arch/loongarch/kernel/vdso.c
>> > index f32c38abd791..8c9826062652 100644
>> > --- a/arch/loongarch/kernel/vdso.c
>> > +++ b/arch/loongarch/kernel/vdso.c
>> > @@ -78,7 +78,7 @@ static unsigned long vdso_base(void)
>> >unsigned long base = STACK_TOP;
>> >
>> >if (current->flags & PF_RANDOMIZE) {
>> > - base += get_random_int() & (VDSO_RANDOMIZE_SIZE - 1);
>> > + base += prandom_u32_max(VDSO_RANDOMIZE_SIZE);
>> >base = PAGE_ALIGN(base);
>> >}
>> >
>>
>> @minus_one@
>> expression FULL;
>> @@
>>
>> - (get_random_int() & ((FULL) - 1)
>> + prandom_u32_max(FULL)
>
>Ahh, well, okay, this is the example I mentioned above. Only works if
>FULL is saturated. Any clever way to get coccinelle to prove that? Can
>it look at the value of constants?
I'm not sure if Cocci will do that without a lot of work. The literals trick I
used below would need a lot of fanciness. :)
>
>>
>> > diff --git a/arch/parisc/kernel/vdso.c b/arch/parisc/kernel/vdso.c
>> > index 63dc44c4c246..47e5960a2f96 100644
>> > --- a/arch/parisc/kernel/vdso.c
>> > +++ b/arch/parisc/kernel/vdso.c
>> > @@ -75,7 +75,7 @@ int arch_setup_additional_pages(struct linux_binprm
>> > *bprm,
>> >
>> >map_base = mm->mmap_base;
>> >if (current->flags & PF_RANDOMIZE)
>> > - map_base -= (get_random_int() & 0x1f) * PAGE_SIZE;
>> > + map_base -= prandom_u32_max(0x20) * PAGE_SIZE;
>> >
>> >vdso_text_start = get_unmapped_area(NULL, map_base, vdso_text_len, 0,
>> > 0);
>> >
>>
>> These are more
Re: [PATCH v4 2/6] treewide: use prandom_u32_max() when possible
(ngroups);
> for (i = 0; i < ngroups; i++) {
> group = (parent_group + i) % ngroups;
> desc = ext2_get_group_desc (sb, group, NULL);
Okay, that one is too much for me -- checking that group is never used
after the assignment removal is likely possible, but beyond my cocci
know-how. :)
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index f73e5eb43eae..36d5bc595cc2 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -463,10 +463,9 @@ static int find_group_orlov(struct super_block *sb,
> struct inode *parent,
> hinfo.hash_version = DX_HASH_HALF_MD4;
> hinfo.seed = sbi->s_hash_seed;
> ext4fs_dirhash(parent, qstr->name, qstr->len, );
> - grp = hinfo.hash;
> + parent_group = hinfo.hash % ngroups;
> } else
> - grp = prandom_u32();
> - parent_group = (unsigned)grp % ngroups;
> + parent_group = prandom_u32_max(ngroups);
> for (i = 0; i < ngroups; i++) {
> g = (parent_group + i) % ngroups;
> get_orlov_stats(sb, g, flex_size, );
Much less easy mechanically. :)
> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> index 0927f44cd478..41a0321f641a 100644
> --- a/lib/test_hexdump.c
> +++ b/lib/test_hexdump.c
> @@ -208,7 +208,7 @@ static void __init test_hexdump_overflow(size_t buflen,
> size_t len,
> static void __init test_hexdump_overflow_set(size_t buflen, bool ascii)
> {
> unsigned int i = 0;
> - int rs = (prandom_u32_max(2) + 1) * 16;
> + int rs = prandom_u32_max(2) + 1 * 16;
>
> do {
> int gs = 1 << i;
This looks wrong. Cocci says:
- int rs = (get_random_int() % 2 + 1) * 16;
+ int rs = (prandom_u32_max(2) + 1) * 16;
> diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
> index 4f2f2d1bac56..56ffaa8dd3f6 100644
> --- a/lib/test_vmalloc.c
> +++ b/lib/test_vmalloc.c
> @@ -151,9 +151,7 @@ static int random_size_alloc_test(void)
> int i;
>
> for (i = 0; i < test_loop_count; i++) {
> - n = prandom_u32();
> - n = (n % 100) + 1;
> -
> + n = prandom_u32_max(n % 100) + 1;
> p = vmalloc(n * PAGE_SIZE);
>
> if (!p)
This looks wrong. Cocci says:
- n = prandom_u32();
- n = (n % 100) + 1;
+ n = prandom_u32_max(100) + 1;
> @@ -293,16 +291,12 @@ pcpu_alloc_test(void)
> return -1;
>
> for (i = 0; i < 35000; i++) {
> - unsigned int r;
> -
> - r = prandom_u32();
> - size = (r % (PAGE_SIZE / 4)) + 1;
> + size = prandom_u32_max(PAGE_SIZE / 4) + 1;
>
> /*
>* Maximum PAGE_SIZE
>*/
> - r = prandom_u32();
> - align = 1 << ((r % 11) + 1);
> + align = 1 << (prandom_u32_max(11) + 1);
>
> pcpu[i] = __alloc_percpu(size, align);
> if (!pcpu[i])
> @@ -393,14 +387,11 @@ static struct test_driver {
>
> static void shuffle_array(int *arr, int n)
> {
> - unsigned int rnd;
> int i, j;
>
> for (i = n - 1; i > 0; i--) {
> - rnd = prandom_u32();
> -
> /* Cut the range. */
> - j = rnd % i;
> + j = prandom_u32_max(i);
>
> /* Swap indexes. */
> swap(arr[i], arr[j]);
Yup, agrees with Cocci on these.
--
Kees Cook
Re: [PATCH v3 3/5] treewide: use get_random_u32() when possible
On Fri, Oct 07, 2022 at 08:07:58AM -0600, Jason A. Donenfeld wrote:
> On Fri, Oct 07, 2022 at 04:57:24AM +, Christophe Leroy wrote:
> >
> >
> > Le 07/10/2022 à 01:36, Jason A. Donenfeld a écrit :
> > > On 10/6/22, Christophe Leroy wrote:
> > >>
> > >>
> > >> Le 06/10/2022 à 19:31, Christophe Leroy a écrit :
> > >>>
> > >>>
> > >>> Le 06/10/2022 à 19:24, Jason A. Donenfeld a écrit :
> > >>>> Hi Christophe,
> > >>>>
> > >>>> On Thu, Oct 6, 2022 at 11:21 AM Christophe Leroy
> > >>>> wrote:
> > >>>>> Le 06/10/2022 à 18:53, Jason A. Donenfeld a écrit :
> > >>>>>> The prandom_u32() function has been a deprecated inline wrapper
> > >>>>>> around
> > >>>>>> get_random_u32() for several releases now, and compiles down to the
> > >>>>>> exact same code. Replace the deprecated wrapper with a direct call to
> > >>>>>> the real function. The same also applies to get_random_int(), which
> > >>>>>> is
> > >>>>>> just a wrapper around get_random_u32().
> > >>>>>>
> > >>>>>> Reviewed-by: Kees Cook
> > >>>>>> Acked-by: Toke Høiland-Jørgensen # for sch_cake
> > >>>>>> Acked-by: Chuck Lever # for nfsd
> > >>>>>> Reviewed-by: Jan Kara # for ext4
> > >>>>>> Signed-off-by: Jason A. Donenfeld
> > >>>>>> ---
> > >>>>>
> > >>>>>> diff --git a/arch/powerpc/kernel/process.c
> > >>>>>> b/arch/powerpc/kernel/process.c
> > >>>>>> index 0fbda89cd1bb..9c4c15afbbe8 100644
> > >>>>>> --- a/arch/powerpc/kernel/process.c
> > >>>>>> +++ b/arch/powerpc/kernel/process.c
> > >>>>>> @@ -2308,6 +2308,6 @@ void notrace __ppc64_runlatch_off(void)
> > >>>>>> unsigned long arch_align_stack(unsigned long sp)
> > >>>>>> {
> > >>>>>> if (!(current->personality & ADDR_NO_RANDOMIZE) &&
> > >>>>>> randomize_va_space)
> > >>>>>> - sp -= get_random_int() & ~PAGE_MASK;
> > >>>>>> + sp -= get_random_u32() & ~PAGE_MASK;
> > >>>>>> return sp & ~0xf;
> > >>>>>
> > >>>>> Isn't that a candidate for prandom_u32_max() ?
> > >>>>>
> > >>>>> Note that sp is deemed to be 16 bytes aligned at all time.
> > >>>>
> > >>>> Yes, probably. It seemed non-trivial to think about, so I didn't. But
> > >>>> let's see here... maybe it's not too bad:
> > >>>>
> > >>>> If PAGE_MASK is always ~(PAGE_SIZE-1), then ~PAGE_MASK is
> > >>>> (PAGE_SIZE-1), so prandom_u32_max(PAGE_SIZE) should yield the same
> > >>>> thing? Is that accurate? And holds across platforms (this comes up a
> > >>>> few places)? If so, I'll do that for a v4.
> > >>>>
> > >>>
> > >>> On powerpc it is always (from arch/powerpc/include/asm/page.h) :
> > >>>
> > >>> /*
> > >>>* Subtle: (1 << PAGE_SHIFT) is an int, not an unsigned long. So if we
> > >>>* assign PAGE_MASK to a larger type it gets extended the way we want
> > >>>* (i.e. with 1s in the high bits)
> > >>>*/
> > >>> #define PAGE_MASK (~((1 << PAGE_SHIFT) - 1))
> > >>>
> > >>> #define PAGE_SIZE(1UL << PAGE_SHIFT)
> > >>>
> > >>>
> > >>> So it would work I guess.
> > >>
> > >> But taking into account that sp must remain 16 bytes aligned, would it
> > >> be better to do something like ?
> > >>
> > >> sp -= prandom_u32_max(PAGE_SIZE >> 4) << 4;
> > >>
> > >> return sp;
> > >
> > > Does this assume that sp is already aligned at the beginning of the
> > > function? I'd assume from the function's name that this isn't the
> > > case?
> >
> > Ah you are right, I overlooked it.
>
> So I think to stay on the safe side, I'm going to go with
> `prandom_u32_max(PAGE_SIZE)`. Sound good?
Given these kinds of less mechanical changes, it may make sense to split
these from the "trivial" conversions in a treewide patch. The chance of
needing a revert from the simple 1:1 conversions is much lower than the
need to revert by-hand changes.
The Cocci script I suggested in my v1 review gets 80% of the first
patch, for example.
--
Kees Cook
Re: [PATCH v1 0/5] treewide cleanup of random integer usage
On Wed, Oct 05, 2022 at 09:55:43PM -0700, Kees Cook wrote: > If any of the subsystems ask you to break this up (I hope not), I've got > this[1], which does a reasonable job of splitting a commit up into > separate commits for each matching subsystem. [1] https://github.com/kees/kernel-tools/blob/trunk/split-on-maintainer -- Kees Cook
Re: [PATCH v1 0/5] treewide cleanup of random integer usage
On Wed, Oct 05, 2022 at 11:48:39PM +0200, Jason A. Donenfeld wrote: > Hi folks, > > This is a five part treewide cleanup of random integer handling. The > rules for random integers are: > > - If you want a secure or an insecure random u64, use get_random_u64(). > - If you want a secure or an insecure random u32, use get_random_u32(). > * The old function prandom_u32() has been deprecated for a while now > and is just a wrapper around get_random_u32(). > - If you want a secure or an insecure random u16, use get_random_u16(). > - If you want a secure or an insecure random u8, use get_random_u8(). > - If you want secure or insecure random bytes, use get_random_bytes(). > * The old function prandom_bytes() has been deprecated for a while now > and has long been a wrapper around get_random_bytes(). > - If you want a non-uniform random u32, u16, or u8 bounded by a certain > open interval maximum, use prandom_u32_max(). > * I say "non-uniform", because it doesn't do any rejection sampling or > divisions. Hence, it stays within the prandom_* namespace. > > These rules ought to be applied uniformly, so that we can clean up the > deprecated functions, and earn the benefits of using the modern > functions. In particular, in addition to the boring substitutions, this > patchset accomplishes a few nice effects: > > - By using prandom_u32_max() with an upper-bound that the compiler can > prove at compile-time is ≤65536 or ≤256, internally get_random_u16() > or get_random_u8() is used, which wastes fewer batched random bytes, > and hence has higher throughput. > > - By using prandom_u32_max() instead of %, when the upper-bound is not a > constant, division is still avoided, because prandom_u32_max() uses > a faster multiplication-based trick instead. > > - By using get_random_u16() or get_random_u8() in cases where the return > value is intended to indeed be a u16 or a u8, we waste fewer batched > random bytes, and hence have higher throughput. > > So, based on those rules and benefits from following them, this patchset > breaks down into the following five steps: > > 1) Replace `prandom_u32() % max` and variants thereof with >prandom_u32_max(max). > > 2) Replace `(type)get_random_u32()` and variants thereof with >get_random_u16() or get_random_u8(). I took the pains to actually >look and see what every lvalue type was across the entire tree. > > 3) Replace remaining deprecated uses of prandom_u32() with >get_random_u32(). > > 4) Replace remaining deprecated uses of prandom_bytes() with >get_random_bytes(). > > 5) Remove the deprecated and now-unused prandom_u32() and >prandom_bytes() inline wrapper functions. > > I was thinking of taking this through my random.git tree (on which this > series is currently based) and submitting it near the end of the merge > window, or waiting for the very end of the 6.1 cycle when there will be > the fewest new patches brewing. If somebody with some treewide-cleanup > experience might share some wisdom about what the best timing usually > winds up being, I'm all ears. It'd be nice to capture some (all?) of the above somewhere. Perhaps just a massive comment in the header? > I've CC'd get_maintainers.pl, which is a pretty big list. Probably some > portion of those are going to bounce, too, and everytime you reply to > this thread, you'll have to deal with a bunch of bounces coming > immediately after. And a recipient list this big will probably dock my > email domain's spam reputation, at least temporarily. Sigh. I think > that's just how it goes with treewide cleanups though. Again, let me > know if I'm doing it wrong. I usually stick to just mailing lists and subsystem maintainers. If any of the subsystems ask you to break this up (I hope not), I've got this[1], which does a reasonable job of splitting a commit up into separate commits for each matching subsystem. Showing that a treewide change can be reproduced mechanically helps with keeping it together as one bit treewide patch, too, I've found. :) Thank you for the cleanup! The "u8 rnd = get_random_u32()" in the tree has bothered me for a lng time. -Kees -- Kees Cook
Re: [PATCH v1 4/5] treewide: use get_random_bytes when possible
On Wed, Oct 05, 2022 at 11:48:43PM +0200, Jason A. Donenfeld wrote: > The prandom_bytes() function has been a deprecated inline wrapper around > get_random_bytes() for several releases now, and compiles down to the > exact same code. Replace the deprecated wrapper with a direct call to > the real function. > > Signed-off-by: Jason A. Donenfeld Global search/replace matches. :) Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v1 4/5] treewide: use get_random_bytes when possible
On Wed, Oct 05, 2022 at 11:48:43PM +0200, Jason A. Donenfeld wrote: > The prandom_bytes() function has been a deprecated inline wrapper around > get_random_bytes() for several releases now, and compiles down to the > exact same code. Replace the deprecated wrapper with a direct call to > the real function. Global search/replace matches. :) Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v1 5/5] prandom: remove unused functions
On Wed, Oct 05, 2022 at 11:48:44PM +0200, Jason A. Donenfeld wrote: > With no callers left of prandom_u32() and prandom_bytes(), remove these > deprecated wrappers. > > Signed-off-by: Jason A. Donenfeld Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v1 2/5] treewide: use get_random_{u8,u16}() when possible
On Wed, Oct 05, 2022 at 11:48:41PM +0200, Jason A. Donenfeld wrote:
> Rather than truncate a 32-bit value to a 16-bit value or an 8-bit value,
> simply use the get_random_{u8,u16}() functions, which are faster than
> wasting the additional bytes from a 32-bit value.
>
> Signed-off-by: Jason A. Donenfeld
Same question about "mechanism of transformation".
> diff --git a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> index ddfe9208529a..ac452a0111a9 100644
> --- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> +++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
> @@ -1467,7 +1467,7 @@ static void make_established(struct sock *sk, u32
> snd_isn, unsigned int opt)
> tp->write_seq = snd_isn;
> tp->snd_nxt = snd_isn;
> tp->snd_una = snd_isn;
> - inet_sk(sk)->inet_id = prandom_u32();
> + inet_sk(sk)->inet_id = get_random_u16();
> assign_rxopt(sk, opt);
>
> if (tp->rcv_wnd > (RCV_BUFSIZ_M << 10))
This one I had to go look at -- inet_id is u16, so yeah. :)
> diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
> index 56ffaa8dd3f6..0131ed2cd1bd 100644
> --- a/lib/test_vmalloc.c
> +++ b/lib/test_vmalloc.c
> @@ -80,7 +80,7 @@ static int random_size_align_alloc_test(void)
> int i;
>
> for (i = 0; i < test_loop_count; i++) {
> - rnd = prandom_u32();
> + rnd = get_random_u8();
>
> /*
>* Maximum 1024 pages, if PAGE_SIZE is 4096.
This wasn't obvious either, but it looks like it's because it never
consumes more than u8?
> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> index 7981be526f26..57c7686ac485 100644
> --- a/net/netfilter/nf_nat_core.c
> +++ b/net/netfilter/nf_nat_core.c
> @@ -468,7 +468,7 @@ static void nf_nat_l4proto_unique_tuple(struct
> nf_conntrack_tuple *tuple,
> if (range->flags & NF_NAT_RANGE_PROTO_OFFSET)
> off = (ntohs(*keyptr) - ntohs(range->base_proto.all));
> else
> - off = prandom_u32();
> + off = get_random_u16();
>
> attempts = range_size;
Yup, u16 off;
> diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c
> index 2829455211f8..7eb70acb4d58 100644
> --- a/net/sched/sch_sfb.c
> +++ b/net/sched/sch_sfb.c
> @@ -379,7 +379,7 @@ static int sfb_enqueue(struct sk_buff *skb, struct Qdisc
> *sch,
> goto enqueue;
> }
>
> - r = prandom_u32() & SFB_MAX_PROB;
> + r = get_random_u16() & SFB_MAX_PROB;
>
> if (unlikely(r < p_min)) {
> if (unlikely(p_min > SFB_MAX_PROB / 2)) {
include/uapi/linux/pkt_sched.h:#define SFB_MAX_PROB 0x
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH v1 1/5] treewide: use prandom_u32_max() when possible
On Wed, Oct 05, 2022 at 11:48:40PM +0200, Jason A. Donenfeld wrote:
> Rather than incurring a division or requesting too many random bytes for
> the given range, use the prandom_u32_max() function, which only takes
> the minimum required bytes from the RNG and avoids divisions.
Yes please!
Since this is a treewide patch, it's helpful for (me at least) doing
reviews to detail the mechanism of the transformation.
e.g. I imagine this could be done with something like Coccinelle and
@no_modulo@
expression E;
@@
- (prandom_u32() % (E))
+ prandom_u32_max(E)
> diff --git a/drivers/mtd/ubi/debug.h b/drivers/mtd/ubi/debug.h
> index 118248a5d7d4..4236c799a47c 100644
> --- a/drivers/mtd/ubi/debug.h
> +++ b/drivers/mtd/ubi/debug.h
> @@ -73,7 +73,7 @@ static inline int ubi_dbg_is_bgt_disabled(const struct
> ubi_device *ubi)
> static inline int ubi_dbg_is_bitflip(const struct ubi_device *ubi)
> {
> if (ubi->dbg.emulate_bitflips)
> - return !(prandom_u32() % 200);
> + return !(prandom_u32_max(200));
> return 0;
> }
>
Because some looks automated (why the parens?)
> @@ -393,14 +387,11 @@ static struct test_driver {
>
> static void shuffle_array(int *arr, int n)
> {
> - unsigned int rnd;
> int i, j;
>
> for (i = n - 1; i > 0; i--) {
> - rnd = prandom_u32();
> -
> /* Cut the range. */
> - j = rnd % i;
> + j = prandom_u32_max(i);
>
> /* Swap indexes. */
> swap(arr[i], arr[j]);
And some by hand. :)
Reviewed-by: Kees Cook
--
Kees Cook
Re: [powerpc] Build failure include/linux/compiler_types.h __alloc_size__ (next-20220928)
On Thu, Sep 29, 2022 at 11:49:28AM +0530, Sachin Sant wrote: > Linux-next 6.0.0-rc7-next-20220928 fails to build on powerpc with > following error: > > make -j 17 -s && make modules_install && make install > In file included from : > ./include/linux/percpu.h: In function '__alloc_reserved_percpu': > ././include/linux/compiler_types.h:279:30: error: expected declaration > specifiers before '__alloc_size__' > #define __alloc_size(x, ...) __alloc_size__(x, ## __VA_ARGS__) __malloc > ^~ Apologies for the breakage! This should be fixed by: https://lore.kernel.org/lkml/20220929081642.1932200-1-keesc...@chromium.org -- Kees Cook
Re: [powerpc] memcpy warning drivers/scsi/scsi_transport_fc.c:581 (next-20220921)
memcpy(>event_data, data_buf, data_len);
+ memcpy(event->event_data_flex, data_buf, data_len);
+ padding = len - offsetof(typeof(*event), event_data_flex) - data_len;
+ memset(event->event_data_flex + data_len, 0, padding);
nlmsg_multicast(scsi_nl_sock, skb, 0, SCSI_NL_GRP_FC_EVENTS,
GFP_KERNEL);
diff --git a/include/uapi/scsi/scsi_netlink_fc.h
b/include/uapi/scsi/scsi_netlink_fc.h
index 7535253f1a96..b46e9cbeb001 100644
--- a/include/uapi/scsi/scsi_netlink_fc.h
+++ b/include/uapi/scsi/scsi_netlink_fc.h
@@ -35,7 +35,7 @@
* FC Transport Broadcast Event Message :
* FC_NL_ASYNC_EVENT
*
- * Note: if Vendor Unique message, _data will be start of
+ * Note: if Vendor Unique message, event_data_flex will be start of
* vendor unique payload, and the length of the payload is
* per event_datalen
*
@@ -50,7 +50,10 @@ struct fc_nl_event {
__u16 event_datalen;
__u32 event_num;
__u32 event_code;
- __u32 event_data;
+ union {
+ __u32 event_data;
+ __DECLARE_FLEX_ARRAY(__u8, event_data_flex);
+ };
} __attribute__((aligned(sizeof(__u64;
--
Kees Cook
Re: [PATCH][next] powerpc/xmon: Fix -Wswitch-unreachable warning in bpt_cmds
On Fri, Sep 16, 2022 at 03:15:04PM +0100, Gustavo A. R. Silva wrote:
> When building with automatic stack variable initialization, GCC 12
> complains about variables defined outside of switch case statements.
> Move the variable into the case that uses it, which silences the warning:
>
> arch/powerpc/xmon/xmon.c: In function ‘bpt_cmds’:
> arch/powerpc/xmon/xmon.c:1529:13: warning: statement will never be executed
> [-Wswitch-unreachable]
> 1529 | int mode;
> | ^~~~
>
> Fixes: 09b6c1129f89 ("powerpc/xmon: Fix compile error with PPC_8xx=y")
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH][next] powerpc: Fix fall-through warning for Clang
On Tue, Sep 06, 2022 at 10:32:13PM +0100, Gustavo A. R. Silva wrote: > Fix the following fallthrough warning: > > arch/powerpc/platforms/85xx/mpc85xx_cds.c:161:3: warning: unannotated > fall-through between switch labels [-Wimplicit-fallthrough] > > Link: https://github.com/KSPP/linux/issues/198 > Reported-by: kernel test robot > Link: https://lore.kernel.org/lkml/202209061224.kxorrgvg-...@intel.com/ > Signed-off-by: Gustavo A. R. Silva Thanks! Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] gcc-plugins: Undefine LATENT_ENTROPY_PLUGIN when plugin disabled for a file
On Tue, 16 Aug 2022 15:17:20 +1000, Andrew Donnellan wrote:
> Commit 36d4b36b6959 ("lib/nodemask: inline next_node_in() and
> node_random()") refactored some code by moving node_random() from
> lib/nodemask.c to include/linux/nodemask.h, thus requiring nodemask.h to
> include random.h, which conditionally defines add_latent_entropy()
> depending on whether the macro LATENT_ENTROPY_PLUGIN is defined.
>
> This broke the build on powerpc, where nodemask.h is indirectly included
> in arch/powerpc/kernel/prom_init.c, part of the early boot machinery that
> is excluded from the latent entropy plugin using
> DISABLE_LATENT_ENTROPY_PLUGIN. It turns out that while we add a gcc flag
> to disable the actual plugin, we don't undefine LATENT_ENTROPY_PLUGIN.
>
> [...]
Applied to for-next/hardening, thanks!
[1/1] gcc-plugins: Undefine LATENT_ENTROPY_PLUGIN when plugin disabled for a
file
https://git.kernel.org/kees/c/2d08c71d2c79
--
Kees Cook
Re: [PATCH -next v3 2/2] powerpc: add support for syscall stack randomization
On Fri, Jul 01, 2022 at 04:24:35PM +0800, Xiu Jianfeng wrote: > Add support for adding a random offset to the stack while handling > syscalls. This patch uses mftb() instead of get_random_int() for better > performance. > > In order to avoid unconditional stack canaries on syscall entry (due to > the use of alloca()), also disable stack protector to avoid triggering > needless checks and slowing down the entry path. As there is no general > way to control stack protector coverage with a function attribute, this > must be disabled at the compilation unit level. > > Signed-off-by: Xiu Jianfeng Reviewed-by: Kees Cook -Kees -- Kees Cook
Re: [PATCH] macintosh:fix oob read in do_adb_query function
On Wed, Jul 13, 2022 at 11:37:34PM +0800, Ning Qiang wrote:
> In do_adb_query function of drivers/macintosh/adb.c, req->data is copy
> form userland. the parameter "req->data[2]" is Missing check, the
> array size of adb_handler[] is 16, so "adb_handler[
> req->data[2]].original_address" and "adb_handler[
> req->data[2]].handler_id" will lead to oob read.
>
> Signed-off-by: Ning Qiang
Thanks for catching this!
Do you have a reproducer for this? I'd expect CONFIG_UBSAN_BOUNDS=y to
notice this at runtime, at least.
> ---
> drivers/macintosh/adb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/macintosh/adb.c b/drivers/macintosh/adb.c
> index 439fab4eaa85..1bbb9ca08d40 100644
> --- a/drivers/macintosh/adb.c
> +++ b/drivers/macintosh/adb.c
> @@ -647,7 +647,7 @@ do_adb_query(struct adb_request *req)
>
> switch(req->data[1]) {
> case ADB_QUERY_GETDEVINFO:
> - if (req->nbytes < 3)
> + if (req->nbytes < 3 || req->data[2] >= 16)
I'd prefer this was:
+ if (req->nbytes < 3 || req->data[2] >= ARRAY_SIZE(adb_handler))
so it's tied to the actual variable (if its size ever changes).
With that:
Reviewed-by: Kees Cook
-Kees
> break;
> mutex_lock(_handler_mutex);
> req->reply[0] = adb_handler[req->data[2]].original_address;
> --
> 2.25.1
>
--
Kees Cook
Re: [PATCH v2] stack: Declare {randomize_,}kstack_offset to fix Sparse warnings
On Wed, 29 Jun 2022 14:04:23 +0800, GONG, Ruiqi wrote:
> Fix the following Sparse warnings that got noticed when the PPC-dev
> patchwork was checking another patch (see the link below):
>
> init/main.c:862:1: warning: symbol 'randomize_kstack_offset' was not
> declared. Should it be static?
> init/main.c:864:1: warning: symbol 'kstack_offset' was not declared. Should
> it be static?
>
> Which in fact are triggered on all architectures that have
> HAVE_ARCH_RANDOMIZE_KSTACK_OFFSET support (for instances x86, arm64
> etc).
>
> [...]
Applied to for-next/hardening, thanks!
[1/1] stack: Declare {randomize_,}kstack_offset to fix Sparse warnings
https://git.kernel.org/kees/c/375561bd6195
--
Kees Cook
Re: [PATCH] powerpc: Restore CONFIG_DEBUG_INFO in defconfigs
On Sat, Jun 11, 2022 at 08:51:57AM +0200, Christophe Leroy wrote:
> Commit f9b3cd245784 ("Kconfig.debug: make DEBUG_INFO selectable from a
> choice") broke the selection of CONFIG_DEBUG_INFO by powerpc defconfigs.
>
> It is now necessary to select one of the three DEBUG_INFO_DWARF*
> options to get DEBUG_INFO enabled.
>
> Replace DEBUG_INFO=y by DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y in all
> defconfigs using the following command:
>
> sed -i s/DEBUG_INFO=y/DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y/g `git grep -l
> DEBUG_INFO arch/powerpc/configs/`
>
> Fixes: f9b3cd245784 ("Kconfig.debug: make DEBUG_INFO selectable from a
> choice")
> Cc: sta...@vger.kernel.org
> Cc: Kees Cook
> Signed-off-by: Christophe Leroy
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH 0/5] kallsyms: make kallsym APIs more safe with scnprintf
On Sat, May 21, 2022 at 11:07:52PM -0700, Christoph Hellwig wrote: > On Fri, May 20, 2022 at 02:06:56PM +0530, Maninder Singh wrote: > > kallsyms functionality depends on KSYM_NAME_LEN directly. > > but if user passed array length lesser than it, sprintf > > can cause issues of buffer overflow attack. > > > > So changing *sprint* and *lookup* APIs in this patch set > > to have buffer size as an argument and replacing sprintf with > > scnprintf. > > This is still a pretty horrible API. Passing something like > a struct seq_buf seems like the much better API here. Also with > the amount of arguments and by reference passing it might be worth > to pass them as a structure while you're at it. Yeah, I agree. It really seems like seq_buf would be nicer. -- Kees Cook
Re: [PATCH -next] powerpc: add support for syscall stack randomization
On Tue, May 10, 2022 at 07:23:46PM +1000, Nicholas Piggin wrote:
> Excerpts from Xiu Jianfeng's message of May 5, 2022 9:19 pm:
> > Add support for adding a random offset to the stack while handling
> > syscalls. This patch uses mftb() instead of get_random_int() for better
> > performance.
>
> Hey, very nice.
Agreed! :)
> > [...]
> > @@ -82,6 +83,7 @@ notrace long system_call_exception(long r3, long r4, long
> > r5,
> >
> > kuap_lock();
> >
> > + add_random_kstack_offset();
> > regs->orig_gpr3 = r3;
> >
> > if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
>
> This looks like the right place. I wonder why other interrupts don't
> get the same treatment. Userspace can induce the kernel to take a
> synchronous interrupt, or wait for async ones. Smaller surface area
> maybe but certain instruction emulation for example could result in
> significant logic that depends on user state. Anyway that's for
> hardening gurus to ponder.
I welcome it being used for any userspace controllable entry to the
kernel! :)
Also, related, have you validated the result using the LKDTM test?
See tools/testing/selftests/lkdtm/stack-entropy.sh
>
> > @@ -405,6 +407,7 @@ interrupt_exit_user_prepare_main(unsigned long ret,
> > struct pt_regs *regs)
> >
> > /* Restore user access locks last */
> > kuap_user_restore(regs);
> > + choose_random_kstack_offset(mftb() & 0xFF);
> >
> > return ret;
> > }
>
> So this seems to be what x86 and s390 do, but why are we choosing a
> new offset for every interrupt when it's only used on a syscall?
> I would rather you do what arm64 does and just choose the offset
> at the end of system_call_exception.
>
> I wonder why the choose is separated from the add? I guess it's to
> avoid a data dependency for stack access on an expensive random
> function, so that makes sense (a comment would be nice in the
> generic code).
How does this read? I can send a "real" patch if it looks good:
diff --git a/include/linux/randomize_kstack.h b/include/linux/randomize_kstack.h
index 1468caf001c0..ad3e80275c74 100644
--- a/include/linux/randomize_kstack.h
+++ b/include/linux/randomize_kstack.h
@@ -40,8 +40,11 @@ DECLARE_PER_CPU(u32, kstack_offset);
*/
#define KSTACK_OFFSET_MAX(x) ((x) & 0x3FF)
-/*
- * These macros must be used during syscall entry when interrupts and
+/**
+ * add_random_kstack_offset - Increase stack utilization by previously
+ * chosen random offset
+ *
+ * This should be used in the syscall entry path when interrupts and
* preempt are disabled, and after user registers have been stored to
* the stack.
*/
@@ -55,6 +58,24 @@ DECLARE_PER_CPU(u32, kstack_offset);
} \
} while (0)
+/**
+ * choose_random_kstack_offset - Choose the random offsset for the next
+ * add_random_kstack_offset()
+ *
+ * This should only be used during syscall exit when interrupts and
+ * preempt are disabled, and before user registers have been restored
+ * from the stack. This is done to frustrate attack attempts from
+ * userspace to learn the offset:
+ * - Maximize the timing uncertainty visible from userspace: if the
+ * the offset is chosen at syscall entry, userspace has much more
+ * control over the timing between chosen offsets. "How long will we
+ * be in kernel mode?" tends to be more difficult to know than "how
+ * long will be be in user mode?"
+ * - Reduce the lifetime of the new offset sitting in memory during
+ * kernel mode execution. Exposures of "thread-local" (e.g. current,
+ * percpu, etc) memory contents tends to be easier than arbitrary
+ * location memory exposures.
+ */
#define choose_random_kstack_offset(rand) do { \
if (static_branch_maybe(CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT, \
_kstack_offset)) {\
--
Kees Cook
Re: [PATCH v6 00/23] Rust support
On Sat, May 07, 2022 at 07:23:58AM +0200, Miguel Ojeda wrote: > ## Patch series status > > The Rust support is still to be considered experimental. However, > support is good enough that kernel developers can start working on the > Rust abstractions for subsystems and write drivers and other modules. I'd really like to see this landed for a few reasons: - It's under active development, and I'd rather review the changes "normally", incrementally, etc. Right now it can be hard to re-review some of the "mostly the same each version" patches in the series. - I'd like to break the catch-22 of "ask for a new driver to be written in rust but the rust support isn't landed" vs "the rust support isn't landed because there aren't enough drivers". It really feels like "release early, release often" is needed here; it's hard to develop against -next. :) Should we give it a try for this coming merge window? -- Kees Cook
Re: [PATCH RFC 2/8] arm64: stacktrace: Add arch_within_stack_frames
On Mon, Apr 18, 2022 at 09:22:11PM +0800, He Zhe wrote: > This function checks if the given address range crosses frame boundary. > It is based on the existing x86 algorithm, but implemented via stacktrace. > This can be tested by USERCOPY_STACK_FRAME_FROM and > USERCOPY_STACK_FRAME_TO in lkdtm. Hi, Thanks for doing this implementation! One reason usercopy hardening didn't persue doing a "full" stacktrace was because it seemed relatively expensive. Did you do any usercopy-heavily workload testing to see if there was a noticeable performance impact? It would be nice to block the exposure of canaries and PAC bits, though, so I'm not opposed, but I'd like to get a better sense of how "heavy" this might be. Thanks! -Kees -- Kees Cook
Re: [RFC PATCH] lkdtm: Replace lkdtm_rodata_do_nothing() by do_nothing()
On Sun, Oct 17, 2021 at 07:19:47PM +0200, Christophe Leroy wrote: > But for EXEC_RODATA test, execute_location() uses > lkdtm_rodata_do_nothing() which is already in rodata section > at build time instead of using a copy of do_nothing(). However > it still uses the function descriptor of do_nothing(). There > is a risk that running lkdtm_rodata_do_nothing() with the > function descriptor of do_thing() is wrong. Wrong how? (Could there be two descriptors?) > To remove the above risk, change the approach and do the same > as for other EXEC tests: use a copy of do_nothing(). The copy > cannot be done during the test because RODATA area is write > protected. Do the copy during init, before RODATA becomes > write protected. Hmm, hmm. This is a nice way to handle it, but I'm not sure which "weird" way is better. I kind of prefer the code going through all the "regular" linking goo to end up in .rodata, but is it really any different from doing this via the ro_after_init section? It makes me nervous because they can technically be handled differently. For example, .rodata is mapped differently on some architectures compared to ro_after_init. Honestly, I actually this this patch should be modified to _add_ a new test for EXEC_RO_AFTER_INIT, and leave the existing .rodata one alone... -Kees -- Kees Cook
Re: [PATCH v5 3/8] powerpc/mm/ptdump: debugfs handler for W+X checks at runtime
*thread necromancy*
Is this patch still something folks are working on? It'd be nice to be
able to trigger this check at runtime.
-Kees
On Wed, Feb 26, 2020 at 05:35:46PM +1100, Russell Currey wrote:
> Very rudimentary, just
>
> echo 1 > [debugfs]/check_wx_pages
>
> and check the kernel log. Useful for testing strict module RWX.
>
> Updated the Kconfig entry to reflect this.
>
> Also fixed a typo.
>
> Signed-off-by: Russell Currey
> ---
> arch/powerpc/Kconfig.debug | 6 --
> arch/powerpc/mm/ptdump/ptdump.c | 21 -
> 2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> index 0b063830eea8..e37960ef68c6 100644
> --- a/arch/powerpc/Kconfig.debug
> +++ b/arch/powerpc/Kconfig.debug
> @@ -370,7 +370,7 @@ config PPC_PTDUMP
> If you are unsure, say N.
>
> config PPC_DEBUG_WX
> - bool "Warn on W+X mappings at boot"
> + bool "Warn on W+X mappings at boot & enable manual checks at runtime"
> depends on PPC_PTDUMP && STRICT_KERNEL_RWX
> help
> Generate a warning if any W+X mappings are found at boot.
> @@ -384,7 +384,9 @@ config PPC_DEBUG_WX
> of other unfixed kernel bugs easier.
>
> There is no runtime or memory usage effect of this option
> - once the kernel has booted up - it's a one time check.
> + once the kernel has booted up, it only automatically checks once.
> +
> + Enables the "check_wx_pages" debugfs entry for checking at runtime.
>
> If in doubt, say "Y".
>
> diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
> index 206156255247..a15e19a3b14e 100644
> --- a/arch/powerpc/mm/ptdump/ptdump.c
> +++ b/arch/powerpc/mm/ptdump/ptdump.c
> @@ -4,7 +4,7 @@
> *
> * This traverses the kernel pagetables and dumps the
> * information about the used sections of memory to
> - * /sys/kernel/debug/kernel_pagetables.
> + * /sys/kernel/debug/kernel_page_tables.
> *
> * Derived from the arm64 implementation:
> * Copyright (c) 2014, The Linux Foundation, Laura Abbott.
> @@ -413,6 +413,25 @@ void ptdump_check_wx(void)
> else
> pr_info("Checked W+X mappings: passed, no W+X pages found\n");
> }
> +
> +static int check_wx_debugfs_set(void *data, u64 val)
> +{
> + if (val != 1ULL)
> + return -EINVAL;
> +
> + ptdump_check_wx();
> +
> + return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(check_wx_fops, NULL, check_wx_debugfs_set, "%llu\n");
> +
> +static int ptdump_check_wx_init(void)
> +{
> + return debugfs_create_file("check_wx_pages", 0200, NULL,
> +NULL, _wx_fops) ? 0 : -ENOMEM;
> +}
> +device_initcall(ptdump_check_wx_init);
> #endif
>
> static int ptdump_init(void)
> --
> 2.25.1
>
--
Kees Cook
Re: [PATCH v4 00/13] Fix LKDTM for PPC64/IA64/PARISC v4
On Wed, Mar 09, 2022 at 12:37:14PM +1100, Michael Ellerman wrote: > Michael Ellerman writes: > > On Tue, 15 Feb 2022 13:40:55 +0100, Christophe Leroy wrote: > >> PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work > >> on those three architectures because LKDTM messes up function > >> descriptors with functions. > >> > >> This series does some cleanup in the three architectures and > >> refactors function descriptors so that it can then easily use it > >> in a generic way in LKDTM. > >> > >> [...] > > > > Applied to powerpc/next. > > I also have it in an rc2-based topic branch if there are any merge > conflicts that people want to resolve, I don't see any in linux-next at > the moment though. > > https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/log/?h=topic/func-desc-lkdtm Thanks! I've got some core changes coming for lkdtm, but I'm waiting until after the merge window to rebase them and get them into -next. -- Kees Cook
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 12:18:45PM -0800, Linus Torvalds wrote: > On Wed, Mar 2, 2022 at 12:07 PM Kees Cook wrote: > > > > I've long wanted to change kfree() to explicitly set pointers to NULL on > > free. https://github.com/KSPP/linux/issues/87 > > We've had this discussion with the gcc people in the past, and gcc > actually has some support for it, but it's sadly tied to the actual > function name (ie gcc has some special-casing for "free()") > > See > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94527 > > for some of that discussion. > > Oh, and I see some patch actually got merged since I looked there last > so that you can mark "deallocator" functions, but I think it's only > for the context matching, not for actually killing accesses to the > pointer afterwards. Ah! I missed that getting added in GCC 11. But yes, there it is: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute Hah, now we may need to split __malloc from __alloc_size. ;) I'd still like the NULL assignment behavior, though, since some things can easily avoid static analysis. -- Kees Cook
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 10:29:31AM +0100, Rasmus Villemoes wrote:
> This won't help the current issue (because it doesn't exist and might
> never), but just in case some compiler people are listening, I'd like to
> have some sort of way to tell the compiler "treat this variable as
> uninitialized from here on". So one could do
>
> #define kfree(p) do { __kfree(p); __magic_uninit(p); } while (0)
>
> with __magic_uninit being a magic no-op that doesn't affect the
> semantics of the code, but could be used by the compiler's "[is/may be]
> used uninitialized" machinery to flag e.g. double frees on some odd
> error path etc. It would probably only work for local automatic
> variables, but it should be possible to just ignore the hint if p is
> some expression like foo->bar or has side effects. If we had that, the
> end-of-loop test could include that to "uninitialize" the iterator.
I've long wanted to change kfree() to explicitly set pointers to NULL on
free. https://github.com/KSPP/linux/issues/87
The thing stopping a trivial transformation of kfree() is:
kfree(get_some_pointer());
I would argue, though, that the above is poor form: the thing holding
the pointer should be the thing freeing it, so these cases should be
refactored and kfree() could do the NULLing by default.
Quoting myself in the above issue:
Without doing massive tree-wide changes, I think we need compiler
support. If we had something like __builtin_is_lvalue(), we could
distinguish function returns from lvalues. For example, right now a
common case are things like:
kfree(get_some_ptr());
But if we could at least gain coverage of the lvalue cases, and detect
them statically at compile-time, we could do:
#define __kfree_and_null(x) do { __kfree(*x); *x = NULL; } while (0)
#define kfree(x) __builtin_choose_expr(__builtin_is_lvalue(x),
__kfree_and_null(&(x)), __kfree(x))
Alternatively, we could do a tree-wide change of the former case (findable
with Coccinelle) and change them into something like kfree_no_null()
and redefine kfree() itself:
#define kfree_no_null(x) do { void *__ptr = (x); __kfree(__ptr); } while (0)
#define kfree(x) do { __kfree(x); x = NULL; } while (0)
--
Kees Cook
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote: > Based on the coccinelle script there are ~480 cases that need fixing > in total. I'll now finish all of them and then split them by > submodules as Greg suggested and repost a patch set per submodule. > Sounds good? To help with this splitting, see: https://github.com/kees/kernel-tools/blob/trunk/split-on-maintainer It's not perfect, but it'll get you really close. For example, if you had a single big tree-wide patch applied to your tree: $ rm 0*.patch $ git format-patch -1 HEAD $ mv 0*.patch treewide.patch $ split-on-maintainer treewide.patch $ ls 0*.patch If you have a build log before the patch that spits out warnings, the --build-log argument can extract those warnings on a per-file basis, too (though this can be fragile). -- Kees Cook
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 04:45:11PM -0800, Linus Torvalds wrote: > Really. The "-Wshadow doesn't work on the kernel" is not some new > issue, because you have to do completely insane things to the source > code to enable it. The first big glitch with -Wshadow was with shadowed global variables. GCC 4.8 fixed that, but it still yells about shadowed functions. What _almost_ works is -Wshadow=local. At first glace, all the warnings look solvable, but then one will eventually discover __wait_event() and associated macros that mix when and how deeply it intentionally shadows variables. :) Another way to try to catch misused shadow variables is -Wunused-but-set-varible, but it, too, has tons of false positives. I tried to capture some of the rationale and research here: https://github.com/KSPP/linux/issues/152 -- Kees Cook
Re: [PATCH v2] usercopy: Check valid lifetime via stack depth
On Thu, Feb 24, 2022 at 08:58:20AM +, David Laight wrote:
> From: Kees Cook
> > Sent: 24 February 2022 06:04
> >
> > Under CONFIG_HARDENED_USERCOPY=y, when exact stack frame boundary checking
> > is not available (i.e. everything except x86 with FRAME_POINTER), check
> > a stack object as being at least "current depth valid", in the sense
> > that any object within the stack region but not between start-of-stack
> > and current_stack_pointer should be considered unavailable (i.e. its
> > lifetime is from a call no longer present on the stack).
> >
> ...
> > diff --git a/mm/usercopy.c b/mm/usercopy.c
> > index d0d268135d96..5d28725af95f 100644
> > --- a/mm/usercopy.c
> > +++ b/mm/usercopy.c
> > @@ -22,6 +22,30 @@
> > #include
> > #include "slab.h"
> >
> > +/*
> > + * Only called if obj is within stack/stackend bounds. Determine if within
> > + * current stack depth.
> > + */
> > +static inline int check_stack_object_depth(const void *obj,
> > + unsigned long len)
> > +{
> > +#ifdef CONFIG_ARCH_HAS_CURRENT_STACK_POINTER
> > +#ifndef CONFIG_STACK_GROWSUP
>
> Pointless negation
>
> > + const void * const high = stackend;
> > + const void * const low = (void *)current_stack_pointer;
> > +#else
> > + const void * const high = (void *)current_stack_pointer;
> > + const void * const low = stack;
> > +#endif
> > +
> > + /* Reject: object not within current stack depth. */
> > + if (obj < low || high < obj + len)
> > + return BAD_STACK;
> > +
> > +#endif
> > + return GOOD_STACK;
> > +}
>
> If the comment at the top of the function is correct then
> only a single test for the correct end of the buffer against
> the current stack pointer is needed.
> Something like:
> #ifdef CONFIG_STACK_GROWSUP
> if ((void *)current_stack_pointer < obj + len)
> return BAD_STACK;
> #else
> if (obj < (void *)current_stack_pointer)
> return BAD_STACK;
> #endif
> return GOOD_STACK;
Oh, yeah, excellent point. I suspect the compiler would probably
optimize it all away, but yes, this is, in fact, easier to read, and
short enough I should probably just not bother with a separate function.
Thanks!
-Kees
>
> Although it may depend on exactly where the stack pointer
> points to - especially for GROWSUP.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1
> 1PT, UK
> Registration No: 1397386 (Wales)
>
--
Kees Cook
[PATCH v2] usercopy: Check valid lifetime via stack depth
Under CONFIG_HARDENED_USERCOPY=y, when exact stack frame boundary checking
is not available (i.e. everything except x86 with FRAME_POINTER), check
a stack object as being at least "current depth valid", in the sense
that any object within the stack region but not between start-of-stack
and current_stack_pointer should be considered unavailable (i.e. its
lifetime is from a call no longer present on the stack).
Introduce ARCH_HAS_CURRENT_STACK_POINTER to track which architectures
have actually implemented the common global register alias.
Additionally report usercopy bounds checking failures with an offset
from current_stack_pointer, which may assist with diagnosing failures.
The LKDTM USERCOPY_STACK_FRAME_TO and USERCOPY_STACK_FRAME_FROM tests
(once slightly adjusted in a separate patch) will pass again with
this fixed.
Cc: Matthew Wilcox (Oracle)
Cc: Josh Poimboeuf
Cc: Andrew Morton
Cc: linux...@kvack.org
Reported-by: Muhammad Usama Anjum
Signed-off-by: Kees Cook
---
v1: https://lore.kernel.org/all/20220216201449.2087956-1-keesc...@chromium.org/
v2: adjust for only some archs having current_stack_pointer
---
arch/arm/Kconfig | 1 +
arch/arm64/Kconfig | 1 +
arch/powerpc/Kconfig | 1 +
arch/s390/Kconfig| 1 +
arch/sh/Kconfig | 1 +
arch/x86/Kconfig | 1 +
mm/usercopy.c| 41 ++---
7 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 4c97cb40eebb..a7a09eef1852 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -5,6 +5,7 @@ config ARM
select ARCH_32BIT_OFF_T
select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE if HAVE_KRETPROBES &&
FRAME_POINTER && !ARM_UNWIND
select ARCH_HAS_BINFMT_FLAT
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_DEBUG_VIRTUAL if MMU
select ARCH_HAS_DMA_WRITE_COMBINE if !ARM_DMA_MEM_BUFFERABLE
select ARCH_HAS_ELF_RANDOMIZE
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index f2b5a4abef21..b8ab790555c8 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -18,6 +18,7 @@ config ARM64
select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
select ARCH_ENABLE_THP_MIGRATION if TRANSPARENT_HUGEPAGE
select ARCH_HAS_CACHE_LINE_SIZE
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_DEBUG_VIRTUAL
select ARCH_HAS_DEBUG_VM_PGTABLE
select ARCH_HAS_DMA_PREP_COHERENT
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index b779603978e1..7e7387bd7d53 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -108,6 +108,7 @@ config PPC
select ARCH_ENABLE_MEMORY_HOTPLUG
select ARCH_ENABLE_MEMORY_HOTREMOVE
select ARCH_HAS_COPY_MC if PPC64
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_DEBUG_VIRTUAL
select ARCH_HAS_DEBUG_VM_PGTABLE
select ARCH_HAS_DEBUG_WXif STRICT_KERNEL_RWX
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index be9f39fd06df..4845ab549dd1 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -60,6 +60,7 @@ config S390
select ARCH_ENABLE_MEMORY_HOTPLUG if SPARSEMEM
select ARCH_ENABLE_MEMORY_HOTREMOVE
select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_DEBUG_VM_PGTABLE
select ARCH_HAS_DEBUG_WX
select ARCH_HAS_DEVMEM_IS_ALLOWED
diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 2474a04ceac4..1c2b53bf3093 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -7,6 +7,7 @@ config SUPERH
select ARCH_HAVE_CUSTOM_GPIO_H
select ARCH_HAVE_NMI_SAFE_CMPXCHG if (GUSA_RB || CPU_SH4A)
select ARCH_HAS_BINFMT_FLAT if !MMU
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_GIGANTIC_PAGE
select ARCH_HAS_GCOV_PROFILE_ALL
select ARCH_HAS_PTE_SPECIAL
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9f5bd41bf660..90494fba3620 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -69,6 +69,7 @@ config X86
select ARCH_ENABLE_THP_MIGRATION if X86_64 && TRANSPARENT_HUGEPAGE
select ARCH_HAS_ACPI_TABLE_UPGRADE if ACPI
select ARCH_HAS_CACHE_LINE_SIZE
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_DEBUG_VIRTUAL
select ARCH_HAS_DEBUG_VM_PGTABLEif !X86_PAE
select ARCH_HAS_DEVMEM_IS_ALLOWED
diff --git a/mm/usercopy.c b/mm/usercopy.c
index d0d268135d96..5d28725af95f 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -22,6 +22,30 @@
#include
#include "slab.h"
+/*
+ * Only called if obj is within stack/stackend bounds. Determine if within
+ * current stack depth.
+ */
+static inline int check_stack_object_depth(const void *obj,
+ unsigned long len)
+{
+#ifdef CONFIG_ARCH_HAS_CURRENT_STACK_POINTER
+#ifnd
Re: [PATCH] powerpc/32: Clear volatile regs on syscall exit
On Wed, Feb 23, 2022 at 06:11:36PM +0100, Christophe Leroy wrote:
> Commit a82adfd5c7cb ("hardening: Introduce CONFIG_ZERO_CALL_USED_REGS")
> added zeroing of used registers at function exit.
>
> At the time being, PPC64 clears volatile registers on syscall exit but
> PPC32 doesn't do it for performance reason.
>
> Add that clearing in PPC32 syscall exit as well, but only when
> CONFIG_ZERO_CALL_USED_REGS is selected.
>
> On an 8xx, the null_syscall selftest gives:
> - Without CONFIG_ZERO_CALL_USED_REGS : 288 cycles
> - With CONFIG_ZERO_CALL_USED_REGS : 305 cycles
> - With CONFIG_ZERO_CALL_USED_REGS + this patch: 319 cycles
>
> Note that (independent of this patch), with pmac32_defconfig,
> vmlinux size is as follows with/without CONFIG_ZERO_CALL_USED_REGS:
>
>text databss dec hex filename
> 9578869 2525210 194400 12298479bba8ef vmlinux.without
> 10318045 2525210 194400 13037655c6f057 vmlinux.with
>
> That is a 7.7% increase on text size, 6.0% on overall size.
>
> Signed-off-by: Christophe Leroy
> ---
> arch/powerpc/kernel/entry_32.S | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> index 7748c278d13c..199f23092c02 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -151,6 +151,21 @@ syscall_exit_finish:
> bne 3f
> mtcrr5
>
> +#ifdef CONFIG_ZERO_CALL_USED_REGS
> + /* Zero volatile regs that may contain sensitive kernel data */
> + li r0,0
> + li r4,0
> + li r5,0
> + li r6,0
> + li r7,0
> + li r8,0
> + li r9,0
> + li r10,0
> + li r11,0
> + li r12,0
> + mtctr r0
> + mtxer r0
> +#endif
I think this should probably be unconditional -- if this is actually
leaking kernel pointers (or data) that's pretty bad. :|
If you really want to leave it build-time selectable, maybe add a new
config that gets "select"ed by CONFIG_ZERO_CALL_USED_REGS?
(And you may want to consider wiping all "unused" registers at syscall
entry as well.)
-Kees
> 1: lwz r2,GPR2(r1)
> lwz r1,GPR1(r1)
> rfi
> --
> 2.34.1
>
--
Kees Cook
Re: [PATCH v4 00/13] Fix LKDTM for PPC64/IA64/PARISC v4
On Wed, Feb 16, 2022 at 11:22:33PM +1100, Michael Ellerman wrote: > Kees Cook writes: > > On Tue, Feb 15, 2022 at 01:40:55PM +0100, Christophe Leroy wrote: > >> PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work > >> on those three architectures because LKDTM messes up function > >> descriptors with functions. > >> > >> This series does some cleanup in the three architectures and > >> refactors function descriptors so that it can then easily use it > >> in a generic way in LKDTM. > > > > Thanks for doing this! It looks good to me. :) > > How should we merge this series, it's a bit all over the map. > > I could put it in a topic branch? That's fine by me -- I had assumed it'd go via the ppc tree. But if you'd rather I take it as a topic branch I can do that too. -- Kees Cook
Re: [PATCH v4 00/13] Fix LKDTM for PPC64/IA64/PARISC v4
On Tue, Feb 15, 2022 at 01:40:55PM +0100, Christophe Leroy wrote:
> PPC64/IA64/PARISC have function descriptors. LKDTM doesn't work
> on those three architectures because LKDTM messes up function
> descriptors with functions.
>
> This series does some cleanup in the three architectures and
> refactors function descriptors so that it can then easily use it
> in a generic way in LKDTM.
Thanks for doing this! It looks good to me. :)
-Kees
>
> Changes in v4:
> - Added patch 1 which Fixes 'sparse' for powerpc64le after wrong report on
> previous series, refer
> https://github.com/ruscur/linux-ci/actions/runs/1351427671
> - Exported dereference_function_descriptor() to modules
> - Addressed other received comments
> - Rebased on latest powerpc/next (5a72345e6a78120368fcc841b570331b6c5a50da)
>
> Changes in v3:
> - Addressed received comments
> - Swapped some of the powerpc patches to keep func_descr_t renamed as struct
> func_desc and remove 'struct ppc64_opd_entry'
> - Changed HAVE_FUNCTION_DESCRIPTORS macro to a config item
> CONFIG_HAVE_FUNCTION_DESCRIPTORS
> - Dropped patch 11 ("Fix lkdtm_EXEC_RODATA()")
>
> Changes in v2:
> - Addressed received comments
> - Moved dereference_[kernel]_function_descriptor() out of line
> - Added patches to remove func_descr_t and func_desc_t in powerpc
> - Using func_desc_t instead of funct_descr_t
> - Renamed HAVE_DEREFERENCE_FUNCTION_DESCRIPTOR to HAVE_FUNCTION_DESCRIPTORS
> - Added a new lkdtm test to check protection of function descriptors
>
> Christophe Leroy (13):
> powerpc: Fix 'sparse' checking on PPC64le
> powerpc: Move and rename func_descr_t
> powerpc: Use 'struct func_desc' instead of 'struct ppc64_opd_entry'
> powerpc: Remove 'struct ppc64_opd_entry'
> powerpc: Prepare func_desc_t for refactorisation
> ia64: Rename 'ip' to 'addr' in 'struct fdesc'
> asm-generic: Define CONFIG_HAVE_FUNCTION_DESCRIPTORS
> asm-generic: Define 'func_desc_t' to commonly describe function
> descriptors
> asm-generic: Refactor dereference_[kernel]_function_descriptor()
> lkdtm: Force do_nothing() out of line
> lkdtm: Really write into kernel text in WRITE_KERN
> lkdtm: Fix execute_[user]_location()
> lkdtm: Add a test for function descriptors protection
>
> arch/Kconfig | 3 +
> arch/ia64/Kconfig| 1 +
> arch/ia64/include/asm/elf.h | 2 +-
> arch/ia64/include/asm/sections.h | 24 +---
> arch/ia64/kernel/module.c| 6 +-
> arch/parisc/Kconfig | 1 +
> arch/parisc/include/asm/sections.h | 16 ++
> arch/parisc/kernel/process.c | 21 ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/Makefile| 2 +-
> arch/powerpc/include/asm/code-patching.h | 2 +-
> arch/powerpc/include/asm/elf.h | 6 ++
> arch/powerpc/include/asm/sections.h | 29 ++
> arch/powerpc/include/asm/types.h | 6 --
> arch/powerpc/include/uapi/asm/elf.h | 8 ---
> arch/powerpc/kernel/module_64.c | 42 ++
> arch/powerpc/kernel/ptrace/ptrace.c | 6 ++
> arch/powerpc/kernel/signal_64.c | 8 +--
> drivers/misc/lkdtm/core.c| 1 +
> drivers/misc/lkdtm/lkdtm.h | 1 +
> drivers/misc/lkdtm/perms.c | 71 +++-
> include/asm-generic/sections.h | 15 -
> include/linux/kallsyms.h | 2 +-
> kernel/extable.c | 24 +++-
> tools/testing/selftests/lkdtm/tests.txt | 1 +
> 25 files changed, 155 insertions(+), 144 deletions(-)
>
> --
> 2.34.1
>
--
Kees Cook
Re: [PATCH v4 01/13] powerpc: Fix 'sparse' checking on PPC64le
On Tue, Feb 15, 2022 at 01:40:56PM +0100, Christophe Leroy wrote: > 'sparse' is architecture agnostic and knows nothing about ELF ABI > version. > > Just like it gets arch and powerpc type and endian from Makefile, > it also need to get _CALL_ELF from there, otherwise it won't set > PPC64_ELF_ABI_v2 macro for PPC64le and won't check the correct code. > > Signed-off-by: Christophe Leroy Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v3 12/12] lkdtm: Add a test for function descriptors protection
On Sun, Oct 17, 2021 at 02:38:25PM +0200, Christophe Leroy wrote:
> Add WRITE_OPD to check that you can't modify function
> descriptors.
>
> Gives the following result when function descriptors are
> not protected:
>
> lkdtm: Performing direct entry WRITE_OPD
> lkdtm: attempting bad 16 bytes write at c269b358
> lkdtm: FAIL: survived bad write
> lkdtm: do_nothing was hijacked!
>
> Looks like a standard compiler barrier() is not enough to force
> GCC to use the modified function descriptor. Had to add a fake empty
> inline assembly to force GCC to reload the function descriptor.
>
> Signed-off-by: Christophe Leroy
> ---
> drivers/misc/lkdtm/core.c | 1 +
> drivers/misc/lkdtm/lkdtm.h | 1 +
> drivers/misc/lkdtm/perms.c | 22 ++
> 3 files changed, 24 insertions(+)
>
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index fe6fd34b8caf..de092aa03b5d 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -148,6 +148,7 @@ static const struct crashtype crashtypes[] = {
> CRASHTYPE(WRITE_RO),
> CRASHTYPE(WRITE_RO_AFTER_INIT),
> CRASHTYPE(WRITE_KERN),
> + CRASHTYPE(WRITE_OPD),
> CRASHTYPE(REFCOUNT_INC_OVERFLOW),
> CRASHTYPE(REFCOUNT_ADD_OVERFLOW),
> CRASHTYPE(REFCOUNT_INC_NOT_ZERO_OVERFLOW),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index c212a253edde..188bd0fd6575 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -105,6 +105,7 @@ void __init lkdtm_perms_init(void);
> void lkdtm_WRITE_RO(void);
> void lkdtm_WRITE_RO_AFTER_INIT(void);
> void lkdtm_WRITE_KERN(void);
> +void lkdtm_WRITE_OPD(void);
> void lkdtm_EXEC_DATA(void);
> void lkdtm_EXEC_STACK(void);
> void lkdtm_EXEC_KMALLOC(void);
> diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> index 1cf24c4a79e9..2c6aba3ff32b 100644
> --- a/drivers/misc/lkdtm/perms.c
> +++ b/drivers/misc/lkdtm/perms.c
> @@ -44,6 +44,11 @@ static noinline void do_overwritten(void)
> return;
> }
>
> +static noinline void do_almost_nothing(void)
> +{
> + pr_info("do_nothing was hijacked!\n");
> +}
> +
> static void *setup_function_descriptor(func_desc_t *fdesc, void *dst)
> {
> if (!have_function_descriptors())
> @@ -144,6 +149,23 @@ void lkdtm_WRITE_KERN(void)
> do_overwritten();
> }
>
> +void lkdtm_WRITE_OPD(void)
> +{
> + size_t size = sizeof(func_desc_t);
> + void (*func)(void) = do_nothing;
> +
> + if (!have_function_descriptors()) {
> + pr_info("XFAIL: Platform doesn't use function descriptors.\n");
> + return;
> + }
> + pr_info("attempting bad %zu bytes write at %px\n", size, do_nothing);
> + memcpy(do_nothing, do_almost_nothing, size);
> + pr_err("FAIL: survived bad write\n");
Non-function-descriptor architectures would successfully crash at the
memcpy too, right? (i.e. for them this is just repeating WRITE_KERN)
I'm pondering the utility of the XFAIL vs just letting is succeed, but I
think it more accurate to say "hey, no OPD" as you have it.
> +
> + asm("" : "=m"(func));
> + func();
> +}
> +
> void lkdtm_EXEC_DATA(void)
> {
> execute_location(data_area, CODE_WRITE);
> --
> 2.31.1
>
One tiny suggestion, since I think you need to respin for the
EXPORT_SYMBOL_GPL() anyway. Please update the selftests too:
diff --git a/tools/testing/selftests/lkdtm/tests.txt
b/tools/testing/selftests/lkdtm/tests.txt
index 6b36b7f5dcf9..243c781f0780 100644
--- a/tools/testing/selftests/lkdtm/tests.txt
+++ b/tools/testing/selftests/lkdtm/tests.txt
@@ -44,6 +44,7 @@ ACCESS_NULL
WRITE_RO
WRITE_RO_AFTER_INIT
WRITE_KERN
+WRITE_OPD
REFCOUNT_INC_OVERFLOW
REFCOUNT_ADD_OVERFLOW
REFCOUNT_INC_NOT_ZERO_OVERFLOW
(Though for the future I've been considering making the selftests an
opt-out list so the "normal" stuff doesn't need to keep getting added
there.)
Thanks!
Acked-by: Kees Cook
-Kees
--
Kees Cook
