* Re: [PATCH] docs: packing: move it to core-api book and adjust markups
From: Vladimir Oltean @ 2019-07-04 23:59 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, lkml,
Jonathan Corbet, Mike Rapoport, Andrew Morton, Randy Dunlap,
Matthew Wilcox, Kent Overstreet, Arnd Bergmann,
Jonathan Neuschäfer, David S. Miller, netdev
In-Reply-To: <46cb79dbc4bbff3e5a4e77b548df1e92c105ed0f.1561804613.git.mchehab+samsung@kernel.org>
On Sat, 29 Jun 2019 at 13:37, Mauro Carvalho Chehab
<mchehab+samsung@kernel.org> wrote:
>
> The packing.txt file was misplaced, as docs should be part of
> a documentation book, and not at the root dir.
>
> So, move it to the core-api directory and add to its index.
>
> Also, ensure that the file will be properly parsed and the bitmap
> ascii artwork will use a monotonic font.
>
> Fixes: 554aae35007e ("lib: Add support for generic packing operations")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Tested-by: Vladimir Oltean <olteanv@gmail.com>
> ---
> Documentation/core-api/index.rst | 1 +
> .../{packing.txt => core-api/packing.rst} | 81 +++++++++++--------
> 2 files changed, 50 insertions(+), 32 deletions(-)
> rename Documentation/{packing.txt => core-api/packing.rst} (61%)
>
> diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
> index d1e5b95bf86d..aebb16d7771f 100644
> --- a/Documentation/core-api/index.rst
> +++ b/Documentation/core-api/index.rst
> @@ -25,6 +25,7 @@ Core utilities
> librs
> genalloc
> errseq
> + packing
> printk-formats
> circular-buffers
> generic-radix-tree
> diff --git a/Documentation/packing.txt b/Documentation/core-api/packing.rst
> similarity index 61%
> rename from Documentation/packing.txt
> rename to Documentation/core-api/packing.rst
> index f830c98645f1..d8c341fe383e 100644
> --- a/Documentation/packing.txt
> +++ b/Documentation/core-api/packing.rst
> @@ -30,6 +30,7 @@ The solution
> ------------
>
> This API deals with 2 basic operations:
> +
> - Packing a CPU-usable number into a memory buffer (with hardware
> constraints/quirks)
> - Unpacking a memory buffer (which has hardware constraints/quirks)
> @@ -49,10 +50,12 @@ What the examples show is where the logical bytes and bits sit.
>
> 1. Normally (no quirks), we would do it like this:
>
> -63 62 61 60 59 58 57 56 55 54 53 52 51 50 49 48 47 46 45 44 43 42 41 40 39 38 37 36 35 34 33 32
> -7 6 5 4
> -31 30 29 28 27 26 25 24 23 22 21 20 19 18 17 16 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0
> -3 2 1 0
> +::
> +
> + 63 62 61 60 59 58 57 56 55 54 53 52 51 50 49 48 47 46 45 44 43 42 41 40 39 38 37 36 35 34 33 32
> + 7 6 5 4
> + 31 30 29 28 27 26 25 24 23 22 21 20 19 18 17 16 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0
> + 3 2 1 0
>
> That is, the MSByte (7) of the CPU-usable u64 sits at memory offset 0, and the
> LSByte (0) of the u64 sits at memory offset 7.
> @@ -63,10 +66,12 @@ comments as "logical" notation.
>
> 2. If QUIRK_MSB_ON_THE_RIGHT is set, we do it like this:
>
> -56 57 58 59 60 61 62 63 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47 32 33 34 35 36 37 38 39
> -7 6 5 4
> -24 25 26 27 28 29 30 31 16 17 18 19 20 21 22 23 8 9 10 11 12 13 14 15 0 1 2 3 4 5 6 7
> -3 2 1 0
> +::
If this is not too stylistically different from the rest of the kernel
docs, the RST syntax actually allows you to do "we do it like this::"
(with the two colons coming right after the text and not on their own
line, which looks more natural). The same comment applies to the other
changes below.
> +
> + 56 57 58 59 60 61 62 63 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47 32 33 34 35 36 37 38 39
> + 7 6 5 4
> + 24 25 26 27 28 29 30 31 16 17 18 19 20 21 22 23 8 9 10 11 12 13 14 15 0 1 2 3 4 5 6 7
> + 3 2 1 0
>
> That is, QUIRK_MSB_ON_THE_RIGHT does not affect byte positioning, but
> inverts bit offsets inside a byte.
> @@ -74,10 +79,12 @@ inverts bit offsets inside a byte.
>
> 3. If QUIRK_LITTLE_ENDIAN is set, we do it like this:
>
> -39 38 37 36 35 34 33 32 47 46 45 44 43 42 41 40 55 54 53 52 51 50 49 48 63 62 61 60 59 58 57 56
> -4 5 6 7
> -7 6 5 4 3 2 1 0 15 14 13 12 11 10 9 8 23 22 21 20 19 18 17 16 31 30 29 28 27 26 25 24
> -0 1 2 3
> +::
> +
> + 39 38 37 36 35 34 33 32 47 46 45 44 43 42 41 40 55 54 53 52 51 50 49 48 63 62 61 60 59 58 57 56
> + 4 5 6 7
> + 7 6 5 4 3 2 1 0 15 14 13 12 11 10 9 8 23 22 21 20 19 18 17 16 31 30 29 28 27 26 25 24
> + 0 1 2 3
>
> Therefore, QUIRK_LITTLE_ENDIAN means that inside the memory region, every
> byte from each 4-byte word is placed at its mirrored position compared to
> @@ -86,18 +93,22 @@ the boundary of that word.
> 4. If QUIRK_MSB_ON_THE_RIGHT and QUIRK_LITTLE_ENDIAN are both set, we do it
> like this:
>
> -32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63
> -4 5 6 7
> -0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> -0 1 2 3
> +::
> +
> + 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63
> + 4 5 6 7
> + 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> + 0 1 2 3
>
>
> 5. If just QUIRK_LSW32_IS_FIRST is set, we do it like this:
>
> -31 30 29 28 27 26 25 24 23 22 21 20 19 18 17 16 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0
> -3 2 1 0
> -63 62 61 60 59 58 57 56 55 54 53 52 51 50 49 48 47 46 45 44 43 42 41 40 39 38 37 36 35 34 33 32
> -7 6 5 4
> +::
> +
> + 31 30 29 28 27 26 25 24 23 22 21 20 19 18 17 16 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0
> + 3 2 1 0
> + 63 62 61 60 59 58 57 56 55 54 53 52 51 50 49 48 47 46 45 44 43 42 41 40 39 38 37 36 35 34 33 32
> + 7 6 5 4
>
> In this case the 8 byte memory region is interpreted as follows: first
> 4 bytes correspond to the least significant 4-byte word, next 4 bytes to
> @@ -107,28 +118,34 @@ the more significant 4-byte word.
> 6. If QUIRK_LSW32_IS_FIRST and QUIRK_MSB_ON_THE_RIGHT are set, we do it like
> this:
>
> -24 25 26 27 28 29 30 31 16 17 18 19 20 21 22 23 8 9 10 11 12 13 14 15 0 1 2 3 4 5 6 7
> -3 2 1 0
> -56 57 58 59 60 61 62 63 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47 32 33 34 35 36 37 38 39
> -7 6 5 4
> +::
> +
> + 24 25 26 27 28 29 30 31 16 17 18 19 20 21 22 23 8 9 10 11 12 13 14 15 0 1 2 3 4 5 6 7
> + 3 2 1 0
> + 56 57 58 59 60 61 62 63 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47 32 33 34 35 36 37 38 39
> + 7 6 5 4
>
>
> 7. If QUIRK_LSW32_IS_FIRST and QUIRK_LITTLE_ENDIAN are set, it looks like
> this:
>
> -7 6 5 4 3 2 1 0 15 14 13 12 11 10 9 8 23 22 21 20 19 18 17 16 31 30 29 28 27 26 25 24
> -0 1 2 3
> -39 38 37 36 35 34 33 32 47 46 45 44 43 42 41 40 55 54 53 52 51 50 49 48 63 62 61 60 59 58 57 56
> -4 5 6 7
> +::
> +
> + 7 6 5 4 3 2 1 0 15 14 13 12 11 10 9 8 23 22 21 20 19 18 17 16 31 30 29 28 27 26 25 24
> + 0 1 2 3
> + 39 38 37 36 35 34 33 32 47 46 45 44 43 42 41 40 55 54 53 52 51 50 49 48 63 62 61 60 59 58 57 56
> + 4 5 6 7
>
>
> 8. If QUIRK_LSW32_IS_FIRST, QUIRK_LITTLE_ENDIAN and QUIRK_MSB_ON_THE_RIGHT
> are set, it looks like this:
>
> -0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> -0 1 2 3
> -32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63
> -4 5 6 7
> +::
> +
> + 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> + 0 1 2 3
> + 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63
> + 4 5 6 7
>
>
> We always think of our offsets as if there were no quirk, and we translate
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH] checkpatch: Added warnings in favor of strscpy().
From: Joe Perches @ 2019-07-04 20:46 UTC (permalink / raw)
To: Nitin Gote, akpm
Cc: corbet, apw, keescook, linux-doc, linux-kernel, kernel-hardening
In-Reply-To: <1562219683-15474-1-git-send-email-nitin.r.gote@intel.com>
On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> Added warnings in checkpatch.pl script to :
>
> 1. Deprecate strcpy() in favor of strscpy().
> 2. Deprecate strlcpy() in favor of strscpy().
> 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
>
> Updated strncpy() section in Documentation/process/deprecated.rst
> to cover strscpy_pad() case.
>
> Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
OK, for whatever reason, this when into a spam folder.
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -595,6 +595,11 @@ our %deprecated_apis = (
> "rcu_barrier_sched" => "rcu_barrier",
> "get_state_synchronize_sched" => "get_state_synchronize_rcu",
> "cond_synchronize_sched" => "cond_synchronize_rcu",
> + "strcpy" => "strscpy",
> + "strlcpy" => "strscpy",
> + "strncpy" => "strscpy, strscpy_pad or for
> + non-NUL-terminated strings, strncpy() can still be used, but
> + destinations should be marked with the __nonstring",
> );
$ git grep -w strcpy | wc -l
2239
$ git grep -w strlcpy | wc -l
1760
$ git grep -w strncpy | wc -l
839
These functions are _really_ commonly used in the kernel.
This should probably be a different %deprecated_string_api
and these should probably not be emitted at WARN level
when using command line option -f/--file but at CHECK level
so that novice script users just don't send bad patches.
Also, perhaps there could be some macro for the relatively
commonly used
strscpy(foo, bar, sizeof(foo))
and
strlcpy(foo, bar, sizeof(foo))
so argument 1 doesn't have to be repeated in the sizeof()
Something like:
#define stracpy(to, from) \
({ \
size_t size = ARRAY_SIZE(to); \
BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
\
strscpy(to, from, size); \
})
^ permalink raw reply
* Re: [RFC PATCH] binfmt_elf: Extract .note.gnu.property from an ELF file
From: Pavel Machek @ 2019-07-04 19:50 UTC (permalink / raw)
To: Jann Horn
Cc: Yu-cheng Yu, the arch/x86 maintainers, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, kernel list, linux-doc, Linux-MM,
linux-arch, Linux API, Arnd Bergmann, Andy Lutomirski,
Balbir Singh, Borislav Petkov, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, Florian Weimer, H.J. Lu, Jonathan Corbet,
Kees Cook, Mike Kravetz, Nadav Amit, Oleg Nesterov,
Peter Zijlstra, Randy Dunlap, Ravi V. Shankar, Vedvyas Shanbhogue,
Dave Martin
In-Reply-To: <CAG48ez0rHHfcRgiVZf5FP0YOzxsXigvpg6ci790cmiN6PBwkhQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]
Hi!
> > +static int scan(u8 *buf, u32 buf_size, int item_size, test_item_fn test_item,
> > + next_item_fn next_item, u32 *arg, u32 type, u32 *pos)
> > +{
> > + int found = 0;
> > + u8 *p, *max;
> > +
> > + max = buf + buf_size;
> > + if (max < buf)
> > + return 0;
>
> How can this ever legitimately happen? If it can't, perhaps you meant
> to put a WARN_ON_ONCE() or something like that here?
> Also, computing out-of-bounds pointers is UB (section 6.5.6 of C99:
> "If both the pointer operand and the result point to elements of the
> same array object, or one past the last element of the array object,
> the evaluation shall not produce an overflow; otherwise, the behavior
> is undefined."), and if the addition makes the pointer wrap, that's
> certainly out of bounds; so I don't think this condition can trigger
> without UB.
Kernel assumes sane compiler. We pass flags to get it... C99 does not
quite apply here.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v12 01/11] MODSIGN: Export module signature definitions
From: Thiago Jung Bauermann @ 2019-07-04 18:57 UTC (permalink / raw)
To: Philipp Rudo
Cc: Jessica Yu, linux-integrity, linux-security-module, keyrings,
linux-crypto, linuxppc-dev, linux-doc, linux-kernel, Mimi Zohar,
Dmitry Kasatkin, James Morris, Serge E. Hallyn, David Howells,
David Woodhouse, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Heiko Carstens, linux-s390
In-Reply-To: <20190704125427.31146026@laptop-ibm>
Hello Philipp,
Philipp Rudo <prudo@linux.ibm.com> writes:
> Hi Thiago,
>
>
> On Thu, 04 Jul 2019 03:42:57 -0300
> Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
>
>> Jessica Yu <jeyu@kernel.org> writes:
>>
>> > +++ Thiago Jung Bauermann [27/06/19 23:19 -0300]:
>> >>IMA will use the module_signature format for append signatures, so export
>> >>the relevant definitions and factor out the code which verifies that the
>> >>appended signature trailer is valid.
>> >>
>> >>Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
>> >>and be able to use mod_check_sig() without having to depend on either
>> >>CONFIG_MODULE_SIG or CONFIG_MODULES.
>> >>
>> >>Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> >>Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> >>Cc: Jessica Yu <jeyu@kernel.org>
>> >>---
>> >> include/linux/module.h | 3 --
>> >> include/linux/module_signature.h | 44 +++++++++++++++++++++++++
>> >> init/Kconfig | 6 +++-
>> >> kernel/Makefile | 1 +
>> >> kernel/module.c | 1 +
>> >> kernel/module_signature.c | 46 ++++++++++++++++++++++++++
>> >> kernel/module_signing.c | 56 +++++---------------------------
>> >> scripts/Makefile | 2 +-
>> >> 8 files changed, 106 insertions(+), 53 deletions(-)
>> >>
>> >>diff --git a/include/linux/module.h b/include/linux/module.h
>> >>index 188998d3dca9..aa56f531cf1e 100644
>> >>--- a/include/linux/module.h
>> >>+++ b/include/linux/module.h
>> >>@@ -25,9 +25,6 @@
>> >> #include <linux/percpu.h>
>> >> #include <asm/module.h>
>> >>
>> >>-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
>> >>-#define MODULE_SIG_STRING "~Module signature appended~\n"
>> >>-
>> >
>> > Hi Thiago, apologies for the delay.
>>
>> Hello Jessica, thanks for reviewing the patch!
>>
>> > It looks like arch/s390/kernel/machine_kexec_file.c also relies on
>> > MODULE_SIG_STRING being defined, so module_signature.h will need to be
>> > included there too, otherwise we'll run into a compilation error.
>>
>> Indeed. Thanks for spotting that. The patch below fixes it. It's
>> identical to the previous version except for the changes in
>> arch/s390/kernel/machine_kexec_file.c and their description in the
>> commit message. I'm also copying some s390 people in this email.
>
> to me the s390 part looks good but for one minor nit.
Thanks for the prompt review!
> In arch/s390/Kconfig KEXEC_VERIFY_SIG currently depends on
> SYSTEM_DATA_VERIFICATION. I'd prefer when you update this to the new
> MODULE_SIG_FORMAT. It shouldn't make any difference right now, as we don't
> use mod_check_sig in our code path. But it could cause problems in the future,
> when more code might be shared.
Makes sense. Here is the updated patch with the Kconfig change.
--
Thiago Jung Bauermann
IBM Linux Technology Center
From d0e870a6eccc7126c0416ad7369888052c15eb18 Mon Sep 17 00:00:00 2001
From: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Date: Thu, 17 May 2018 21:46:12 -0300
Subject: [PATCH 1/1] MODSIGN: Export module signature definitions
IMA will use the module_signature format for append signatures, so export
the relevant definitions and factor out the code which verifies that the
appended signature trailer is valid.
Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
and be able to use mod_check_sig() without having to depend on either
CONFIG_MODULE_SIG or CONFIG_MODULES.
s390 duplicated the definition of struct module_signature so now they can
use the new <linux/module_signature.h> header instead.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Acked-by: Jessica Yu <jeyu@kernel.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Philipp Rudo <prudo@linux.ibm.com>
---
arch/s390/Kconfig | 2 +-
arch/s390/kernel/machine_kexec_file.c | 24 +-----------
include/linux/module.h | 3 --
include/linux/module_signature.h | 44 +++++++++++++++++++++
init/Kconfig | 6 ++-
kernel/Makefile | 1 +
kernel/module.c | 1 +
kernel/module_signature.c | 46 ++++++++++++++++++++++
kernel/module_signing.c | 56 ++++-----------------------
scripts/Makefile | 2 +-
10 files changed, 108 insertions(+), 77 deletions(-)
create mode 100644 include/linux/module_signature.h
create mode 100644 kernel/module_signature.c
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 109243fdb6ec..446b7ffa1294 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -557,7 +557,7 @@ config ARCH_HAS_KEXEC_PURGATORY
config KEXEC_VERIFY_SIG
bool "Verify kernel signature during kexec_file_load() syscall"
- depends on KEXEC_FILE && SYSTEM_DATA_VERIFICATION
+ depends on KEXEC_FILE && MODULE_SIG_FORMAT
help
This option makes kernel signature verification mandatory for
the kexec_file_load() syscall.
diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
index fbdd3ea73667..1ac9fbc6e01e 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -10,7 +10,7 @@
#include <linux/elf.h>
#include <linux/errno.h>
#include <linux/kexec.h>
-#include <linux/module.h>
+#include <linux/module_signature.h>
#include <linux/verification.h>
#include <asm/boot_data.h>
#include <asm/ipl.h>
@@ -23,28 +23,6 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
};
#ifdef CONFIG_KEXEC_VERIFY_SIG
-/*
- * Module signature information block.
- *
- * The constituents of the signature section are, in order:
- *
- * - Signer's name
- * - Key identifier
- * - Signature data
- * - Information block
- */
-struct module_signature {
- u8 algo; /* Public-key crypto algorithm [0] */
- u8 hash; /* Digest algorithm [0] */
- u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
- u8 signer_len; /* Length of signer's name [0] */
- u8 key_id_len; /* Length of key identifier [0] */
- u8 __pad[3];
- __be32 sig_len; /* Length of signature data */
-};
-
-#define PKEY_ID_PKCS7 2
-
int s390_verify_sig(const char *kernel, unsigned long kernel_len)
{
const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
diff --git a/include/linux/module.h b/include/linux/module.h
index 188998d3dca9..aa56f531cf1e 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -25,9 +25,6 @@
#include <linux/percpu.h>
#include <asm/module.h>
-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
-#define MODULE_SIG_STRING "~Module signature appended~\n"
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
new file mode 100644
index 000000000000..523617fc5b6a
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#ifndef _LINUX_MODULE_SIGNATURE_H
+#define _LINUX_MODULE_SIGNATURE_H
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+enum pkey_id_type {
+ PKEY_ID_PGP, /* OpenPGP generated key ID */
+ PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
+ PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
+};
+
+/*
+ * Module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ * - Signer's name
+ * - Key identifier
+ * - Signature data
+ * - Information block
+ */
+struct module_signature {
+ u8 algo; /* Public-key crypto algorithm [0] */
+ u8 hash; /* Digest algorithm [0] */
+ u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
+ u8 signer_len; /* Length of signer's name [0] */
+ u8 key_id_len; /* Length of key identifier [0] */
+ u8 __pad[3];
+ __be32 sig_len; /* Length of signature data */
+};
+
+int mod_check_sig(const struct module_signature *ms, size_t file_len,
+ const char *name);
+
+#endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/init/Kconfig b/init/Kconfig
index 8b9ffe236e4f..c2286a3c74c5 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1852,6 +1852,10 @@ config BASE_SMALL
default 0 if BASE_FULL
default 1 if !BASE_FULL
+config MODULE_SIG_FORMAT
+ def_bool n
+ select SYSTEM_DATA_VERIFICATION
+
menuconfig MODULES
bool "Enable loadable module support"
option modules
@@ -1929,7 +1933,7 @@ config MODULE_SRCVERSION_ALL
config MODULE_SIG
bool "Module signature verification"
depends on MODULES
- select SYSTEM_DATA_VERIFICATION
+ select MODULE_SIG_FORMAT
help
Check modules for valid signatures upon load: the signature
is simply appended to the module. For more information see
diff --git a/kernel/Makefile b/kernel/Makefile
index 33824f0385b3..f29ae2997a43 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -58,6 +58,7 @@ endif
obj-$(CONFIG_UID16) += uid16.o
obj-$(CONFIG_MODULES) += module.o
obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signature.o
obj-$(CONFIG_KALLSYMS) += kallsyms.o
obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
obj-$(CONFIG_CRASH_CORE) += crash_core.o
diff --git a/kernel/module.c b/kernel/module.c
index 6e6712b3aaf5..2712f4d217f5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -19,6 +19,7 @@
#include <linux/export.h>
#include <linux/extable.h>
#include <linux/moduleloader.h>
+#include <linux/module_signature.h>
#include <linux/trace_events.h>
#include <linux/init.h>
#include <linux/kallsyms.h>
diff --git a/kernel/module_signature.c b/kernel/module_signature.c
new file mode 100644
index 000000000000..4224a1086b7d
--- /dev/null
+++ b/kernel/module_signature.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Module signature checker
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#include <linux/errno.h>
+#include <linux/printk.h>
+#include <linux/module_signature.h>
+#include <asm/byteorder.h>
+
+/**
+ * mod_check_sig - check that the given signature is sane
+ *
+ * @ms: Signature to check.
+ * @file_len: Size of the file to which @ms is appended.
+ * @name: What is being checked. Used for error messages.
+ */
+int mod_check_sig(const struct module_signature *ms, size_t file_len,
+ const char *name)
+{
+ if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
+ return -EBADMSG;
+
+ if (ms->id_type != PKEY_ID_PKCS7) {
+ pr_err("%s: Module is not signed with expected PKCS#7 message\n",
+ name);
+ return -ENOPKG;
+ }
+
+ if (ms->algo != 0 ||
+ ms->hash != 0 ||
+ ms->signer_len != 0 ||
+ ms->key_id_len != 0 ||
+ ms->__pad[0] != 0 ||
+ ms->__pad[1] != 0 ||
+ ms->__pad[2] != 0) {
+ pr_err("%s: PKCS#7 signature info has unexpected non-zero params\n",
+ name);
+ return -EBADMSG;
+ }
+
+ return 0;
+}
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 6b9a926fd86b..cdd04a6b8074 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,37 +11,13 @@
#include <linux/kernel.h>
#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/module_signature.h>
#include <linux/string.h>
#include <linux/verification.h>
#include <crypto/public_key.h>
#include "module-internal.h"
-enum pkey_id_type {
- PKEY_ID_PGP, /* OpenPGP generated key ID */
- PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
- PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
-};
-
-/*
- * Module signature information block.
- *
- * The constituents of the signature section are, in order:
- *
- * - Signer's name
- * - Key identifier
- * - Signature data
- * - Information block
- */
-struct module_signature {
- u8 algo; /* Public-key crypto algorithm [0] */
- u8 hash; /* Digest algorithm [0] */
- u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
- u8 signer_len; /* Length of signer's name [0] */
- u8 key_id_len; /* Length of key identifier [0] */
- u8 __pad[3];
- __be32 sig_len; /* Length of signature data */
-};
-
/*
* Verify the signature on a module.
*/
@@ -49,6 +25,7 @@ int mod_verify_sig(const void *mod, struct load_info *info)
{
struct module_signature ms;
size_t sig_len, modlen = info->len;
+ int ret;
pr_devel("==>%s(,%zu)\n", __func__, modlen);
@@ -56,32 +33,15 @@ int mod_verify_sig(const void *mod, struct load_info *info)
return -EBADMSG;
memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
- modlen -= sizeof(ms);
+
+ ret = mod_check_sig(&ms, modlen, info->name);
+ if (ret)
+ return ret;
sig_len = be32_to_cpu(ms.sig_len);
- if (sig_len >= modlen)
- return -EBADMSG;
- modlen -= sig_len;
+ modlen -= sig_len + sizeof(ms);
info->len = modlen;
- if (ms.id_type != PKEY_ID_PKCS7) {
- pr_err("%s: Module is not signed with expected PKCS#7 message\n",
- info->name);
- return -ENOPKG;
- }
-
- if (ms.algo != 0 ||
- ms.hash != 0 ||
- ms.signer_len != 0 ||
- ms.key_id_len != 0 ||
- ms.__pad[0] != 0 ||
- ms.__pad[1] != 0 ||
- ms.__pad[2] != 0) {
- pr_err("%s: PKCS#7 signature info has unexpected non-zero params\n",
- info->name);
- return -EBADMSG;
- }
-
return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
VERIFY_USE_SECONDARY_KEYRING,
VERIFYING_MODULE_SIGNATURE,
diff --git a/scripts/Makefile b/scripts/Makefile
index 9d442ee050bd..52098b080ab7 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -17,7 +17,7 @@ hostprogs-$(CONFIG_VT) += conmakehash
hostprogs-$(BUILD_C_RECORDMCOUNT) += recordmcount
hostprogs-$(CONFIG_BUILDTIME_EXTABLE_SORT) += sortextable
hostprogs-$(CONFIG_ASN1) += asn1_compiler
-hostprogs-$(CONFIG_MODULE_SIG) += sign-file
+hostprogs-$(CONFIG_MODULE_SIG_FORMAT) += sign-file
hostprogs-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += extract-cert
hostprogs-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE) += insert-sys-cert
^ permalink raw reply related
* [PATCH] Documentation: input: Add HID gadget driver's docs to Input subsystem
From: Shreeya Patel @ 2019-07-04 18:30 UTC (permalink / raw)
To: skhan, linux-kernel-mentees, dmitry.torokhov, corbet, gregkh,
linux-input, linux-doc, linux-kernel, linux-usb
Convert gadget_hid file to ReST format, in order to allow it to
be parsed by Sphinx.
Also move the file in the Input subsystem documentation so as to
put it in the right place.
Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
---
.../devices/gadget_hid.rst} | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
rename Documentation/{usb/gadget_hid.txt => input/devices/gadget_hid.rst} (96%)
diff --git a/Documentation/usb/gadget_hid.txt b/Documentation/input/devices/gadget_hid.rst
similarity index 96%
rename from Documentation/usb/gadget_hid.txt
rename to Documentation/input/devices/gadget_hid.rst
index 098d563040cc..132a8d6719f0 100644
--- a/Documentation/usb/gadget_hid.txt
+++ b/Documentation/input/devices/gadget_hid.rst
@@ -1,3 +1,5 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
===========================
Linux USB HID gadget driver
===========================
@@ -8,15 +10,15 @@ Introduction
The HID Gadget driver provides emulation of USB Human Interface
Devices (HID). The basic HID handling is done in the kernel,
and HID reports can be sent/received through I/O on the
-/dev/hidgX character devices.
+:file:`/dev/hidgX` character devices.
For more details about HID, see the developer page on
-http://www.usb.org/developers/hidpage/
+`<http://www.usb.org/developers/hidpage/>`_
Configuration
=============
-g_hid is a platform driver, so to use it you need to add
+*g_hid* is a platform driver, so to use it you need to add
struct platform_device(s) to your platform code defining the
HID function descriptors you want to use - E.G. something
like::
@@ -89,16 +91,16 @@ Send and receive HID reports
============================
HID reports can be sent/received using read/write on the
-/dev/hidgX character devices. See below for an example program
+:file:`/dev/hidgX` character devices. See below for an example program
to do this.
-hid_gadget_test is a small interactive program to test the HID
+*hid_gadget_test* is a small interactive program to test the HID
gadget driver. To use, point it at a hidg device and set the
device type (keyboard / mouse / joystick) - E.G.::
# hid_gadget_test /dev/hidg0 keyboard
-You are now in the prompt of hid_gadget_test. You can type any
+You are now in the prompt of *hid_gadget_test*. You can type any
combination of options and values. Available options and
values are listed at program start. In keyboard mode you can
send up to six values.
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
From: Ilias Apalodimas @ 2019-07-04 18:11 UTC (permalink / raw)
To: Thirupathaiah Annapureddy
Cc: Jarkko Sakkinen, Sasha Levin, peterhuewe@gmx.de, jgg@ziepe.ca,
corbet@lwn.net, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org,
Microsoft Linux Kernel List, Bryan Kelly (CSI),
tee-dev@lists.linaro.org, sumit.garg@linaro.org,
rdunlap@infradead.org, Joakim Bech
In-Reply-To: <CY4PR21MB02791B5EF653514DC0223694BCFA0@CY4PR21MB0279.namprd21.prod.outlook.com>
Hi Thirupathaiah,
[...]
> > > > > I managed to do some quick testing in QEMU.
> > > > > Everything works fine when i build this as a module (using IBM's TPM 2.0
> > > > > TSS)
> > > > >
> > > > > - As module
> > > > > # insmod /lib/modules/5.2.0-rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko
> > > > > # getrandom -by 8
> > > > > randomBytes length 8
> > > > > 23 b9 3d c3 90 13 d9 6b
> > > > >
> > > > > - Built-in
> > > > > # dmesg | grep optee
> > > > > ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed,
> > > > > err=ffff0008
> > > > This (0xffff0008) translates to TEE_ERROR_ITEM_NOT_FOUND.
> > > >
> > > > Where is fTPM TA located in the your test setup?
> > > > Is it stitched into TEE binary as an EARLY_TA or
> > > > Is it expected to be loaded during run-time with the help of user mode OP-
> > TEE supplicant?
> > > >
> > > > My guess is that you are trying to load fTPM TA through user mode OP-TEE
> > supplicant.
> > > > Can you confirm?
> > > I tried both
> > >
> >
> > Ok apparently there was a failure with my built-in binary which i
> > didn't notice. I did a full rebuilt and checked the elf this time :)
> >
> > Built as an earlyTA my error now is:
> > ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session
> > failed, err=ffff3024 (translates to TEE_ERROR_TARGET_DEAD)
> > Since you tested it on real hardware i guess you tried both
> > module/built-in. Which TEE version are you using?
>
> I am glad that the first issue (TEE_ERROR_ITEM_NOT_FOUND) is resolved after stitching
> fTPM TA as an EARLY_TA.
>
> Regarding TEE_ERROR_TARGET_DEAD error, may I know which HW platform you are using to test?
QEMU, on armv7
> What is the preboot environment (UEFI or U-boot)?
> Where is the secure storage in that HW platform?
> I could think of two classes of secure storage.
> 1. UFS/eMMC RPMB : If Supplicant in U-boot/UEFI initializes the
> fTPM TA NV Storage, there should be no issue.
> If fTPM TA NV storage is not initialized in pre-boot environment and you are using
> built-in fTPM Linux driver, you can run into this issue as TA will try to initialize
> NV store and fail.
>
> 2. other storage devices like QSPI accessible to only secure mode after
> EBS/ReadyToBoot mile posts during boot. In this case, there should be no issue at all
> as there is no dependency on non-secure side services provided by supplicant.
>
Please check the previous mail from Sumit. It explains exaclty what's going on.
The tl;dr version is that the storage is up only when the supplicant is running.
> If you let me know the HW platform details, I am happy to work with you to enable/integrate
> fTPM TA on that HW platform.
>
Thanks,
The hardware i am waiting for for has an eMMC RPMB. In theory the U-Boot
supplicant support will be there so i'll be able to test it.
Thanks
/Ilias
^ permalink raw reply
* [PATCH v2 0/3] x86/boot: Introduce the kernel_info et consortes
From: Daniel Kiper @ 2019-07-04 16:36 UTC (permalink / raw)
To: linux-doc, linux-kernel, x86
Cc: bp, corbet, dpsmith, eric.snowberg, hpa, kanth.ghatraju,
konrad.wilk, mingo, ross.philipson, tglx
Hi,
Due to very limited space in the setup_header this patch series introduces new
kernel_info struct which will be used to convey information from the kernel to
the bootloader. This way the boot protocol can be extended regardless of the
setup_header limitations. Additionally, the patch series introduces some
convenience features like the setup_indirect struct and the
kernel_info.setup_type_max field.
Daniel
Documentation/x86/boot.rst | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
arch/x86/boot/Makefile | 2 +-
arch/x86/boot/compressed/Makefile | 4 +--
arch/x86/boot/compressed/kernel_info.S | 16 ++++++++++
arch/x86/boot/header.S | 3 +-
arch/x86/boot/tools/build.c | 5 +++
arch/x86/include/uapi/asm/bootparam.h | 15 ++++++++-
7 files changed, 173 insertions(+), 5 deletions(-)
Daniel Kiper (3):
x86/boot: Introduce the kernel_info
x86/boot: Introduce the setup_indirect
x86/boot: Introduce the kernel_info.setup_type_max
^ permalink raw reply
* [PATCH v2 2/3] x86/boot: Introduce the setup_indirect
From: Daniel Kiper @ 2019-07-04 16:36 UTC (permalink / raw)
To: linux-doc, linux-kernel, x86
Cc: bp, corbet, dpsmith, eric.snowberg, hpa, kanth.ghatraju,
konrad.wilk, mingo, ross.philipson, tglx
In-Reply-To: <20190704163612.14311-1-daniel.kiper@oracle.com>
The setup_data is a bit awkward to use for extremely large data objects,
both because the setup_data header has to be adjacent to the data object
and because it has a 32-bit length field. However, it is important that
intermediate stages of the boot process have a way to identify which
chunks of memory are occupied by kernel data.
Thus we introduce a uniform way to specify such indirect data as
setup_indirect struct and SETUP_INDIRECT type.
This patch does not bump setup_header version in arch/x86/boot/header.S
because it will be followed by additional changes coming into the
Linux/x86 boot protocol.
Suggested-by: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Ross Philipson <ross.philipson@oracle.com>
---
v2 - suggestions/fixes:
- add setup_indirect usage example
(suggested by Eric Snowberg and Ross Philipson).
---
Documentation/x86/boot.rst | 38 ++++++++++++++++++++++++++++++++++-
arch/x86/include/uapi/asm/bootparam.h | 11 +++++++++-
2 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
index a934a56f0516..23d3726d54fc 100644
--- a/Documentation/x86/boot.rst
+++ b/Documentation/x86/boot.rst
@@ -73,7 +73,7 @@ Protocol 2.14: BURNT BY INCORRECT COMMIT ae7e1238e68f2a472a125673ab506d49158c188
(x86/boot: Add ACPI RSDP address to setup_header)
DO NOT USE!!! ASSUME SAME AS 2.13.
-Protocol 2.15: (Kernel 5.3) Added the kernel_info.
+Protocol 2.15: (Kernel 5.3) Added the kernel_info and setup_indirect.
============= ============================================================
.. note::
@@ -827,6 +827,42 @@ Protocol: 2.09+
sure to consider the case where the linked list already contains
entries.
+ The setup_data is a bit awkward to use for extremely large data objects,
+ both because the setup_data header has to be adjacent to the data object
+ and because it has a 32-bit length field. However, it is important that
+ intermediate stages of the boot process have a way to identify which
+ chunks of memory are occupied by kernel data.
+
+ Thus setup_indirect struct and SETUP_INDIRECT type were introduced in
+ protocol 2.15.
+
+ struct setup_indirect {
+ __u32 type;
+ __u32 reserved; /* Reserved, must be set to zero. */
+ __u64 len;
+ __u64 addr;
+ };
+
+ The type member is itself simply a SETUP_* type. However, it cannot be
+ SETUP_INDIRECT since making the setup_indirect a tree structure could
+ require a lot of stack space in something that needs to parse it and
+ stack space can be limited in boot contexts.
+
+ Let's give an example how to point to SETUP_E820_EXT data using setup_indirect.
+ In this case setup_data and setup_indirect will look like this:
+
+ struct setup_data {
+ __u64 next = 0 or <addr_of_next_setup_data_struct>;
+ __u32 type = SETUP_INDIRECT;
+ __u32 len = sizeof(setup_data);
+ __u8 data[sizeof(setup_indirect)] = struct setup_indirect {
+ __u32 type = SETUP_E820_EXT;
+ __u32 reserved = 0;
+ __u64 len = <len_of_SETUP_E820_EXT_data>;
+ __u64 addr = <addr_of_SETUP_E820_EXT_data>;
+ }
+ }
+
============ ============
Field name: pref_address
Type: read (reloc)
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index b05318112452..aaaa17fa6ad6 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -2,7 +2,7 @@
#ifndef _ASM_X86_BOOTPARAM_H
#define _ASM_X86_BOOTPARAM_H
-/* setup_data types */
+/* setup_data/setup_indirect types */
#define SETUP_NONE 0
#define SETUP_E820_EXT 1
#define SETUP_DTB 2
@@ -10,6 +10,7 @@
#define SETUP_EFI 4
#define SETUP_APPLE_PROPERTIES 5
#define SETUP_JAILHOUSE 6
+#define SETUP_INDIRECT 7
/* ram_size flags */
#define RAMDISK_IMAGE_START_MASK 0x07FF
@@ -47,6 +48,14 @@ struct setup_data {
__u8 data[0];
};
+/* extensible setup indirect data node */
+struct setup_indirect {
+ __u32 type;
+ __u32 reserved; /* Reserved, must be set to zero. */
+ __u64 len;
+ __u64 addr;
+};
+
struct setup_header {
__u8 setup_sects;
__u16 root_flags;
--
2.11.0
^ permalink raw reply related
* [PATCH v2 1/3] x86/boot: Introduce the kernel_info
From: Daniel Kiper @ 2019-07-04 16:36 UTC (permalink / raw)
To: linux-doc, linux-kernel, x86
Cc: bp, corbet, dpsmith, eric.snowberg, hpa, kanth.ghatraju,
konrad.wilk, mingo, ross.philipson, tglx
In-Reply-To: <20190704163612.14311-1-daniel.kiper@oracle.com>
The relationships between the headers are analogous to the various data
sections:
setup_header = .data
boot_params/setup_data = .bss
What is missing from the above list? That's right:
kernel_info = .rodata
We have been (ab)using .data for things that could go into .rodata or .bss for
a long time, for lack of alternatives and -- especially early on -- inertia.
Also, the BIOS stub is responsible for creating boot_params, so it isn't
available to a BIOS-based loader (setup_data is, though).
setup_header is permanently limited to 144 bytes due to the reach of the
2-byte jump field, which doubles as a length field for the structure, combined
with the size of the "hole" in struct boot_params that a protected-mode loader
or the BIOS stub has to copy it into. It is currently 119 bytes long, which
leaves us with 25 very precious bytes. This isn't something that can be fixed
without revising the boot protocol entirely, breaking backwards compatibility.
boot_params proper is limited to 4096 bytes, but can be arbitrarily extended
by adding setup_data entries. It cannot be used to communicate properties of
the kernel image, because it is .bss and has no image-provided content.
kernel_info solves this by providing an extensible place for information about
the kernel image. It is readonly, because the kernel cannot rely on a
bootloader copying its contents anywhere, but that is OK; if it becomes
necessary it can still contain data items that an enabled bootloader would be
expected to copy into a setup_data chunk.
This patch does not bump setup_header version in arch/x86/boot/header.S
because it will be followed by additional changes coming into the
Linux/x86 boot protocol.
Suggested-by: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Ross Philipson <ross.philipson@oracle.com>
---
v2 - suggestions/fixes:
- rename setup_header2 to kernel_info,
(suggested by H. Peter Anvin),
- change kernel_info.header value to "InfO" (0x4f666e49),
- new kernel_info description in Documentation/x86/boot.rst,
(suggested by H. Peter Anvin),
- drop kernel_info_offset_update() as an overkill and
update kernel_info offset directly from main(),
(suggested by Eric Snowberg),
- new commit message
(suggested by H. Peter Anvin),
- fix some commit message misspellings
(suggested by Eric Snowberg).
---
Documentation/x86/boot.rst | 89 ++++++++++++++++++++++++++++++++++
arch/x86/boot/Makefile | 2 +-
arch/x86/boot/compressed/Makefile | 4 +-
arch/x86/boot/compressed/kernel_info.S | 12 +++++
| 1 +
arch/x86/boot/tools/build.c | 5 ++
arch/x86/include/uapi/asm/bootparam.h | 1 +
7 files changed, 111 insertions(+), 3 deletions(-)
create mode 100644 arch/x86/boot/compressed/kernel_info.S
diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
index 08a2f100c0e6..a934a56f0516 100644
--- a/Documentation/x86/boot.rst
+++ b/Documentation/x86/boot.rst
@@ -68,8 +68,25 @@ Protocol 2.12 (Kernel 3.8) Added the xloadflags field and extension fields
Protocol 2.13 (Kernel 3.14) Support 32- and 64-bit flags being set in
xloadflags to support booting a 64-bit kernel from 32-bit
EFI
+
+Protocol 2.14: BURNT BY INCORRECT COMMIT ae7e1238e68f2a472a125673ab506d49158c1889
+ (x86/boot: Add ACPI RSDP address to setup_header)
+ DO NOT USE!!! ASSUME SAME AS 2.13.
+
+Protocol 2.15: (Kernel 5.3) Added the kernel_info.
============= ============================================================
+.. note::
+ The protocol version number should be changed only if the setup header
+ is changed. There is no need to update the version number if boot_params
+ or kernel_info are changed. Additionally, it is recommended to use
+ xloadflags (in this case the protocol version number should not be
+ updated either) or kernel_info to communicate supported Linux kernel
+ features to the boot loader. Due to very limited space available in
+ the original setup header every update to it should be considered
+ with great care. Starting from the protocol 2.15 the primary way to
+ communicate things to the boot loader is the kernel_info.
+
Memory Layout
=============
@@ -207,6 +224,7 @@ Offset/Size Proto Name Meaning
0258/8 2.10+ pref_address Preferred loading address
0260/4 2.10+ init_size Linear memory required during initialization
0264/4 2.11+ handover_offset Offset of handover entry point
+0268/4 2.15+ kernel_info_offset Offset of the kernel_info
=========== ======== ===================== ============================================
.. note::
@@ -855,6 +873,77 @@ Offset/size: 0x264/4
See EFI HANDOVER PROTOCOL below for more details.
+============ ==================
+Field name: kernel_info_offset
+Type: read
+Offset/size: 0x268/4
+Protocol: 2.15+
+============ ==================
+
+ This field is the offset from the beginning of the kernel image to the
+ kernel_info. It is embedded in the Linux image in the uncompressed
+ protected mode region.
+
+
+The kernel_info
+===============
+
+The relationships between the headers are analogous to the various data
+sections:
+
+ setup_header = .data
+ boot_params/setup_data = .bss
+
+What is missing from the above list? That's right:
+
+ kernel_info = .rodata
+
+We have been (ab)using .data for things that could go into .rodata or .bss for
+a long time, for lack of alternatives and -- especially early on -- inertia.
+Also, the BIOS stub is responsible for creating boot_params, so it isn't
+available to a BIOS-based loader (setup_data is, though).
+
+setup_header is permanently limited to 144 bytes due to the reach of the
+2-byte jump field, which doubles as a length field for the structure, combined
+with the size of the "hole" in struct boot_params that a protected-mode loader
+or the BIOS stub has to copy it into. It is currently 119 bytes long, which
+leaves us with 25 very precious bytes. This isn't something that can be fixed
+without revising the boot protocol entirely, breaking backwards compatibility.
+
+boot_params proper is limited to 4096 bytes, but can be arbitrarily extended
+by adding setup_data entries. It cannot be used to communicate properties of
+the kernel image, because it is .bss and has no image-provided content.
+
+kernel_info solves this by providing an extensible place for information about
+the kernel image. It is readonly, because the kernel cannot rely on a
+bootloader copying its contents anywhere, but that is OK; if it becomes
+necessary it can still contain data items that an enabled bootloader would be
+expected to copy into a setup_data chunk.
+
+It is recommended to not store large data chunks, e.g. strings, directly in the
+kernel_info struct. Such data should be placed outside of it and pointed from
+the kernel_info structure using offsets from the beginning of the structure,
+the kernel_info.header field.
+
+
+Details of the kernel_info Fields
+=================================
+
+============ ========
+Field name: header
+Offset/size: 0x0000/4
+============ ========
+
+ Contains the magic number "InfO" (0x4f666e49).
+
+============ ========
+Field name: size
+Offset/size: 0x0004/4
+============ ========
+
+ This field contains the size of the kernel_info including kernel_info.header.
+ It should be used by the boot loader to detect supported fields in the kernel_info.
+
The Image Checksum
==================
diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
index e2839b5c246c..c30a9b642a86 100644
--- a/arch/x86/boot/Makefile
+++ b/arch/x86/boot/Makefile
@@ -87,7 +87,7 @@ $(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
SETUP_OBJS = $(addprefix $(obj)/,$(setup-y))
-sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [ABCDGRSTVW] \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|input_data\|_end\|_ehead\|_text\|z_.*\)$$/\#define ZO_\2 0x\1/p'
+sed-zoffset := -e 's/^\([0-9a-fA-F]*\) [ABCDGRSTVW] \(startup_32\|startup_64\|efi32_stub_entry\|efi64_stub_entry\|efi_pe_entry\|input_data\|kernel_info\|_end\|_ehead\|_text\|z_.*\)$$/\#define ZO_\2 0x\1/p'
quiet_cmd_zoffset = ZOFFSET $@
cmd_zoffset = $(NM) $< | sed -n $(sed-zoffset) > $@
diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 6b84afdd7538..fad3b18e2cc3 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -72,8 +72,8 @@ $(obj)/../voffset.h: vmlinux FORCE
$(obj)/misc.o: $(obj)/../voffset.h
-vmlinux-objs-y := $(obj)/vmlinux.lds $(obj)/head_$(BITS).o $(obj)/misc.o \
- $(obj)/string.o $(obj)/cmdline.o $(obj)/error.o \
+vmlinux-objs-y := $(obj)/vmlinux.lds $(obj)/kernel_info.o $(obj)/head_$(BITS).o \
+ $(obj)/misc.o $(obj)/string.o $(obj)/cmdline.o $(obj)/error.o \
$(obj)/piggy.o $(obj)/cpuflags.o
vmlinux-objs-$(CONFIG_EARLY_PRINTK) += $(obj)/early_serial_console.o
diff --git a/arch/x86/boot/compressed/kernel_info.S b/arch/x86/boot/compressed/kernel_info.S
new file mode 100644
index 000000000000..3f1cb301b9ff
--- /dev/null
+++ b/arch/x86/boot/compressed/kernel_info.S
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+ .section ".rodata.kernel_info", "a"
+
+ .global kernel_info
+
+kernel_info:
+ /* Header. */
+ .ascii "InfO"
+ /* Size. */
+ .long kernel_info_end - kernel_info
+kernel_info_end:
--git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index 850b8762e889..ec6a25a43148 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -557,6 +557,7 @@ pref_address: .quad LOAD_PHYSICAL_ADDR # preferred load addr
init_size: .long INIT_SIZE # kernel initialization size
handover_offset: .long 0 # Filled in by build.c
+kernel_info_offset: .long 0 # Filled in by build.c
# End of setup header #####################################################
diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
index a93d44e58f9c..55e669d29e54 100644
--- a/arch/x86/boot/tools/build.c
+++ b/arch/x86/boot/tools/build.c
@@ -56,6 +56,7 @@ u8 buf[SETUP_SECT_MAX*512];
unsigned long efi32_stub_entry;
unsigned long efi64_stub_entry;
unsigned long efi_pe_entry;
+unsigned long kernel_info;
unsigned long startup_64;
/*----------------------------------------------------------------------*/
@@ -321,6 +322,7 @@ static void parse_zoffset(char *fname)
PARSE_ZOFS(p, efi32_stub_entry);
PARSE_ZOFS(p, efi64_stub_entry);
PARSE_ZOFS(p, efi_pe_entry);
+ PARSE_ZOFS(p, kernel_info);
PARSE_ZOFS(p, startup_64);
p = strchr(p, '\n');
@@ -410,6 +412,9 @@ int main(int argc, char ** argv)
efi_stub_entry_update();
+ /* Update kernel_info offset. */
+ put_unaligned_le32(kernel_info, &buf[0x268]);
+
crc = partial_crc32(buf, i, crc);
if (fwrite(buf, 1, i, dest) != i)
die("Writing setup failed");
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index 60733f137e9a..b05318112452 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -86,6 +86,7 @@ struct setup_header {
__u64 pref_address;
__u32 init_size;
__u32 handover_offset;
+ __u32 kernel_info_offset;
} __attribute__((packed));
struct sys_desc_table {
--
2.11.0
^ permalink raw reply related
* [PATCH v2 3/3] x86/boot: Introduce the kernel_info.setup_type_max
From: Daniel Kiper @ 2019-07-04 16:36 UTC (permalink / raw)
To: linux-doc, linux-kernel, x86
Cc: bp, corbet, dpsmith, eric.snowberg, hpa, kanth.ghatraju,
konrad.wilk, mingo, ross.philipson, tglx
In-Reply-To: <20190704163612.14311-1-daniel.kiper@oracle.com>
This field contains maximal allowed type for setup_data and
setup_indirect structs.
And finally bump setup_header version in arch/x86/boot/header.S.
Suggested-by: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Ross Philipson <ross.philipson@oracle.com>
Reviewed-by: Eric Snowberg <eric.snowberg@oracle.com>
---
Documentation/x86/boot.rst | 10 +++++++++-
arch/x86/boot/compressed/kernel_info.S | 4 ++++
| 2 +-
arch/x86/include/uapi/asm/bootparam.h | 3 +++
4 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
index 23d3726d54fc..63609fd0517f 100644
--- a/Documentation/x86/boot.rst
+++ b/Documentation/x86/boot.rst
@@ -73,7 +73,8 @@ Protocol 2.14: BURNT BY INCORRECT COMMIT ae7e1238e68f2a472a125673ab506d49158c188
(x86/boot: Add ACPI RSDP address to setup_header)
DO NOT USE!!! ASSUME SAME AS 2.13.
-Protocol 2.15: (Kernel 5.3) Added the kernel_info and setup_indirect.
+Protocol 2.15: (Kernel 5.3) Added the kernel_info, kernel_info.setup_type_max
+ and setup_indirect.
============= ============================================================
.. note::
@@ -980,6 +981,13 @@ Offset/size: 0x0004/4
This field contains the size of the kernel_info including kernel_info.header.
It should be used by the boot loader to detect supported fields in the kernel_info.
+============ ==============
+Field name: setup_type_max
+Offset/size: 0x0008/4
+============ ==============
+
+ This field contains maximal allowed type for setup_data and setup_indirect structs.
+
The Image Checksum
==================
diff --git a/arch/x86/boot/compressed/kernel_info.S b/arch/x86/boot/compressed/kernel_info.S
index 3f1cb301b9ff..2f28aabf6558 100644
--- a/arch/x86/boot/compressed/kernel_info.S
+++ b/arch/x86/boot/compressed/kernel_info.S
@@ -1,5 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0 */
+#include <asm/bootparam.h>
+
.section ".rodata.kernel_info", "a"
.global kernel_info
@@ -9,4 +11,6 @@ kernel_info:
.ascii "InfO"
/* Size. */
.long kernel_info_end - kernel_info
+ /* Maximal allowed type for setup_data and setup_indirect structs. */
+ .long SETUP_TYPE_MAX
kernel_info_end:
--git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
index ec6a25a43148..893a456663ab 100644
--- a/arch/x86/boot/header.S
+++ b/arch/x86/boot/header.S
@@ -300,7 +300,7 @@ _start:
# Part 2 of the header, from the old setup.S
.ascii "HdrS" # header signature
- .word 0x020d # header version number (>= 0x0105)
+ .word 0x020f # header version number (>= 0x0105)
# or else old loadlin-1.5 will fail)
.globl realmode_swtch
realmode_swtch: .word 0, 0 # default_switch, SETUPSEG
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index aaaa17fa6ad6..2ba870dae6f3 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -12,6 +12,9 @@
#define SETUP_JAILHOUSE 6
#define SETUP_INDIRECT 7
+/* max(SETUP_*) */
+#define SETUP_TYPE_MAX SETUP_INDIRECT
+
/* ram_size flags */
#define RAMDISK_IMAGE_START_MASK 0x07FF
#define RAMDISK_PROMPT_FLAG 0x8000
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
From: Wu Hao @ 2019-07-04 11:29 UTC (permalink / raw)
To: Greg KH
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-doc, Zhang Yi Z,
Xu Yilun, Alan Tull
In-Reply-To: <20190704110449.GC1404@kroah.com>
On Thu, Jul 04, 2019 at 01:04:49PM +0200, Greg KH wrote:
> On Thu, Jul 04, 2019 at 04:58:55PM +0800, Wu Hao wrote:
> > > > Hope things could be more clear now. :)
> > >
> > > That's nice for the vfio stuff, but you are just a "normal" driver here.
> > > You want an ioctl that just does one thing, no arguments, no flags, no
> > > anything. No need for a size argument then at all. These ioctls don't
> > > even need a structure for them!
> > >
> > > Don't try to be fancy, it's not needed, it's not like you are running
> > > out of ioctl space...
> >
> > Thanks a lot for the comments and suggestions.
> >
> > That's true, it's a "normal" driver, maybe I overly considered the
> > extensibility of it. OK, Let me rework this patch to remove argsz from
> > these two ioctls.
> >
> > What about the existing ioctls for this driver, they have argsz too.
> > shall I prepare another patch to remove them as well?
>
> I am hoping you actually have users for those ioctls in userspace today?
> If not, and no one is using them, then yes, please fix those too.
Yes, we have a few users, not many, e.g. https://github.com/OPAE/opae-sdk
I believe we may have more users as we are submitting code to make this
driver more usable.
Let me think about this, if we want to do this clean up, we have to
increase the API version to tell everybody, things are changed. If
finally we decide to do this clean up, that will be a new patch after
this patchset.
Many Thanks for your patient guide and suggestions. :)
Hao
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH 06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
From: Greg KH @ 2019-07-04 11:04 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-doc, Zhang Yi Z,
Xu Yilun, Alan Tull
In-Reply-To: <20190704085855.GB7391@hao-dev>
On Thu, Jul 04, 2019 at 04:58:55PM +0800, Wu Hao wrote:
> > > Hope things could be more clear now. :)
> >
> > That's nice for the vfio stuff, but you are just a "normal" driver here.
> > You want an ioctl that just does one thing, no arguments, no flags, no
> > anything. No need for a size argument then at all. These ioctls don't
> > even need a structure for them!
> >
> > Don't try to be fancy, it's not needed, it's not like you are running
> > out of ioctl space...
>
> Thanks a lot for the comments and suggestions.
>
> That's true, it's a "normal" driver, maybe I overly considered the
> extensibility of it. OK, Let me rework this patch to remove argsz from
> these two ioctls.
>
> What about the existing ioctls for this driver, they have argsz too.
> shall I prepare another patch to remove them as well?
I am hoping you actually have users for those ioctls in userspace today?
If not, and no one is using them, then yes, please fix those too.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v12 01/11] MODSIGN: Export module signature definitions
From: Philipp Rudo @ 2019-07-04 10:54 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Jessica Yu, linux-integrity, linux-security-module, keyrings,
linux-crypto, linuxppc-dev, linux-doc, linux-kernel, Mimi Zohar,
Dmitry Kasatkin, James Morris, Serge E. Hallyn, David Howells,
David Woodhouse, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Heiko Carstens, linux-s390
In-Reply-To: <87lfxel2q6.fsf@morokweng.localdomain>
Hi Thiago,
On Thu, 04 Jul 2019 03:42:57 -0300
Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
> Jessica Yu <jeyu@kernel.org> writes:
>
> > +++ Thiago Jung Bauermann [27/06/19 23:19 -0300]:
> >>IMA will use the module_signature format for append signatures, so export
> >>the relevant definitions and factor out the code which verifies that the
> >>appended signature trailer is valid.
> >>
> >>Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
> >>and be able to use mod_check_sig() without having to depend on either
> >>CONFIG_MODULE_SIG or CONFIG_MODULES.
> >>
> >>Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> >>Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> >>Cc: Jessica Yu <jeyu@kernel.org>
> >>---
> >> include/linux/module.h | 3 --
> >> include/linux/module_signature.h | 44 +++++++++++++++++++++++++
> >> init/Kconfig | 6 +++-
> >> kernel/Makefile | 1 +
> >> kernel/module.c | 1 +
> >> kernel/module_signature.c | 46 ++++++++++++++++++++++++++
> >> kernel/module_signing.c | 56 +++++---------------------------
> >> scripts/Makefile | 2 +-
> >> 8 files changed, 106 insertions(+), 53 deletions(-)
> >>
> >>diff --git a/include/linux/module.h b/include/linux/module.h
> >>index 188998d3dca9..aa56f531cf1e 100644
> >>--- a/include/linux/module.h
> >>+++ b/include/linux/module.h
> >>@@ -25,9 +25,6 @@
> >> #include <linux/percpu.h>
> >> #include <asm/module.h>
> >>
> >>-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
> >>-#define MODULE_SIG_STRING "~Module signature appended~\n"
> >>-
> >
> > Hi Thiago, apologies for the delay.
>
> Hello Jessica, thanks for reviewing the patch!
>
> > It looks like arch/s390/kernel/machine_kexec_file.c also relies on
> > MODULE_SIG_STRING being defined, so module_signature.h will need to be
> > included there too, otherwise we'll run into a compilation error.
>
> Indeed. Thanks for spotting that. The patch below fixes it. It's
> identical to the previous version except for the changes in
> arch/s390/kernel/machine_kexec_file.c and their description in the
> commit message. I'm also copying some s390 people in this email.
to me the s390 part looks good but for one minor nit.
In arch/s390/Kconfig KEXEC_VERIFY_SIG currently depends on
SYSTEM_DATA_VERIFICATION. I'd prefer when you update this to the new
MODULE_SIG_FORMAT. It shouldn't make any difference right now, as we don't
use mod_check_sig in our code path. But it could cause problems in the future,
when more code might be shared.
Thanks
Philipp
> > Other than that, the module-related changes look good to me:
> >
> > Acked-by: Jessica Yu <jeyu@kernel.org>
>
> Thank you very much!
>
^ permalink raw reply
* Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
From: Jarkko Sakkinen @ 2019-07-04 9:20 UTC (permalink / raw)
To: Sasha Levin
Cc: peterhuewe, jgg, corbet, linux-kernel, linux-doc, linux-integrity,
linux-kernel, thiruan, bryankel, tee-dev, ilias.apalodimas,
sumit.garg, rdunlap
In-Reply-To: <20190629150145.GL11506@sasha-vm>
On Sat, 2019-06-29 at 11:01 -0400, Sasha Levin wrote:
> On Thu, Jun 27, 2019 at 02:31:35AM +0300, Jarkko Sakkinen wrote:
> > On Tue, 2019-06-25 at 16:13 -0400, Sasha Levin wrote:
> > > +static const uuid_t ftpm_ta_uuid =
> > > + UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4,
> > > + 0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96);
> > > +
> > > +/**
> > > + * ftpm_tee_tpm_op_recv - retrieve fTPM response.
> > > + *
> >
> > Should not have an empty line here.
> >
> > > + * @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h.
> > > + * @buf: the buffer to store data.
> > > + * @count: the number of bytes to read.
>
> Jarkko, w.r.t your comment above, there is an empty line between the
> function name and variables in drivers/char/tpm, and in particular
> tpm_crb.c which you authored and I used as reference. Do you want us to
> diverge here?
There is divergence and that was the first thing I've contributed to
the TPM driver. I use this as the reference for formatting function
descriptions these days:
https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
According to that the legit way to format would be:
* ftpm_tee_tpm_op_recv() - retrieve fTPM response.
* @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h.
* @buf: the buffer to store data.
* @count: the number of bytes to read.
Since it is both a callback to an interface defined elsewhere
and a static function and it does not document anything useful,
I would just remove this comment. I'd do it for all callbacks
that are part of tpm_call_ops.
/Jarkko
^ permalink raw reply
* Re: [PATCH 06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
From: Wu Hao @ 2019-07-04 8:58 UTC (permalink / raw)
To: Greg KH
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-doc, Zhang Yi Z,
Xu Yilun, Alan Tull
In-Reply-To: <20190704082013.GE6438@kroah.com>
On Thu, Jul 04, 2019 at 10:20:13AM +0200, Greg KH wrote:
> On Thu, Jul 04, 2019 at 02:31:06PM +0800, Wu Hao wrote:
> > On Thu, Jul 04, 2019 at 07:39:27AM +0200, Greg KH wrote:
> > > On Thu, Jul 04, 2019 at 07:30:58AM +0800, Wu Hao wrote:
> > > > On Wed, Jul 03, 2019 at 08:07:53PM +0200, Greg KH wrote:
> > > > > On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> > > > > > From: Wu Hao <hao.wu@intel.com>
> > > > > >
> > > > > > In order to support virtualization usage via PCIe SRIOV, this patch
> > > > > > adds two ioctls under FPGA Management Engine (FME) to release and
> > > > > > assign back the port device. In order to safely turn Port from PF
> > > > > > into VF and enable PCIe SRIOV, it requires user to invoke this
> > > > > > PORT_RELEASE ioctl to release port firstly to remove userspace
> > > > > > interfaces, and then configure the PF/VF access register in FME.
> > > > > > After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> > > > > > ioctl to attach the port back to PF.
> > > > > >
> > > > > > Ioctl interfaces:
> > > > > > * DFL_FPGA_FME_PORT_RELEASE
> > > > > > Release platform device of given port, it deletes port platform
> > > > > > device to remove related userspace interfaces on PF, then
> > > > > > configures PF/VF access mode to VF.
> > > > > >
> > > > > > * DFL_FPGA_FME_PORT_ASSIGN
> > > > > > Assign platform device of given port back to PF, it configures
> > > > > > PF/VF access mode to PF, then adds port platform device back to
> > > > > > re-enable related userspace interfaces on PF.
> > > > >
> > > > > Why are you not using the "generic" bind/unbind facility that userspace
> > > > > already has for this with binding drivers to devices? Why a special
> > > > > ioctl?
> > > >
> > > > Hi Greg,
> > > >
> > > > Actually we think it should be safer that making the device invisble than
> > > > just unbinding its driver. Looks like user can try to rebind it at any
> > > > time and we don't have any method to stop them.
> > >
> > > Why do you want to "stop" the user from doing something? They asked to
> > > do it, why prevent it? If they ask to do something foolish, well, they
> > > get to keep the pieces :)
> >
> > Actually this is for SRIOV support, as we are moving FPGA accelerator from
> > PF to VF, so we don't want users to see the FPGA accelerator from PF any
> > more. We can't allow user to touch same FPGA accelerator from both PF and
> > VF side (it leads to hardware erros).
>
> Ick, ok, this needs to be documented really well then.
Yes, we have add relateded descriptions for virtualization support in that
documentation patch.
[PATCH 05/15] Documentation: fpga: dfl: add descriptions for virtualization
and new interfaces.
>
> > > > > > --- a/include/uapi/linux/fpga-dfl.h
> > > > > > +++ b/include/uapi/linux/fpga-dfl.h
> > > > > > @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
> > > > > >
> > > > > > #define DFL_FPGA_FME_PORT_PR _IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
> > > > > >
> > > > > > +/**
> > > > > > + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> > > > > > + * struct dfl_fpga_fme_port_release)
> > > > > > + *
> > > > > > + * Driver releases the port per Port ID provided by caller.
> > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > + */
> > > > > > +struct dfl_fpga_fme_port_release {
> > > > > > + /* Input */
> > > > > > + __u32 argsz; /* Structure length */
> > > > > > + __u32 flags; /* Zero for now */
> > > > > > + __u32 port_id;
> > > > > > +};
> > > > >
> > > > > meta-comment, why do all of your structures for ioctls have argsz? You
> > > > > "know" the size of the structure already, it's part of the ioctl
> > > > > definition. You shouldn't need to also set it again, right? Otherwise
> > > > > ALL Linux ioctls would need something crazy like this.
> > > >
> > > > Actually we followed the same method as vfio.
> > >
> > > vfio is a protocol on "the wire", right? Not an ioctl.
> > >
> > > > The major purpose should be extendibility, as we really need this to
> > > > be sth long term maintainable.
> > >
> > > You can't change ioctl structure sizes at any time.
> > >
> > > > It really helps, if we add some new members for extentions/enhancement
> > > > under the same ioctl.
> > >
> > > You don't do that.
> > >
> > > > I don't think everybody needs this, but my consideration here is if
> > > > newer generations of hardware/specs come with some extentions, I still
> > > > hope we can resue these IOCTLs as much as we could, instead of
> > > > creating more new ones.
> > >
> > > You create new ones, like everyone else does, as you can not change old
> > > code. By trying to "version" structures like this, it's just going to
> > > be a nightmare.
> >
> > Actually i learned this from vfio code here, it's not trying to "version"
> > structures, let me copy the comments from vfio header file. It should be
> > more clear than above short description from me.
> >
> > "include/uapi/linux/vfio.h"
> >
> > /*
> > * The IOCTL interface is designed for extensibility by embedding the
> > * structure length (argsz) and flags into structures passed between
> > * kernel and userspace. We therefore use the _IO() macro for these
> > * defines to avoid implicitly embedding a size into the ioctl request.
> > * As structure fields are added, argsz will increase to match and flag
> > * bits will be defined to indicate additional fields with valid data.
> > * It's *always* the caller's responsibility to indicate the size of
> > * the structure passed by setting argsz appropriately.
> > */
> >
> > For example.
> >
> > struct vfio_device_info {
> > __u32 argsz;
> > __u32 flags;
> > #define VFIO_DEVICE_FLAGS_RESET (1 << 0) /* Device supports reset */
> > #define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */
> > #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
> > #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */
> > #define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */
> > #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */
> > __u32 num_regions; /* Max region index + 1 */
> > __u32 num_irqs; /* Max IRQ index + 1 */
> >
> > Hope things could be more clear now. :)
>
> That's nice for the vfio stuff, but you are just a "normal" driver here.
> You want an ioctl that just does one thing, no arguments, no flags, no
> anything. No need for a size argument then at all. These ioctls don't
> even need a structure for them!
>
> Don't try to be fancy, it's not needed, it's not like you are running
> out of ioctl space...
Thanks a lot for the comments and suggestions.
That's true, it's a "normal" driver, maybe I overly considered the
extensibility of it. OK, Let me rework this patch to remove argsz from
these two ioctls.
What about the existing ioctls for this driver, they have argsz too.
shall I prepare another patch to remove them as well?
Hao
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH 05/15] Documentation: fpga: dfl: add descriptions for virtualization and new interfaces.
From: Wu Hao @ 2019-07-04 8:19 UTC (permalink / raw)
To: Greg KH
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-doc, Xu Yilun,
Alan Tull
In-Reply-To: <20190704081713.GC6438@kroah.com>
On Thu, Jul 04, 2019 at 10:17:13AM +0200, Greg KH wrote:
> On Thu, Jul 04, 2019 at 02:42:08PM +0800, Wu Hao wrote:
> > On Thu, Jul 04, 2019 at 07:37:19AM +0200, Greg KH wrote:
> > > On Thu, Jul 04, 2019 at 07:38:22AM +0800, Wu Hao wrote:
> > > > On Wed, Jul 03, 2019 at 07:59:26PM +0200, Greg KH wrote:
> > > > > On Thu, Jun 27, 2019 at 05:49:41PM -0700, Moritz Fischer wrote:
> > > > > > From: Wu Hao <hao.wu@intel.com>
> > > > > >
> > > > > > This patch adds virtualization support description for DFL based
> > > > > > FPGA devices (based on PCIe SRIOV), and introductions to new
> > > > > > interfaces added by new dfl private feature drivers.
> > > > > >
> > > > > > [mdf@kernel.org: Fixed up to make it work with new reStructuredText docs]
> > > > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > > > > > Acked-by: Alan Tull <atull@kernel.org>
> > > > > > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > > > > > ---
> > > > > > Documentation/fpga/dfl.rst | 100 +++++++++++++++++++++++++++++++++++++
> > > > > > 1 file changed, 100 insertions(+)
> > > > >
> > > > > This doesn't apply to my tree, where is this file created?
> > > >
> > > > Hi Greg,
> > > >
> > > > >From the cover-letter, Moritz mentioned, dfl.txt has been converted to .rst
> > > > in linux-next. I think this patch is created on top of that by Moritz.
> > > >
> > > > "Note: I've seen that Mauro touched Documentation/fpga/dfl.rst in linux-next
> > > > commit c220a1fae6c5d ("docs: fpga: convert docs to ReST and rename to *.rst")"
> > >
> > > Ok, but I can't take this patch just because the file is converted in
> > > someone else's tree :(
> >
> > Oh.. if that patch is merged from fpga tree, then we wont have this problem. :(
>
> You better not be basing your tree on linux-next :)
>
> > So do you have any suggestions on what should i do now?
> > wait that patch goes to offical release, and then resubmit this patch?
>
> Wait until the change hits Linus's tree and then resend it to me.
OK, get it! Thanks!
Hao
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH 06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
From: Greg KH @ 2019-07-04 8:20 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-doc, Zhang Yi Z,
Xu Yilun, Alan Tull
In-Reply-To: <20190704063106.GA24777@hao-dev>
On Thu, Jul 04, 2019 at 02:31:06PM +0800, Wu Hao wrote:
> On Thu, Jul 04, 2019 at 07:39:27AM +0200, Greg KH wrote:
> > On Thu, Jul 04, 2019 at 07:30:58AM +0800, Wu Hao wrote:
> > > On Wed, Jul 03, 2019 at 08:07:53PM +0200, Greg KH wrote:
> > > > On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> > > > > From: Wu Hao <hao.wu@intel.com>
> > > > >
> > > > > In order to support virtualization usage via PCIe SRIOV, this patch
> > > > > adds two ioctls under FPGA Management Engine (FME) to release and
> > > > > assign back the port device. In order to safely turn Port from PF
> > > > > into VF and enable PCIe SRIOV, it requires user to invoke this
> > > > > PORT_RELEASE ioctl to release port firstly to remove userspace
> > > > > interfaces, and then configure the PF/VF access register in FME.
> > > > > After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> > > > > ioctl to attach the port back to PF.
> > > > >
> > > > > Ioctl interfaces:
> > > > > * DFL_FPGA_FME_PORT_RELEASE
> > > > > Release platform device of given port, it deletes port platform
> > > > > device to remove related userspace interfaces on PF, then
> > > > > configures PF/VF access mode to VF.
> > > > >
> > > > > * DFL_FPGA_FME_PORT_ASSIGN
> > > > > Assign platform device of given port back to PF, it configures
> > > > > PF/VF access mode to PF, then adds port platform device back to
> > > > > re-enable related userspace interfaces on PF.
> > > >
> > > > Why are you not using the "generic" bind/unbind facility that userspace
> > > > already has for this with binding drivers to devices? Why a special
> > > > ioctl?
> > >
> > > Hi Greg,
> > >
> > > Actually we think it should be safer that making the device invisble than
> > > just unbinding its driver. Looks like user can try to rebind it at any
> > > time and we don't have any method to stop them.
> >
> > Why do you want to "stop" the user from doing something? They asked to
> > do it, why prevent it? If they ask to do something foolish, well, they
> > get to keep the pieces :)
>
> Actually this is for SRIOV support, as we are moving FPGA accelerator from
> PF to VF, so we don't want users to see the FPGA accelerator from PF any
> more. We can't allow user to touch same FPGA accelerator from both PF and
> VF side (it leads to hardware erros).
Ick, ok, this needs to be documented really well then.
> > > > > --- a/include/uapi/linux/fpga-dfl.h
> > > > > +++ b/include/uapi/linux/fpga-dfl.h
> > > > > @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
> > > > >
> > > > > #define DFL_FPGA_FME_PORT_PR _IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
> > > > >
> > > > > +/**
> > > > > + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> > > > > + * struct dfl_fpga_fme_port_release)
> > > > > + *
> > > > > + * Driver releases the port per Port ID provided by caller.
> > > > > + * Return: 0 on success, -errno on failure.
> > > > > + */
> > > > > +struct dfl_fpga_fme_port_release {
> > > > > + /* Input */
> > > > > + __u32 argsz; /* Structure length */
> > > > > + __u32 flags; /* Zero for now */
> > > > > + __u32 port_id;
> > > > > +};
> > > >
> > > > meta-comment, why do all of your structures for ioctls have argsz? You
> > > > "know" the size of the structure already, it's part of the ioctl
> > > > definition. You shouldn't need to also set it again, right? Otherwise
> > > > ALL Linux ioctls would need something crazy like this.
> > >
> > > Actually we followed the same method as vfio.
> >
> > vfio is a protocol on "the wire", right? Not an ioctl.
> >
> > > The major purpose should be extendibility, as we really need this to
> > > be sth long term maintainable.
> >
> > You can't change ioctl structure sizes at any time.
> >
> > > It really helps, if we add some new members for extentions/enhancement
> > > under the same ioctl.
> >
> > You don't do that.
> >
> > > I don't think everybody needs this, but my consideration here is if
> > > newer generations of hardware/specs come with some extentions, I still
> > > hope we can resue these IOCTLs as much as we could, instead of
> > > creating more new ones.
> >
> > You create new ones, like everyone else does, as you can not change old
> > code. By trying to "version" structures like this, it's just going to
> > be a nightmare.
>
> Actually i learned this from vfio code here, it's not trying to "version"
> structures, let me copy the comments from vfio header file. It should be
> more clear than above short description from me.
>
> "include/uapi/linux/vfio.h"
>
> /*
> * The IOCTL interface is designed for extensibility by embedding the
> * structure length (argsz) and flags into structures passed between
> * kernel and userspace. We therefore use the _IO() macro for these
> * defines to avoid implicitly embedding a size into the ioctl request.
> * As structure fields are added, argsz will increase to match and flag
> * bits will be defined to indicate additional fields with valid data.
> * It's *always* the caller's responsibility to indicate the size of
> * the structure passed by setting argsz appropriately.
> */
>
> For example.
>
> struct vfio_device_info {
> __u32 argsz;
> __u32 flags;
> #define VFIO_DEVICE_FLAGS_RESET (1 << 0) /* Device supports reset */
> #define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */
> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
> #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */
> #define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */
> #define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */
> __u32 num_regions; /* Max region index + 1 */
> __u32 num_irqs; /* Max IRQ index + 1 */
>
> Hope things could be more clear now. :)
That's nice for the vfio stuff, but you are just a "normal" driver here.
You want an ioctl that just does one thing, no arguments, no flags, no
anything. No need for a size argument then at all. These ioctls don't
even need a structure for them!
Don't try to be fancy, it's not needed, it's not like you are running
out of ioctl space...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 05/15] Documentation: fpga: dfl: add descriptions for virtualization and new interfaces.
From: Greg KH @ 2019-07-04 8:17 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-doc, Xu Yilun,
Alan Tull
In-Reply-To: <20190704064208.GA2722@hao-dev>
On Thu, Jul 04, 2019 at 02:42:08PM +0800, Wu Hao wrote:
> On Thu, Jul 04, 2019 at 07:37:19AM +0200, Greg KH wrote:
> > On Thu, Jul 04, 2019 at 07:38:22AM +0800, Wu Hao wrote:
> > > On Wed, Jul 03, 2019 at 07:59:26PM +0200, Greg KH wrote:
> > > > On Thu, Jun 27, 2019 at 05:49:41PM -0700, Moritz Fischer wrote:
> > > > > From: Wu Hao <hao.wu@intel.com>
> > > > >
> > > > > This patch adds virtualization support description for DFL based
> > > > > FPGA devices (based on PCIe SRIOV), and introductions to new
> > > > > interfaces added by new dfl private feature drivers.
> > > > >
> > > > > [mdf@kernel.org: Fixed up to make it work with new reStructuredText docs]
> > > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > > > > Acked-by: Alan Tull <atull@kernel.org>
> > > > > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > > > > ---
> > > > > Documentation/fpga/dfl.rst | 100 +++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 100 insertions(+)
> > > >
> > > > This doesn't apply to my tree, where is this file created?
> > >
> > > Hi Greg,
> > >
> > > >From the cover-letter, Moritz mentioned, dfl.txt has been converted to .rst
> > > in linux-next. I think this patch is created on top of that by Moritz.
> > >
> > > "Note: I've seen that Mauro touched Documentation/fpga/dfl.rst in linux-next
> > > commit c220a1fae6c5d ("docs: fpga: convert docs to ReST and rename to *.rst")"
> >
> > Ok, but I can't take this patch just because the file is converted in
> > someone else's tree :(
>
> Oh.. if that patch is merged from fpga tree, then we wont have this problem. :(
You better not be basing your tree on linux-next :)
> So do you have any suggestions on what should i do now?
> wait that patch goes to offical release, and then resubmit this patch?
Wait until the change hits Linus's tree and then resend it to me.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] mm, slab: Extend slab/shrink to shrink all the memcg caches
From: Michal Hocko @ 2019-07-04 7:37 UTC (permalink / raw)
To: Waiman Long
Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes,
Joonsoo Kim, Alexander Viro, Jonathan Corbet, Luis Chamberlain,
Kees Cook, Johannes Weiner, Vladimir Davydov, linux-mm, linux-doc,
linux-fsdevel, cgroups, linux-kernel, Roman Gushchin,
Shakeel Butt, Andrea Arcangeli
In-Reply-To: <ca6147ca-25be-cba6-a7b9-fcac6d21345d@redhat.com>
On Wed 03-07-19 12:16:09, Waiman Long wrote:
> On 7/3/19 11:53 AM, Michal Hocko wrote:
> > On Wed 03-07-19 11:21:16, Waiman Long wrote:
> >> On 7/2/19 5:33 PM, Andrew Morton wrote:
> >>> On Tue, 2 Jul 2019 16:44:24 -0400 Waiman Long <longman@redhat.com> wrote:
> >>>
> >>>> On 7/2/19 4:03 PM, Andrew Morton wrote:
> >>>>> On Tue, 2 Jul 2019 14:37:30 -0400 Waiman Long <longman@redhat.com> wrote:
> >>>>>
> >>>>>> Currently, a value of '1" is written to /sys/kernel/slab/<slab>/shrink
> >>>>>> file to shrink the slab by flushing all the per-cpu slabs and free
> >>>>>> slabs in partial lists. This applies only to the root caches, though.
> >>>>>>
> >>>>>> Extends this capability by shrinking all the child memcg caches and
> >>>>>> the root cache when a value of '2' is written to the shrink sysfs file.
> >>>>> Why?
> >>>>>
> >>>>> Please fully describe the value of the proposed feature to or users.
> >>>>> Always.
> >>>> Sure. Essentially, the sysfs shrink interface is not complete. It allows
> >>>> the root cache to be shrunk, but not any of the memcg caches.
> >>> But that doesn't describe anything of value. Who wants to use this,
> >>> and why? How will it be used? What are the use-cases?
> >>>
> >> For me, the primary motivation of posting this patch is to have a way to
> >> make the number of active objects reported in /proc/slabinfo more
> >> accurately reflect the number of objects that are actually being used by
> >> the kernel.
> > I believe we have been through that. If the number is inexact due to
> > caching then lets fix slabinfo rather than trick around it and teach
> > people to do a magic write to some file that will "solve" a problem.
> > This is exactly what drop_caches turned out to be in fact. People just
> > got used to drop caches because they were told so by $random web page.
> > So really, think about the underlying problem and try to fix it.
> >
> > It is true that you could argue that this patch is actually fixing the
> > existing interface because it doesn't really do what it is documented to
> > do and on those grounds I would agree with the change.
>
> I do think that we should correct the shrink file to do what it is
> designed to do to include the memcg caches as well.
>
>
> > But do not teach
> > people that they have to write to some file to get proper numbers.
> > Because that is just a bad idea and it will kick back the same way
> > drop_caches.
>
> The /proc/slabinfo file is a well-known file that is probably used
> relatively extensively. Making it to scan through all the per-cpu
> structures will probably cause performance issues as the slab_mutex has
> to be taken during the whole duration of the scan. That could have
> undesirable side effect.
Please be more specific with some numbers ideally. Also if collecting
data is too expensive, why cannot we simply account cached objects count
in pcp manner?
> Instead, I am thinking about extending the slab/objects sysfs file to
> also show the number of objects hold up by the per-cpu structures and
> thus we can get an accurate count by subtracting it from the reported
> active objects. That will have a more limited performance impact as it
> is just one kmem cache instead of all the kmem caches in the system.
> Also the sysfs files are not as commonly used as slabinfo. That will be
> another patch in the near future.
Both are root only and once it is widespread that slabinfo doesn't
provide precise data you can expect tools will try to fix that by adding
another file(s) and we are back to square one, no? In other words
slabinfo
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] Documentation: gpio: Fix reference to gpiod_get_array()
From: Linus Walleij @ 2019-07-04 7:33 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Bartosz Golaszewski, Jonathan Corbet, Janusz Krzysztofik,
open list:GPIO SUBSYSTEM, Linux Doc Mailing List,
linux-kernel@vger.kernel.org
In-Reply-To: <20190701141005.24631-1-geert+renesas@glider.be>
On Mon, Jul 1, 2019 at 4:10 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> The function is called gpiod_get_array(), not gpiod_array_get().
>
> Fixes: 77588c14ac868cae ("gpiolib: Pass array info to get/set array functions")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Patch applied.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 05/15] Documentation: fpga: dfl: add descriptions for virtualization and new interfaces.
From: Wu Hao @ 2019-07-04 6:42 UTC (permalink / raw)
To: Greg KH
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-doc, Xu Yilun,
Alan Tull
In-Reply-To: <20190704053719.GA347@kroah.com>
On Thu, Jul 04, 2019 at 07:37:19AM +0200, Greg KH wrote:
> On Thu, Jul 04, 2019 at 07:38:22AM +0800, Wu Hao wrote:
> > On Wed, Jul 03, 2019 at 07:59:26PM +0200, Greg KH wrote:
> > > On Thu, Jun 27, 2019 at 05:49:41PM -0700, Moritz Fischer wrote:
> > > > From: Wu Hao <hao.wu@intel.com>
> > > >
> > > > This patch adds virtualization support description for DFL based
> > > > FPGA devices (based on PCIe SRIOV), and introductions to new
> > > > interfaces added by new dfl private feature drivers.
> > > >
> > > > [mdf@kernel.org: Fixed up to make it work with new reStructuredText docs]
> > > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > > > Acked-by: Alan Tull <atull@kernel.org>
> > > > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > > > ---
> > > > Documentation/fpga/dfl.rst | 100 +++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 100 insertions(+)
> > >
> > > This doesn't apply to my tree, where is this file created?
> >
> > Hi Greg,
> >
> > >From the cover-letter, Moritz mentioned, dfl.txt has been converted to .rst
> > in linux-next. I think this patch is created on top of that by Moritz.
> >
> > "Note: I've seen that Mauro touched Documentation/fpga/dfl.rst in linux-next
> > commit c220a1fae6c5d ("docs: fpga: convert docs to ReST and rename to *.rst")"
>
> Ok, but I can't take this patch just because the file is converted in
> someone else's tree :(
Oh.. if that patch is merged from fpga tree, then we wont have this problem. :(
So do you have any suggestions on what should i do now?
wait that patch goes to offical release, and then resubmit this patch?
Hao
^ permalink raw reply
* Re: [PATCH 06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
From: Wu Hao @ 2019-07-04 6:31 UTC (permalink / raw)
To: Greg KH
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-doc, Zhang Yi Z,
Xu Yilun, Alan Tull
In-Reply-To: <20190704053927.GB347@kroah.com>
On Thu, Jul 04, 2019 at 07:39:27AM +0200, Greg KH wrote:
> On Thu, Jul 04, 2019 at 07:30:58AM +0800, Wu Hao wrote:
> > On Wed, Jul 03, 2019 at 08:07:53PM +0200, Greg KH wrote:
> > > On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> > > > From: Wu Hao <hao.wu@intel.com>
> > > >
> > > > In order to support virtualization usage via PCIe SRIOV, this patch
> > > > adds two ioctls under FPGA Management Engine (FME) to release and
> > > > assign back the port device. In order to safely turn Port from PF
> > > > into VF and enable PCIe SRIOV, it requires user to invoke this
> > > > PORT_RELEASE ioctl to release port firstly to remove userspace
> > > > interfaces, and then configure the PF/VF access register in FME.
> > > > After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> > > > ioctl to attach the port back to PF.
> > > >
> > > > Ioctl interfaces:
> > > > * DFL_FPGA_FME_PORT_RELEASE
> > > > Release platform device of given port, it deletes port platform
> > > > device to remove related userspace interfaces on PF, then
> > > > configures PF/VF access mode to VF.
> > > >
> > > > * DFL_FPGA_FME_PORT_ASSIGN
> > > > Assign platform device of given port back to PF, it configures
> > > > PF/VF access mode to PF, then adds port platform device back to
> > > > re-enable related userspace interfaces on PF.
> > >
> > > Why are you not using the "generic" bind/unbind facility that userspace
> > > already has for this with binding drivers to devices? Why a special
> > > ioctl?
> >
> > Hi Greg,
> >
> > Actually we think it should be safer that making the device invisble than
> > just unbinding its driver. Looks like user can try to rebind it at any
> > time and we don't have any method to stop them.
>
> Why do you want to "stop" the user from doing something? They asked to
> do it, why prevent it? If they ask to do something foolish, well, they
> get to keep the pieces :)
Actually this is for SRIOV support, as we are moving FPGA accelerator from
PF to VF, so we don't want users to see the FPGA accelerator from PF any
more. We can't allow user to touch same FPGA accelerator from both PF and
VF side (it leads to hardware erros).
>
> > > > --- a/include/uapi/linux/fpga-dfl.h
> > > > +++ b/include/uapi/linux/fpga-dfl.h
> > > > @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
> > > >
> > > > #define DFL_FPGA_FME_PORT_PR _IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
> > > >
> > > > +/**
> > > > + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> > > > + * struct dfl_fpga_fme_port_release)
> > > > + *
> > > > + * Driver releases the port per Port ID provided by caller.
> > > > + * Return: 0 on success, -errno on failure.
> > > > + */
> > > > +struct dfl_fpga_fme_port_release {
> > > > + /* Input */
> > > > + __u32 argsz; /* Structure length */
> > > > + __u32 flags; /* Zero for now */
> > > > + __u32 port_id;
> > > > +};
> > >
> > > meta-comment, why do all of your structures for ioctls have argsz? You
> > > "know" the size of the structure already, it's part of the ioctl
> > > definition. You shouldn't need to also set it again, right? Otherwise
> > > ALL Linux ioctls would need something crazy like this.
> >
> > Actually we followed the same method as vfio.
>
> vfio is a protocol on "the wire", right? Not an ioctl.
>
> > The major purpose should be extendibility, as we really need this to
> > be sth long term maintainable.
>
> You can't change ioctl structure sizes at any time.
>
> > It really helps, if we add some new members for extentions/enhancement
> > under the same ioctl.
>
> You don't do that.
>
> > I don't think everybody needs this, but my consideration here is if
> > newer generations of hardware/specs come with some extentions, I still
> > hope we can resue these IOCTLs as much as we could, instead of
> > creating more new ones.
>
> You create new ones, like everyone else does, as you can not change old
> code. By trying to "version" structures like this, it's just going to
> be a nightmare.
Actually i learned this from vfio code here, it's not trying to "version"
structures, let me copy the comments from vfio header file. It should be
more clear than above short description from me.
"include/uapi/linux/vfio.h"
/*
* The IOCTL interface is designed for extensibility by embedding the
* structure length (argsz) and flags into structures passed between
* kernel and userspace. We therefore use the _IO() macro for these
* defines to avoid implicitly embedding a size into the ioctl request.
* As structure fields are added, argsz will increase to match and flag
* bits will be defined to indicate additional fields with valid data.
* It's *always* the caller's responsibility to indicate the size of
* the structure passed by setting argsz appropriately.
*/
For example.
struct vfio_device_info {
__u32 argsz;
__u32 flags;
#define VFIO_DEVICE_FLAGS_RESET (1 << 0) /* Device supports reset */
#define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */
#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
#define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */
#define VFIO_DEVICE_FLAGS_CCW (1 << 4) /* vfio-ccw device */
#define VFIO_DEVICE_FLAGS_AP (1 << 5) /* vfio-ap device */
__u32 num_regions; /* Max region index + 1 */
__u32 num_irqs; /* Max IRQ index + 1 */
Hope things could be more clear now. :)
Thanks
Hao
};
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH v12 00/11] Appended signatures support for IMA appraisal
From: Thiago Jung Bauermann @ 2019-07-04 6:45 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-integrity, linux-security-module, keyrings, linux-crypto,
linuxppc-dev, linux-doc, linux-kernel, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, David Howells, David Woodhouse,
Jessica Yu, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro
In-Reply-To: <1561991934.4067.17.camel@linux.ibm.com>
Mimi Zohar <zohar@linux.ibm.com> writes:
> On Thu, 2019-06-27 at 23:19 -0300, Thiago Jung Bauermann wrote:
>> Hello,
>>
>> This version is essentially identical to the last one.
>>
>> It is only a rebase on top of today's linux-integrity/next-queued-testing,
>> prompted by conflicts with Prakhar Srivastava's patches to measure the
>> kernel command line. It also drops two patches that are already present in
>> that branch.
>
> Thanks, Thiago. These patches are now in next-queued-testing waiting
> for some additional reviews/acks.
Thank you very much, Mimi! Now I think I'm only missing acks for the
PKCS#7 changes in patches 2 and 3, and an ack for the s390 changes in
patch 1.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v12 01/11] MODSIGN: Export module signature definitions
From: Thiago Jung Bauermann @ 2019-07-04 6:42 UTC (permalink / raw)
To: Jessica Yu
Cc: linux-integrity, linux-security-module, keyrings, linux-crypto,
linuxppc-dev, linux-doc, linux-kernel, Mimi Zohar,
Dmitry Kasatkin, James Morris, Serge E. Hallyn, David Howells,
David Woodhouse, Herbert Xu, David S. Miller, Jonathan Corbet,
AKASHI, Takahiro, Heiko Carstens, Philipp Rudo, linux-s390
In-Reply-To: <20190701144752.GC25484@linux-8ccs>
Jessica Yu <jeyu@kernel.org> writes:
> +++ Thiago Jung Bauermann [27/06/19 23:19 -0300]:
>>IMA will use the module_signature format for append signatures, so export
>>the relevant definitions and factor out the code which verifies that the
>>appended signature trailer is valid.
>>
>>Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
>>and be able to use mod_check_sig() without having to depend on either
>>CONFIG_MODULE_SIG or CONFIG_MODULES.
>>
>>Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>>Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>>Cc: Jessica Yu <jeyu@kernel.org>
>>---
>> include/linux/module.h | 3 --
>> include/linux/module_signature.h | 44 +++++++++++++++++++++++++
>> init/Kconfig | 6 +++-
>> kernel/Makefile | 1 +
>> kernel/module.c | 1 +
>> kernel/module_signature.c | 46 ++++++++++++++++++++++++++
>> kernel/module_signing.c | 56 +++++---------------------------
>> scripts/Makefile | 2 +-
>> 8 files changed, 106 insertions(+), 53 deletions(-)
>>
>>diff --git a/include/linux/module.h b/include/linux/module.h
>>index 188998d3dca9..aa56f531cf1e 100644
>>--- a/include/linux/module.h
>>+++ b/include/linux/module.h
>>@@ -25,9 +25,6 @@
>> #include <linux/percpu.h>
>> #include <asm/module.h>
>>
>>-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
>>-#define MODULE_SIG_STRING "~Module signature appended~\n"
>>-
>
> Hi Thiago, apologies for the delay.
Hello Jessica, thanks for reviewing the patch!
> It looks like arch/s390/kernel/machine_kexec_file.c also relies on
> MODULE_SIG_STRING being defined, so module_signature.h will need to be
> included there too, otherwise we'll run into a compilation error.
Indeed. Thanks for spotting that. The patch below fixes it. It's
identical to the previous version except for the changes in
arch/s390/kernel/machine_kexec_file.c and their description in the
commit message. I'm also copying some s390 people in this email.
> Other than that, the module-related changes look good to me:
>
> Acked-by: Jessica Yu <jeyu@kernel.org>
Thank you very much!
--
Thiago Jung Bauermann
IBM Linux Technology Center
From 0ca180c66f4cff8b1fcd51f3457cc06dac2f0e81 Mon Sep 17 00:00:00 2001
From: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Date: Thu, 17 May 2018 21:46:12 -0300
Subject: [PATCH 1/1] MODSIGN: Export module signature definitions
IMA will use the module_signature format for append signatures, so export
the relevant definitions and factor out the code which verifies that the
appended signature trailer is valid.
Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
and be able to use mod_check_sig() without having to depend on either
CONFIG_MODULE_SIG or CONFIG_MODULES.
s390 duplicated the definition of struct module_signature so now they can
use the new <linux/module_signature.h> header instead.
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Acked-by: Jessica Yu <jeyu@kernel.org>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Philipp Rudo <prudo@linux.ibm.com>
---
arch/s390/kernel/machine_kexec_file.c | 24 +-----------
include/linux/module.h | 3 --
include/linux/module_signature.h | 44 +++++++++++++++++++++
init/Kconfig | 6 ++-
kernel/Makefile | 1 +
kernel/module.c | 1 +
kernel/module_signature.c | 46 ++++++++++++++++++++++
kernel/module_signing.c | 56 ++++-----------------------
scripts/Makefile | 2 +-
9 files changed, 107 insertions(+), 76 deletions(-)
create mode 100644 include/linux/module_signature.h
create mode 100644 kernel/module_signature.c
diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
index fbdd3ea73667..1ac9fbc6e01e 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -10,7 +10,7 @@
#include <linux/elf.h>
#include <linux/errno.h>
#include <linux/kexec.h>
-#include <linux/module.h>
+#include <linux/module_signature.h>
#include <linux/verification.h>
#include <asm/boot_data.h>
#include <asm/ipl.h>
@@ -23,28 +23,6 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
};
#ifdef CONFIG_KEXEC_VERIFY_SIG
-/*
- * Module signature information block.
- *
- * The constituents of the signature section are, in order:
- *
- * - Signer's name
- * - Key identifier
- * - Signature data
- * - Information block
- */
-struct module_signature {
- u8 algo; /* Public-key crypto algorithm [0] */
- u8 hash; /* Digest algorithm [0] */
- u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
- u8 signer_len; /* Length of signer's name [0] */
- u8 key_id_len; /* Length of key identifier [0] */
- u8 __pad[3];
- __be32 sig_len; /* Length of signature data */
-};
-
-#define PKEY_ID_PKCS7 2
-
int s390_verify_sig(const char *kernel, unsigned long kernel_len)
{
const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
diff --git a/include/linux/module.h b/include/linux/module.h
index 188998d3dca9..aa56f531cf1e 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -25,9 +25,6 @@
#include <linux/percpu.h>
#include <asm/module.h>
-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
-#define MODULE_SIG_STRING "~Module signature appended~\n"
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
new file mode 100644
index 000000000000..523617fc5b6a
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#ifndef _LINUX_MODULE_SIGNATURE_H
+#define _LINUX_MODULE_SIGNATURE_H
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+enum pkey_id_type {
+ PKEY_ID_PGP, /* OpenPGP generated key ID */
+ PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
+ PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
+};
+
+/*
+ * Module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ * - Signer's name
+ * - Key identifier
+ * - Signature data
+ * - Information block
+ */
+struct module_signature {
+ u8 algo; /* Public-key crypto algorithm [0] */
+ u8 hash; /* Digest algorithm [0] */
+ u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
+ u8 signer_len; /* Length of signer's name [0] */
+ u8 key_id_len; /* Length of key identifier [0] */
+ u8 __pad[3];
+ __be32 sig_len; /* Length of signature data */
+};
+
+int mod_check_sig(const struct module_signature *ms, size_t file_len,
+ const char *name);
+
+#endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/init/Kconfig b/init/Kconfig
index 8b9ffe236e4f..c2286a3c74c5 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1852,6 +1852,10 @@ config BASE_SMALL
default 0 if BASE_FULL
default 1 if !BASE_FULL
+config MODULE_SIG_FORMAT
+ def_bool n
+ select SYSTEM_DATA_VERIFICATION
+
menuconfig MODULES
bool "Enable loadable module support"
option modules
@@ -1929,7 +1933,7 @@ config MODULE_SRCVERSION_ALL
config MODULE_SIG
bool "Module signature verification"
depends on MODULES
- select SYSTEM_DATA_VERIFICATION
+ select MODULE_SIG_FORMAT
help
Check modules for valid signatures upon load: the signature
is simply appended to the module. For more information see
diff --git a/kernel/Makefile b/kernel/Makefile
index 33824f0385b3..f29ae2997a43 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -58,6 +58,7 @@ endif
obj-$(CONFIG_UID16) += uid16.o
obj-$(CONFIG_MODULES) += module.o
obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signature.o
obj-$(CONFIG_KALLSYMS) += kallsyms.o
obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
obj-$(CONFIG_CRASH_CORE) += crash_core.o
diff --git a/kernel/module.c b/kernel/module.c
index 6e6712b3aaf5..2712f4d217f5 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -19,6 +19,7 @@
#include <linux/export.h>
#include <linux/extable.h>
#include <linux/moduleloader.h>
+#include <linux/module_signature.h>
#include <linux/trace_events.h>
#include <linux/init.h>
#include <linux/kallsyms.h>
diff --git a/kernel/module_signature.c b/kernel/module_signature.c
new file mode 100644
index 000000000000..4224a1086b7d
--- /dev/null
+++ b/kernel/module_signature.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Module signature checker
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#include <linux/errno.h>
+#include <linux/printk.h>
+#include <linux/module_signature.h>
+#include <asm/byteorder.h>
+
+/**
+ * mod_check_sig - check that the given signature is sane
+ *
+ * @ms: Signature to check.
+ * @file_len: Size of the file to which @ms is appended.
+ * @name: What is being checked. Used for error messages.
+ */
+int mod_check_sig(const struct module_signature *ms, size_t file_len,
+ const char *name)
+{
+ if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
+ return -EBADMSG;
+
+ if (ms->id_type != PKEY_ID_PKCS7) {
+ pr_err("%s: Module is not signed with expected PKCS#7 message\n",
+ name);
+ return -ENOPKG;
+ }
+
+ if (ms->algo != 0 ||
+ ms->hash != 0 ||
+ ms->signer_len != 0 ||
+ ms->key_id_len != 0 ||
+ ms->__pad[0] != 0 ||
+ ms->__pad[1] != 0 ||
+ ms->__pad[2] != 0) {
+ pr_err("%s: PKCS#7 signature info has unexpected non-zero params\n",
+ name);
+ return -EBADMSG;
+ }
+
+ return 0;
+}
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 6b9a926fd86b..cdd04a6b8074 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,37 +11,13 @@
#include <linux/kernel.h>
#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/module_signature.h>
#include <linux/string.h>
#include <linux/verification.h>
#include <crypto/public_key.h>
#include "module-internal.h"
-enum pkey_id_type {
- PKEY_ID_PGP, /* OpenPGP generated key ID */
- PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
- PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
-};
-
-/*
- * Module signature information block.
- *
- * The constituents of the signature section are, in order:
- *
- * - Signer's name
- * - Key identifier
- * - Signature data
- * - Information block
- */
-struct module_signature {
- u8 algo; /* Public-key crypto algorithm [0] */
- u8 hash; /* Digest algorithm [0] */
- u8 id_type; /* Key identifier type [PKEY_ID_PKCS7] */
- u8 signer_len; /* Length of signer's name [0] */
- u8 key_id_len; /* Length of key identifier [0] */
- u8 __pad[3];
- __be32 sig_len; /* Length of signature data */
-};
-
/*
* Verify the signature on a module.
*/
@@ -49,6 +25,7 @@ int mod_verify_sig(const void *mod, struct load_info *info)
{
struct module_signature ms;
size_t sig_len, modlen = info->len;
+ int ret;
pr_devel("==>%s(,%zu)\n", __func__, modlen);
@@ -56,32 +33,15 @@ int mod_verify_sig(const void *mod, struct load_info *info)
return -EBADMSG;
memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
- modlen -= sizeof(ms);
+
+ ret = mod_check_sig(&ms, modlen, info->name);
+ if (ret)
+ return ret;
sig_len = be32_to_cpu(ms.sig_len);
- if (sig_len >= modlen)
- return -EBADMSG;
- modlen -= sig_len;
+ modlen -= sig_len + sizeof(ms);
info->len = modlen;
- if (ms.id_type != PKEY_ID_PKCS7) {
- pr_err("%s: Module is not signed with expected PKCS#7 message\n",
- info->name);
- return -ENOPKG;
- }
-
- if (ms.algo != 0 ||
- ms.hash != 0 ||
- ms.signer_len != 0 ||
- ms.key_id_len != 0 ||
- ms.__pad[0] != 0 ||
- ms.__pad[1] != 0 ||
- ms.__pad[2] != 0) {
- pr_err("%s: PKCS#7 signature info has unexpected non-zero params\n",
- info->name);
- return -EBADMSG;
- }
-
return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
VERIFY_USE_SECONDARY_KEYRING,
VERIFYING_MODULE_SIGNATURE,
diff --git a/scripts/Makefile b/scripts/Makefile
index 9d442ee050bd..52098b080ab7 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -17,7 +17,7 @@ hostprogs-$(CONFIG_VT) += conmakehash
hostprogs-$(BUILD_C_RECORDMCOUNT) += recordmcount
hostprogs-$(CONFIG_BUILDTIME_EXTABLE_SORT) += sortextable
hostprogs-$(CONFIG_ASN1) += asn1_compiler
-hostprogs-$(CONFIG_MODULE_SIG) += sign-file
+hostprogs-$(CONFIG_MODULE_SIG_FORMAT) += sign-file
hostprogs-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += extract-cert
hostprogs-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE) += insert-sys-cert
^ permalink raw reply related
* RE: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
From: Thirupathaiah Annapureddy @ 2019-07-04 6:28 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Jarkko Sakkinen, Sasha Levin, peterhuewe@gmx.de, jgg@ziepe.ca,
corbet@lwn.net, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org,
Microsoft Linux Kernel List, Bryan Kelly (CSI),
tee-dev@lists.linaro.org, sumit.garg@linaro.org,
rdunlap@infradead.org, Joakim Bech
In-Reply-To: <CAC_iWjK2F13QxjuvqzqNLx00SiGz_FQ5X=MQxJyDev57bo3=LQ@mail.gmail.com>
Hi Ilias,
> -----Original Message-----
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Sent: Wednesday, July 3, 2019 1:12 AM
> To: Thirupathaiah Annapureddy <thiruan@microsoft.com>
> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Sasha Levin
> <sashal@kernel.org>; peterhuewe@gmx.de; jgg@ziepe.ca; corbet@lwn.net; linux-
> kernel@vger.kernel.org; linux-doc@vger.kernel.org; linux-
> integrity@vger.kernel.org; Microsoft Linux Kernel List <linux-
> kernel@microsoft.com>; Bryan Kelly (CSI) <bryankel@microsoft.com>; tee-
> dev@lists.linaro.org; sumit.garg@linaro.org; rdunlap@infradead.org; Joakim Bech
> <joakim.bech@linaro.org>
> Subject: Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
>
> Hi Thirupathaiah,
>
> (+Joakim)
>
> On Wed, 3 Jul 2019 at 09:58, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Thirupathaiah,
> > >
> > > First of all, Thanks a lot for trying to test the driver.
> > >
> > np
> >
> > [...]
> > > > I managed to do some quick testing in QEMU.
> > > > Everything works fine when i build this as a module (using IBM's TPM 2.0
> > > > TSS)
> > > >
> > > > - As module
> > > > # insmod /lib/modules/5.2.0-rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko
> > > > # getrandom -by 8
> > > > randomBytes length 8
> > > > 23 b9 3d c3 90 13 d9 6b
> > > >
> > > > - Built-in
> > > > # dmesg | grep optee
> > > > ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed,
> > > > err=ffff0008
> > > This (0xffff0008) translates to TEE_ERROR_ITEM_NOT_FOUND.
> > >
> > > Where is fTPM TA located in the your test setup?
> > > Is it stitched into TEE binary as an EARLY_TA or
> > > Is it expected to be loaded during run-time with the help of user mode OP-
> TEE supplicant?
> > >
> > > My guess is that you are trying to load fTPM TA through user mode OP-TEE
> supplicant.
> > > Can you confirm?
> > I tried both
> >
>
> Ok apparently there was a failure with my built-in binary which i
> didn't notice. I did a full rebuilt and checked the elf this time :)
>
> Built as an earlyTA my error now is:
> ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session
> failed, err=ffff3024 (translates to TEE_ERROR_TARGET_DEAD)
> Since you tested it on real hardware i guess you tried both
> module/built-in. Which TEE version are you using?
I am glad that the first issue (TEE_ERROR_ITEM_NOT_FOUND) is resolved after stitching
fTPM TA as an EARLY_TA.
Regarding TEE_ERROR_TARGET_DEAD error, may I know which HW platform you are using to test?
What is the preboot environment (UEFI or U-boot)?
Where is the secure storage in that HW platform?
I could think of two classes of secure storage.
1. UFS/eMMC RPMB : If Supplicant in U-boot/UEFI initializes the
fTPM TA NV Storage, there should be no issue.
If fTPM TA NV storage is not initialized in pre-boot environment and you are using
built-in fTPM Linux driver, you can run into this issue as TA will try to initialize
NV store and fail.
2. other storage devices like QSPI accessible to only secure mode after
EBS/ReadyToBoot mile posts during boot. In this case, there should be no issue at all
as there is no dependency on non-secure side services provided by supplicant.
If you let me know the HW platform details, I am happy to work with you to enable/integrate
fTPM TA on that HW platform.
Best Regards,
Thiru
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox