Re: [PATCH v1 0/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Wed, 2021-03-31 at 20:36 +0200, Richard Weinberger wrote:
> James,
>
> - Ursprüngliche Mail -
> > Von: "James Bottomley"
> > > On Wed, Mar 17, 2021 at 3:08 PM Ahmad Fatoum <
> > > a.fat...@pengutronix.de wrote:
> > > > keyctl add trusted $KEYNAME "load $(cat ~/kmk.blob)" @s
> > >
> > > Is there a reason why we can't pass the desired backend name in
> > > the
> > > trusted key parameters?
> > > e.g.
> > > keyctl add trusted $KEYNAME "backendtype caam load $(cat
> > > ~/kmk.blob)"
> > > @s
> >
> > Why would you want to in the load? The blob should be type
> > specific, so a TPM key shouldn't load as a CAAM key and vice versa
> > ... and if they're not they need to be made so before the patches
> > go upstream.
>
> I fear right now there is no good way to detect whether a blob is
> desired for CAAM or TPM.
At least for the TPM the old format is two TPM2B structures, and the
new one is ASN.1 so either should be completely distinguishable over
what CAAM does.
> > I could possibly see that you might want to be type specific in the
> > create, but once you're simply loading an already created key, the
> > trusted key subsystem should be able to figure what to do on its
> > own.
>
> So you have some kind of container format in mind which denotes the
> type of the blob?
Well, yes. For the TPM, there's a defined ASN.1 format for the keys:
https://git.kernel.org/pub/scm/linux/kernel/git/jejb/openssl_tpm2_engine.git/tree/tpm2-asn.h
and part of the design of the file is that it's distinguishable either
in DER or PEM (by the guards) format so any crypto application can know
it's dealing with a TPM key simply by inspecting the file. I think you
need the same thing for CAAM and any other format.
We're encouraging new ASN.1 formats to be of the form
SEQUENCE {
type OBJECT IDENTIFIER
... key specific fields ...
}
Where you choose a defined OID to represent the key and that means
every key even in DER form begins with a unique binary signature.
James
Re: [PATCH v1 0/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
James, - Ursprüngliche Mail - > Von: "James Bottomley" >> On Wed, Mar 17, 2021 at 3:08 PM Ahmad Fatoum > > wrote: >> > keyctl add trusted $KEYNAME "load $(cat ~/kmk.blob)" @s >> >> Is there a reason why we can't pass the desired backend name in the >> trusted key parameters? >> e.g. >> keyctl add trusted $KEYNAME "backendtype caam load $(cat ~/kmk.blob)" >> @s > > Why would you want to in the load? The blob should be type specific, > so a TPM key shouldn't load as a CAAM key and vice versa ... and if > they're not they need to be made so before the patches go upstream. I fear right now there is no good way to detect whether a blob is desired for CAAM or TPM. > I could possibly see that you might want to be type specific in the > create, but once you're simply loading an already created key, the > trusted key subsystem should be able to figure what to do on its own. So you have some kind of container format in mind which denotes the type of the blob? Thanks, //richard
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
Ahmad, On Tue, Mar 16, 2021 at 6:24 PM Ahmad Fatoum wrote: > +#define KEYMOD "kernel:trusted" why is the CAAM key modifier hard coded? I'd love to have way to pass my own modifier. That way existing blobs can also be used with this implementation. IIRC the NXP vendor tree uses "SECURE_KEY" as default modifier. -- Thanks, //richard
Re: [GIT PULL][PATCH v9 0/3] Update to zstd-1.4.10
Hello. On Tue, Mar 30, 2021 at 03:51:09PM -0700, Nick Terrell wrote: > From: Nick Terrell > > Please pull from > > g...@github.com:terrelln/linux.git tags/v9-zstd-1.4.10 > > to get these changes. Alternatively the patchset is included. > > This patchset upgrades the zstd library to the latest upstream release. The > current zstd version in the kernel is a modified version of upstream > zstd-1.3.1. > At the time it was integrated, zstd wasn't ready to be used in the kernel > as-is. > But, it is now possible to use upstream zstd directly in the kernel. > > I have not yet released zstd-1.4.10 upstream. I want the zstd version in the > kernel to match up with a known upstream release, so we know exactly what code > is running. Whenever this patchset is ready for merge, I will cut a release at > the upstream commit that gets merged. This should not be necessary for future > releases. > > The kernel zstd library is automatically generated from upstream zstd. A > script > makes the necessary changes and imports it into the kernel. The changes are: > > 1. Replace all libc dependencies with kernel replacements and rewrite > includes. > 2. Remove unncessary portability macros like: #if defined(_MSC_VER). > 3. Use the kernel xxhash instead of bundling it. > > This automation gets tested every commit by upstream's continuous integration. > When we cut a new zstd release, we will submit a patch to the kernel to update > the zstd version in the kernel. > > I've updated zstd to upstream with one big patch because every commit must > build, > so that precludes partial updates. Since the commit is 100% generated, I hope > the > review burden is lightened. I considered replaying upstream commits, but that > is > not possible because there have been ~3500 upstream commits since the last > zstd > import, and the commits don't all build individually. The bulk update > preserves > bisectablity because bugs can be bisected to the zstd version update. At that > point the update can be reverted, and we can work with upstream to find and > fix > the bug. After this big switch in how the kernel consumes zstd, future patches > will be smaller, because they will only have one upstream release worth of > changes each. > > This patchset adds a new kernel-style wrapper around zstd. This wrapper API is > functionally equivalent to the subset of the current zstd API that is > currently > used. The wrapper API changes to be kernel style so that the symbols don't > collide with zstd's symbols. The update to zstd-1.4.6 maintains the same API > and preserves the semantics, so that none of the callers need to be updated. > > This patchset comes in 2 parts: > 1. The first 2 patches prepare for the zstd upgrade. The first patch adds the >new kernel style API so zstd can be upgraded without modifying any callers. >The second patch adds an indirection for the lib/decompress_unzstd.c >including of all decompression source files. > 2. Import zstd-1.4.10. This patch is completely generated from upstream using >automated tooling. > > I tested every caller of zstd on x86_64. I tested both after the 1.4.10 > upgrade > using the compatibility wrapper, and after the final patch in this series. > > I tested kernel and initramfs decompression in i386 and arm. > > I ran benchmarks to compare the current zstd in the kernel with zstd-1.4.6. > I benchmarked on x86_64 using QEMU with KVM enabled on an Intel i9-9900k. > I found: > * BtrFS zstd compression at levels 1 and 3 is 5% faster > * BtrFS zstd decompression+read is 15% faster > * SquashFS zstd decompression+read is 15% faster > * F2FS zstd compression+write at level 3 is 8% faster > * F2FS zstd decompression+read is 20% faster > * ZRAM decompression+read is 30% faster > * Kernel zstd decompression is 35% faster > * Initramfs zstd decompression+build is 5% faster > > The latest zstd also offers bug fixes. For example the problem with large > kernel > decompression has been fixed upstream for over 2 years > https://lkml.org/lkml/2020/9/29/27. > > Please let me know if there is anything that I can do to ease the way for > these > patches. I think it is important because it gets large performance > improvements, > contains bug fixes, and is switching to a more maintainable model of consuming > upstream zstd directly, making it easy to keep up to date. > > Best, > Nick Terrell > > v1 -> v2: > * Successfully tested F2FS with help from Chao Yu to fix my test. > * (1/9) Fix ZSTD_initCStream() wrapper to handle pledged_src_size=0 means > unknown. > This fixes F2FS with the zstd-1.4.6 compatibility wrapper, exposed by the > test. > > v2 -> v3: > * (3/9) Silence warnings by Kernel Test Robot: > https://github.com/facebook/zstd/pull/2324 > Stack size warnings remain, but these aren't new, and the functions it > warns on > are either unused or not in the maximum stack path. This patchset reduces > zstd > compression stack usage by 1 KB overall. I've got
Re: crypto: FIPS 200 mode
Am Dienstag, dem 30.03.2021 um 15:26 -0700 schrieb Randy Dunlap: > > The Kconfig help text for CRYPTO_FIPS says > > config CRYPTO_FIPS > bool "FIPS 200 compliance" > ... > help > This option enables the fips boot option which is > required if you want the system to operate in a FIPS 200 > certification. You should say no unless you know what > this is. > > This seems confusing to me since it says "compliance" in one place and > "certification" in another place. And AFAICT, those two words don't > mean the same thing as far as NIST & FIPS are concerned. > > > Should it say "compliance" in both places? E.g. > > help > This option enables the fips boot option which is > required if you want the system to operate in FIPS 200 > compliance mode. You should say no unless you know what > this is. Sounds good to me. Ciao Stephan > > > thanks.
Re: [PATCH v2] Documentation: crypto: add info about "fips=" boot option
Am Dienstag, dem 30.03.2021 um 15:44 -0700 schrieb Eric Biggers:
> On Tue, Mar 30, 2021 at 09:38:55AM -0700, Randy Dunlap wrote:
> > On 3/29/21 10:29 PM, Eric Biggers wrote:
> > > On Mon, Mar 29, 2021 at 10:06:51PM -0700, Randy Dunlap wrote:
> > > > Having just seen a report of using "fips=1" on the kernel command
> > > > line,
> > > > I could not find it documented anywhere, so add some help for it.
> > > >
> > > > Signed-off-by: Randy Dunlap
> > > > Cc: Dexuan Cui
> > > > Cc: linux-crypto@vger.kernel.org
> > > > Cc: Eric Biggers
> > > > Cc: Herbert Xu
> > > > Cc: "David S. Miller"
> > > > Cc: Jonathan Corbet
> > > > Cc: linux-...@vger.kernel.org
> > > > ---
> > > > Updates/corrections welcome.
> > > >
> > > > v2: drop comment that "fips_enabled can cause some tests to be
> > > > skipped".
> > > >
> > > > Documentation/admin-guide/kernel-parameters.txt | 14 ++
> > > > 1 file changed, 14 insertions(+)
> > > >
> > > > --- linux-next-20210329.orig/Documentation/admin-guide/kernel-
> > > > parameters.txt
> > > > +++ linux-next-20210329/Documentation/admin-guide/kernel-
> > > > parameters.txt
> > > > @@ -1370,6 +1370,20 @@
> > > > See Documentation/admin-guide/sysctl/net.rst
> > > > for
> > > > fb_tunnels_only_for_init_ns
> > > >
> > > > + fips= Format: { 0 | 1}
> > > > + Use to disable (0) or enable (1) FIPS mode.
> > > > + If enabled, any process that is waiting on the
> > > > + 'fips_fail_notif_chain' will be notified of
> > > > fips
> > > > + failures.
> > > > + This setting can also be modified via sysctl
> > > > at
> > > > + /proc/sysctl/crypto/fips_enabled, i.e.,
> > > > + crypto.fips_enabled.
> > > > + If fips_enabled = 1 and a test fails, it will
> > > > cause a
> > > > + kernel panic.
> > > > + If fips_enabled = 1, RSA test requires a key
> > > > size of
> > > > + 2K or larger.
> > > > + It can also effect which ECC curve is used.
> > >
> > > This doesn't really explain why anyone would want to give this option.
> > > What high-level thing is this option meant to be accomplishing?
> > > That's what the documentation should explain.
> >
> > Yes, clearly, even to me.
> >
> > But I could not find anything in the kernel source tree that would help me
> > explain that. So to repeat:
> >
> > > > Updates/corrections welcome.
> >
> > thanks.
> > --
>
> I'm by no means an expert on this, but the main thing I have in mind is that
> (IIUC) the "fips" option is only useful if your whole kernel binary is
> certified
> as a "FIPS cryptographic module", *and* you actually need the FIPS
> compliance.
> And the upstream kernel doesn't have a FIPS certification out of the box;
> that's
> a task for specific Linux distributors like Red Hat, SUSE, Ubuntu, who get
> specific kernel binaries certified.
>
> So, compiling a kernel and using the "fips" option is useless by itself, as
> your
> kernel image won't actually have a FIPS certification in that case anyway.
>
> So, I would expect an explanation like that about under what circumstances
> the
> "fips" option is actually useful and intended for.
>
> The people who actually use this option should be able to explain it
> properly
> though; the above is just my understanding...
The fips=1 flag serves the following purposes:
In-kernel:
- it restricts crypto algos to those which are marked as .fips_allowed in the
testmgr.c
- it causes the panic() if the signature verification of a KO providing a
crypto algo implementation fails
- it causes a specific behavior in driver/char/random.c (which was correct
till 4.8 but then got modified - patches to correct it in current kernels were
ignored)
- elevates the priority of crypto/drbg.c to ensure that when using stdrng the
DRBG is invoked
- ensures that the Jitter RNG is allocated as one seed source for
crypto/drbg.c
In user space:
- Various crypto libraries (OpenSSL, GnuTLS, libgcrypt, NSS) use the flag as
the trigger point to enable their FIPS-compliance with the goal to have one
central "knob" that enables the FIPS mode system-wide
- The boot system (e.g. dracut) starts its FIPS work (see dracut-fips).
Ciao
Stephan
>
> - Eric
Re: [PATCH 01/18] X.509: Parse RSASSA-PSS style certificates
Hi Varad,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on cryptodev/master]
[also build test WARNING on crypto/master security/next-testing linus/master
v5.12-rc5 next-20210330]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Varad-Gautam/Implement-RSASSA-PSS-signature-verification/20210331-043846
base:
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: x86_64-randconfig-s022-20210330 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-279-g6d5d9b42-dirty
#
https://github.com/0day-ci/linux/commit/5fa5152bbf75d015ed5e85d2f0631c902bb8fbe0
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Varad-Gautam/Implement-RSASSA-PSS-signature-verification/20210331-043846
git checkout 5fa5152bbf75d015ed5e85d2f0631c902bb8fbe0
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
sparse warnings: (new ones prefixed by >>)
>> crypto/asymmetric_keys/x509_cert_parser.c:887:17: sparse: sparse: cast to
>> restricted __be16
>> crypto/asymmetric_keys/x509_cert_parser.c:887:17: sparse: sparse: cast to
>> restricted __be16
>> crypto/asymmetric_keys/x509_cert_parser.c:887:17: sparse: sparse: cast to
>> restricted __be16
>> crypto/asymmetric_keys/x509_cert_parser.c:887:17: sparse: sparse: cast to
>> restricted __be16
vim +887 crypto/asymmetric_keys/x509_cert_parser.c
873
874 int x509_note_salt_length(void *context, size_t hdrlen,
875unsigned char tag,
876const void *value, size_t vlen)
877 {
878 struct x509_parse_context *ctx = context;
879
880 if (ctx->last_oid != OID_rsassaPSS)
881 return -EBADMSG;
882
883 if (!value || !vlen || vlen >
sizeof(ctx->cert->sig->salt_length))
884 return -EINVAL;
885
886 ctx->cert->sig->salt_length = (vlen == 2) ?
> 887 be16_to_cpu(*((u16 *) value)) : *((u8 *) value);
888
889 return 0;
890 }
891
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH v2] Documentation: crypto: add info about "fips=" boot option
On Tue, Mar 30, 2021 at 09:38:55AM -0700, Randy Dunlap wrote:
> On 3/29/21 10:29 PM, Eric Biggers wrote:
> > On Mon, Mar 29, 2021 at 10:06:51PM -0700, Randy Dunlap wrote:
> >> Having just seen a report of using "fips=1" on the kernel command line,
> >> I could not find it documented anywhere, so add some help for it.
> >>
> >> Signed-off-by: Randy Dunlap
> >> Cc: Dexuan Cui
> >> Cc: linux-crypto@vger.kernel.org
> >> Cc: Eric Biggers
> >> Cc: Herbert Xu
> >> Cc: "David S. Miller"
> >> Cc: Jonathan Corbet
> >> Cc: linux-...@vger.kernel.org
> >> ---
> >> Updates/corrections welcome.
> >>
> >> v2: drop comment that "fips_enabled can cause some tests to be skipped".
> >>
> >> Documentation/admin-guide/kernel-parameters.txt | 14 ++
> >> 1 file changed, 14 insertions(+)
> >>
> >> ---
> >> linux-next-20210329.orig/Documentation/admin-guide/kernel-parameters.txt
> >> +++ linux-next-20210329/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -1370,6 +1370,20 @@
> >>See Documentation/admin-guide/sysctl/net.rst for
> >>fb_tunnels_only_for_init_ns
> >>
> >> + fips= Format: { 0 | 1}
> >> + Use to disable (0) or enable (1) FIPS mode.
> >> + If enabled, any process that is waiting on the
> >> + 'fips_fail_notif_chain' will be notified of fips
> >> + failures.
> >> + This setting can also be modified via sysctl at
> >> + /proc/sysctl/crypto/fips_enabled, i.e.,
> >> + crypto.fips_enabled.
> >> + If fips_enabled = 1 and a test fails, it will cause a
> >> + kernel panic.
> >> + If fips_enabled = 1, RSA test requires a key size of
> >> + 2K or larger.
> >> + It can also effect which ECC curve is used.
> >
> > This doesn't really explain why anyone would want to give this option.
> > What high-level thing is this option meant to be accomplishing?
> > That's what the documentation should explain.
>
> Yes, clearly, even to me.
>
> But I could not find anything in the kernel source tree that would help me
> explain that. So to repeat:
>
> >> Updates/corrections welcome.
>
> thanks.
> --
I'm by no means an expert on this, but the main thing I have in mind is that
(IIUC) the "fips" option is only useful if your whole kernel binary is certified
as a "FIPS cryptographic module", *and* you actually need the FIPS compliance.
And the upstream kernel doesn't have a FIPS certification out of the box; that's
a task for specific Linux distributors like Red Hat, SUSE, Ubuntu, who get
specific kernel binaries certified.
So, compiling a kernel and using the "fips" option is useless by itself, as your
kernel image won't actually have a FIPS certification in that case anyway.
So, I would expect an explanation like that about under what circumstances the
"fips" option is actually useful and intended for.
The people who actually use this option should be able to explain it properly
though; the above is just my understanding...
- Eric
Re: [PATCH v1 0/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Wed, 2021-03-31 at 00:04 +0200, Richard Weinberger wrote: > Ahmad, > > On Wed, Mar 17, 2021 at 3:08 PM Ahmad Fatoum > wrote: > > keyctl add trusted $KEYNAME "load $(cat ~/kmk.blob)" @s > > Is there a reason why we can't pass the desired backend name in the > trusted key parameters? > e.g. > keyctl add trusted $KEYNAME "backendtype caam load $(cat ~/kmk.blob)" > @s Why would you want to in the load? The blob should be type specific, so a TPM key shouldn't load as a CAAM key and vice versa ... and if they're not they need to be made so before the patches go upstream. I could possibly see that you might want to be type specific in the create, but once you're simply loading an already created key, the trusted key subsystem should be able to figure what to do on its own. James
Re: [PATCH v1 0/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
Ahmad, On Wed, Mar 17, 2021 at 3:08 PM Ahmad Fatoum wrote: > keyctl add trusted $KEYNAME "load $(cat ~/kmk.blob)" @s Is there a reason why we can't pass the desired backend name in the trusted key parameters? e.g. keyctl add trusted $KEYNAME "backendtype caam load $(cat ~/kmk.blob)" @s -- Thanks, //richard
Re: [PATCH v1 0/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
Ahmad, On Wed, Mar 17, 2021 at 3:08 PM Ahmad Fatoum wrote: > TABLE="0 $BLOCKS crypt $ALGO :32:trusted:$KEYNAME 0 $DEV 0 1 > allow_discards" > echo $TABLE | dmsetup create mydev > echo $TABLE | dmsetup load mydev Do you also plan to add support for this to cryptsetup? David and I have added (rough) support for our CAAM/DCP based keyrings to cryptsetup: https://github.com/sigma-star/cryptsetup/tree/rw/plain I'm pretty sure with minimal changes it will work with your recent approach too. -- Thanks, //richard
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Sun, Mar 28, 2021 at 11:37:23PM +0300, Jarkko Sakkinen wrote: > > Unfortunately, TPM trusted keys started this bad security practice, and > obviously it cannot be fixed without breaking uapi backwards compatibility. > The whole point of a randomness source is that it is random. So userspace can't be depending on any particular output, and the randomness source can be changed without breaking backwards compatibility. So IMO, trusted keys should simply be fixed to use get_random_bytes(). - Eric
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
Ahmad, On Wed, Mar 17, 2021 at 3:03 PM Ahmad Fatoum wrote: > > I didn't closely follow the previous discussions, but is a module > > parameter really the right approach? > > Is there also a way to set it via something like device tree? > > Compiled-on sources are considered in the order: tpm, tee then caam. > Module parameters are the only override currently available. Okay. So in the ideal case only one of these backends is compiled in, but the list can get long. I'm asking because David and I currently port another caam-like mechanism to the most recent kernel which will also hook in there. Out driver adds trusted keys support (with caam alike blobs) for i.mx SoCs that come with DCP instead of CAAM. Patches will hopefully materialize soon. -- Thanks, //richard
Re: Fix hibernation in FIPS mode?
On Tue, 2021-03-30 at 21:45 +0200, Ard Biesheuvel wrote:
> On Tue, 30 Mar 2021 at 20:05, Simo Sorce wrote:
> > On Tue, 2021-03-30 at 16:46 +0200, Rafael J. Wysocki wrote:
> > > On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui wrote:
> > > > Hi,
> > > > MD5 was marked incompliant with FIPS in 2009:
> > > > a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged fips_allowed in
> > > > fips mode")
> > > > a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips mode")
> > > >
> > > > But hibernation_e820_save() is still using MD5, and fails in FIPS mode
> > > > due to the 2018 patch:
> > > > 749fa17093ff ("PM / hibernate: Check the success of generating md5
> > > > digest before hibernation")
> > > >
> > > > As a result, hibernation doesn't work when FIPS is on.
> > > >
> > > > Do you think if hibernation_e820_save() should be changed to use a
> > > > FIPS-compliant algorithm like SHA-1?
> > >
> > > I would say yes, it should.
> > >
> > > > PS, currently it looks like FIPS mode is broken in the mainline:
> > > > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
> >
> > FYI, SHA-1 is not a good choice, it is only permitted in HMAC
> > constructions and only for specified uses. If you need to change
> > algorithm you should go straight to SHA-2 or SHA-3 based hashes.
> >
>
> What is the reason for using a [broken] cryptographic hash here? if
> this is just an integrity check, better use CRC32
If the integrity check is used exclusively to verify there were no
accidental changes and is not used as a security measure, by all means
I agree that using crc32 is a better idea.
Simo.
--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc
Re: Fix hibernation in FIPS mode?
On Tue, 30 Mar 2021 at 20:05, Simo Sorce wrote:
>
> On Tue, 2021-03-30 at 16:46 +0200, Rafael J. Wysocki wrote:
> > On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui wrote:
> > > Hi,
> > > MD5 was marked incompliant with FIPS in 2009:
> > > a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged fips_allowed in
> > > fips mode")
> > > a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips mode")
> > >
> > > But hibernation_e820_save() is still using MD5, and fails in FIPS mode
> > > due to the 2018 patch:
> > > 749fa17093ff ("PM / hibernate: Check the success of generating md5 digest
> > > before hibernation")
> > >
> > > As a result, hibernation doesn't work when FIPS is on.
> > >
> > > Do you think if hibernation_e820_save() should be changed to use a
> > > FIPS-compliant algorithm like SHA-1?
> >
> > I would say yes, it should.
> >
> > > PS, currently it looks like FIPS mode is broken in the mainline:
> > > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
>
>
> FYI, SHA-1 is not a good choice, it is only permitted in HMAC
> constructions and only for specified uses. If you need to change
> algorithm you should go straight to SHA-2 or SHA-3 based hashes.
>
What is the reason for using a [broken] cryptographic hash here? if
this is just an integrity check, better use CRC32
Re: Fix hibernation in FIPS mode?
On Tue, 2021-03-30 at 16:46 +0200, Rafael J. Wysocki wrote:
> On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui wrote:
> > Hi,
> > MD5 was marked incompliant with FIPS in 2009:
> > a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged fips_allowed in fips
> > mode")
> > a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips mode")
> >
> > But hibernation_e820_save() is still using MD5, and fails in FIPS mode
> > due to the 2018 patch:
> > 749fa17093ff ("PM / hibernate: Check the success of generating md5 digest
> > before hibernation")
> >
> > As a result, hibernation doesn't work when FIPS is on.
> >
> > Do you think if hibernation_e820_save() should be changed to use a
> > FIPS-compliant algorithm like SHA-1?
>
> I would say yes, it should.
>
> > PS, currently it looks like FIPS mode is broken in the mainline:
> > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
FYI, SHA-1 is not a good choice, it is only permitted in HMAC
constructions and only for specified uses. If you need to change
algorithm you should go straight to SHA-2 or SHA-3 based hashes.
HTH,
Simo.
--
Simo Sorce
RHEL Crypto Team
Red Hat, Inc
Re: [PATCH v2] Documentation: crypto: add info about "fips=" boot option
On 3/29/21 10:29 PM, Eric Biggers wrote:
> On Mon, Mar 29, 2021 at 10:06:51PM -0700, Randy Dunlap wrote:
>> Having just seen a report of using "fips=1" on the kernel command line,
>> I could not find it documented anywhere, so add some help for it.
>>
>> Signed-off-by: Randy Dunlap
>> Cc: Dexuan Cui
>> Cc: linux-crypto@vger.kernel.org
>> Cc: Eric Biggers
>> Cc: Herbert Xu
>> Cc: "David S. Miller"
>> Cc: Jonathan Corbet
>> Cc: linux-...@vger.kernel.org
>> ---
>> Updates/corrections welcome.
>>
>> v2: drop comment that "fips_enabled can cause some tests to be skipped".
>>
>> Documentation/admin-guide/kernel-parameters.txt | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> --- linux-next-20210329.orig/Documentation/admin-guide/kernel-parameters.txt
>> +++ linux-next-20210329/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1370,6 +1370,20 @@
>> See Documentation/admin-guide/sysctl/net.rst for
>> fb_tunnels_only_for_init_ns
>>
>> +fips= Format: { 0 | 1}
>> +Use to disable (0) or enable (1) FIPS mode.
>> +If enabled, any process that is waiting on the
>> +'fips_fail_notif_chain' will be notified of fips
>> +failures.
>> +This setting can also be modified via sysctl at
>> +/proc/sysctl/crypto/fips_enabled, i.e.,
>> +crypto.fips_enabled.
>> +If fips_enabled = 1 and a test fails, it will cause a
>> +kernel panic.
>> +If fips_enabled = 1, RSA test requires a key size of
>> +2K or larger.
>> +It can also effect which ECC curve is used.
>
> This doesn't really explain why anyone would want to give this option.
> What high-level thing is this option meant to be accomplishing?
> That's what the documentation should explain.
Yes, clearly, even to me.
But I could not find anything in the kernel source tree that would help me
explain that. So to repeat:
>> Updates/corrections welcome.
thanks.
--
~Randy
Re: Fix hibernation in FIPS mode?
On 3/30/21 10:46 AM, Rafael J. Wysocki wrote:
On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui wrote:
Hi,
MD5 was marked incompliant with FIPS in 2009:
a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged fips_allowed in fips
mode")
a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips mode")
But hibernation_e820_save() is still using MD5, and fails in FIPS mode
due to the 2018 patch:
749fa17093ff ("PM / hibernate: Check the success of generating md5 digest before
hibernation")
As a result, hibernation doesn't work when FIPS is on.
Do you think if hibernation_e820_save() should be changed to use a
FIPS-compliant algorithm like SHA-1?
I would say yes, it should.
If we're not actually encrypting anything, shouldn't something like
ghash work as well?
PS, currently it looks like FIPS mode is broken in the mainline:
https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
Re: Fix hibernation in FIPS mode?
On Tue, Mar 30, 2021 at 12:14 AM Dexuan Cui wrote:
>
> Hi,
> MD5 was marked incompliant with FIPS in 2009:
> a3bef3a31a19 ("crypto: testmgr - Skip algs not flagged fips_allowed in fips
> mode")
> a1915d51e8e7 ("crypto: testmgr - Mark algs allowed in fips mode")
>
> But hibernation_e820_save() is still using MD5, and fails in FIPS mode
> due to the 2018 patch:
> 749fa17093ff ("PM / hibernate: Check the success of generating md5 digest
> before hibernation")
>
> As a result, hibernation doesn't work when FIPS is on.
>
> Do you think if hibernation_e820_save() should be changed to use a
> FIPS-compliant algorithm like SHA-1?
I would say yes, it should.
> PS, currently it looks like FIPS mode is broken in the mainline:
> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg49414.html
Re: [PATCH] crypto: hisilicon - check if debugfs opened
On Tue, Mar 30, 2021 at 08:40:07PM +0800, tanghui20 wrote: > > > On 2021/3/30 20:23, Greg KH wrote: > > On Tue, Mar 30, 2021 at 08:09:46PM +0800, tanghui20 wrote: > > > > > > > > > On 2021/3/28 23:09, Greg KH wrote: > > > > On Sat, Mar 27, 2021 at 04:33:00PM +0800, Hui Tang wrote: > > > > > 'xx_debugfs_init' check if debugfs opened. > > > > > > > > > > Signed-off-by: Hui Tang > > > > > --- > > > > > drivers/crypto/hisilicon/hpre/hpre_main.c | 5 - > > > > > drivers/crypto/hisilicon/qm.c | 3 +++ > > > > > drivers/crypto/hisilicon/sec2/sec_main.c | 5 - > > > > > drivers/crypto/hisilicon/zip/zip_main.c | 3 +++ > > > > > 4 files changed, 14 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/crypto/hisilicon/hpre/hpre_main.c > > > > > b/drivers/crypto/hisilicon/hpre/hpre_main.c > > > > > index c7ab06d..f2605c4 100644 > > > > > --- a/drivers/crypto/hisilicon/hpre/hpre_main.c > > > > > +++ b/drivers/crypto/hisilicon/hpre/hpre_main.c > > > > > @@ -779,6 +779,9 @@ static int hpre_debugfs_init(struct hisi_qm *qm) > > > > > struct device *dev = &qm->pdev->dev; > > > > > int ret; > > > > > > > > > > + if (!debugfs_initialized()) > > > > > + return -ENOENT; > > > > > > > > Why? What does this help with? Why does the code care if debugfs is > > > > running or not? > > > > > > > When !CONFIG_DEBUG_FS, there is no problem if debugfs is not checked, > > > but if checking debugfs, a series of stub functions of debugfs can be > > > skipped and 'xx_debugfs_init' will be return immediately. > > > > And have you measured an actual speed difference for that? I would be > > amazed if you could... > > > > I think what you said makes sense. > I am confused when to use 'debugfs_initialized'. Never, you should not care about that at all. thanks, greg k-h
Re: [PATCH] crypto: hisilicon - check if debugfs opened
On 2021/3/30 20:23, Greg KH wrote: On Tue, Mar 30, 2021 at 08:09:46PM +0800, tanghui20 wrote: On 2021/3/28 23:09, Greg KH wrote: On Sat, Mar 27, 2021 at 04:33:00PM +0800, Hui Tang wrote: 'xx_debugfs_init' check if debugfs opened. Signed-off-by: Hui Tang --- drivers/crypto/hisilicon/hpre/hpre_main.c | 5 - drivers/crypto/hisilicon/qm.c | 3 +++ drivers/crypto/hisilicon/sec2/sec_main.c | 5 - drivers/crypto/hisilicon/zip/zip_main.c | 3 +++ 4 files changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/crypto/hisilicon/hpre/hpre_main.c b/drivers/crypto/hisilicon/hpre/hpre_main.c index c7ab06d..f2605c4 100644 --- a/drivers/crypto/hisilicon/hpre/hpre_main.c +++ b/drivers/crypto/hisilicon/hpre/hpre_main.c @@ -779,6 +779,9 @@ static int hpre_debugfs_init(struct hisi_qm *qm) struct device *dev = &qm->pdev->dev; int ret; + if (!debugfs_initialized()) + return -ENOENT; Why? What does this help with? Why does the code care if debugfs is running or not? When !CONFIG_DEBUG_FS, there is no problem if debugfs is not checked, but if checking debugfs, a series of stub functions of debugfs can be skipped and 'xx_debugfs_init' will be return immediately. And have you measured an actual speed difference for that? I would be amazed if you could... I think what you said makes sense. I am confused when to use 'debugfs_initialized'. Thanks
Re: [PATCH] crypto: hisilicon - check if debugfs opened
On Tue, Mar 30, 2021 at 08:09:46PM +0800, tanghui20 wrote: > > > On 2021/3/28 23:09, Greg KH wrote: > > On Sat, Mar 27, 2021 at 04:33:00PM +0800, Hui Tang wrote: > > > 'xx_debugfs_init' check if debugfs opened. > > > > > > Signed-off-by: Hui Tang > > > --- > > > drivers/crypto/hisilicon/hpre/hpre_main.c | 5 - > > > drivers/crypto/hisilicon/qm.c | 3 +++ > > > drivers/crypto/hisilicon/sec2/sec_main.c | 5 - > > > drivers/crypto/hisilicon/zip/zip_main.c | 3 +++ > > > 4 files changed, 14 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/crypto/hisilicon/hpre/hpre_main.c > > > b/drivers/crypto/hisilicon/hpre/hpre_main.c > > > index c7ab06d..f2605c4 100644 > > > --- a/drivers/crypto/hisilicon/hpre/hpre_main.c > > > +++ b/drivers/crypto/hisilicon/hpre/hpre_main.c > > > @@ -779,6 +779,9 @@ static int hpre_debugfs_init(struct hisi_qm *qm) > > > struct device *dev = &qm->pdev->dev; > > > int ret; > > > > > > + if (!debugfs_initialized()) > > > + return -ENOENT; > > > > Why? What does this help with? Why does the code care if debugfs is > > running or not? > > > When !CONFIG_DEBUG_FS, there is no problem if debugfs is not checked, > but if checking debugfs, a series of stub functions of debugfs can be > skipped and 'xx_debugfs_init' will be return immediately. And have you measured an actual speed difference for that? I would be amazed if you could... thanks, greg k-h
Re: [PATCH] crypto: hisilicon - check if debugfs opened
On 2021/3/28 23:09, Greg KH wrote: On Sat, Mar 27, 2021 at 04:33:00PM +0800, Hui Tang wrote: 'xx_debugfs_init' check if debugfs opened. Signed-off-by: Hui Tang --- drivers/crypto/hisilicon/hpre/hpre_main.c | 5 - drivers/crypto/hisilicon/qm.c | 3 +++ drivers/crypto/hisilicon/sec2/sec_main.c | 5 - drivers/crypto/hisilicon/zip/zip_main.c | 3 +++ 4 files changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/crypto/hisilicon/hpre/hpre_main.c b/drivers/crypto/hisilicon/hpre/hpre_main.c index c7ab06d..f2605c4 100644 --- a/drivers/crypto/hisilicon/hpre/hpre_main.c +++ b/drivers/crypto/hisilicon/hpre/hpre_main.c @@ -779,6 +779,9 @@ static int hpre_debugfs_init(struct hisi_qm *qm) struct device *dev = &qm->pdev->dev; int ret; + if (!debugfs_initialized()) + return -ENOENT; Why? What does this help with? Why does the code care if debugfs is running or not? When !CONFIG_DEBUG_FS, there is no problem if debugfs is not checked, but if checking debugfs, a series of stub functions of debugfs can be skipped and 'xx_debugfs_init' will be return immediately. Thanks.
Re: [PATCH v2 3/9] arm64: fpsimd: run kernel mode NEON with softirqs disabled
On Tue, Mar 02, 2021 at 10:01:12AM +0100, Ard Biesheuvel wrote: > Kernel mode NEON can be used in task or softirq context, but only in > a non-nesting manner, i.e., softirq context is only permitted if the > interrupt was not taken at a point where the kernel was using the NEON > in task context. > > This means all users of kernel mode NEON have to be aware of this > limitation, and either need to provide scalar fallbacks that may be much > slower (up to 20x for AES instructions) and potentially less safe, or > use an asynchronous interface that defers processing to a later time > when the NEON is guaranteed to be available. > > Given that grabbing and releasing the NEON is cheap, we can relax this > restriction, by increasing the granularity of kernel mode NEON code, and > always disabling softirq processing while the NEON is being used in task > context. > > Signed-off-by: Ard Biesheuvel > --- > arch/arm64/crypto/aes-modes.S | 2 +- > arch/arm64/crypto/sha1-ce-core.S | 2 +- > arch/arm64/crypto/sha2-ce-core.S | 2 +- > arch/arm64/crypto/sha3-ce-core.S | 4 +-- > arch/arm64/crypto/sha512-ce-core.S | 2 +- > arch/arm64/include/asm/assembler.h | 28 +++- > arch/arm64/kernel/asm-offsets.c| 2 ++ > arch/arm64/kernel/fpsimd.c | 4 +-- > 8 files changed, 31 insertions(+), 15 deletions(-) Acked-by: Will Deacon Will
Re: [PATCH v2 2/9] arm64: assembler: introduce wxN aliases for wN registers
On Tue, Mar 02, 2021 at 10:01:11AM +0100, Ard Biesheuvel wrote: > The AArch64 asm syntax has this slightly tedious property that the names > used in mnemonics to refer to registers depend on whether the opcode in > question targets the entire 64-bits (xN), or only the least significant > 8, 16 or 32 bits (wN). When writing parameterized code such as macros, > this can be annoying, as macro arguments don't lend themselves to > indexed lookups, and so generating a reference to wN in a macro that > receives xN as an argument is problematic. > > For instance, an upcoming patch that modifies the implementation of the > cond_yield macro to be able to refer to 32-bit registers would need to > modify invocations such as > > cond_yield 3f, x8 > > to > > cond_yield 3f, 8 > > so that the second argument can be token pasted after x or w to emit the > correct register reference. Unfortunately, this interferes with the self > documenting nature of the first example, where the second argument is > obviously a register, whereas in the second example, one would need to > go and look at the code to find out what '8' means. > > So let's fix this by defining wxN aliases for all xN registers, which > resolve to the 32-bit alias of each respective 64-bit register. This > allows the macro implementation to paste the xN reference after a w to > obtain the correct register name. > > Signed-off-by: Ard Biesheuvel > --- > arch/arm64/include/asm/assembler.h | 8 > 1 file changed, 8 insertions(+) > > diff --git a/arch/arm64/include/asm/assembler.h > b/arch/arm64/include/asm/assembler.h > index e0fc1d424f9b..7b076ccd1a54 100644 > --- a/arch/arm64/include/asm/assembler.h > +++ b/arch/arm64/include/asm/assembler.h > @@ -23,6 +23,14 @@ > #include > #include > > + /* > + * Provide a wxN alias for each wN register so what we can paste a xN > + * reference after a 'w' to obtain the 32-bit version. > + */ > + .irp > n,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 > + wx\n.reqw\n > + .endr That's a pretty neat hack! I remember seeing code elsewhere which would benefit from this, so might be worth a look at our other macros as I'm sure I got annoyed by one the other day... ah yes, the SVE macros in fpsimdmacros.h Acked-by: Will Deacon Will
Re: [PATCH v2 1/9] arm64: assembler: remove conditional NEON yield macros
On Tue, Mar 02, 2021 at 10:01:10AM +0100, Ard Biesheuvel wrote: > The users of the conditional NEON yield macros have all been switched to > the simplified cond_yield macro, and so the NEON specific ones can be > removed. > > Signed-off-by: Ard Biesheuvel > --- > arch/arm64/include/asm/assembler.h | 70 > 1 file changed, 70 deletions(-) Acked-by: Will Deacon Will
Re: [PATCH v2 3/5] crypto: hisilicon/sgl - add some dfx logs
However, I think this log can be used to quickly locate the function or
module if dma alloc failed.
On 2021/3/30 15:56, Joe Perches wrote:
On Tue, 2021-03-30 at 15:39 +0800, Kai Ye wrote:
Add some dfx logs in some abnormal exit situations.
[]
diff --git a/drivers/crypto/hisilicon/sgl.c b/drivers/crypto/hisilicon/sgl.c
[]
@@ -87,8 +87,10 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct
device *dev,
block[i].sgl = dma_alloc_coherent(dev, block_size,
&block[i].sgl_dma,
GFP_KERNEL);
- if (!block[i].sgl)
+ if (!block[i].sgl) {
+ dev_err(dev, "Fail to allocate hw SG buffer!\n");
This doesn't seem useful as dma_alloc_coherent does a dump_stack
by default on OOM.
.
Re: [PATCH v2 3/5] crypto: hisilicon/sgl - add some dfx logs
On Tue, 2021-03-30 at 15:39 +0800, Kai Ye wrote:
> Add some dfx logs in some abnormal exit situations.
[]
> diff --git a/drivers/crypto/hisilicon/sgl.c b/drivers/crypto/hisilicon/sgl.c
[]
> @@ -87,8 +87,10 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct
> device *dev,
> block[i].sgl = dma_alloc_coherent(dev, block_size,
> &block[i].sgl_dma,
> GFP_KERNEL);
> - if (!block[i].sgl)
> + if (!block[i].sgl) {
> + dev_err(dev, "Fail to allocate hw SG buffer!\n");
This doesn't seem useful as dma_alloc_coherent does a dump_stack
by default on OOM.
Re: [PATCH v2 1/5] crypto: hisilicon/sgl - fixup coding style
On Tue, 2021-03-30 at 15:39 +0800, Kai Ye wrote:
> use a macro replace of a magic number.
Given the use of 32 in the same test, this
seems more obfuscating that useful to me.
> diff --git a/drivers/crypto/hisilicon/sgl.c b/drivers/crypto/hisilicon/sgl.c
[]
> @@ -9,6 +9,7 @@
> #define HISI_ACC_SGL_NR_MAX 256
> #define HISI_ACC_SGL_ALIGN_SIZE 64
> #define HISI_ACC_MEM_BLOCK_NR5
> +#define HISI_ACC_BLOCK_SIZE_MAX_SHIFT31
>
>
> struct acc_hw_sge {
> dma_addr_t buf;
> @@ -67,7 +68,8 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct
> device *dev,
> sgl_size = sizeof(struct acc_hw_sge) * sge_nr +
> sizeof(struct hisi_acc_hw_sgl);
> block_size = 1 << (PAGE_SHIFT + MAX_ORDER <= 32 ?
> -PAGE_SHIFT + MAX_ORDER - 1 : 31);
> +PAGE_SHIFT + MAX_ORDER - 1 :
> +HISI_ACC_BLOCK_SIZE_MAX_SHIFT);
> sgl_num_per_block = block_size / sgl_size;
> block_num = count / sgl_num_per_block;
> remain_sgl = count % sgl_num_per_block;
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Mon, 29 Mar 2021 at 01:07, Jarkko Sakkinen wrote: > > On Sat, Mar 27, 2021 at 01:41:24PM +0100, David Gstir wrote: > > Hi! > > > > > On 25.03.2021, at 06:26, Sumit Garg wrote: > > > > > > On Wed, 24 Mar 2021 at 19:37, Ahmad Fatoum > > > wrote: > > >> > > >> Hello Sumit, > > >> > > >> On 24.03.21 11:47, Sumit Garg wrote: > > >>> On Wed, 24 Mar 2021 at 14:56, Ahmad Fatoum > > >>> wrote: > > > > Hello Mimi, > > > > On 23.03.21 19:07, Mimi Zohar wrote: > > > On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote: > > >> On 21.03.21 21:48, Horia Geantă wrote: > > >>> caam has random number generation capabilities, so it's worth using > > >>> that > > >>> by implementing .get_random. > > >> > > >> If the CAAM HWRNG is already seeding the kernel RNG, why not use the > > >> kernel's? > > >> > > >> Makes for less code duplication IMO. > > > > > > Using kernel RNG, in general, for trusted keys has been discussed > > > before. Please refer to Dave Safford's detailed explanation for not > > > using it [1]. > > > > The argument seems to boil down to: > > > > - TPM RNG are known to be of good quality > > - Trusted keys always used it so far > > > > Both are fine by me for TPMs, but the CAAM backend is new code and > > neither point > > really applies. > > > > get_random_bytes_wait is already used for generating key material > > elsewhere. > > Why shouldn't new trusted key backends be able to do the same thing? > > > > >>> > > >>> Please refer to documented trusted keys behaviour here [1]. New > > >>> trusted key backends should align to this behaviour and in your case > > >>> CAAM offers HWRNG so we should be better using that. > > >> > > >> Why is it better? > > >> > > >> Can you explain what benefit a CAAM user would have if the trusted key > > >> randomness comes directly out of the CAAM instead of indirectly from > > >> the kernel entropy pool that is seeded by it? > > > > > > IMO, user trust in case of trusted keys comes from trusted keys > > > backend which is CAAM here. If a user doesn't trust that CAAM would > > > act as a reliable source for RNG then CAAM shouldn't be used as a > > > trust source in the first place. > > > > > > And I think building user's trust for kernel RNG implementation with > > > multiple entropy contributions is pretty difficult when compared with > > > CAAM HWRNG implementation. > > > > Generally speaking, I’d say trusting the CAAM RNG and trusting in it’s > > other features are two separate things. However, reading through the CAAM > > key blob spec I’ve got here, CAAM key blob keys (the keys that secure a > > blob’s > > content) are generated using its internal RNG. So I’d save if the CAAM RNG > > is insecure, so are generated key blobs. Maybe somebody with more insight > > into the CAAM internals can verify that, but I don’t see any point in using > > the kernel’s RNG as long as we let CAAM generate the key blob keys for us. > > Here's my long'ish analysis. Please read it to the end if by ever means > possible, and apologies, I usually try to keep usually my comms short, but > this requires some more meat than the usual. > > The Bad News > > > Now that we add multiple hardware trust sources for trusted keys, will > there ever be a scenario where a trusted key is originally sealed with a > backing hardware A, unsealed, and resealed with hardware B? > > The hardware and vendor neutral way to generate the key material would be > unconditionally always just the kernel RNG. > > CAAM is actually worse than TCG because it's not even a standards body, if > I got it right. Not a lot but at least a tiny fraction. > > This brings an open item in TEE patches: trusted_tee_get_random() is an > issue in generating kernel material. I would rather replace that with > kernel RNG *for now*, because the same open question applies also to ARM > TEE. It's also a single company controlled backing technology. > > By all practical means, I do trust ARM TEE in my personal life but this is > not important. > > CAAM *and* TEE backends break the golden rule of putting as little trust as > possible to anything, even not anything weird is clear at sight, as > security is essentially a game of known unknowns and unknown unknowns. > > Unfortunately, TPM trusted keys started this bad security practice, and > obviously it cannot be fixed without breaking uapi backwards compatibility. > > This leaves me exactly two rational options: > > A. Add a patch to remove trusted_tee_get_random() and use kernel RNG >instead. > B. Drop the whole TEE patch set up until I have good reasons to believe >that it's the best possible idea ever to use TEE RNG. > > Doing does (A) does not disclude of doing (B) later on, if someone some > day sends a patch with sound reasoning. > > It's also good to understand that when some day a vendor D, other than TCG, > CAAM
RE: v5.12.0-rc5: the kernel panics if FIPS mode is on
> From: Eric Biggers > Sent: Monday, March 29, 2021 6:26 PM > ... > It looks like your userspace is using tcrypt.ko to request that the kernel > test > "ofb(aes)", but your kernel doesn't have CONFIG_CRYPTO_OFB enabled so the > test fails as expected. Hi Eric, Thanks for the explanation! Yes, that's it! Sorry for the false alarm! Actually the kernel is faultless here. > Are you sure that anything changed on the kernel side > besides the kconfig you are using? It looks like this was always the behavior > when tcrypt.ko is used to test a non-existing algorithm. After I rebuilt the kernel with the 3 options: CONFIG_CRYPTO_OFB=y CONFIG_CRYPTO_DEV_PADLOCK_AES=y CONFIG_CRYPTO_ANSI_CPRNG=y and generated the .hmac file: sha512hmac /boot/vmlinuz-5.12.0-rc5+ > /boot/.vmlinuz-5.12.0-rc5+.hmac now the kernel boots up successfully with fips=1. :-) > Is your userspace code intentionally trying to test "ofb(aes)", or is it > accidental? > > - Eric I'm not sure. This is a CentOS 8.3 VM, and I use the default configuration. I have been trying to build & run a v5.12.0-rc5+ kernel with fips=1, and now this is working for me, thanks to your explanation. Thanks again! Thanks, -- Dexuan
Re: [PATCH v2] Documentation: crypto: add info about "fips=" boot option
On Mon, Mar 29, 2021 at 10:06:51PM -0700, Randy Dunlap wrote:
> Having just seen a report of using "fips=1" on the kernel command line,
> I could not find it documented anywhere, so add some help for it.
>
> Signed-off-by: Randy Dunlap
> Cc: Dexuan Cui
> Cc: linux-crypto@vger.kernel.org
> Cc: Eric Biggers
> Cc: Herbert Xu
> Cc: "David S. Miller"
> Cc: Jonathan Corbet
> Cc: linux-...@vger.kernel.org
> ---
> Updates/corrections welcome.
>
> v2: drop comment that "fips_enabled can cause some tests to be skipped".
>
> Documentation/admin-guide/kernel-parameters.txt | 14 ++
> 1 file changed, 14 insertions(+)
>
> --- linux-next-20210329.orig/Documentation/admin-guide/kernel-parameters.txt
> +++ linux-next-20210329/Documentation/admin-guide/kernel-parameters.txt
> @@ -1370,6 +1370,20 @@
> See Documentation/admin-guide/sysctl/net.rst for
> fb_tunnels_only_for_init_ns
>
> + fips= Format: { 0 | 1}
> + Use to disable (0) or enable (1) FIPS mode.
> + If enabled, any process that is waiting on the
> + 'fips_fail_notif_chain' will be notified of fips
> + failures.
> + This setting can also be modified via sysctl at
> + /proc/sysctl/crypto/fips_enabled, i.e.,
> + crypto.fips_enabled.
> + If fips_enabled = 1 and a test fails, it will cause a
> + kernel panic.
> + If fips_enabled = 1, RSA test requires a key size of
> + 2K or larger.
> + It can also effect which ECC curve is used.
This doesn't really explain why anyone would want to give this option.
What high-level thing is this option meant to be accomplishing?
That's what the documentation should explain.
- Eric
Re: [PATCH] Documentation: crypto: add info about "fips=" boot option
On Mon, Mar 29, 2021 at 10:00:45PM -0700, Randy Dunlap wrote: > On 3/29/21 9:37 PM, Herbert Xu wrote: > > On Mon, Mar 29, 2021 at 09:00:01PM -0700, Randy Dunlap wrote: > >> > >> + If fips_enabled = 1, some crypto tests are skipped. > > > > I don't think any tests are skipped. It does however disable > > many algorithms by essentially failing them at the testing stage. > > That statement was based on crypto/testmgr.c (in 4 places): > > if (fips_enabled && template[i].fips_skip) > continue; By skipping the test, the algorithm effectively fails the self-test and therefore is disabled. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] Documentation: crypto: add info about "fips=" boot option
On 3/29/21 9:37 PM, Herbert Xu wrote: > On Mon, Mar 29, 2021 at 09:00:01PM -0700, Randy Dunlap wrote: >> >> +If fips_enabled = 1, some crypto tests are skipped. > > I don't think any tests are skipped. It does however disable > many algorithms by essentially failing them at the testing stage. That statement was based on crypto/testmgr.c (in 4 places): if (fips_enabled && template[i].fips_skip) continue; and if (fips_enabled && vec->fips_skip) return 0; and if (fips_enabled && !alg_test_descs[i].fips_allowed) goto non_fips_alg; and if (fips_enabled && ((i >= 0 && !alg_test_descs[i].fips_allowed) || (j >= 0 && !alg_test_descs[j].fips_allowed))) goto non_fips_alg; so it appears (at least to me) that there are some methods (infrastructure) for tests to be skipped, but maybe none are actually using that possiblilty. In any case, I don't mind dropping that part of the documentation. thanks. -- ~Randy
Re: [PATCH] Documentation: crypto: add info about "fips=" boot option
On Mon, Mar 29, 2021 at 09:00:01PM -0700, Randy Dunlap wrote: > > + If fips_enabled = 1, some crypto tests are skipped. I don't think any tests are skipped. It does however disable many algorithms by essentially failing them at the testing stage. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: v5.12.0-rc5: the kernel panics if FIPS mode is on
On Mon, Mar 29, 2021 at 09:56:18PM +, Dexuan Cui wrote: > Hi all, > The v5.12.0-rc5 kernel (1e43c377a79f) panics with fips=1. > > Please refer to the below panic call-trace. The kernel config file and > the full kernel messages are also attached. > > Is this a known issue? > > Thanks, > -- Dexuan > > Starting dracut pre-udev hook... > [7.260424] alg: self-tests for sha512-generic (sha512) passed > [7.265917] alg: self-tests for sha384-generic (sha384) passed > [7.272426] alg: self-tests for sha512-ssse3 (sha512) passed > [7.276500] alg: self-tests for sha384-ssse3 (sha384) passed > [7.281722] alg: self-tests for sha512-avx (sha512) passed > [7.286579] alg: self-tests for sha384-avx (sha384) passed > [7.291631] alg: self-tests for sha512-avx2 (sha512) passed > [7.296950] alg: self-tests for sha384-avx2 (sha384) passed > [7.321040] alg: self-tests for sha3-224-generic (sha3-224) passed > [7.330291] alg: self-tests for sha3-256-generic (sha3-256) passed > [7.335918] alg: self-tests for sha3-384-generic (sha3-384) passed > [7.341508] alg: self-tests for sha3-512-generic (sha3-512) passed > [7.381918] alg: self-tests for crc32c-intel (crc32c) passed > [7.396694] alg: self-tests for crct10dif-pclmul (crct10dif) passed > [7.453515] alg: self-tests for ghash-clmulni (ghash) passed > [7.469558] alg: self-tests for des3_ede-asm (des3_ede) passed > [7.475355] alg: self-tests for ecb-des3_ede-asm (ecb(des3_ede)) passed > [7.481361] alg: self-tests for cbc-des3_ede-asm (cbc(des3_ede)) passed > [7.488656] alg: self-tests for des3_ede-generic (des3_ede) passed > [7.304930] dracut-pre-udev[502]: modprobe: ERROR: could not insert > 'padlock_aes': No such device > [7.579580] alg: No test for fips(ansi_cprng) (fips_ansi_cprng) > [7.606547] alg: self-tests for sha1 (sha1) passed > [7.610624] alg: self-tests for ecb(des3_ede) (ecb(des3_ede)) passed > [7.615746] alg: self-tests for cbc(des3_ede) (cbc(des3_ede)) passed > [7.638067] alg: self-tests for ctr(des3_ede-asm) (ctr(des3_ede)) passed > [7.644781] alg: self-tests for ctr(des3_ede) (ctr(des3_ede)) passed > [7.653810] alg: self-tests for sha256 (sha256) passed > [7.658945] alg: self-tests for ecb(aes) (ecb(aes)) passed > [7.663493] alg: self-tests for cbc(aes) (cbc(aes)) passed > [7.668421] alg: self-tests for xts(aes) (xts(aes)) passed > [7.672389] alg: self-tests for ctr(aes) (ctr(aes)) passed > [7.692973] alg: self-tests for rfc3686(ctr-aes-aesni) (rfc3686(ctr(aes))) > passed > [7.699446] alg: self-tests for rfc3686(ctr(aes)) (rfc3686(ctr(aes))) > passed > [7.730149] alg: skcipher: failed to allocate transform for ofb(aes): -2 > [7.735959] Kernel panic - not syncing: alg: self-tests for ofb(aes) > (ofb(aes)) failed in fips mode! > [7.736952] CPU: 13 PID: 560 Comm: modprobe Tainted: GW > 5.12.0-rc5+ #3 > [7.736952] Hardware name: Microsoft Corporation Virtual Machine/Virtual > Machine, BIOS 090008 12/07/2018 > [7.736952] Call Trace: > [7.736952] dump_stack+0x64/0x7c > [7.736952] panic+0xfb/0x2d7 > [7.736952] alg_test+0x42d/0x460 > [7.736952] ? __kernfs_new_node+0x175/0x1d0 > [7.736952] do_test+0x3248/0x57ea [tcrypt] > [7.736952] do_test+0x1f2c/0x57ea [tcrypt] > [7.736952] ? 0xc031d000 > [7.736952] tcrypt_mod_init+0x55/0x1000 [tcrypt] It looks like your userspace is using tcrypt.ko to request that the kernel test "ofb(aes)", but your kernel doesn't have CONFIG_CRYPTO_OFB enabled so the test fails as expected. Are you sure that anything changed on the kernel side besides the kconfig you are using? It looks like this was always the behavior when tcrypt.ko is used to test a non-existing algorithm. Is your userspace code intentionally trying to test "ofb(aes)", or is it accidental? - Eric
Re: [PATCH] crypto: chelsio: fix incorrect kernel-doc comment syntax in file
On 3/29/21 3:45 AM, Aditya Srivastava wrote: > The opening comment mark '/**' is used for highlighting the beginning of > kernel-doc comments. > The header for drivers/crypto/chelsio/chcr_core.c follows this syntax, but > the content inside does not comply with kernel-doc. > > This line was probably not meant for kernel-doc parsing, but is parsed > due to the presence of kernel-doc like comment syntax(i.e, '/**'), which > causes unexpected warning from kernel-doc: > "warning: wrong kernel-doc identifier on line: > * This file is part of the Chelsio T4/T5/T6 Ethernet driver for Linux." > > Provide a simple fix by replacing this occurrence with general comment > format, i.e. '/*', to prevent kernel-doc from parsing it. > > Signed-off-by: Aditya Srivastava Acked-by: Randy Dunlap Thanks. > --- > drivers/crypto/chelsio/chcr_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/crypto/chelsio/chcr_core.c > b/drivers/crypto/chelsio/chcr_core.c > index f91f9d762a45..f03ef4a23f96 100644 > --- a/drivers/crypto/chelsio/chcr_core.c > +++ b/drivers/crypto/chelsio/chcr_core.c > @@ -1,4 +1,4 @@ > -/** > +/* > * This file is part of the Chelsio T4/T5/T6 Ethernet driver for Linux. > * > * Copyright (C) 2011-2016 Chelsio Communications. All rights reserved. > -- ~Randy
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
Hello Jarkko, On 28.03.21 22:37, Jarkko Sakkinen wrote: > On Sat, Mar 27, 2021 at 01:41:24PM +0100, David Gstir wrote: >> Generally speaking, I’d say trusting the CAAM RNG and trusting in it’s >> other features are two separate things. However, reading through the CAAM >> key blob spec I’ve got here, CAAM key blob keys (the keys that secure a >> blob’s >> content) are generated using its internal RNG. So I’d save if the CAAM RNG >> is insecure, so are generated key blobs. Maybe somebody with more insight >> into the CAAM internals can verify that, but I don’t see any point in using >> the kernel’s RNG as long as we let CAAM generate the key blob keys for us. > > Here's my long'ish analysis. Please read it to the end if by ever means > possible, and apologies, I usually try to keep usually my comms short, but > this requires some more meat than the usual. Thanks for the write-up! > The Bad News > > > Now that we add multiple hardware trust sources for trusted keys, will > there ever be a scenario where a trusted key is originally sealed with a > backing hardware A, unsealed, and resealed with hardware B? > > The hardware and vendor neutral way to generate the key material would be > unconditionally always just the kernel RNG. > > CAAM is actually worse than TCG because it's not even a standards body, if > I got it right. Not a lot but at least a tiny fraction. CAAM is how NXP calls the crypto accelerator built into some of its SoCs. > This brings an open item in TEE patches: trusted_tee_get_random() is an > issue in generating kernel material. I would rather replace that with > kernel RNG *for now*, because the same open question applies also to ARM > TEE. It's also a single company controlled backing technology. > > By all practical means, I do trust ARM TEE in my personal life but this is > not important. > > CAAM *and* TEE backends break the golden rule of putting as little trust as > possible to anything, even not anything weird is clear at sight, as > security is essentially a game of known unknowns and unknown unknowns. Agreed. > The GOOD News > = > > So there's actually option (C) that also fixes the TPM trustd keys issue: > > Add a new kernel patch, which: > > 1. Adds the use of kernel RNG as a boot option. > 2. If this boot option is not active, the subsystem will print a warning >to klog denoting this. > 3. Default is of course vendor RNG given the bad design issue in the TPM >trusted keys, but the warning in klog will help to address it at least >a bit. Why should the TPM backend's choice influence later backends? We could add a new option for key creation time, e.g.: keyctl add trusted kmk "new keylen rng=kernel" @s The default would be rng=vendor if available with a fallback to rng=kernel, which should always be available. > 4. Document all this to Documentation/security/keys/trusted-encrypted.rst. Yes, backends would then document whether they support a rng=vendor or not. > I'd prefer the choice between A, B and C be concluded rather sooner than > later. FWIW, my vote is for option C, with the change described above. Cheers, Ahmad > > /Jarkko > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v3] crypto: mips: add poly1305-core.S to .gitignore
On Sat, Mar 27, 2021 at 07:39:43PM -0700, Ilya Lipnitskiy wrote:
> poly1305-core.S is an auto-generated file, so it should be ignored.
>
> Fixes: a11d055e7a64 ("crypto: mips/poly1305 - incorporate OpenSSL/CRYPTOGAMS
> optimized implementation")
> Signed-off-by: Ilya Lipnitskiy
> Cc: Ard Biesheuvel
> ---
> arch/mips/crypto/.gitignore | 2 ++
> 1 file changed, 2 insertions(+)
> create mode 100644 arch/mips/crypto/.gitignore
applied to mips-next.
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.[ RFC1925, 2.3 ]
Re: [PATCH v8 1/3] lib: zstd: Add kernel-specific API
On Sat, Mar 27, 2021 at 2:48 PM Oleksandr Natalenko
wrote:
>
> Hello.
>
> On Sat, Mar 27, 2021 at 05:48:01PM +0800, kernel test robot wrote:
> > >> ERROR: modpost: "ZSTD_maxCLevel" [fs/f2fs/f2fs.ko] undefined!
>
> Since f2fs can be built as a module, the following correction seems to
> be needed:
Thanks Oleksandr! Looks like f2fs has been updated to use
ZSTD_maxCLevel() since the first version of these patches. I'll put up
a new version shortly with the fix, and update my test suite to build
f2fs and other users as modules, so it can catch this.
Best,
Nick
> ```
> diff --git a/lib/zstd/compress/zstd_compress.c
> b/lib/zstd/compress/zstd_compress.c
> index 9c998052a0e5..584c92c51169 100644
> --- a/lib/zstd/compress/zstd_compress.c
> +++ b/lib/zstd/compress/zstd_compress.c
> @@ -4860,6 +4860,7 @@ size_t ZSTD_endStream(ZSTD_CStream* zcs,
> ZSTD_outBuffer* output)
>
> #define ZSTD_MAX_CLEVEL 22
> int ZSTD_maxCLevel(void) { return ZSTD_MAX_CLEVEL; }
> +EXPORT_SYMBOL(ZSTD_maxCLevel);
> int ZSTD_minCLevel(void) { return (int)-ZSTD_TARGETLENGTH_MAX; }
>
> static const ZSTD_compressionParameters
> ZSTD_defaultCParameters[4][ZSTD_MAX_CLEVEL+1] = {
> ```
>
> Not sure if the same should be done for `ZSTD_minCLevel()` since I don't
> see it being used anywhere else.
>
> --
> Oleksandr Natalenko (post-factum)
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Sat, Mar 27, 2021 at 01:41:24PM +0100, David Gstir wrote: > Hi! > > > On 25.03.2021, at 06:26, Sumit Garg wrote: > > > > On Wed, 24 Mar 2021 at 19:37, Ahmad Fatoum wrote: > >> > >> Hello Sumit, > >> > >> On 24.03.21 11:47, Sumit Garg wrote: > >>> On Wed, 24 Mar 2021 at 14:56, Ahmad Fatoum > >>> wrote: > > Hello Mimi, > > On 23.03.21 19:07, Mimi Zohar wrote: > > On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote: > >> On 21.03.21 21:48, Horia Geantă wrote: > >>> caam has random number generation capabilities, so it's worth using > >>> that > >>> by implementing .get_random. > >> > >> If the CAAM HWRNG is already seeding the kernel RNG, why not use the > >> kernel's? > >> > >> Makes for less code duplication IMO. > > > > Using kernel RNG, in general, for trusted keys has been discussed > > before. Please refer to Dave Safford's detailed explanation for not > > using it [1]. > > The argument seems to boil down to: > > - TPM RNG are known to be of good quality > - Trusted keys always used it so far > > Both are fine by me for TPMs, but the CAAM backend is new code and > neither point > really applies. > > get_random_bytes_wait is already used for generating key material > elsewhere. > Why shouldn't new trusted key backends be able to do the same thing? > > >>> > >>> Please refer to documented trusted keys behaviour here [1]. New > >>> trusted key backends should align to this behaviour and in your case > >>> CAAM offers HWRNG so we should be better using that. > >> > >> Why is it better? > >> > >> Can you explain what benefit a CAAM user would have if the trusted key > >> randomness comes directly out of the CAAM instead of indirectly from > >> the kernel entropy pool that is seeded by it? > > > > IMO, user trust in case of trusted keys comes from trusted keys > > backend which is CAAM here. If a user doesn't trust that CAAM would > > act as a reliable source for RNG then CAAM shouldn't be used as a > > trust source in the first place. > > > > And I think building user's trust for kernel RNG implementation with > > multiple entropy contributions is pretty difficult when compared with > > CAAM HWRNG implementation. > > Generally speaking, I’d say trusting the CAAM RNG and trusting in it’s > other features are two separate things. However, reading through the CAAM > key blob spec I’ve got here, CAAM key blob keys (the keys that secure a blob’s > content) are generated using its internal RNG. So I’d save if the CAAM RNG > is insecure, so are generated key blobs. Maybe somebody with more insight > into the CAAM internals can verify that, but I don’t see any point in using > the kernel’s RNG as long as we let CAAM generate the key blob keys for us. Here's my long'ish analysis. Please read it to the end if by ever means possible, and apologies, I usually try to keep usually my comms short, but this requires some more meat than the usual. The Bad News Now that we add multiple hardware trust sources for trusted keys, will there ever be a scenario where a trusted key is originally sealed with a backing hardware A, unsealed, and resealed with hardware B? The hardware and vendor neutral way to generate the key material would be unconditionally always just the kernel RNG. CAAM is actually worse than TCG because it's not even a standards body, if I got it right. Not a lot but at least a tiny fraction. This brings an open item in TEE patches: trusted_tee_get_random() is an issue in generating kernel material. I would rather replace that with kernel RNG *for now*, because the same open question applies also to ARM TEE. It's also a single company controlled backing technology. By all practical means, I do trust ARM TEE in my personal life but this is not important. CAAM *and* TEE backends break the golden rule of putting as little trust as possible to anything, even not anything weird is clear at sight, as security is essentially a game of known unknowns and unknown unknowns. Unfortunately, TPM trusted keys started this bad security practice, and obviously it cannot be fixed without breaking uapi backwards compatibility. This leaves me exactly two rational options: A. Add a patch to remove trusted_tee_get_random() and use kernel RNG instead. B. Drop the whole TEE patch set up until I have good reasons to believe that it's the best possible idea ever to use TEE RNG. Doing does (A) does not disclude of doing (B) later on, if someone some day sends a patch with sound reasoning. It's also good to understand that when some day a vendor D, other than TCG, CAAM or ARM, comes up, we need to go again this lenghty and messy discussion. Now this already puts an already accepted patch set into a risk, because by being a responsible maintainer I would have legit reasons just simply to drop it. OK, but The GOOD News ===
Re: [PATCH] crypto: hisilicon - check if debugfs opened
On Sat, Mar 27, 2021 at 04:33:00PM +0800, Hui Tang wrote: > 'xx_debugfs_init' check if debugfs opened. > > Signed-off-by: Hui Tang > --- > drivers/crypto/hisilicon/hpre/hpre_main.c | 5 - > drivers/crypto/hisilicon/qm.c | 3 +++ > drivers/crypto/hisilicon/sec2/sec_main.c | 5 - > drivers/crypto/hisilicon/zip/zip_main.c | 3 +++ > 4 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/crypto/hisilicon/hpre/hpre_main.c > b/drivers/crypto/hisilicon/hpre/hpre_main.c > index c7ab06d..f2605c4 100644 > --- a/drivers/crypto/hisilicon/hpre/hpre_main.c > +++ b/drivers/crypto/hisilicon/hpre/hpre_main.c > @@ -779,6 +779,9 @@ static int hpre_debugfs_init(struct hisi_qm *qm) > struct device *dev = &qm->pdev->dev; > int ret; > > + if (!debugfs_initialized()) > + return -ENOENT; Why? What does this help with? Why does the code care if debugfs is running or not? thanks, greg k-h
Re: [PATCH v8 1/3] lib: zstd: Add kernel-specific API
Hello.
On Sat, Mar 27, 2021 at 05:48:01PM +0800, kernel test robot wrote:
> >> ERROR: modpost: "ZSTD_maxCLevel" [fs/f2fs/f2fs.ko] undefined!
Since f2fs can be built as a module, the following correction seems to
be needed:
```
diff --git a/lib/zstd/compress/zstd_compress.c
b/lib/zstd/compress/zstd_compress.c
index 9c998052a0e5..584c92c51169 100644
--- a/lib/zstd/compress/zstd_compress.c
+++ b/lib/zstd/compress/zstd_compress.c
@@ -4860,6 +4860,7 @@ size_t ZSTD_endStream(ZSTD_CStream* zcs, ZSTD_outBuffer*
output)
#define ZSTD_MAX_CLEVEL 22
int ZSTD_maxCLevel(void) { return ZSTD_MAX_CLEVEL; }
+EXPORT_SYMBOL(ZSTD_maxCLevel);
int ZSTD_minCLevel(void) { return (int)-ZSTD_TARGETLENGTH_MAX; }
static const ZSTD_compressionParameters
ZSTD_defaultCParameters[4][ZSTD_MAX_CLEVEL+1] = {
```
Not sure if the same should be done for `ZSTD_minCLevel()` since I don't
see it being used anywhere else.
--
Oleksandr Natalenko (post-factum)
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
Hi! > On 25.03.2021, at 06:26, Sumit Garg wrote: > > On Wed, 24 Mar 2021 at 19:37, Ahmad Fatoum wrote: >> >> Hello Sumit, >> >> On 24.03.21 11:47, Sumit Garg wrote: >>> On Wed, 24 Mar 2021 at 14:56, Ahmad Fatoum wrote: Hello Mimi, On 23.03.21 19:07, Mimi Zohar wrote: > On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote: >> On 21.03.21 21:48, Horia Geantă wrote: >>> caam has random number generation capabilities, so it's worth using that >>> by implementing .get_random. >> >> If the CAAM HWRNG is already seeding the kernel RNG, why not use the >> kernel's? >> >> Makes for less code duplication IMO. > > Using kernel RNG, in general, for trusted keys has been discussed > before. Please refer to Dave Safford's detailed explanation for not > using it [1]. The argument seems to boil down to: - TPM RNG are known to be of good quality - Trusted keys always used it so far Both are fine by me for TPMs, but the CAAM backend is new code and neither point really applies. get_random_bytes_wait is already used for generating key material elsewhere. Why shouldn't new trusted key backends be able to do the same thing? >>> >>> Please refer to documented trusted keys behaviour here [1]. New >>> trusted key backends should align to this behaviour and in your case >>> CAAM offers HWRNG so we should be better using that. >> >> Why is it better? >> >> Can you explain what benefit a CAAM user would have if the trusted key >> randomness comes directly out of the CAAM instead of indirectly from >> the kernel entropy pool that is seeded by it? > > IMO, user trust in case of trusted keys comes from trusted keys > backend which is CAAM here. If a user doesn't trust that CAAM would > act as a reliable source for RNG then CAAM shouldn't be used as a > trust source in the first place. > > And I think building user's trust for kernel RNG implementation with > multiple entropy contributions is pretty difficult when compared with > CAAM HWRNG implementation. Generally speaking, I’d say trusting the CAAM RNG and trusting in it’s other features are two separate things. However, reading through the CAAM key blob spec I’ve got here, CAAM key blob keys (the keys that secure a blob’s content) are generated using its internal RNG. So I’d save if the CAAM RNG is insecure, so are generated key blobs. Maybe somebody with more insight into the CAAM internals can verify that, but I don’t see any point in using the kernel’s RNG as long as we let CAAM generate the key blob keys for us. Cheers, dave
Re: [PATCH] crypto: hisilicon/sec - Fix a module parameter error
On 2021/3/27 17:29, Longfang Liu Wrote:
> ctx_q_num is a module parameter set by the user to specify the
> number of qp queues required to create a ctx.
>
> When the number of qp queues allocated by PF or VF is less than
> the ctx_q_num, an error will be reported when ctx is initialized
> in kernel mode, which leads to the problem that the registered
> algorithms cannot be used.
>
> Therefore, when PF or VF is initialized, if the number of qp queues
> is not enough to create a ctx, the kernel mode cannot be used,
> and there is no need to register the kernel mode algorithms.
>
> Signed-off-by: Longfang Liu
> ---
> drivers/crypto/hisilicon/sec2/sec_main.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/crypto/hisilicon/sec2/sec_main.c
> b/drivers/crypto/hisilicon/sec2/sec_main.c
> index b1818f7..c7b71b6 100644
> --- a/drivers/crypto/hisilicon/sec2/sec_main.c
> +++ b/drivers/crypto/hisilicon/sec2/sec_main.c
> @@ -867,10 +867,15 @@ static int sec_probe(struct pci_dev *pdev, const struct
> pci_device_id *id)
> if (ret)
> pci_warn(pdev, "Failed to init debugfs!\n");
>
> - ret = hisi_qm_alg_register(qm, &sec_devices);
> - if (ret < 0) {
> - pr_err("Failed to register driver to crypto.\n");
> - goto err_qm_stop;
> + if (qm->qp_num >= ctx_q_num) {
> + ret = hisi_qm_alg_register(qm, &sec_devices);
> + if (ret < 0) {
> + pr_err("Failed to register driver to crypto.\n");
> + goto err_qm_stop;
> + }
> + } else {
> + pci_warn(qm->pdev,
> + "Failed to use kernel mode, qp not enough!\n");
> }
>
> if (qm->uacce) {
>
Sorry, please don't reply to this patch, I will resend it again.
Thanks,
Longfang
Re: [PATCH v8 1/3] lib: zstd: Add kernel-specific API
Hi Nick, Thank you for the patch! Yet something to improve: [auto build test ERROR on cryptodev/master] [also build test ERROR on kdave/for-next f2fs/dev-test linus/master v5.12-rc4 next-20210326] [cannot apply to crypto/master kees/for-next/pstore squashfs/master] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Nick-Terrell/Update-to-zstd-1-4-10/20210327-031827 base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master config: sh-allmodconfig (attached as .config) compiler: sh4-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/c7b74f012a26721a207dd6a9a9a05210a8b1d627 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Nick-Terrell/Update-to-zstd-1-4-10/20210327-031827 git checkout c7b74f012a26721a207dd6a9a9a05210a8b1d627 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>, old ones prefixed by <<): ERROR: modpost: "__delay" [drivers/net/mdio/mdio-cavium.ko] undefined! >> ERROR: modpost: "ZSTD_maxCLevel" [fs/f2fs/f2fs.ko] undefined! ERROR: modpost: "__udivdi3" [fs/btrfs/btrfs.ko] undefined! ERROR: modpost: "__umoddi3" [fs/btrfs/btrfs.ko] undefined! Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA Selected by - SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC - SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
Re: [PATCH 4/4] crypto: hisilicon/zip - support new 'sqe' type in Kunpeng930
On 2021/3/26 17:14, Herbert Xu wrote:
On Fri, Mar 19, 2021 at 03:33:07PM +0800, Yang Shen wrote:
+const struct hisi_zip_sqe_ops hisi_zip_ops_v2 = {
+ .sqe_type = 0x3,
+ .fill_addr = hisi_zip_fill_addr,
+ .fill_buf_size = hisi_zip_fill_buf_size,
+ .fill_buf_type = hisi_zip_fill_buf_type,
+ .fill_req_type = hisi_zip_fill_req_type,
+ .fill_tag = hisi_zip_fill_tag_v2,
+ .fill_sqe_type = hisi_zip_fill_sqe_type,
+ .get_tag= hisi_zip_get_tag_v2,
+ .get_status = hisi_zip_get_status,
+ .get_dstlen = hisi_zip_get_dstlen,
+};
+
This triggers a new warning:
CHECK ../drivers/crypto/hisilicon/zip/zip_crypto.c
../drivers/crypto/hisilicon/zip/zip_crypto.c:527:31: warning: symbol
'hisi_zip_ops_v1' was not declared. Should it be static?
../drivers/crypto/hisilicon/zip/zip_crypto.c:540:31: warning: symbol
'hisi_zip_ops_v2' was not declared. Should it be static?
Please fix. Thanks.
Sorry， I'll fix this in next version.
Thanks.
Re: [PATCH v8 3/3] lib: zstd: Upgrade to latest upstream zstd version 1.4.10
On Fri, Mar 26, 2021 at 3:02 PM kernel test robot wrote: > > Hi Nick, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on cryptodev/master] > [also build test WARNING on kdave/for-next f2fs/dev-test linus/master > v5.12-rc4 next-20210326] > [cannot apply to crypto/master kees/for-next/pstore squashfs/master] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: > https://github.com/0day-ci/linux/commits/Nick-Terrell/Update-to-zstd-1-4-10/20210327-031827 > base: > https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git > master > config: um-allmodconfig (attached as .config) > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > reproduce (this is a W=1 build): > # > https://github.com/0day-ci/linux/commit/ebbff13fa6a537fb8b3dc6b42c3093f9ce4358f8 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review > Nick-Terrell/Update-to-zstd-1-4-10/20210327-031827 > git checkout ebbff13fa6a537fb8b3dc6b42c3093f9ce4358f8 > # save the attached .config to linux build tree > make W=1 ARCH=um > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > All warnings (new ones prefixed by >>): > >lib/zstd/compress/zstd_compress_sequences.c:17: warning: Cannot understand > * -log2(x / 256) lookup table for x in [0, 256). > on line 17 - I thought it was a doc line >lib/zstd/compress/zstd_compress_sequences.c:58: warning: Function > parameter or member 'nbSeq' not described in 'ZSTD_useLowProbCount' > >> lib/zstd/compress/zstd_compress_sequences.c:58: warning: expecting > >> prototype for 1 else we should(). Prototype was for ZSTD_useLowProbCount() > >> instead > >> lib/zstd/compress/zstd_compress_sequences.c:67: warning: wrong kernel-doc > >> identifier on line: > * Returns the cost in bytes of encoding the normalized count header. >lib/zstd/compress/zstd_compress_sequences.c:85: warning: Function > parameter or member 'count' not described in 'ZSTD_entropyCost' >lib/zstd/compress/zstd_compress_sequences.c:85: warning: Function > parameter or member 'max' not described in 'ZSTD_entropyCost' >lib/zstd/compress/zstd_compress_sequences.c:85: warning: Function > parameter or member 'total' not described in 'ZSTD_entropyCost' > >> lib/zstd/compress/zstd_compress_sequences.c:85: warning: expecting > >> prototype for Returns the cost in bits of encoding the distribution > >> described by count(). Prototype was for ZSTD_entropyCost() instead >lib/zstd/compress/zstd_compress_sequences.c:99: warning: wrong kernel-doc > identifier on line: > * Returns the cost in bits of encoding the distribution in count using > ctable. >lib/zstd/compress/zstd_compress_sequences.c:139: warning: Function > parameter or member 'norm' not described in 'ZSTD_crossEntropyCost' >lib/zstd/compress/zstd_compress_sequences.c:139: warning: Function > parameter or member 'accuracyLog' not described in 'ZSTD_crossEntropyCost' >lib/zstd/compress/zstd_compress_sequences.c:139: warning: Function > parameter or member 'count' not described in 'ZSTD_crossEntropyCost' >lib/zstd/compress/zstd_compress_sequences.c:139: warning: Function > parameter or member 'max' not described in 'ZSTD_crossEntropyCost' > >> lib/zstd/compress/zstd_compress_sequences.c:139: warning: expecting > >> prototype for Returns the cost in bits of encoding the distribution in > >> count using the(). Prototype was for ZSTD_crossEntropyCost() instead > -- >lib/zstd/compress/zstd_ldm.c:584: warning: Function parameter or member > 'rawSeqStore' not described in 'maybeSplitSequence' >lib/zstd/compress/zstd_ldm.c:584: warning: Function parameter or member > 'remaining' not described in 'maybeSplitSequence' >lib/zstd/compress/zstd_ldm.c:584: warning: Function parameter or member > 'minMatch' not described in 'maybeSplitSequence' > >> lib/zstd/compress/zstd_ldm.c:584: warning: expecting prototype for If the > >> sequence length is longer than remaining then the sequence is split(). > >> Prototype was for maybeSplitSequence() instead > -- > >> lib/zstd/decompress/zstd_decompress.c:992: warning: wrong kernel-doc > >> identifier on line: > * Similar to ZSTD_nextSrcSizeToDecompress(), but when when a block input > can be streamed, > -- >lib/zstd/decompress/huf_decompress.c:122: warning: Function parameter or > member 'symbol' not described in 'HUF_DEltX1_set4' >lib/zstd/decompress/huf_decompress.c:122: warning: Function parameter or > member 'nbBits' not described in 'HUF_DEltX1_set4' > >> lib/zstd/decompress/huf_decompress.c:122: warning: expecting prototype for > >> This is used to lay down 4 entries at(). Prototype was for > >> HUF_DEltX1_set4() instead > -- > >> lib/zstd/
Re: [PATCH v8 3/3] lib: zstd: Upgrade to latest upstream zstd version 1.4.10
Hi Nick, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on cryptodev/master] [also build test WARNING on kdave/for-next f2fs/dev-test linus/master v5.12-rc4 next-20210326] [cannot apply to crypto/master kees/for-next/pstore squashfs/master] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Nick-Terrell/Update-to-zstd-1-4-10/20210327-031827 base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master config: um-allmodconfig (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/ebbff13fa6a537fb8b3dc6b42c3093f9ce4358f8 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Nick-Terrell/Update-to-zstd-1-4-10/20210327-031827 git checkout ebbff13fa6a537fb8b3dc6b42c3093f9ce4358f8 # save the attached .config to linux build tree make W=1 ARCH=um If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): lib/zstd/compress/zstd_compress_sequences.c:17: warning: Cannot understand * -log2(x / 256) lookup table for x in [0, 256). on line 17 - I thought it was a doc line lib/zstd/compress/zstd_compress_sequences.c:58: warning: Function parameter or member 'nbSeq' not described in 'ZSTD_useLowProbCount' >> lib/zstd/compress/zstd_compress_sequences.c:58: warning: expecting prototype >> for 1 else we should(). Prototype was for ZSTD_useLowProbCount() instead >> lib/zstd/compress/zstd_compress_sequences.c:67: warning: wrong kernel-doc >> identifier on line: * Returns the cost in bytes of encoding the normalized count header. lib/zstd/compress/zstd_compress_sequences.c:85: warning: Function parameter or member 'count' not described in 'ZSTD_entropyCost' lib/zstd/compress/zstd_compress_sequences.c:85: warning: Function parameter or member 'max' not described in 'ZSTD_entropyCost' lib/zstd/compress/zstd_compress_sequences.c:85: warning: Function parameter or member 'total' not described in 'ZSTD_entropyCost' >> lib/zstd/compress/zstd_compress_sequences.c:85: warning: expecting prototype >> for Returns the cost in bits of encoding the distribution described by >> count(). Prototype was for ZSTD_entropyCost() instead lib/zstd/compress/zstd_compress_sequences.c:99: warning: wrong kernel-doc identifier on line: * Returns the cost in bits of encoding the distribution in count using ctable. lib/zstd/compress/zstd_compress_sequences.c:139: warning: Function parameter or member 'norm' not described in 'ZSTD_crossEntropyCost' lib/zstd/compress/zstd_compress_sequences.c:139: warning: Function parameter or member 'accuracyLog' not described in 'ZSTD_crossEntropyCost' lib/zstd/compress/zstd_compress_sequences.c:139: warning: Function parameter or member 'count' not described in 'ZSTD_crossEntropyCost' lib/zstd/compress/zstd_compress_sequences.c:139: warning: Function parameter or member 'max' not described in 'ZSTD_crossEntropyCost' >> lib/zstd/compress/zstd_compress_sequences.c:139: warning: expecting >> prototype for Returns the cost in bits of encoding the distribution in count >> using the(). Prototype was for ZSTD_crossEntropyCost() instead -- lib/zstd/compress/zstd_ldm.c:584: warning: Function parameter or member 'rawSeqStore' not described in 'maybeSplitSequence' lib/zstd/compress/zstd_ldm.c:584: warning: Function parameter or member 'remaining' not described in 'maybeSplitSequence' lib/zstd/compress/zstd_ldm.c:584: warning: Function parameter or member 'minMatch' not described in 'maybeSplitSequence' >> lib/zstd/compress/zstd_ldm.c:584: warning: expecting prototype for If the >> sequence length is longer than remaining then the sequence is split(). >> Prototype was for maybeSplitSequence() instead -- >> lib/zstd/decompress/zstd_decompress.c:992: warning: wrong kernel-doc >> identifier on line: * Similar to ZSTD_nextSrcSizeToDecompress(), but when when a block input can be streamed, -- lib/zstd/decompress/huf_decompress.c:122: warning: Function parameter or member 'symbol' not described in 'HUF_DEltX1_set4' lib/zstd/decompress/huf_decompress.c:122: warning: Function parameter or member 'nbBits' not described in 'HUF_DEltX1_set4' >> lib/zstd/decompress/huf_decompress.c:122: warning: expecting prototype for >> This is used to lay down 4 entries at(). Prototype was for HUF_DEltX1_set4() >> instead -- >> lib/zstd/compress/zstd_compress.c:128: warning: wrong kernel-doc identifier >> on line: * Clears and frees all of the dictionaries in the CCtx. lib/zstd/compress/zstd_compress.c:265: warning: wrong kernel-doc identifier on line: * Initializes
Re: [PATCH] crypto: sm3 - use the more precise type u32 instead of unsigned int
Hi,
Thank you for the patch!
On Fri, Mar 26, 2021 at 5:21 AM Tianjia Zhang
wrote:
>
> In the process of calculating the hash, use the more accurate type
> 'u32' instead of the original 'unsigned int' to avoid ambiguity.
I don't think there is any ambiguity here, as both forms are always
the same size.
Generally, I tend to use the convention of using 'u32' as denoting
variables where the size is meaningful - e.g. mathematical operations
that are defined in the standard on 32 bit buffers, versus using
plain 'int' types where it isn't - e.g. loop counters etc.
Having said that, even under my own definition possibly the w and wt
arrays in sm3_trandform() should be changed to u32.
I don't object to changing those if it bugs you :-)
Cheers,
Gilad
> Signed-off-by: Tianjia Zhang
> ---
> crypto/sm3_generic.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/crypto/sm3_generic.c b/crypto/sm3_generic.c
> index 193c4584bd00..562e96f92f64 100644
> --- a/crypto/sm3_generic.c
> +++ b/crypto/sm3_generic.c
> @@ -36,17 +36,17 @@ static inline u32 p1(u32 x)
> return x ^ rol32(x, 15) ^ rol32(x, 23);
> }
>
> -static inline u32 ff(unsigned int n, u32 a, u32 b, u32 c)
> +static inline u32 ff(u32 n, u32 a, u32 b, u32 c)
> {
> return (n < 16) ? (a ^ b ^ c) : ((a & b) | (a & c) | (b & c));
> }
>
> -static inline u32 gg(unsigned int n, u32 e, u32 f, u32 g)
> +static inline u32 gg(u32 n, u32 e, u32 f, u32 g)
> {
> return (n < 16) ? (e ^ f ^ g) : ((e & f) | ((~e) & g));
> }
>
> -static inline u32 t(unsigned int n)
> +static inline u32 t(u32 n)
> {
> return (n < 16) ? SM3_T1 : SM3_T2;
> }
> @@ -54,7 +54,7 @@ static inline u32 t(unsigned int n)
> static void sm3_expand(u32 *t, u32 *w, u32 *wt)
> {
> int i;
> - unsigned int tmp;
> + u32 tmp;
>
> /* load the input */
> for (i = 0; i <= 15; i++)
> @@ -123,8 +123,8 @@ static void sm3_compress(u32 *w, u32 *wt, u32 *m)
>
> static void sm3_transform(struct sm3_state *sst, u8 const *src)
> {
> - unsigned int w[68];
> - unsigned int wt[64];
> + u32 w[68];
> + u32 wt[64];
>
> sm3_expand((u32 *)src, w, wt);
> sm3_compress(w, wt, sst->state);
> --
> 2.19.1.3.ge56e4f7
>
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
Re: [PATCH] crypto: nx: fix incorrect kernel-doc comment syntax in files
On Sun, Mar 21, 2021 at 06:00:07PM +0530, Aditya Srivastava wrote: > The opening comment mark '/**' is used for highlighting the beginning of > kernel-doc comments. > There are certain files in drivers/crypto/nx, which follow this syntax, > but the content inside does not comply with kernel-doc. > Such lines were probably not meant for kernel-doc parsing, but are parsed > due to the presence of kernel-doc like comment syntax(i.e, '/**'), which > causes unexpected warnings from kernel-doc. > > E.g., presence of kernel-doc like comment in the header lines for > drivers/crypto/nx/nx-sha256.c at header causes these warnings: > "warning: Function parameter or member 'tfm' not described in > 'nx_crypto_ctx_sha256_init'" > "warning: expecting prototype for SHA(). Prototype was for > nx_crypto_ctx_sha256_init() instead" > > Similarly for other files too. > > Provide a simple fix by replacing such occurrences with general comment > format, i.e. '/*', to prevent kernel-doc from parsing it. > > Signed-off-by: Aditya Srivastava > --- > * Applies perfectly on next-20210319 > > drivers/crypto/nx/nx-aes-cbc.c | 2 +- > drivers/crypto/nx/nx-aes-ccm.c | 2 +- > drivers/crypto/nx/nx-aes-ctr.c | 2 +- > drivers/crypto/nx/nx-aes-ecb.c | 2 +- > drivers/crypto/nx/nx-aes-gcm.c | 2 +- > drivers/crypto/nx/nx-aes-xcbc.c | 2 +- > drivers/crypto/nx/nx-sha256.c | 2 +- > drivers/crypto/nx/nx-sha512.c | 2 +- > drivers/crypto/nx/nx.c | 2 +- > drivers/crypto/nx/nx_debugfs.c | 2 +- > 10 files changed, 10 insertions(+), 10 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: vmx: fix incorrect kernel-doc comment syntax in files
On Sun, Mar 21, 2021 at 01:55:25AM +0530, Aditya Srivastava wrote: > The opening comment mark '/**' is used for highlighting the beginning of > kernel-doc comments. > There are certain files in drivers/crypto/vmx, which follow this syntax, > but the content inside does not comply with kernel-doc. > Such lines were probably not meant for kernel-doc parsing, but are parsed > due to the presence of kernel-doc like comment syntax(i.e, '/**'), which > causes unexpected warnings from kernel-doc. > > E.g., presence of kernel-doc like comment in the header line for > drivers/crypto/vmx/vmx.c causes this warning by kernel-doc: > > "warning: expecting prototype for Routines supporting VMX instructions on the > Power 8(). Prototype was for p8_init() instead" > > Similarly for other files too. > > Provide a simple fix by replacing such occurrences with general comment > format, i.e. '/*', to prevent kernel-doc from parsing it. > > Signed-off-by: Aditya Srivastava > --- > * Applies perfectly on next-20210319 > > drivers/crypto/vmx/aes.c | 2 +- > drivers/crypto/vmx/aes_cbc.c | 2 +- > drivers/crypto/vmx/aes_ctr.c | 2 +- > drivers/crypto/vmx/aes_xts.c | 2 +- > drivers/crypto/vmx/ghash.c | 2 +- > drivers/crypto/vmx/vmx.c | 2 +- > 6 files changed, 6 insertions(+), 6 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: ux500: fix incorrect kernel-doc comment syntax
On Sun, Mar 21, 2021 at 05:39:12PM +0530, Aditya Srivastava wrote: > The opening comment mark '/**' is used for highlighting the beginning of > kernel-doc comments. > There are certain files in drivers/crypto/ux500, which follow this syntax, > but the content inside does not comply with kernel-doc. > Such lines were probably not meant for kernel-doc parsing, but are parsed > due to the presence of kernel-doc like comment syntax(i.e, '/**'), which > causes unexpected warnings from kernel-doc. > > E.g., presence of kernel-doc like comment in the header lines for > drivers/crypto/ux500/cryp/cryp.h at header causes this warning: > > "warning: expecting prototype for ST(). Prototype was for _CRYP_H_() instead" > > Similarly for other files too. > > Provide a simple fix by replacing such occurrences with general comment > format, i.e. '/*', to prevent kernel-doc from parsing it. > > Signed-off-by: Aditya Srivastava > --- > drivers/crypto/ux500/cryp/cryp.c | 2 +- > drivers/crypto/ux500/cryp/cryp.h | 2 +- > drivers/crypto/ux500/cryp/cryp_core.c | 2 +- > drivers/crypto/ux500/cryp/cryp_irq.c | 2 +- > drivers/crypto/ux500/cryp/cryp_irq.h | 4 ++-- > drivers/crypto/ux500/cryp/cryp_irqp.h | 4 ++-- > drivers/crypto/ux500/cryp/cryp_p.h| 14 +++--- > 7 files changed, 15 insertions(+), 15 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: amcc: fix incorrect kernel-doc comment syntax in files
On Sun, Mar 21, 2021 at 05:38:32PM +0530, Aditya Srivastava wrote: > The opening comment mark '/**' is used for highlighting the beginning of > kernel-doc comments. > There are certain files in drivers/crypto/amcc, which follow this syntax, > but the content inside does not comply with kernel-doc. > Such lines were probably not meant for kernel-doc parsing, but are parsed > due to the presence of kernel-doc like comment syntax(i.e, '/**'), which > causes unexpected warnings from kernel-doc. > > E.g., presence of kernel-doc like comment in > drivers/crypto/amcc/crypto4xx_alg.c at header, and some other lines, > causes these warnings by kernel-doc: > > "warning: expecting prototype for AMCC SoC PPC4xx Crypto Driver(). Prototype > was for set_dynamic_sa_command_0() instead" > "warning: Function parameter or member 'dir' not described in > 'set_dynamic_sa_command_0'" > etc.. > > Provide a simple fix by replacing such occurrences with general comment > format, i.e. '/*', to prevent kernel-doc from parsing it. > > Signed-off-by: Aditya Srivastava > --- > drivers/crypto/amcc/crypto4xx_alg.c | 12 ++-- > drivers/crypto/amcc/crypto4xx_core.c| 18 +- > drivers/crypto/amcc/crypto4xx_core.h| 4 ++-- > drivers/crypto/amcc/crypto4xx_reg_def.h | 8 > drivers/crypto/amcc/crypto4xx_sa.h | 18 +- > drivers/crypto/amcc/crypto4xx_trng.h| 2 +- > 6 files changed, 31 insertions(+), 31 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 0/4] crypto: hisilicon/qm - support doorbell isolation and queue number configuration
On Sat, Mar 20, 2021 at 07:27:42PM +0800, Weili Qian wrote: > Kunpeng930 supports getting the number of queues from hardware registers > and queue doorbell isolation. > > This patchset configures the total number of hardware queues and the > maximum number of function queues, and supports queue doorbell isolation. > > Weili Qian (4): > crypto: hisilicon/qm - set the total number of queues > crypto: hisilicon/qm - move 'CURRENT_QM' code to qm.c > crypto: hisilicon/qm - set the number of queues for function > crypto: hisilicon/qm - add queue isolation support for Kunpeng 930 > > drivers/crypto/hisilicon/hpre/hpre.h | 1 - > drivers/crypto/hisilicon/hpre/hpre_main.c | 64 -- > drivers/crypto/hisilicon/qm.c | 332 > +++--- > drivers/crypto/hisilicon/qm.h | 17 +- > drivers/crypto/hisilicon/sec2/sec.h | 1 - > drivers/crypto/hisilicon/sec2/sec_main.c | 65 +- > drivers/crypto/hisilicon/zip/zip_main.c | 65 +- > include/uapi/misc/uacce/hisi_qm.h | 1 + > 8 files changed, 272 insertions(+), 274 deletions(-) All applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: hisilicon/hpre - fix "hpre_ctx_init" resource leak
On Fri, Mar 19, 2021 at 06:45:27PM +0800, Hui Tang wrote: > When calling "hpre_ctx_set" fails, stop and put qp, > otherwise will leak qp resource. > > Signed-off-by: Hui Tang > --- > drivers/crypto/hisilicon/hpre/hpre_crypto.c | 13 - > 1 file changed, 12 insertions(+), 1 deletion(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: hisilicon/hpre - fix Kconfig
On Fri, Mar 19, 2021 at 06:45:39PM +0800, Hui Tang wrote: > hpre select 'CRYPTO_ECDH' and 'CRYPTO_CURVE25519'. > > Signed-off-by: Hui Tang > --- > drivers/crypto/hisilicon/Kconfig | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: hisilicon - fix the check on dma address
On Fri, Mar 19, 2021 at 06:45:05PM +0800, Hui Tang wrote: > System may be able to get physical address of zero if not reserved by > firmware. > > The dma address obtained by 'dma_alloc_coherent' is valid, since already > checking cpu va before, so do not check again. > > Signed-off-by: Hui Tang > --- > drivers/crypto/hisilicon/hpre/hpre_crypto.c | 12 > drivers/crypto/hisilicon/qm.c | 2 +- > drivers/crypto/hisilicon/sec2/sec_crypto.c | 6 -- > 3 files changed, 1 insertion(+), 19 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 0/2] crypto: hisilicon/hpre - remove 'CONFIG_CRYPTO_DH'
On Fri, Mar 19, 2021 at 06:44:17PM +0800, Hui Tang wrote: > Remove 'CONFIG_CRYPTO_DH' and optimise 'hpre_algs_register' error path. > > Hui Tang (2): > crypto: hisilicon/hpre - delete wrap of 'CONFIG_CRYPTO_DH' > crypto: hisilicon/hpre - optimise 'hpre_algs_register' error path > > drivers/crypto/hisilicon/hpre/hpre_crypto.c | 30 > ++--- > 1 file changed, 10 insertions(+), 20 deletions(-) All applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: Correct an error in the comments
On Fri, Mar 19, 2021 at 05:13:34PM +0800, Meng Yu wrote: > Remove repeated word 'bit' in comments. > > Signed-off-by: Meng Yu > --- > crypto/ecc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v3 00/10] Rid W=1 warnings in Crypto
On Thu, Mar 18, 2021 at 12:44:12PM +, Lee Jones wrote: > This is set 1 of 2 sets required to fully clean Crypto. > > v2: No functional changes since v1. > v3: Description change and additional struct header fix > > Lee Jones (10): > crypto: hisilicon: sec_drv: Supply missing description for > 'sec_queue_empty()'s 'queue' param > crypto: bcm: Fix a whole host of kernel-doc misdemeanours > crypto: chelsio: chcr_core: Fix some kernel-doc issues > crypto: ux500: hash: hash_core: Fix worthy kernel-doc headers and > remove others > crypto: keembay: ocs-hcu: Fix incorrectly named functions/structs > crypto: atmel-ecc: Struct headers need to start with keyword 'struct' > crypto: caam: caampkc: Provide the name of the function and provide > missing descriptions > crypto: vmx: Source headers are not good kernel-doc candidates > crypto: nx: nx-aes-cbc: Repair some kernel-doc problems > crypto: cavium: nitrox_isr: Demote non-compliant kernel-doc headers > > drivers/crypto/atmel-ecc.c| 2 +- > drivers/crypto/bcm/cipher.c | 7 ++-- > drivers/crypto/bcm/spu.c | 16 - > drivers/crypto/bcm/spu2.c | 43 +-- > drivers/crypto/bcm/util.c | 4 +-- > drivers/crypto/caam/caamalg_qi2.c | 3 ++ > drivers/crypto/caam/caampkc.c | 3 +- > drivers/crypto/cavium/nitrox/nitrox_isr.c | 4 +-- > drivers/crypto/chelsio/chcr_algo.c| 8 ++--- > drivers/crypto/chelsio/chcr_core.c| 2 +- > drivers/crypto/hisilicon/sec/sec_drv.c| 1 + > drivers/crypto/keembay/ocs-hcu.c | 8 ++--- > drivers/crypto/nx/nx-aes-cbc.c| 2 +- > drivers/crypto/nx/nx.c| 5 +-- > drivers/crypto/nx/nx_debugfs.c| 2 +- > drivers/crypto/ux500/cryp/cryp.c | 5 +-- > drivers/crypto/ux500/cryp/cryp_core.c | 5 +-- > drivers/crypto/ux500/cryp/cryp_irq.c | 2 +- > drivers/crypto/ux500/hash/hash_core.c | 15 +++- > drivers/crypto/vmx/vmx.c | 2 +- > 20 files changed, 73 insertions(+), 66 deletions(-) > > Cc: Alexandre Belloni > Cc: Andreas Westin > Cc: Atul Gupta > Cc: Aymen Sghaier > Cc: Ayush Sawal > Cc: Benjamin Herrenschmidt > Cc: Berne Hebark > Cc: "Breno Leitão" > Cc: Daniele Alessandrelli > Cc: "David S. Miller" > Cc: Declan Murphy > Cc: Harsh Jain > Cc: Henrique Cerri > Cc: Herbert Xu > Cc: "Horia Geantă" > Cc: Jitendra Lulla > Cc: Joakim Bech > Cc: Jonas Linde > Cc: Jonathan Cameron > Cc: Kent Yoder > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-crypto@vger.kernel.org > Cc: linuxppc-...@lists.ozlabs.org > Cc: Ludovic Desroches > Cc: Manoj Malviya > Cc: Michael Ellerman > Cc: M R Gowda > Cc: Nayna Jain > Cc: Nicolas Ferre > Cc: Niklas Hernaeus > Cc: Paul Mackerras > Cc: Paulo Flabiano Smorigo > Cc: Rob Rice > Cc: Rohit Maheshwari > Cc: Shujuan Chen > Cc: Tudor Ambarus > Cc: Vinay Kumar Yadav > Cc: Zaibo Xu All applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v5 0/2] crypto: qat - fix couple crashes duing error handling
On Thu, Mar 18, 2021 at 11:39:58PM -0400, Tong Zhang wrote: > There are a couple of issues in qat error handling. Those drivers tries to > release resources that is not initialized. This patch series tries to fix > crashes caused by incorrect error handling. > > v2: removed excessive dump in commit log as suggested by Andy Shevchenko > > v3: collect tags as suggested by Andy Shevchenko > v4: fix commit log typos > v5: fix headline > > Tong Zhang (2): > crypto: qat - don't release uninitialized resources > crypto: qat - ADF_STATUS_PF_RUNNING should be set after adf_dev_init > > drivers/crypto/qat/qat_c3xxxvf/adf_drv.c| 4 ++-- > drivers/crypto/qat/qat_c62xvf/adf_drv.c | 4 ++-- > drivers/crypto/qat/qat_common/adf_vf_isr.c | 17 + > drivers/crypto/qat/qat_dh895xccvf/adf_drv.c | 4 ++-- > 4 files changed, 19 insertions(+), 10 deletions(-) All applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: inside-secure: Minor typo fix in the file safexcel.c
On Wed, Mar 17, 2021 at 02:44:45PM +0530, Bhaskar Chowdhury wrote: > > s/procesing/processing/ > > Signed-off-by: Bhaskar Chowdhury > --- > drivers/crypto/inside-secure/safexcel.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: jitterentropy: Put constants on the right side of the expression
On Tue, Mar 16, 2021 at 06:44:03PM -0700, Milan Djurovic wrote: > This patch fixes the following checkpatch.pl warnings: > > crypto/jitterentropy.c:600: WARNING: Comparisons should place the constant on > the right side of the test > crypto/jitterentropy.c:681: WARNING: Comparisons should place the constant on > the right side of the test > crypto/jitterentropy.c:772: WARNING: Comparisons should place the constant on > the right side of the test > crypto/jitterentropy.c:829: WARNING: Comparisons should place the constant on > the right side of the test > > Signed-off-by: Milan Djurovic > --- > crypto/jitterentropy.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v12 00/10] Add support for x509 certs with NIST P384/256/192 keys
On Tue, Mar 16, 2021 at 05:07:30PM -0400, Stefan Berger wrote:
> This series of patches adds support for x509 certificates signed by a CA
> that uses NIST P384, P256 or P192 keys for signing. It also adds support for
> certificates where the public key is one of this type of a key. The math
> for ECDSA signature verification is also added as well as the math for fast
> mmod operation for NIST P384.
>
> Since self-signed certificates are verified upon loading, the following
> script can be used for testing of NIST P256 keys:
>
> k=$(keyctl newring test @u)
>
> while :; do
> for hash in sha1 sha224 sha256 sha384 sha512; do
> openssl req \
> -x509 \
> -${hash} \
> -newkey ec \
> -pkeyopt ec_paramgen_curve:prime256v1 \
> -keyout key.pem \
> -days 365 \
> -subj '/CN=test' \
> -nodes \
> -outform der \
> -out cert.der
> keyctl padd asymmetric testkey $k < cert.der
> if [ $? -ne 0 ]; then
> echo "ERROR"
> exit 1
> fi
> done
> done
>
> Ecdsa support also works with restricted keyrings where an RSA key is used
> to sign a NIST P384/256/192 key. Scripts for testing are here:
>
> https://github.com/stefanberger/eckey-testing
>
> The ECDSA signature verification will be used by IMA Appraisal where ECDSA
> file signatures stored in RPM packages will use substantially less space
> than if RSA signatures were to be used.
>
> Further, a patch is added that allows kernel modules to be signed with a NIST
> P384 key.
>
> Testing was also done with a Pkcs11 device using an ECC key for module
> signing:
>
> https://github.com/stefanberger/eckey-testing/wiki/Using-Pkcs11-Device-(SoftHSM)-for-Signing-Linux-Kernel-Modules
>
>Stefan and Saulo
>
> v11->v12:
> - Added Jarkko's Acked-by's
>
> v10->v11:
> - Addressed Jarkko's comments
> - Split off OID definitions from first patch into own patch
> - Renamed OID_id_secp384r1 to OID_id_ansip384r1 (spec name) in 09/10
>
> v9->v10:
> - rearranged order of patches to have crypto patches first
> - moved hunk from patch 3 to patch 2 to avoid compilation warning due to
> unused symbol
>
> v8->v9:
> - Appended Saulo's patches
> - Appended patch to support kernel modules signed with NIST p384 key. This
> patch requires Nayna's series here: https://lkml.org/lkml/2021/2/18/856
>
> v7->v8:
> - patch 3/4: Do not determine key algo using parse_OID in public_key.c
> but do this when parsing the certificate. This addresses an issue
> with certain build configurations where OID_REGISTRY is not available
> as 'Reported-by: kernel test robot '.
>
> v6->v7:
> - Moved some OID defintions to patch 1 for bisectability
> - Applied R-b's
>
> v5->v6:
> - moved ecdsa code into its own module ecdsa_generic built from ecdsa.c
> - added script-generated test vectors for NIST P256 & P192 and all hashes
> - parsing of OID that contain header with new parse_oid()
>
> v4->v5:
> - registering crypto support under names ecdsa-nist-p256/p192 following
> Hubert Xu's suggestion in other thread
> - appended IMA ECDSA support patch
>
> v3->v4:
> - split off of ecdsa crypto part; registering akcipher as "ecdsa" and
> deriving used curve from digits in parsed key
>
> v2->v3:
> - patch 2 now includes linux/scatterlist.h
>
> v1->v2:
> - using faster vli_sub rather than newly added vli_mod_fast to 'reduce'
> result
> - rearranged switch statements to follow after RSA
> - 3rd patch from 1st posting is now 1st patch
>
>
> Saulo Alessandre (4):
> crypto: Add NIST P384 curve parameters
> crypto: Add math to support fast NIST P384
> ecdsa: Register NIST P384 and extend test suite
> x509: Add OID for NIST P384 and extend parser for it
>
> Stefan Berger (6):
> oid_registry: Add OIDs for ECDSA with SHA224/256/384/512
> crypto: Add support for ECDSA signature verification
> x509: Detect sm2 keys by their parameters OID
> x509: Add support for parsing x509 certs with ECDSA keys
> ima: Support EC keys for signature verification
> certs: Add support for using elliptic curve keys for signing modules
>
> certs/Kconfig | 22 ++
> certs/Makefile| 14 +
> crypto/Kconfig| 10 +
> crypto/Makefile | 6 +
> crypto/asymmetric_keys/pkcs7_parser.c | 4 +
> crypto/asymmetric_keys/public_key.c | 4 +-
> crypto/asymmetric_keys/x509_cert_parser.c | 49 ++-
> crypto/asymmetric_keys/x509_public_key.c | 4 +-
> crypto/ecc.c | 281 +-
> crypto/ecc.h | 28 +-
> crypto/ecc_curve_defs.h | 32 ++
Re: [PATCH] hwrng: core - convert sysfs sprintf/snprintf family to sysfs_emit
On Tue, Mar 16, 2021 at 08:34:12PM +0800, Jay Fang wrote: > From: Zihao Tang > > Fix the following coccicheck warning: > > drivers/char/hw_random/core.c:399:8-16: WARNING: use scnprintf or sprintf. > > Signed-off-by: Zihao Tang > Signed-off-by: Jay Fang > --- > drivers/char/hw_random/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2 0/4] Fix the parameter of dma_map_sg()
On Tue, Mar 16, 2021 at 09:55:22AM +0800, chenxiang wrote: > From: Xiang Chen > > According to Documentation/core-api/dma-api-howto.rst, the parameters > of dma_unmap_sg() must be the same as those which are passed in to the > scatter/gather mapping API. > But for some drivers under crypto, the parameter of dma_unmap_sg() > is number of elements after mapping. So fix them. > > Part of the document is as follows: > > To unmap a scatterlist, just call:: > > dma_unmap_sg(dev, sglist, nents, direction); > > Again, make sure DMA activity has already finished. > > .. note:: > > The 'nents' argument to the dma_unmap_sg call must be > the _same_ one you passed into the dma_map_sg call, > it should _NOT_ be the 'count' value _returned_ from the > dma_map_sg call. > > Change Log: > v1 -> v2: Remove changing the count passed to create_sg_component > in driver cavium; > > Xiang Chen (4): > crypto: amlogic - Fix the parameter of dma_unmap_sg() > crypto: cavium - Fix the parameter of dma_unmap_sg() > crypto: ux500 - Fix the parameter of dma_unmap_sg() > crypto: allwinner - Fix the parameter of dma_unmap_sg() > > drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c | 9 ++--- > drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c | 3 ++- > drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c | 9 ++--- > drivers/crypto/allwinner/sun8i-ss/sun8i-ss-hash.c | 3 ++- > drivers/crypto/amlogic/amlogic-gxl-cipher.c | 6 +++--- > drivers/crypto/cavium/nitrox/nitrox_reqmgr.c| 9 + > drivers/crypto/ux500/cryp/cryp_core.c | 4 ++-- > drivers/crypto/ux500/hash/hash_core.c | 2 +- > 8 files changed, 27 insertions(+), 18 deletions(-) All applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v3 0/2] PSP TEE driver update and bug fixes
On Mon, Mar 15, 2021 at 01:55:27PM +0530, Rijo Thomas wrote: > The first patch helps to improve the response time by reducing the > polling time of the tee command status variable. > > Second patch is a bug fix to handle multi-threaded use-case. > During testing, race condition was seen due to missing synchronisation > in writes to the TEE ring buffer. This patch helps to resolve that. > > v3: > * Fixed checkpatch.pl warning > > v2: > * Updated copyright year as a part of code change > > Rijo Thomas (2): > crypto: ccp - reduce tee command status polling interval from 5ms to > 1ms > crypto: ccp - fix command queuing to TEE ring buffer > > drivers/crypto/ccp/tee-dev.c | 57 > drivers/crypto/ccp/tee-dev.h | 20 +++-- > 2 files changed, 57 insertions(+), 20 deletions(-) All applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] hwrng: intel - Fix included header from 'asm
On Mon, Mar 15, 2021 at 02:12:04PM +0800, Tian Tao wrote: > This commit fixes the checkpatch warning: > WARNING: Use #include instead of > 34: FILE: drivers/char/hw_random/intel-rng.c:34: > > Signed-off-by: Tian Tao > --- > drivers/char/hw_random/intel-rng.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 4/4] crypto: hisilicon/zip - support new 'sqe' type in Kunpeng930
On Fri, Mar 19, 2021 at 03:33:07PM +0800, Yang Shen wrote:
>
> +const struct hisi_zip_sqe_ops hisi_zip_ops_v2 = {
> + .sqe_type = 0x3,
> + .fill_addr = hisi_zip_fill_addr,
> + .fill_buf_size = hisi_zip_fill_buf_size,
> + .fill_buf_type = hisi_zip_fill_buf_type,
> + .fill_req_type = hisi_zip_fill_req_type,
> + .fill_tag = hisi_zip_fill_tag_v2,
> + .fill_sqe_type = hisi_zip_fill_sqe_type,
> + .get_tag= hisi_zip_get_tag_v2,
> + .get_status = hisi_zip_get_status,
> + .get_dstlen = hisi_zip_get_dstlen,
> +};
> +
This triggers a new warning:
CHECK ../drivers/crypto/hisilicon/zip/zip_crypto.c
../drivers/crypto/hisilicon/zip/zip_crypto.c:527:31: warning: symbol
'hisi_zip_ops_v1' was not declared. Should it be static?
../drivers/crypto/hisilicon/zip/zip_crypto.c:540:31: warning: symbol
'hisi_zip_ops_v2' was not declared. Should it be static?
Please fix. Thanks.
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: sm3 - use the more precise type u32 instead of unsigned int
On Fri, 26 Mar 2021 at 03:22, Tianjia Zhang
wrote:
>
> In the process of calculating the hash, use the more accurate type
> 'u32' instead of the original 'unsigned int' to avoid ambiguity.
>
> Signed-off-by: Tianjia Zhang
I don't see the point of this patch. u32 and unsigned int are always
the same type, regardless of architecture.
> ---
> crypto/sm3_generic.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/crypto/sm3_generic.c b/crypto/sm3_generic.c
> index 193c4584bd00..562e96f92f64 100644
> --- a/crypto/sm3_generic.c
> +++ b/crypto/sm3_generic.c
> @@ -36,17 +36,17 @@ static inline u32 p1(u32 x)
> return x ^ rol32(x, 15) ^ rol32(x, 23);
> }
>
> -static inline u32 ff(unsigned int n, u32 a, u32 b, u32 c)
> +static inline u32 ff(u32 n, u32 a, u32 b, u32 c)
> {
> return (n < 16) ? (a ^ b ^ c) : ((a & b) | (a & c) | (b & c));
> }
>
> -static inline u32 gg(unsigned int n, u32 e, u32 f, u32 g)
> +static inline u32 gg(u32 n, u32 e, u32 f, u32 g)
> {
> return (n < 16) ? (e ^ f ^ g) : ((e & f) | ((~e) & g));
> }
>
> -static inline u32 t(unsigned int n)
> +static inline u32 t(u32 n)
> {
> return (n < 16) ? SM3_T1 : SM3_T2;
> }
> @@ -54,7 +54,7 @@ static inline u32 t(unsigned int n)
> static void sm3_expand(u32 *t, u32 *w, u32 *wt)
> {
> int i;
> - unsigned int tmp;
> + u32 tmp;
>
> /* load the input */
> for (i = 0; i <= 15; i++)
> @@ -123,8 +123,8 @@ static void sm3_compress(u32 *w, u32 *wt, u32 *m)
>
> static void sm3_transform(struct sm3_state *sst, u8 const *src)
> {
> - unsigned int w[68];
> - unsigned int wt[64];
> + u32 w[68];
> + u32 wt[64];
>
> sm3_expand((u32 *)src, w, wt);
> sm3_compress(w, wt, sst->state);
> --
> 2.19.1.3.ge56e4f7
>
Re: [PATCH -next] drivers: crypto: CRYPTO_DEV_HISI_HPRE select CRYPTO_ECC and CRYPTO_ECDH
Thanks, there is a similar patch to yours that was sent in advance: https://www.spinics.net/lists/linux-crypto/msg54238.html 在 2021/3/26 15:16, Zhang Jianhua 写道: If CRYPTO_DEV_HISI_HPRE=y, the following errors will be seen while building hpre_crypto.c drivers/crypto/hisilicon/hpre/hpre_crypto.o: In function `hpre_curve25519_compute_value': hpre_crypto.c:(.text+0x151b): undefined reference to `ecc_get_curve25519' drivers/crypto/hisilicon/hpre/hpre_crypto.o: In function `hpre_curve25519_set_secret': hpre_crypto.c:(.text+0x2714): undefined reference to `ecc_get_curve25519' drivers/crypto/hisilicon/hpre/hpre_crypto.o: In function `hpre_ecdh_set_secret': hpre_crypto.c:(.text+0x27ed): undefined reference to `crypto_ecdh_decode_key' hpre_crypto.c:(.text+0x2954): undefined reference to `ecc_get_curve' Reported-by: Hulk Robot Signed-off-by: Zhang Jianhua --- drivers/crypto/hisilicon/Kconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/crypto/hisilicon/Kconfig b/drivers/crypto/hisilicon/Kconfig index c45adb15ce8d..d87c89af2a7f 100644 --- a/drivers/crypto/hisilicon/Kconfig +++ b/drivers/crypto/hisilicon/Kconfig @@ -69,6 +69,8 @@ config CRYPTO_DEV_HISI_HPRE select CRYPTO_DEV_HISI_QM select CRYPTO_DH select CRYPTO_RSA + select CRYPTO_ECC + select CRYPTO_ECDH help Support for HiSilicon HPRE(High Performance RSA Engine) accelerator, which can accelerate RSA and DH algorithms.
Re: [RFC 1/1] crypto: dcp - add power management support
On Fri, Mar 19, 2021 at 02:22:57AM +0200, Dragos Rosioru (OSS) wrote: > From: Dragos Rosioru > > Added suspend/resume operations for PM support in the DCP driver. > After a suspend/resume cycle DCP would still be in a low-power mode > and have its clocks gated, thus requiring state to be saved beforehand: > - Control register value(DCP_CTRL) > - Channel control register value(DCP_CHANNELCTRL) > > Signed-off-by: Dragos Rosioru > --- > drivers/crypto/mxs-dcp.c | 76 > ++-- > 1 file changed, 74 insertions(+), 2 deletions(-) > > diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c > index d6a7784..6748a4a 100644 > --- a/drivers/crypto/mxs-dcp.c > +++ b/drivers/crypto/mxs-dcp.c > @@ -23,6 +23,10 @@ > #include > #include > > +#ifdef CONFIG_PM_SLEEP > +#include > +#endif Please find a way to rework this patch so that it's not full of these ifdefs. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: vmx: fix incorrect kernel-doc comment syntax in files
Hi Aditya, Thanks for your patch! > The opening comment mark '/**' is used for highlighting the beginning of > kernel-doc comments. > There are certain files in drivers/crypto/vmx, which follow this syntax, > but the content inside does not comply with kernel-doc. > Such lines were probably not meant for kernel-doc parsing, but are parsed > due to the presence of kernel-doc like comment syntax(i.e, '/**'), which > causes unexpected warnings from kernel-doc. > > E.g., presence of kernel-doc like comment in the header line for > drivers/crypto/vmx/vmx.c causes this warning by kernel-doc: > > "warning: expecting prototype for Routines supporting VMX instructions on the > Power 8(). Prototype was for p8_init() instead" checkpatch (scripts/checkpatch.pl --strict -g HEAD) complains about this line: WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) but checkpatch should be ignored here, as you did the right thing by not breaking an error message across multiple lines. > Similarly for other files too. > > Provide a simple fix by replacing such occurrences with general comment > format, i.e. '/*', to prevent kernel-doc from parsing it. This makes sense. Reviewed-by: Daniel Axtens Kind regards, Daniel > > Signed-off-by: Aditya Srivastava > --- > * Applies perfectly on next-20210319 > > drivers/crypto/vmx/aes.c | 2 +- > drivers/crypto/vmx/aes_cbc.c | 2 +- > drivers/crypto/vmx/aes_ctr.c | 2 +- > drivers/crypto/vmx/aes_xts.c | 2 +- > drivers/crypto/vmx/ghash.c | 2 +- > drivers/crypto/vmx/vmx.c | 2 +- > 6 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/crypto/vmx/aes.c b/drivers/crypto/vmx/aes.c > index d05c02baebcf..ec06189fbf99 100644 > --- a/drivers/crypto/vmx/aes.c > +++ b/drivers/crypto/vmx/aes.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0-only > -/** > +/* > * AES routines supporting VMX instructions on the Power 8 > * > * Copyright (C) 2015 International Business Machines Inc. > diff --git a/drivers/crypto/vmx/aes_cbc.c b/drivers/crypto/vmx/aes_cbc.c > index d88084447f1c..ed0debc7acb5 100644 > --- a/drivers/crypto/vmx/aes_cbc.c > +++ b/drivers/crypto/vmx/aes_cbc.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0-only > -/** > +/* > * AES CBC routines supporting VMX instructions on the Power 8 > * > * Copyright (C) 2015 International Business Machines Inc. > diff --git a/drivers/crypto/vmx/aes_ctr.c b/drivers/crypto/vmx/aes_ctr.c > index 79ba062ee1c1..9a3da8cd62f3 100644 > --- a/drivers/crypto/vmx/aes_ctr.c > +++ b/drivers/crypto/vmx/aes_ctr.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0-only > -/** > +/* > * AES CTR routines supporting VMX instructions on the Power 8 > * > * Copyright (C) 2015 International Business Machines Inc. > diff --git a/drivers/crypto/vmx/aes_xts.c b/drivers/crypto/vmx/aes_xts.c > index 9fee1b1532a4..dabbccb41550 100644 > --- a/drivers/crypto/vmx/aes_xts.c > +++ b/drivers/crypto/vmx/aes_xts.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0-only > -/** > +/* > * AES XTS routines supporting VMX In-core instructions on Power 8 > * > * Copyright (C) 2015 International Business Machines Inc. > diff --git a/drivers/crypto/vmx/ghash.c b/drivers/crypto/vmx/ghash.c > index 14807ac2e3b9..5bc5710a6de0 100644 > --- a/drivers/crypto/vmx/ghash.c > +++ b/drivers/crypto/vmx/ghash.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0 > -/** > +/* > * GHASH routines supporting VMX instructions on the Power 8 > * > * Copyright (C) 2015, 2019 International Business Machines Inc. > diff --git a/drivers/crypto/vmx/vmx.c b/drivers/crypto/vmx/vmx.c > index a40d08e75fc0..7eb713cc87c8 100644 > --- a/drivers/crypto/vmx/vmx.c > +++ b/drivers/crypto/vmx/vmx.c > @@ -1,5 +1,5 @@ > // SPDX-License-Identifier: GPL-2.0-only > -/** > +/* > * Routines supporting VMX instructions on the Power 8 > * > * Copyright (C) 2015 International Business Machines Inc. > -- > 2.17.1
Re: [PATCH] crypto: async_tx/async_xor.c: Few mundane spello fixes
On 3/25/21 7:34 PM, Bhaskar Chowdhury wrote: > > s/eninges/engines/ ...two different places. > s/explicity/explicitly/ two different places. > > Signed-off-by: Bhaskar Chowdhury Acked-by: Randy Dunlap > --- > crypto/async_tx/async_xor.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/crypto/async_tx/async_xor.c b/crypto/async_tx/async_xor.c > index a057ecb1288d..fee33f76cdd9 100644 > --- a/crypto/async_tx/async_xor.c > +++ b/crypto/async_tx/async_xor.c > @@ -170,8 +170,8 @@ dma_xor_aligned_offsets(struct dma_device *device, > unsigned int offset, > * > * xor_blocks always uses the dest as a source so the > * ASYNC_TX_XOR_ZERO_DST flag must be set to not include dest data in > - * the calculation. The assumption with dma eninges is that they only > - * use the destination buffer as a source when it is explicity specified > + * the calculation. The assumption with dma engines is that they only > + * use the destination buffer as a source when it is explicitly specified > * in the source list. > * > * src_list note: if the dest is also a source it must be at index zero. > @@ -259,8 +259,8 @@ EXPORT_SYMBOL_GPL(async_xor_offs); > * > * xor_blocks always uses the dest as a source so the > * ASYNC_TX_XOR_ZERO_DST flag must be set to not include dest data in > - * the calculation. The assumption with dma eninges is that they only > - * use the destination buffer as a source when it is explicity specified > + * the calculation. The assumption with dma engines is that they only > + * use the destination buffer as a source when it is explicitly specified > * in the source list. > * > * src_list note: if the dest is also a source it must be at index zero. > -- -- ~Randy
Re: [RFC Part2 PATCH 01/30] x86: Add the host SEV-SNP initialization support
On 3/25/21 10:51 AM, Dave Hansen wrote:
> On 3/25/21 8:31 AM, Brijesh Singh wrote:
>> On 3/25/21 9:58 AM, Dave Hansen wrote:
+static int __init mem_encrypt_snp_init(void)
+{
+ if (!boot_cpu_has(X86_FEATURE_SEV_SNP))
+ return 1;
+
+ if (rmptable_init()) {
+ setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
+ return 1;
+ }
+
+ static_branch_enable(&snp_enable_key);
+
+ return 0;
+}
>>> Could you explain a bit why 'snp_enable_key' is needed in addition to
>>> X86_FEATURE_SEV_SNP?
>>
>> The X86_FEATURE_SEV_SNP indicates that hardware supports the feature --
>> this does not necessary means that SEV-SNP is enabled in the host.
> I think you're confusing the CPUID bit that initially populates
> X86_FEATURE_SEV_SNP with the X86_FEATURE bit. We clear X86_FEATURE bits
> all the time for features that the kernel turns off, even while the
> hardware supports it.
Ah, yes I was getting mixed up. I will see if we can remove the
snp_key_enabled and use the feature check.
> Look at what we do in init_ia32_feat_ctl() for SGX, for instance. We
> then go on to use X86_FEATURE_SGX at runtime to see if SGX was disabled,
> even though the hardware supports it.
>
>>> For a lot of features, we just use cpu_feature_enabled(), which does
>>> both compile-time and static_cpu_has(). This whole series seems to lack
>>> compile-time disables for the code that it adds, like the code it adds
>>> to arch/x86/mm/fault.c or even mm/memory.c.
>> Noted, I will add the #ifdef to make sure that its compiled out when
>> the config does not have the AMD_MEM_ENCRYPTION enabled.
> IS_ENABLED() tends to be nicer for these things.
>
> Even better is if you coordinate these with your X86_FEATURE_SEV_SNP
> checks. Then, put X86_FEATURE_SEV_SNP in disabled-features.h, and you
> can use cpu_feature_enabled(X86_FEATURE_SEV_SNP) as both a
> (statically-patched) runtime *AND* compile-time check without an
> explicit #ifdefs.
I will try improve this in v2 and will try IS_ENABLED().
Re: [RFC Part2 PATCH 07/30] mm: add support to split the large THP based on RMP violation
On 3/25/21 8:24 AM, Brijesh Singh wrote: > On 3/25/21 9:48 AM, Dave Hansen wrote: >> On 3/24/21 10:04 AM, Brijesh Singh wrote: >>> When SEV-SNP is enabled globally in the system, a write from the hypervisor >>> can raise an RMP violation. We can resolve the RMP violation by splitting >>> the virtual address to a lower page level. >>> >>> e.g >>> - guest made a page shared in the RMP entry so that the hypervisor >>> can write to it. >>> - the hypervisor has mapped the pfn as a large page. A write access >>> will cause an RMP violation if one of the pages within the 2MB region >>> is a guest private page. >>> >>> The above RMP violation can be resolved by simply splitting the large >>> page. >> What if the large page is provided by hugetlbfs? > I was not able to find a method to split the large pages in the > hugetlbfs. Unfortunately, at this time a VMM cannot use the backing > memory from the hugetlbfs pool. An SEV-SNP aware VMM can use either > transparent hugepage or small pages. That's really, really nasty. Especially since it might not be evident until long after boot and the guest is killed. It's even nastier because hugetlbfs is actually a great fit for SEV-SNP memory. It's physically contiguous, so it would keep you from having to fracture the direct map all the way down to 4k, it also can't be reclaimed (just like all SEV memory). I think the minimal thing you can do here is to fail to add memory to the RMP in the first place if you can't split it. That way, users will at least fail to _start_ their VM versus dying randomly for no good reason. Even better would be to come up with a stronger contract between host and guest. I really don't think the host should be exposed to random RMP faults on the direct map. If the guest wants to share memory, then it needs to tell the host and give the host an opportunity to move the guest physical memory. It might, for instance, sequester all the shared pages in a single spot to minimize direct map fragmentation. I'll let the other x86 folks chime in on this, but I really think this needs a different approach than what's being proposed.
Re: [RFC Part2 PATCH 01/30] x86: Add the host SEV-SNP initialization support
On 3/25/21 8:31 AM, Brijesh Singh wrote:
>
> On 3/25/21 9:58 AM, Dave Hansen wrote:
>>> +static int __init mem_encrypt_snp_init(void)
>>> +{
>>> + if (!boot_cpu_has(X86_FEATURE_SEV_SNP))
>>> + return 1;
>>> +
>>> + if (rmptable_init()) {
>>> + setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
>>> + return 1;
>>> + }
>>> +
>>> + static_branch_enable(&snp_enable_key);
>>> +
>>> + return 0;
>>> +}
>> Could you explain a bit why 'snp_enable_key' is needed in addition to
>> X86_FEATURE_SEV_SNP?
>
>
> The X86_FEATURE_SEV_SNP indicates that hardware supports the feature --
> this does not necessary means that SEV-SNP is enabled in the host.
I think you're confusing the CPUID bit that initially populates
X86_FEATURE_SEV_SNP with the X86_FEATURE bit. We clear X86_FEATURE bits
all the time for features that the kernel turns off, even while the
hardware supports it.
Look at what we do in init_ia32_feat_ctl() for SGX, for instance. We
then go on to use X86_FEATURE_SGX at runtime to see if SGX was disabled,
even though the hardware supports it.
>> For a lot of features, we just use cpu_feature_enabled(), which does
>> both compile-time and static_cpu_has(). This whole series seems to lack
>> compile-time disables for the code that it adds, like the code it adds
>> to arch/x86/mm/fault.c or even mm/memory.c.
>
> Noted, I will add the #ifdef to make sure that its compiled out when
> the config does not have the AMD_MEM_ENCRYPTION enabled.
IS_ENABLED() tends to be nicer for these things.
Even better is if you coordinate these with your X86_FEATURE_SEV_SNP
checks. Then, put X86_FEATURE_SEV_SNP in disabled-features.h, and you
can use cpu_feature_enabled(X86_FEATURE_SEV_SNP) as both a
(statically-patched) runtime *AND* compile-time check without an
explicit #ifdefs.
Re: [RFC Part2 PATCH 01/30] x86: Add the host SEV-SNP initialization support
On 3/25/21 9:58 AM, Dave Hansen wrote:
>> +static int __init mem_encrypt_snp_init(void)
>> +{
>> +if (!boot_cpu_has(X86_FEATURE_SEV_SNP))
>> +return 1;
>> +
>> +if (rmptable_init()) {
>> +setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
>> +return 1;
>> +}
>> +
>> +static_branch_enable(&snp_enable_key);
>> +
>> +return 0;
>> +}
> Could you explain a bit why 'snp_enable_key' is needed in addition to
> X86_FEATURE_SEV_SNP?
The X86_FEATURE_SEV_SNP indicates that hardware supports the feature --
this does not necessary means that SEV-SNP is enabled in the host. The
snp_enabled_key() helper is later used by kernel and drivers to check
whether SEV-SNP is enabled. e.g. when a driver calls the RMPUPDATE
instruction, the rmpupdate helper routine checks whether the SNP is
enabled. If SEV-SNP is not enabled then instruction will cause a #UD.
>
> For a lot of features, we just use cpu_feature_enabled(), which does
> both compile-time and static_cpu_has(). This whole series seems to lack
> compile-time disables for the code that it adds, like the code it adds
> to arch/x86/mm/fault.c or even mm/memory.c.
Noted, I will add the #ifdef to make sure that its compiled out when
the config does not have the AMD_MEM_ENCRYPTION enabled.
>
Re: [RFC Part2 PATCH 07/30] mm: add support to split the large THP based on RMP violation
On 3/25/21 9:48 AM, Dave Hansen wrote:
> On 3/24/21 10:04 AM, Brijesh Singh wrote:
>> When SEV-SNP is enabled globally in the system, a write from the hypervisor
>> can raise an RMP violation. We can resolve the RMP violation by splitting
>> the virtual address to a lower page level.
>>
>> e.g
>> - guest made a page shared in the RMP entry so that the hypervisor
>> can write to it.
>> - the hypervisor has mapped the pfn as a large page. A write access
>> will cause an RMP violation if one of the pages within the 2MB region
>> is a guest private page.
>>
>> The above RMP violation can be resolved by simply splitting the large
>> page.
> What if the large page is provided by hugetlbfs?
I was not able to find a method to split the large pages in the
hugetlbfs. Unfortunately, at this time a VMM cannot use the backing
memory from the hugetlbfs pool. An SEV-SNP aware VMM can use either
transparent hugepage or small pages.
>
> What if the kernel uses the direct map to access the page instead of the
> userspace mapping?
See the Patch 04/30. Currently, we split the kernel direct maps to 4K
before adding the page in the RMP table to avoid the need to split the
pages due to the RMP fault.
>
>> The architecture specific code will read the RMP entry to determine
>> if the fault can be resolved by splitting and propagating the request
>> to split the page by setting newly introduced fault flag
>> (FAULT_FLAG_PAGE_SPLIT). If the fault cannot be resolved by splitting,
>> then a SIGBUS signal is sent to terminate the process.
> Are users just supposed to know what memory types are compatible with
> SEV-SNP? Basically, don't use anything that might map a guest using
> non-4k entries, except THP?
Currently, VMM will need to know the compatible memory type and use it
for allocating the backing pages.
>
> This does seem like a rather nasty aspect of the hardware. For
> everything else, if the virtualization page tables and the x86 tables
> disagree, the TLB just sees the smallest page size.
>
>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> index 7605e06a6dd9..f6571563f433 100644
>> --- a/arch/x86/mm/fault.c
>> +++ b/arch/x86/mm/fault.c
>> @@ -1305,6 +1305,70 @@ do_kern_addr_fault(struct pt_regs *regs, unsigned
>> long hw_error_code,
>> }
>> NOKPROBE_SYMBOL(do_kern_addr_fault);
>>
>> +#define RMP_FAULT_RETRY 0
>> +#define RMP_FAULT_KILL 1
>> +#define RMP_FAULT_PAGE_SPLIT2
>> +
>> +static inline size_t pages_per_hpage(int level)
>> +{
>> +return page_level_size(level) / PAGE_SIZE;
>> +}
>> +
>> +/*
>> + * The RMP fault can happen when a hypervisor attempts to write to:
>> + * 1. a guest owned page or
>> + * 2. any pages in the large page is a guest owned page.
>> + *
>> + * #1 will happen only when a process or VMM is attempting to modify the
>> guest page
>> + * without the guests cooperation. If a guest wants a VMM to be able to
>> write to its memory
>> + * then it should make the page shared. If we detect #1, kill the process
>> because we can not
>> + * resolve the fault.
>> + *
>> + * #2 can happen when the page level does not match between the RMP entry
>> and x86
>> + * page table walk, e.g the page is mapped as a large page in the x86 page
>> table but its
>> + * added as a 4K shared page in the RMP entry. This can be resolved by
>> splitting the address
>> + * into a smaller page level.
>> + */
> These comments need to get wrapped a bit sooner. Could you try to match
> some of the others in the file?
Noted.
>
>> +static int handle_rmp_page_fault(unsigned long hw_error_code, unsigned long
>> address)
>> +{
>> +unsigned long pfn, mask;
>> +int rmp_level, level;
>> +rmpentry_t *e;
>> +pte_t *pte;
>> +
>> +/* Get the native page level */
>> +pte = lookup_address_in_mm(current->mm, address, &level);
>> +if (unlikely(!pte))
>> +return RMP_FAULT_KILL;
>> +
>> +pfn = pte_pfn(*pte);
>> +if (level > PG_LEVEL_4K) {
>> +mask = pages_per_hpage(level) - pages_per_hpage(level - 1);
>> +pfn |= (address >> PAGE_SHIFT) & mask;
>> +}
> What is this trying to do, exactly?
Trying to calculate the pfn within the large entry.
The lookup above will return a base pfn of a large page. Need to find
index within the large page to calculate the PFN.
>
>> +/* Get the page level from the RMP entry. */
>> +e = lookup_page_in_rmptable(pfn_to_page(pfn), &rmp_level);
>> +if (!e) {
>> +pr_alert("SEV-SNP: failed to lookup RMP entry for address 0x%lx
>> pfn 0x%lx\n",
>> + address, pfn);
>> +return RMP_FAULT_KILL;
>> +}
>> +
>> +/* Its a guest owned page */
>> +if (rmpentry_assigned(e))
>> +return RMP_FAULT_KILL;
>> +
>> +/*
>> + * Its a shared page but the page level does not match between the
>> native walk
>> + * and RMP entry.
>> + */
> For these two-line comments, please try to split the text
Re: [RFC Part2 PATCH 04/30] x86/mm: split the physmap when adding the page in RMP table
On 3/24/21 10:04 AM, Brijesh Singh wrote: > The spliting of the physmap is a temporary solution until we work to > improve the kernel page fault handler to split the pages on demand. > One of the disadvtange of splitting is that eventually, we will end up > breaking down the entire physmap unless we combine the split pages back to > a large page. I am open to the suggestation on various approaches we could > take to address this problem. Other than suggesting that the hardware be fixed to do the fracturing itself? :) I suspect that this code is trying to be *too* generic. I would expect that very little of guest memory is actually shared with the host. It's also not going to be random guest pages. The guest and the host have to agree on these things, and I *think* the host is free to move the physical backing around once it's shared. So, let's say that there a guest->host paravirt interface where the guest says in advance, "I want to share this page." The host can split at *that* point and *only* split that one page's mapping. Any page faults would occur only if the host screws up, and would result in an oops. That also gives a point where the host can say, "nope, that hugetlbfs, I can't split it".
Re: [RFC Part2 PATCH 01/30] x86: Add the host SEV-SNP initialization support
> +static int __init mem_encrypt_snp_init(void)
> +{
> + if (!boot_cpu_has(X86_FEATURE_SEV_SNP))
> + return 1;
> +
> + if (rmptable_init()) {
> + setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
> + return 1;
> + }
> +
> + static_branch_enable(&snp_enable_key);
> +
> + return 0;
> +}
Could you explain a bit why 'snp_enable_key' is needed in addition to
X86_FEATURE_SEV_SNP?
For a lot of features, we just use cpu_feature_enabled(), which does
both compile-time and static_cpu_has(). This whole series seems to lack
compile-time disables for the code that it adds, like the code it adds
to arch/x86/mm/fault.c or even mm/memory.c.
Re: [RFC Part2 PATCH 07/30] mm: add support to split the large THP based on RMP violation
On 3/24/21 10:04 AM, Brijesh Singh wrote:
> When SEV-SNP is enabled globally in the system, a write from the hypervisor
> can raise an RMP violation. We can resolve the RMP violation by splitting
> the virtual address to a lower page level.
>
> e.g
> - guest made a page shared in the RMP entry so that the hypervisor
> can write to it.
> - the hypervisor has mapped the pfn as a large page. A write access
> will cause an RMP violation if one of the pages within the 2MB region
> is a guest private page.
>
> The above RMP violation can be resolved by simply splitting the large
> page.
What if the large page is provided by hugetlbfs?
What if the kernel uses the direct map to access the page instead of the
userspace mapping?
> The architecture specific code will read the RMP entry to determine
> if the fault can be resolved by splitting and propagating the request
> to split the page by setting newly introduced fault flag
> (FAULT_FLAG_PAGE_SPLIT). If the fault cannot be resolved by splitting,
> then a SIGBUS signal is sent to terminate the process.
Are users just supposed to know what memory types are compatible with
SEV-SNP? Basically, don't use anything that might map a guest using
non-4k entries, except THP?
This does seem like a rather nasty aspect of the hardware. For
everything else, if the virtualization page tables and the x86 tables
disagree, the TLB just sees the smallest page size.
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 7605e06a6dd9..f6571563f433 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1305,6 +1305,70 @@ do_kern_addr_fault(struct pt_regs *regs, unsigned long
> hw_error_code,
> }
> NOKPROBE_SYMBOL(do_kern_addr_fault);
>
> +#define RMP_FAULT_RETRY 0
> +#define RMP_FAULT_KILL 1
> +#define RMP_FAULT_PAGE_SPLIT 2
> +
> +static inline size_t pages_per_hpage(int level)
> +{
> + return page_level_size(level) / PAGE_SIZE;
> +}
> +
> +/*
> + * The RMP fault can happen when a hypervisor attempts to write to:
> + * 1. a guest owned page or
> + * 2. any pages in the large page is a guest owned page.
> + *
> + * #1 will happen only when a process or VMM is attempting to modify the
> guest page
> + * without the guests cooperation. If a guest wants a VMM to be able to
> write to its memory
> + * then it should make the page shared. If we detect #1, kill the process
> because we can not
> + * resolve the fault.
> + *
> + * #2 can happen when the page level does not match between the RMP entry
> and x86
> + * page table walk, e.g the page is mapped as a large page in the x86 page
> table but its
> + * added as a 4K shared page in the RMP entry. This can be resolved by
> splitting the address
> + * into a smaller page level.
> + */
These comments need to get wrapped a bit sooner. Could you try to match
some of the others in the file?
> +static int handle_rmp_page_fault(unsigned long hw_error_code, unsigned long
> address)
> +{
> + unsigned long pfn, mask;
> + int rmp_level, level;
> + rmpentry_t *e;
> + pte_t *pte;
> +
> + /* Get the native page level */
> + pte = lookup_address_in_mm(current->mm, address, &level);
> + if (unlikely(!pte))
> + return RMP_FAULT_KILL;
> +
> + pfn = pte_pfn(*pte);
> + if (level > PG_LEVEL_4K) {
> + mask = pages_per_hpage(level) - pages_per_hpage(level - 1);
> + pfn |= (address >> PAGE_SHIFT) & mask;
> + }
What is this trying to do, exactly?
> + /* Get the page level from the RMP entry. */
> + e = lookup_page_in_rmptable(pfn_to_page(pfn), &rmp_level);
> + if (!e) {
> + pr_alert("SEV-SNP: failed to lookup RMP entry for address 0x%lx
> pfn 0x%lx\n",
> + address, pfn);
> + return RMP_FAULT_KILL;
> + }
> +
> + /* Its a guest owned page */
> + if (rmpentry_assigned(e))
> + return RMP_FAULT_KILL;
> +
> + /*
> + * Its a shared page but the page level does not match between the
> native walk
> + * and RMP entry.
> + */
For these two-line comments, please try to split the text fairly evenly
between the lines.
> + if (level > rmp_level)
> + return RMP_FAULT_PAGE_SPLIT;
> +
> + return RMP_FAULT_RETRY;
> +}
> +
> /* Handle faults in the user portion of the address space */
> static inline
> void do_user_addr_fault(struct pt_regs *regs,
> @@ -1315,6 +1379,7 @@ void do_user_addr_fault(struct pt_regs *regs,
> struct task_struct *tsk;
> struct mm_struct *mm;
> vm_fault_t fault;
> + int ret;
> unsigned int flags = FAULT_FLAG_DEFAULT;
>
> tsk = current;
> @@ -1377,6 +1442,22 @@ void do_user_addr_fault(struct pt_regs *regs,
> if (hw_error_code & X86_PF_INSTR)
> flags |= FAULT_FLAG_INSTRUCTION;
>
> + /*
> + * If its an RMP violation, see if we can resolve it.
> + */
> + if ((hw_error_code & X86_PF_RMP)) {
> +
Re: [RFC Part2 PATCH 05/30] x86: define RMP violation #PF error code
On 3/25/21 7:32 AM, Brijesh Singh wrote:
>>> enum x86_pf_error_code {
>>> X86_PF_PROT = 1 << 0,
>>> @@ -21,6 +22,7 @@ enum x86_pf_error_code {
>>> X86_PF_INSTR= 1 << 4,
>>> X86_PF_PK = 1 << 5,
>>> X86_PF_SGX = 1 << 15,
>>> + X86_PF_RMP = 1ull << 31,
>>> };
...
>> Could we at least start declaring these with BIT()?
>
> Sure, I can bit the BIT() macro to define the bits. Do you want me to
> update all of the fault codes to use BIT() or just the one I am adding
> in this patch ?
Please update all of them for consistency.
Re: [RFC Part2 PATCH 05/30] x86: define RMP violation #PF error code
On 3/24/21 1:03 PM, Dave Hansen wrote:
>> diff --git a/arch/x86/include/asm/trap_pf.h b/arch/x86/include/asm/trap_pf.h
>> index 10b1de500ab1..107f9d947e8d 100644
>> --- a/arch/x86/include/asm/trap_pf.h
>> +++ b/arch/x86/include/asm/trap_pf.h
>> @@ -12,6 +12,7 @@
>> * bit 4 == 1: fault was an instruction
>> fetch
>> * bit 5 == 1: protection keys block access
>> * bit 15 == 1: SGX MMU page-fault
>> + * bit 31 == 1: fault was an RMP violation
>> */
>> enum x86_pf_error_code {
>> X86_PF_PROT = 1 << 0,
>> @@ -21,6 +22,7 @@ enum x86_pf_error_code {
>> X86_PF_INSTR= 1 << 4,
>> X86_PF_PK = 1 << 5,
>> X86_PF_SGX = 1 << 15,
>> +X86_PF_RMP = 1ull << 31,
>> };
> Man, I hope AMD and Intel are talking to each other about these bits. :)
>
> Either way, this is hitting the limits of what I know about how enums
> are implemented. I had internalized that they are just an 'int', but
> that doesn't seem quite right. It sounds like they must be implemented
> using *an* integer type, but not necessarily 'int' itself.
>
> Either way, '1<<31' doesn't fit in a 32-bit signed int. But, gcc at
> least doesn't seem to blow the enum up into a 64-bit type, which is nice.
>
> Could we at least start declaring these with BIT()?
Sure, I can bit the BIT() macro to define the bits. Do you want me to
update all of the fault codes to use BIT() or just the one I am adding
in this patch ?
Re: [RFC Part2 PATCH 07/30] mm: add support to split the large THP based on RMP violation
On 3/24/21 10:04 AM, Brijesh Singh wrote:
> @@ -1377,6 +1442,22 @@ void do_user_addr_fault(struct pt_regs *regs,
> if (hw_error_code & X86_PF_INSTR)
> flags |= FAULT_FLAG_INSTRUCTION;
>
> + /*
> + * If its an RMP violation, see if we can resolve it.
> + */
> + if ((hw_error_code & X86_PF_RMP)) {
> + ret = handle_rmp_page_fault(hw_error_code, address);
> + if (ret == RMP_FAULT_PAGE_SPLIT) {
> + flags |= FAULT_FLAG_PAGE_SPLIT;
> + } else if (ret == RMP_FAULT_KILL) {
> + fault |= VM_FAULT_SIGBUS;
> + mm_fault_error(regs, hw_error_code, address, fault);
> + return;
> + } else {
> + return;
> + }
> + }
Won't khugepaged come right back around and coalesce this page again?
Re: [PATCH v7 0/5] Enable root to update the blacklist keyring
Hi David, What is the status of this patchset? Could you please push it to -next? Regards, Mickaël On 12/03/2021 18:12, Mickaël Salaün wrote: > This new patch series is a rebase on David Howells's and Eric Snowberg's > keys-cve-2020-26541-v3. > > I successfully tested this patch series with the 186 entries from > https://uefi.org/sites/default/files/resources/dbxupdate_x64.bin (184 > binary hashes and 2 certificates). > > The goal of these patches is to add a new configuration option to enable the > root user to load signed keys in the blacklist keyring. This keyring is > useful > to "untrust" certificates or files. Enabling to safely update this keyring > without recompiling the kernel makes it more usable. > > This can be applied on top of David Howells's keys-cve-2020-26541-branch: > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-cve-2020-26541-branch > > Previous patch series: > https://lore.kernel.org/lkml/20210210120410.471693-1-...@digikod.net/ > > Regards, > > Mickaël Salaün (5): > tools/certs: Add print-cert-tbs-hash.sh > certs: Check that builtin blacklist hashes are valid > certs: Make blacklist_vet_description() more strict > certs: Factor out the blacklist hash creation > certs: Allow root user to append signed hashes to the blacklist > keyring > > MAINTAINERS | 2 + > certs/.gitignore | 1 + > certs/Kconfig | 17 +- > certs/Makefile| 17 +- > certs/blacklist.c | 218 ++ > crypto/asymmetric_keys/x509_public_key.c | 3 +- > include/keys/system_keyring.h | 14 +- > scripts/check-blacklist-hashes.awk| 37 +++ > .../platform_certs/keyring_handler.c | 26 +-- > tools/certs/print-cert-tbs-hash.sh| 91 > 10 files changed, 346 insertions(+), 80 deletions(-) > create mode 100755 scripts/check-blacklist-hashes.awk > create mode 100755 tools/certs/print-cert-tbs-hash.sh > > > base-commit: ebd9c2ae369a45bdd9f8615484db09be58fc242b >
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Wed, 24 Mar 2021 at 19:37, Ahmad Fatoum wrote: > > Hello Sumit, > > On 24.03.21 11:47, Sumit Garg wrote: > > On Wed, 24 Mar 2021 at 14:56, Ahmad Fatoum wrote: > >> > >> Hello Mimi, > >> > >> On 23.03.21 19:07, Mimi Zohar wrote: > >>> On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote: > On 21.03.21 21:48, Horia Geantă wrote: > > caam has random number generation capabilities, so it's worth using that > > by implementing .get_random. > > If the CAAM HWRNG is already seeding the kernel RNG, why not use the > kernel's? > > Makes for less code duplication IMO. > >>> > >>> Using kernel RNG, in general, for trusted keys has been discussed > >>> before. Please refer to Dave Safford's detailed explanation for not > >>> using it [1]. > >> > >> The argument seems to boil down to: > >> > >> - TPM RNG are known to be of good quality > >> - Trusted keys always used it so far > >> > >> Both are fine by me for TPMs, but the CAAM backend is new code and neither > >> point > >> really applies. > >> > >> get_random_bytes_wait is already used for generating key material > >> elsewhere. > >> Why shouldn't new trusted key backends be able to do the same thing? > >> > > > > Please refer to documented trusted keys behaviour here [1]. New > > trusted key backends should align to this behaviour and in your case > > CAAM offers HWRNG so we should be better using that. > > Why is it better? > > Can you explain what benefit a CAAM user would have if the trusted key > randomness comes directly out of the CAAM instead of indirectly from > the kernel entropy pool that is seeded by it? IMO, user trust in case of trusted keys comes from trusted keys backend which is CAAM here. If a user doesn't trust that CAAM would act as a reliable source for RNG then CAAM shouldn't be used as a trust source in the first place. And I think building user's trust for kernel RNG implementation with multiple entropy contributions is pretty difficult when compared with CAAM HWRNG implementation. -Sumit > > > Also, do update documentation corresponding to CAAM as a trusted keys > > backend. > > Yes. The documentation should be updated for CAAM and it should describe > how the key material is derived. Will do so for v2. > > Cheers, > Ahmad > > > > > [1] > > https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/tree/Documentation/security/keys/trusted-encrypted.rst#n87 > > > > -Sumit > > > >> Cheers, > >> Ahmad > >> > >>> > >>> thanks, > >>> > >>> Mimi > >>> > >>> [1] > >>> https://lore.kernel.org/linux-integrity/bca04d5d9a3b764c9b7405bba4d4a3c035f2a...@alpmbapa12.e2k.ad.ge.com/ > >>> > >>> > >>> > >> > >> -- > >> Pengutronix e.K. | | > >> Steuerwalder Str. 21 | http://www.pengutronix.de/ | > >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| > >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH -next] crypto: hisilicon/hpre - fix build error without CONFIG_CRYPTO_ECDH
Thinks, there is a similar patch to yours that was send in advance: https://www.spinics.net/lists/linux-crypto/msg54238.html On 2021/3/24 22:42, 'Wei Yongjun wrote: From: Wei Yongjun GCC reports build error as following: x86_64-linux-gnu-ld: drivers/crypto/hisilicon/hpre/hpre_crypto.o: in function `hpre_ecdh_set_secret': hpre_crypto.c:(.text+0x269c): undefined reference to `crypto_ecdh_decode_key' Fix it by selecting CRYPTO_ECDH. Reported-by: Hulk Robot Signed-off-by: Wei Yongjun --- drivers/crypto/hisilicon/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/crypto/hisilicon/Kconfig b/drivers/crypto/hisilicon/Kconfig index c45adb15ce8d..bb327d6e365a 100644 --- a/drivers/crypto/hisilicon/Kconfig +++ b/drivers/crypto/hisilicon/Kconfig @@ -69,6 +69,7 @@ config CRYPTO_DEV_HISI_HPRE select CRYPTO_DEV_HISI_QM select CRYPTO_DH select CRYPTO_RSA + select CRYPTO_ECDH help Support for HiSilicon HPRE(High Performance RSA Engine) accelerator, which can accelerate RSA and DH algorithms. .
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Wed, 2021-03-24 at 16:49 -0400, Mimi Zohar wrote:
> On Wed, 2021-03-24 at 09:14 -0700, James Bottomley wrote:
> > On Tue, 2021-03-23 at 14:07 -0400, Mimi Zohar wrote:
> > > On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote:
> > > > Hello Horia,
> > > >
> > > > On 21.03.21 21:48, Horia Geantă wrote:
> > > > > On 3/16/2021 7:02 PM, Ahmad Fatoum wrote:
> > > > > [...]
> > > > > > +struct trusted_key_ops caam_trusted_key_ops = {
> > > > > > + .migratable = 0, /* non-migratable */
> > > > > > + .init = trusted_caam_init,
> > > > > > + .seal = trusted_caam_seal,
> > > > > > + .unseal = trusted_caam_unseal,
> > > > > > + .exit = trusted_caam_exit,
> > > > > > +};
> > > > > caam has random number generation capabilities, so it's worth
> > > > > using that
> > > > > by implementing .get_random.
> > > >
> > > > If the CAAM HWRNG is already seeding the kernel RNG, why not
> > > > use
> > > > the kernel's?
> > > >
> > > > Makes for less code duplication IMO.
> > >
> > > Using kernel RNG, in general, for trusted keys has been discussed
> > > before. Please refer to Dave Safford's detailed explanation for
> > > not
> > > using it [1].
> > >
> > > [1]
> > > https://lore.kernel.org/linux-integrity/bca04d5d9a3b764c9b7405bba4d4a3c035f2a...@alpmbapa12.e2k.ad.ge.com/
> >
> > I still don't think relying on one source of randomness to be
> > cryptographically secure is a good idea. The fear of bugs in the
> > kernel entropy pool is reasonable, but since it's widely used
> > they're
> > unlikely to persist very long. Studies have shown that some TPMs
> > (notably the chinese manufactured ones) have suspicious failures in
> > their RNGs:
> >
> > https://www.researchgate.net/publication/45934562_Benchmarking_the_True_Random_Number_Generator_of_TPM_Chips
> >
> > And most cryptograhpers recommend using a TPM for entropy mixing
> > rather
> > than directly:
> >
> > https://blog.cryptographyengineering.com/category/rngs/
> >
> > The TPMFail paper also shows that in spite of NIST certification
> > things can go wrong with a TPM:
> >
> > https://tpm.fail/
>
> We already had a lengthy discussion on replacing the TPM RNG with the
> kernel RNG for trusted keys, when TEE was being introduced
> [2,3]. I'm not interested in re-hashing that discussion here. The
> only difference now is that CAAM is a new trust source. I suspect
> the same concerns/issues persist, but at least in this case using the
> kernel RNG would not be a regression.
Upstreaming the ASN.1 parser gives us a way to create trusted keys
outside the kernel and so choose any RNG that suits the user, so I
don't think there's any need to rehash for TPM based keys either.
However CaaM doesn't have the ability to create keys outside the kernel
yet, so they do need to consider the problem.
James
> [2] Pascal Van Leeuwen on mixing different sources of entropy and
> certification -
>
> https://lore.kernel.org/linux-integrity/mn2pr20mb29732a856a40131a671f949fca...@mn2pr20mb2973.namprd20.prod.outlook.com/
> [3] Jarrko on "regression" and tpm_asym.c -
> https://lore.kernel.org/linux-integrity/20191014190033.ga15...@linux.intel.com/
>
>
> Mimi
>
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Wed, 2021-03-24 at 09:14 -0700, James Bottomley wrote:
> On Tue, 2021-03-23 at 14:07 -0400, Mimi Zohar wrote:
> > On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote:
> > > Hello Horia,
> > >
> > > On 21.03.21 21:48, Horia Geantă wrote:
> > > > On 3/16/2021 7:02 PM, Ahmad Fatoum wrote:
> > > > [...]
> > > > > +struct trusted_key_ops caam_trusted_key_ops = {
> > > > > + .migratable = 0, /* non-migratable */
> > > > > + .init = trusted_caam_init,
> > > > > + .seal = trusted_caam_seal,
> > > > > + .unseal = trusted_caam_unseal,
> > > > > + .exit = trusted_caam_exit,
> > > > > +};
> > > > caam has random number generation capabilities, so it's worth
> > > > using that
> > > > by implementing .get_random.
> > >
> > > If the CAAM HWRNG is already seeding the kernel RNG, why not use
> > > the kernel's?
> > >
> > > Makes for less code duplication IMO.
> >
> > Using kernel RNG, in general, for trusted keys has been discussed
> > before. Please refer to Dave Safford's detailed explanation for not
> > using it [1].
> >
> > [1]
> > https://lore.kernel.org/linux-integrity/bca04d5d9a3b764c9b7405bba4d4a3c035f2a...@alpmbapa12.e2k.ad.ge.com/
>
> I still don't think relying on one source of randomness to be
> cryptographically secure is a good idea. The fear of bugs in the
> kernel entropy pool is reasonable, but since it's widely used they're
> unlikely to persist very long. Studies have shown that some TPMs
> (notably the chinese manufactured ones) have suspicious failures in
> their RNGs:
>
> https://www.researchgate.net/publication/45934562_Benchmarking_the_True_Random_Number_Generator_of_TPM_Chips
>
> And most cryptograhpers recommend using a TPM for entropy mixing rather
> than directly:
>
> https://blog.cryptographyengineering.com/category/rngs/
>
> The TPMFail paper also shows that in spite of NIST certification
> things can go wrong with a TPM:
>
> https://tpm.fail/
We already had a lengthy discussion on replacing the TPM RNG with the
kernel RNG for trusted keys, when TEE was being introduced [2,3]. I'm
not interested in re-hashing that discussion here. The only
difference now is that CAAM is a new trust source. I suspect the same
concerns/issues persist, but at least in this case using the kernel RNG
would not be a regression.
[2] Pascal Van Leeuwen on mixing different sources of entropy and certification
-
https://lore.kernel.org/linux-integrity/mn2pr20mb29732a856a40131a671f949fca...@mn2pr20mb2973.namprd20.prod.outlook.com/
[3] Jarrko on "regression" and tpm_asym.c -
https://lore.kernel.org/linux-integrity/20191014190033.ga15...@linux.intel.com/
Mimi
Re: [RFC Part2 PATCH 06/30] x86/fault: dump the RMP entry on #PF
On 3/24/21 12:47 PM, Andy Lutomirski wrote:
> On Wed, Mar 24, 2021 at 10:04 AM Brijesh Singh wrote:
>> If hardware detects an RMP violation, it will raise a page-fault exception
>> with the RMP bit set. To help the debug, dump the RMP entry of the faulting
>> address.
>>
>> Cc: Thomas Gleixner
>> Cc: Ingo Molnar
>> Cc: Borislav Petkov
>> Cc: Joerg Roedel
>> Cc: "H. Peter Anvin"
>> Cc: Tony Luck
>> Cc: Dave Hansen
>> Cc: "Peter Zijlstra (Intel)"
>> Cc: Paolo Bonzini
>> Cc: Tom Lendacky
>> Cc: David Rientjes
>> Cc: Sean Christopherson
>> Cc: x...@kernel.org
>> Cc: k...@vger.kernel.org
>> Signed-off-by: Brijesh Singh
>> ---
>> arch/x86/mm/fault.c | 75 +
>> 1 file changed, 75 insertions(+)
>>
>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> index f39b551f89a6..7605e06a6dd9 100644
>> --- a/arch/x86/mm/fault.c
>> +++ b/arch/x86/mm/fault.c
>> @@ -31,6 +31,7 @@
>> #include /* VMALLOC_START, ... */
>> #include /* kvm_handle_async_pf */
>> #include /* fixup_vdso_exception() */
>> +#include/* lookup_rmpentry ... */
>>
>> #define CREATE_TRACE_POINTS
>> #include
>> @@ -147,6 +148,76 @@ is_prefetch(struct pt_regs *regs, unsigned long
>> error_code, unsigned long addr)
>> DEFINE_SPINLOCK(pgd_lock);
>> LIST_HEAD(pgd_list);
>>
>> +static void dump_rmpentry(struct page *page, rmpentry_t *e)
>> +{
>> + unsigned long paddr = page_to_pfn(page) << PAGE_SHIFT;
>> +
>> + pr_alert("RMPEntry paddr 0x%lx [assigned=%d immutable=%d pagesize=%d
>> gpa=0x%lx asid=%d "
>> + "vmsa=%d validated=%d]\n", paddr, rmpentry_assigned(e),
>> rmpentry_immutable(e),
>> + rmpentry_pagesize(e), rmpentry_gpa(e), rmpentry_asid(e),
>> rmpentry_vmsa(e),
>> + rmpentry_validated(e));
>> + pr_alert("RMPEntry paddr 0x%lx %016llx %016llx\n", paddr, e->high,
>> e->low);
>> +}
>> +
>> +static void show_rmpentry(unsigned long address)
>> +{
>> + struct page *page = virt_to_page(address);
> This is an error path, and I don't think you have any particular
> guarantee that virt_to_page(address) is valid. Please add appropriate
> validation or use one of the slow lookup helpers.
Noted, thanks for the quick feedback.
Re: [RFC Part2 PATCH 05/30] x86: define RMP violation #PF error code
> diff --git a/arch/x86/include/asm/trap_pf.h b/arch/x86/include/asm/trap_pf.h
> index 10b1de500ab1..107f9d947e8d 100644
> --- a/arch/x86/include/asm/trap_pf.h
> +++ b/arch/x86/include/asm/trap_pf.h
> @@ -12,6 +12,7 @@
> * bit 4 ==1: fault was an instruction
> fetch
> * bit 5 ==1: protection keys block access
> * bit 15 == 1: SGX MMU page-fault
> + * bit 31 == 1: fault was an RMP violation
> */
> enum x86_pf_error_code {
> X86_PF_PROT = 1 << 0,
> @@ -21,6 +22,7 @@ enum x86_pf_error_code {
> X86_PF_INSTR= 1 << 4,
> X86_PF_PK = 1 << 5,
> X86_PF_SGX = 1 << 15,
> + X86_PF_RMP = 1ull << 31,
> };
Man, I hope AMD and Intel are talking to each other about these bits. :)
Either way, this is hitting the limits of what I know about how enums
are implemented. I had internalized that they are just an 'int', but
that doesn't seem quite right. It sounds like they must be implemented
using *an* integer type, but not necessarily 'int' itself.
Either way, '1<<31' doesn't fit in a 32-bit signed int. But, gcc at
least doesn't seem to blow the enum up into a 64-bit type, which is nice.
Could we at least start declaring these with BIT()?
Re: [RFC Part2 PATCH 06/30] x86/fault: dump the RMP entry on #PF
On Wed, Mar 24, 2021 at 10:04 AM Brijesh Singh wrote:
>
> If hardware detects an RMP violation, it will raise a page-fault exception
> with the RMP bit set. To help the debug, dump the RMP entry of the faulting
> address.
>
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: Borislav Petkov
> Cc: Joerg Roedel
> Cc: "H. Peter Anvin"
> Cc: Tony Luck
> Cc: Dave Hansen
> Cc: "Peter Zijlstra (Intel)"
> Cc: Paolo Bonzini
> Cc: Tom Lendacky
> Cc: David Rientjes
> Cc: Sean Christopherson
> Cc: x...@kernel.org
> Cc: k...@vger.kernel.org
> Signed-off-by: Brijesh Singh
> ---
> arch/x86/mm/fault.c | 75 +
> 1 file changed, 75 insertions(+)
>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index f39b551f89a6..7605e06a6dd9 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -31,6 +31,7 @@
> #include /* VMALLOC_START, ... */
> #include /* kvm_handle_async_pf */
> #include /* fixup_vdso_exception() */
> +#include/* lookup_rmpentry ... */
>
> #define CREATE_TRACE_POINTS
> #include
> @@ -147,6 +148,76 @@ is_prefetch(struct pt_regs *regs, unsigned long
> error_code, unsigned long addr)
> DEFINE_SPINLOCK(pgd_lock);
> LIST_HEAD(pgd_list);
>
> +static void dump_rmpentry(struct page *page, rmpentry_t *e)
> +{
> + unsigned long paddr = page_to_pfn(page) << PAGE_SHIFT;
> +
> + pr_alert("RMPEntry paddr 0x%lx [assigned=%d immutable=%d pagesize=%d
> gpa=0x%lx asid=%d "
> + "vmsa=%d validated=%d]\n", paddr, rmpentry_assigned(e),
> rmpentry_immutable(e),
> + rmpentry_pagesize(e), rmpentry_gpa(e), rmpentry_asid(e),
> rmpentry_vmsa(e),
> + rmpentry_validated(e));
> + pr_alert("RMPEntry paddr 0x%lx %016llx %016llx\n", paddr, e->high,
> e->low);
> +}
> +
> +static void show_rmpentry(unsigned long address)
> +{
> + struct page *page = virt_to_page(address);
This is an error path, and I don't think you have any particular
guarantee that virt_to_page(address) is valid. Please add appropriate
validation or use one of the slow lookup helpers.
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Tue, 2021-03-23 at 14:07 -0400, Mimi Zohar wrote:
> On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote:
> > Hello Horia,
> >
> > On 21.03.21 21:48, Horia Geantă wrote:
> > > On 3/16/2021 7:02 PM, Ahmad Fatoum wrote:
> > > [...]
> > > > +struct trusted_key_ops caam_trusted_key_ops = {
> > > > + .migratable = 0, /* non-migratable */
> > > > + .init = trusted_caam_init,
> > > > + .seal = trusted_caam_seal,
> > > > + .unseal = trusted_caam_unseal,
> > > > + .exit = trusted_caam_exit,
> > > > +};
> > > caam has random number generation capabilities, so it's worth
> > > using that
> > > by implementing .get_random.
> >
> > If the CAAM HWRNG is already seeding the kernel RNG, why not use
> > the kernel's?
> >
> > Makes for less code duplication IMO.
>
> Using kernel RNG, in general, for trusted keys has been discussed
> before. Please refer to Dave Safford's detailed explanation for not
> using it [1].
>
> thanks,
>
> Mimi
>
> [1]
> https://lore.kernel.org/linux-integrity/bca04d5d9a3b764c9b7405bba4d4a3c035f2a...@alpmbapa12.e2k.ad.ge.com/
I still don't think relying on one source of randomness to be
cryptographically secure is a good idea. The fear of bugs in the
kernel entropy pool is reasonable, but since it's widely used they're
unlikely to persist very long. Studies have shown that some TPMs
(notably the chinese manufactured ones) have suspicious failures in
their RNGs:
https://www.researchgate.net/publication/45934562_Benchmarking_the_True_Random_Number_Generator_of_TPM_Chips
And most cryptograhpers recommend using a TPM for entropy mixing rather
than directly:
https://blog.cryptographyengineering.com/category/rngs/
The TPMFail paper also shows that in spite of NIST certification
things can go wrong with a TPM:
https://tpm.fail/
James
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
Hello Sumit, On 24.03.21 11:47, Sumit Garg wrote: > On Wed, 24 Mar 2021 at 14:56, Ahmad Fatoum wrote: >> >> Hello Mimi, >> >> On 23.03.21 19:07, Mimi Zohar wrote: >>> On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote: On 21.03.21 21:48, Horia Geantă wrote: > caam has random number generation capabilities, so it's worth using that > by implementing .get_random. If the CAAM HWRNG is already seeding the kernel RNG, why not use the kernel's? Makes for less code duplication IMO. >>> >>> Using kernel RNG, in general, for trusted keys has been discussed >>> before. Please refer to Dave Safford's detailed explanation for not >>> using it [1]. >> >> The argument seems to boil down to: >> >> - TPM RNG are known to be of good quality >> - Trusted keys always used it so far >> >> Both are fine by me for TPMs, but the CAAM backend is new code and neither >> point >> really applies. >> >> get_random_bytes_wait is already used for generating key material elsewhere. >> Why shouldn't new trusted key backends be able to do the same thing? >> > > Please refer to documented trusted keys behaviour here [1]. New > trusted key backends should align to this behaviour and in your case > CAAM offers HWRNG so we should be better using that. Why is it better? Can you explain what benefit a CAAM user would have if the trusted key randomness comes directly out of the CAAM instead of indirectly from the kernel entropy pool that is seeded by it? > Also, do update documentation corresponding to CAAM as a trusted keys backend. Yes. The documentation should be updated for CAAM and it should describe how the key material is derived. Will do so for v2. Cheers, Ahmad > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/tree/Documentation/security/keys/trusted-encrypted.rst#n87 > > -Sumit > >> Cheers, >> Ahmad >> >>> >>> thanks, >>> >>> Mimi >>> >>> [1] >>> https://lore.kernel.org/linux-integrity/bca04d5d9a3b764c9b7405bba4d4a3c035f2a...@alpmbapa12.e2k.ad.ge.com/ >>> >>> >>> >> >> -- >> Pengutronix e.K. | | >> Steuerwalder Str. 21 | http://www.pengutronix.de/ | >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
On Wed, 24 Mar 2021 at 14:56, Ahmad Fatoum wrote: > > Hello Mimi, > > On 23.03.21 19:07, Mimi Zohar wrote: > > On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote: > >> On 21.03.21 21:48, Horia Geantă wrote: > >>> caam has random number generation capabilities, so it's worth using that > >>> by implementing .get_random. > >> > >> If the CAAM HWRNG is already seeding the kernel RNG, why not use the > >> kernel's? > >> > >> Makes for less code duplication IMO. > > > > Using kernel RNG, in general, for trusted keys has been discussed > > before. Please refer to Dave Safford's detailed explanation for not > > using it [1]. > > The argument seems to boil down to: > > - TPM RNG are known to be of good quality > - Trusted keys always used it so far > > Both are fine by me for TPMs, but the CAAM backend is new code and neither > point > really applies. > > get_random_bytes_wait is already used for generating key material elsewhere. > Why shouldn't new trusted key backends be able to do the same thing? > Please refer to documented trusted keys behaviour here [1]. New trusted key backends should align to this behaviour and in your case CAAM offers HWRNG so we should be better using that. Also, do update documentation corresponding to CAAM as a trusted keys backend. [1] https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/tree/Documentation/security/keys/trusted-encrypted.rst#n87 -Sumit > Cheers, > Ahmad > > > > > thanks, > > > > Mimi > > > > [1] > > https://lore.kernel.org/linux-integrity/bca04d5d9a3b764c9b7405bba4d4a3c035f2a...@alpmbapa12.e2k.ad.ge.com/ > > > > > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] init/Kconfig: Support sign module with SM3 hash algorithm
Hi, On 3/24/21 6:14 AM, Ard Biesheuvel wrote: On Tue, 23 Mar 2021 at 09:36, Tianjia Zhang wrote: The kernel module signature supports the option to use the SM3 secure hash (OSCCA GM/T 0004-2012 SM3). Signed-off-by: Tianjia Zhang A secure hash is not the same as a signature. Looking at the patch, the asymmetric algorithm that is used to sign the SM3 digest is SM2, is that correct? How does one create such signed modules? In any case, please provide more context in the commit log on how this is intended to be used. Sorry for the trouble you have caused. You are right. SM2 and SM3 always appear in pairs. The former is used for signatures and the latter is used for hashing algorithms. I will add this information in the next version. It seems This is more appropriate to split into two patches. Best regards, Tianjia
Re: [PATCH] init/Kconfig: Support sign module with SM3 hash algorithm
Hi, On 3/24/21 12:43 AM, Randy Dunlap wrote: On 3/23/21 1:35 AM, Tianjia Zhang wrote: The kernel module signature supports the option to use the SM3 secure hash (OSCCA GM/T 0004-2012 SM3). Signed-off-by: Tianjia Zhang --- Documentation/admin-guide/module-signing.rst | 5 +++-- crypto/asymmetric_keys/pkcs7_parser.c| 7 +++ init/Kconfig | 5 + 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/init/Kconfig b/init/Kconfig index 5f5c776ef192..fed9236078e4 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -2202,6 +2202,10 @@ config MODULE_SIG_SHA512 bool "Sign modules with SHA-512" select CRYPTO_SHA512 +config MODULE_SIG_SM3 + bool "Sign modules with SM3" + select CRYPTO_SM3 + endchoice config MODULE_SIG_HASH @@ -2212,6 +2216,7 @@ config MODULE_SIG_HASH default "sha256" if MODULE_SIG_SHA256 default "sha384" if MODULE_SIG_SHA384 default "sha512" if MODULE_SIG_SHA512 + default "sm3" if MODULE_SIG_SM3 config MODULE_COMPRESS bool "Compress modules on installation" checkpatch tells me: WARNING: please write a paragraph that describes the config symbol fully #74: FILE: init/Kconfig:2205: +config MODULE_SIG_SM3 so yes, it should have some help text there. thanks. I noticed, but this is just a list of algorithms, this warning can be ignored. Best regards, Tianjia
Re: [PATCH v1 3/3] KEYS: trusted: Introduce support for NXP CAAM-based trusted keys
Hello Mimi, On 23.03.21 19:07, Mimi Zohar wrote: > On Tue, 2021-03-23 at 17:35 +0100, Ahmad Fatoum wrote: >> On 21.03.21 21:48, Horia Geantă wrote: >>> caam has random number generation capabilities, so it's worth using that >>> by implementing .get_random. >> >> If the CAAM HWRNG is already seeding the kernel RNG, why not use the >> kernel's? >> >> Makes for less code duplication IMO. > > Using kernel RNG, in general, for trusted keys has been discussed > before. Please refer to Dave Safford's detailed explanation for not > using it [1]. The argument seems to boil down to: - TPM RNG are known to be of good quality - Trusted keys always used it so far Both are fine by me for TPMs, but the CAAM backend is new code and neither point really applies. get_random_bytes_wait is already used for generating key material elsewhere. Why shouldn't new trusted key backends be able to do the same thing? Cheers, Ahmad > > thanks, > > Mimi > > [1] > https://lore.kernel.org/linux-integrity/bca04d5d9a3b764c9b7405bba4d4a3c035f2a...@alpmbapa12.e2k.ad.ge.com/ > > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
