2022-04-12 05:23:58

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v2] lkdtm/bugs: Don't expect thread termination without CONFIG_UBSAN_TRAP

When you don't select CONFIG_UBSAN_TRAP, you get:

# echo ARRAY_BOUNDS > /sys/kernel/debug/provoke-crash/DIRECT
[ 102.265827] ================================================================================
[ 102.278433] UBSAN: array-index-out-of-bounds in drivers/misc/lkdtm/bugs.c:342:16
[ 102.287207] index 8 is out of range for type 'char [8]'
[ 102.298722] ================================================================================
[ 102.313712] lkdtm: FAIL: survived array bounds overflow!
[ 102.318770] lkdtm: Unexpected! This kernel (5.16.0-rc1-s3k-dev-01884-g720dcf79314a ppc) was built with CONFIG_UBSAN_BOUNDS=y

It is not correct because when CONFIG_UBSAN_TRAP is not selected
you can't expect array bounds overflow to kill the thread.

Modify the logic so that when the kernel is built with
CONFIG_UBSAN_BOUNDS but without CONFIG_UBSAN_TRAP, you get a warning
about CONFIG_UBSAN_TRAP not been selected instead.

This also require a fix of pr_expected_config(), otherwise the
following error is encountered.

CC drivers/misc/lkdtm/bugs.o
drivers/misc/lkdtm/bugs.c: In function 'lkdtm_ARRAY_BOUNDS':
drivers/misc/lkdtm/bugs.c:351:2: error: 'else' without a previous 'if'
351 | else
| ^~~~

Fixes: c75be56e35b2 ("lkdtm/bugs: Add ARRAY_BOUNDS to selftests")
Signed-off-by: Christophe Leroy <[email protected]>
---
v2: Fix pr_expected_config(), otherwise it can't be used in an if/else sequence.
---
drivers/misc/lkdtm/bugs.c | 5 ++++-
drivers/misc/lkdtm/lkdtm.h | 5 ++---
2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
index f21854ac5cc2..0f4dd9621b75 100644
--- a/drivers/misc/lkdtm/bugs.c
+++ b/drivers/misc/lkdtm/bugs.c
@@ -346,7 +346,10 @@ void lkdtm_ARRAY_BOUNDS(void)
kfree(not_checked);
kfree(checked);
pr_err("FAIL: survived array bounds overflow!\n");
- pr_expected_config(CONFIG_UBSAN_BOUNDS);
+ if (IS_ENABLED(CONFIG_UBSAN_BOUNDS))
+ pr_expected_config(CONFIG_UBSAN_TRAP);
+ else
+ pr_expected_config(CONFIG_UBSAN_BOUNDS);
}

void lkdtm_CORRUPT_LIST_ADD(void)
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index f508096e8fd9..9c21a4ca0482 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -8,15 +8,14 @@

extern char *lkdtm_kernel_info;

-#define pr_expected_config(kconfig) \
-{ \
+#define pr_expected_config(kconfig) do { \
if (IS_ENABLED(kconfig)) \
pr_err("Unexpected! This %s was built with " #kconfig "=y\n", \
lkdtm_kernel_info); \
else \
pr_warn("This is probably expected, since this %s was built *without* " #kconfig "=y\n", \
lkdtm_kernel_info); \
-}
+} while (0)

#ifndef MODULE
int lkdtm_check_bool_cmdline(const char *param);
--
2.35.1


2022-04-12 23:46:54

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm/bugs: Don't expect thread termination without CONFIG_UBSAN_TRAP

On Mon, Apr 11, 2022 at 09:13:39PM +0200, Christophe Leroy wrote:
> Signed-off-by: Christophe Leroy <[email protected]>

Thanks! I will fetch this in a moment, though I tripped over this:

Checking for newer revisions on https://lore.kernel.org/all/
Analyzing 1 messages in the thread
Checking attestation on all messages, may take a moment...
---
[PATCH v2] lkdtm/bugs: Don't expect thread termination without CONFIG_UBSAN_TRAP
+ Signed-off-by: Kees Cook <[email protected]>
+ Link: https://lore.kernel.org/r/363b58690e907c677252467a94fe49444c80ea76.1649704381.git.christophe.leroy@csgroup.eu
---
✗ No key: ed25519/[email protected]

Is there some place I can fetch your key from that has a chain of trust?

Also, Konstantin, I note that
https://git.kernel.org/pub/scm/docs/kernel/pgpkeys.git/
does not have a .keyring/ed25519 directory. Should it? I added one
locally for at least one other developer, as I use this setting:

[patatt]
keyringsrc = ~/korg/pgpkeys/.keyring

Am I holding this thing wrong? :)

--
Kees Cook

2022-04-13 08:02:54

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm/bugs: Don't expect thread termination without CONFIG_UBSAN_TRAP



Le 13/04/2022 à 01:06, Kees Cook a écrit :
> On Mon, Apr 11, 2022 at 09:13:39PM +0200, Christophe Leroy wrote:
>> Signed-off-by: Christophe Leroy <[email protected]>
>
> Thanks! I will fetch this in a moment, though I tripped over this:
>
> Checking for newer revisions on https://lore.kernel.org/all/
> Analyzing 1 messages in the thread
> Checking attestation on all messages, may take a moment...
> ---
> [PATCH v2] lkdtm/bugs: Don't expect thread termination without CONFIG_UBSAN_TRAP
> + Signed-off-by: Kees Cook <[email protected]>
> + Link: https://lore.kernel.org/r/363b58690e907c677252467a94fe49444c80ea76.1649704381.git.christophe.leroy@csgroup.eu
> ---
> ✗ No key: ed25519/[email protected]
>
> Is there some place I can fetch your key from that has a chain of trust?


Euh ... I don't know.

I set up patatt in octobre when you asked. I guess I generated a key
with patatt keygen.

I have a [patatt] section in .gitconfig which contains:
signingkey = ed25519:xxxxxxxx
selector = xxxxxxxx (the same value as above)

What should I do now for you to get the key ? I don't even know where
the key is stored in my computer.

Thanks
Christophe

2022-04-14 08:58:43

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm/bugs: Don't expect thread termination without CONFIG_UBSAN_TRAP

On Wed, Apr 13, 2022 at 04:57:14PM -0400, Konstantin Ryabitsev wrote:
> On Tue, Apr 12, 2022 at 04:06:20PM -0700, Kees Cook wrote:
> > Also, Konstantin, I note that
> > https://git.kernel.org/pub/scm/docs/kernel/pgpkeys.git/
> > does not have a .keyring/ed25519 directory. Should it?
>
> No, because it's not a "pgpkey". :)
>
> > I added one
> > locally for at least one other developer, as I use this setting:
> >
> > [patatt]
> > keyringsrc = ~/korg/pgpkeys/.keyring
> >
> > Am I holding this thing wrong? :)
>
> Nope, but you can also list multiple locations where patatt can look, for
> example:
>
> [patatt]
> keyringsrc = ~/korg/pgpkeys/.keyring
> keyringsrc = ~/.local/share/patatt/public
>
> In fact, if you take Christophe's patches all the time, you can add a keyring
> ref to your tree. The process is documented here:
> https://github.com/mricon/patatt#managing-the-keyring-large-teams
>
> This way I'm not managing the keys of your trusted contributors.
>
> I'll be happy to explain further -- in fact, I'm happy anyone uses it at all!
> :)

I read emails out of order. :) Thanks!

If the expectation is other kernel devs are using ed25519 over gpg,
should there be a central repo for those?

--
Kees Cook

2022-04-14 14:02:57

by Konstantin Ryabitsev

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm/bugs: Don't expect thread termination without CONFIG_UBSAN_TRAP

On Wed, Apr 13, 2022 at 06:29:36AM +0000, Christophe Leroy wrote:
> I have a [patatt] section in .gitconfig which contains:
> signingkey = ed25519:xxxxxxxx
> selector = xxxxxxxx (the same value as above)
>
> What should I do now for you to get the key ? I don't even know where
> the key is stored in my computer.

Your key is stored in ~/.local/share/patatt, but you don't really need to do
anything, Kees can do the following:

b4 kr --show-keys 363b58690e907c677252467a94fe49444c80ea76.1649704381.git.christophe.leroy@csgroup.eu

For now, this just provides instructions on what to do with the key:

[email protected]: (unknown)
keytype: ed25519
pubkey: HIzTzUj91asvincQGOFx6+ZF5AoUuP9GdOtQChs7Mm0=
krpath: ed25519/csgroup.eu/christophe.leroy/20211009
fullpath: /home/user/.local/share/b4/keyring/ed25519/csgroup.eu/christophe.leroy/20211009
---
For ed25519 keys:
echo [pubkey] > [fullpath]

So, for Kees to start being aware of your key, he needs to do:

mkdir -p /home/user/.local/share/b4/keyring/ed25519/csgroup.eu/christophe.leroy
echo HIzTzUj91asvincQGOFx6+ZF5AoUuP9GdOtQChs7Mm0= > /home/user/.local/share/b4/keyring/ed25519/csgroup.eu/christophe.leroy/20211009

I know this is awkward and clunky right now. Future versions of b4 will
streamline keyring management to make it a lot easier, I promise.

-K

2022-04-14 15:24:26

by Konstantin Ryabitsev

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm/bugs: Don't expect thread termination without CONFIG_UBSAN_TRAP

On Tue, Apr 12, 2022 at 04:06:20PM -0700, Kees Cook wrote:
> Also, Konstantin, I note that
> https://git.kernel.org/pub/scm/docs/kernel/pgpkeys.git/
> does not have a .keyring/ed25519 directory. Should it?

No, because it's not a "pgpkey". :)

> I added one
> locally for at least one other developer, as I use this setting:
>
> [patatt]
> keyringsrc = ~/korg/pgpkeys/.keyring
>
> Am I holding this thing wrong? :)

Nope, but you can also list multiple locations where patatt can look, for
example:

[patatt]
keyringsrc = ~/korg/pgpkeys/.keyring
keyringsrc = ~/.local/share/patatt/public

In fact, if you take Christophe's patches all the time, you can add a keyring
ref to your tree. The process is documented here:
https://github.com/mricon/patatt#managing-the-keyring-large-teams

This way I'm not managing the keys of your trusted contributors.

I'll be happy to explain further -- in fact, I'm happy anyone uses it at all!
:)

-K

2022-04-14 15:32:01

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm/bugs: Don't expect thread termination without CONFIG_UBSAN_TRAP

On Wed, Apr 13, 2022 at 05:01:31PM -0400, Konstantin Ryabitsev wrote:
> On Wed, Apr 13, 2022 at 06:29:36AM +0000, Christophe Leroy wrote:
> > I have a [patatt] section in .gitconfig which contains:
> > signingkey = ed25519:xxxxxxxx
> > selector = xxxxxxxx (the same value as above)
> >
> > What should I do now for you to get the key ? I don't even know where
> > the key is stored in my computer.
>
> Your key is stored in ~/.local/share/patatt, but you don't really need to do
> anything, Kees can do the following:
>
> b4 kr --show-keys 363b58690e907c677252467a94fe49444c80ea76.1649704381.git.christophe.leroy@csgroup.eu

Ah-ha, excellent.

>
> For now, this just provides instructions on what to do with the key:
>
> [email protected]: (unknown)
> keytype: ed25519
> pubkey: HIzTzUj91asvincQGOFx6+ZF5AoUuP9GdOtQChs7Mm0=
> krpath: ed25519/csgroup.eu/christophe.leroy/20211009
> fullpath: /home/user/.local/share/b4/keyring/ed25519/csgroup.eu/christophe.leroy/20211009

"fullpath" seems misleading for my config, given:

[patatt]
...
keyringsrc = ~/korg/pgpkeys/.keyring

Shouldn't this report fullpath as:

/home/kees/korg/pgpkeys/.keyring/ed25519/csgroup.eu/christophe.leroy/20211009

And as a side note, should I prefer .local/share/b4/keyring over adding
keys to a branch of the kernel keyring git tree?

> ---
> For ed25519 keys:
> echo [pubkey] > [fullpath]
>
> So, for Kees to start being aware of your key, he needs to do:
>
> mkdir -p /home/user/.local/share/b4/keyring/ed25519/csgroup.eu/christophe.leroy
> echo HIzTzUj91asvincQGOFx6+ZF5AoUuP9GdOtQChs7Mm0= > /home/user/.local/share/b4/keyring/ed25519/csgroup.eu/christophe.leroy/20211009
>
> I know this is awkward and clunky right now. Future versions of b4 will
> streamline keyring management to make it a lot easier, I promise.

Thanks for this walk-through! I think I managed this in the past with
another ed25519 key, but I failed to figure it out this time. ;)

Now it works! :)

✓ [PATCH v2] lkdtm/bugs: Don't expect thread termination without
CONFIG_UBSAN_TRAP
+ Signed-off-by: Kees Cook <[email protected]>
+ Link: https://lore.kernel.org/r/363b58690e907c677252467a94fe49444c80ea76.1649704381.git.christophe.leroy@csgroup.eu
---
✓ Signed: ed25519/[email protected]


--
Kees Cook