diff mbox series

[v2] lkdtm/bugs: Don't expect thread termination without CONFIG_UBSAN_TRAP

Message ID 363b58690e907c677252467a94fe49444c80ea76.1649704381.git.christophe.leroy@csgroup.eu (mailing list archive)
State New
Headers show
Series [v2] lkdtm/bugs: Don't expect thread termination without CONFIG_UBSAN_TRAP | expand

Commit Message

Christophe Leroy April 11, 2022, 7:13 p.m. UTC 
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 <christophe.leroy@csgroup.eu>
---
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(-)

Comments

Kees Cook April 12, 2022, 11:06 p.m. UTC | #1 
On Mon, Apr 11, 2022 at 09:13:39PM +0200, Christophe Leroy wrote:
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

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 <keescook@chromium.org>
    + Link: https://lore.kernel.org/r/363b58690e907c677252467a94fe49444c80ea76.1649704381.git.christophe.leroy@csgroup.eu
  ---
  ✗ No key: ed25519/christophe.leroy@csgroup.eu

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? :)
Christophe Leroy April 13, 2022, 6:29 a.m. UTC | #2 
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 <christophe.leroy@csgroup.eu>
> 
> 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 <keescook@chromium.org>
>      + Link: https://lore.kernel.org/r/363b58690e907c677252467a94fe49444c80ea76.1649704381.git.christophe.leroy@csgroup.eu
>    ---
>    ✗ No key: ed25519/christophe.leroy@csgroup.eu
> 
> 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
Konstantin Ryabitsev April 13, 2022, 8:57 p.m. UTC | #3 
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
Konstantin Ryabitsev April 13, 2022, 9:01 p.m. UTC | #4 
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:

	christophe.leroy@csgroup.eu: (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
Kees Cook April 13, 2022, 9:20 p.m. UTC | #5 
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:
> 
> 	christophe.leroy@csgroup.eu: (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 <keescook@chromium.org>
    + Link: https://lore.kernel.org/r/363b58690e907c677252467a94fe49444c80ea76.1649704381.git.christophe.leroy@csgroup.eu
  ---
  ✓ Signed: ed25519/christophe.leroy@csgroup.eu
Kees Cook April 13, 2022, 9:22 p.m. UTC | #6 
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?
diff mbox series

Patch

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);