[PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Hangbin Liu]

As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not
FIPS certified, the WireGuard module should be disabled in FIPS mode.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/wireguard/main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireguard/main.c b/drivers/net/wireguard/main.c
index 7a7d5f1a80fc..8a9aaea7623c 100644
--- a/drivers/net/wireguard/main.c
+++ b/drivers/net/wireguard/main.c
@@ -12,6 +12,7 @@
 
 #include <uapi/linux/wireguard.h>
 
+#include <linux/fips.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/genetlink.h>
@@ -21,6 +22,9 @@ static int __init mod_init(void)
 {
 	int ret;
 
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 #ifdef DEBUG
 	if (!wg_allowedips_selftest() || !wg_packet_counter_selftest() ||
 	    !wg_ratelimiter_selftest())
-- 
2.26.3

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Eric Biggers]

On Wed, Apr 07, 2021 at 07:39:20PM +0800, Hangbin Liu wrote:
> As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not
> FIPS certified, the WireGuard module should be disabled in FIPS mode.
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

I think you mean "FIPS allowed", not "FIPS certified"?  Even if it used FIPS
allowed algorithms like AES, the Linux kernel doesn't come with any sort of FIPS
certification out of the box.

Also, couldn't you just consider WireGuard to be outside your FIPS module
boundary, which would remove it from the scope of the certification?

And how do you handle all the other places in the kernel that use ChaCha20 and
SipHash?  For example, drivers/char/random.c?

- Eric

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Hangbin Liu]

On Wed, Apr 07, 2021 at 02:12:27PM -0700, Eric Biggers wrote:
> On Wed, Apr 07, 2021 at 07:39:20PM +0800, Hangbin Liu wrote:
> > As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not
> > FIPS certified, the WireGuard module should be disabled in FIPS mode.
> > 
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> I think you mean "FIPS allowed", not "FIPS certified"?  Even if it used FIPS
> allowed algorithms like AES, the Linux kernel doesn't come with any sort of FIPS
> certification out of the box.

Yes, you are right.
> 
> Also, couldn't you just consider WireGuard to be outside your FIPS module
> boundary, which would remove it from the scope of the certification?
> 
> And how do you handle all the other places in the kernel that use ChaCha20 and
> SipHash?  For example, drivers/char/random.c?

Good question, I will check it and reply to you later.

Thanks
Hangbin

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Hangbin Liu]

On Thu, Apr 08, 2021 at 09:06:52AM +0800, Hangbin Liu wrote:
> > Also, couldn't you just consider WireGuard to be outside your FIPS module
> > boundary, which would remove it from the scope of the certification?
> > 
> > And how do you handle all the other places in the kernel that use ChaCha20 and
> > SipHash?  For example, drivers/char/random.c?
> 
> Good question, I will check it and reply to you later.

I just read the code. The drivers/char/random.c do has some fips specific
parts(seems not related to crypto). After commit e192be9d9a30 ("random: replace
non-blocking pool with a Chacha20-based CRNG") we moved part of chacha code to
lib/chacha20.c and make that code out of control.

Thanks
Hangbin

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Eric Biggers]

On Thu, Apr 08, 2021 at 07:58:08PM +0800, Hangbin Liu wrote:
> On Thu, Apr 08, 2021 at 09:06:52AM +0800, Hangbin Liu wrote:
> > > Also, couldn't you just consider WireGuard to be outside your FIPS module
> > > boundary, which would remove it from the scope of the certification?
> > > 
> > > And how do you handle all the other places in the kernel that use ChaCha20 and
> > > SipHash?  For example, drivers/char/random.c?
> > 
> > Good question, I will check it and reply to you later.
> 
> I just read the code. The drivers/char/random.c do has some fips specific
> parts(seems not related to crypto). After commit e192be9d9a30 ("random: replace
> non-blocking pool with a Chacha20-based CRNG") we moved part of chacha code to
> lib/chacha20.c and make that code out of control.
> 

So you are saying that you removed drivers/char/random.c and lib/chacha20.c from
your FIPS module boundary?  Why not do the same for WireGuard?

- Eric

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Hangbin Liu]

On Thu, Apr 08, 2021 at 08:11:34AM -0700, Eric Biggers wrote:
> On Thu, Apr 08, 2021 at 07:58:08PM +0800, Hangbin Liu wrote:
> > On Thu, Apr 08, 2021 at 09:06:52AM +0800, Hangbin Liu wrote:
> > > > Also, couldn't you just consider WireGuard to be outside your FIPS module
> > > > boundary, which would remove it from the scope of the certification?
> > > > 
> > > > And how do you handle all the other places in the kernel that use ChaCha20 and
> > > > SipHash?  For example, drivers/char/random.c?
> > > 
> > > Good question, I will check it and reply to you later.
> > 
> > I just read the code. The drivers/char/random.c do has some fips specific
> > parts(seems not related to crypto). After commit e192be9d9a30 ("random: replace
> > non-blocking pool with a Chacha20-based CRNG") we moved part of chacha code to
> > lib/chacha20.c and make that code out of control.
> > 
> So you are saying that you removed drivers/char/random.c and lib/chacha20.c from
> your FIPS module boundary?  Why not do the same for WireGuard?

No, I mean this looks like a bug (using not allowed crypto in FIPS mode) and
we should fix it.

Thanks
Hangbin

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Stephan Mueller]

Am Freitag, dem 09.04.2021 um 10:11 +0800 schrieb Hangbin Liu:
> On Thu, Apr 08, 2021 at 08:11:34AM -0700, Eric Biggers wrote:
> > On Thu, Apr 08, 2021 at 07:58:08PM +0800, Hangbin Liu wrote:
> > > On Thu, Apr 08, 2021 at 09:06:52AM +0800, Hangbin Liu wrote:
> > > > > Also, couldn't you just consider WireGuard to be outside your FIPS
> > > > > module
> > > > > boundary, which would remove it from the scope of the certification?
> > > > > 
> > > > > And how do you handle all the other places in the kernel that use
> > > > > ChaCha20 and
> > > > > SipHash?  For example, drivers/char/random.c?
> > > > 
> > > > Good question, I will check it and reply to you later.
> > > 
> > > I just read the code. The drivers/char/random.c do has some fips
> > > specific
> > > parts(seems not related to crypto). After commit e192be9d9a30 ("random:
> > > replace
> > > non-blocking pool with a Chacha20-based CRNG") we moved part of chacha
> > > code to
> > > lib/chacha20.c and make that code out of control.
> > > 
> > So you are saying that you removed drivers/char/random.c and
> > lib/chacha20.c from
> > your FIPS module boundary?  Why not do the same for WireGuard?
> 
> No, I mean this looks like a bug (using not allowed crypto in FIPS mode) and
> we should fix it.

The entirety of random.c is not compliant to FIPS rules. ChaCha20 is the least
of the problems. SP800-90B is the challenge. This is one of the motivation of
the design and architecture of the LRNG allowing different types of crypto and
have a different approach to post-process the data.

https://github.com/smuellerDD/lrng

Ciao
Stephan
> 
> Thanks
> Hangbin

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Hangbin Liu]

On Fri, Apr 09, 2021 at 09:08:20AM +0200, Stephan Mueller wrote:
> > > > > > And how do you handle all the other places in the kernel that use
> > > > > > ChaCha20 and
> > > > > > SipHash?  For example, drivers/char/random.c?
> > > > > 
> > > > > Good question, I will check it and reply to you later.
> > > > 
> > > > I just read the code. The drivers/char/random.c do has some fips
> > > > specific
> > > > parts(seems not related to crypto). After commit e192be9d9a30 ("random:
> > > > replace
> > > > non-blocking pool with a Chacha20-based CRNG") we moved part of chacha
> > > > code to
> > > > lib/chacha20.c and make that code out of control.
> > > > 
> > > So you are saying that you removed drivers/char/random.c and
> > > lib/chacha20.c from
> > > your FIPS module boundary?  Why not do the same for WireGuard?
> > 
> > No, I mean this looks like a bug (using not allowed crypto in FIPS mode) and
> > we should fix it.
> 
> The entirety of random.c is not compliant to FIPS rules. ChaCha20 is the least
> of the problems. SP800-90B is the challenge. This is one of the motivation of
> the design and architecture of the LRNG allowing different types of crypto and
> have a different approach to post-process the data.
> 
> https://github.com/smuellerDD/lrng

Thanks Stephan for this info. After offline discussion with Herbert, here is
what he said:

"""
This is not a problem in RHEL8 because the Crypto API RNG replaces /dev/random
in FIPS mode.
"""

I'm not familiar with this code, not sure how upstream handle this.

Thanks
Hangbin

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Simo Sorce]

On Fri, 2021-04-09 at 16:08 +0800, Hangbin Liu wrote:
> On Fri, Apr 09, 2021 at 09:08:20AM +0200, Stephan Mueller wrote:
> > > > > > > And how do you handle all the other places in the kernel that use
> > > > > > > ChaCha20 and
> > > > > > > SipHash?  For example, drivers/char/random.c?
> > > > > > 
> > > > > > Good question, I will check it and reply to you later.
> > > > > 
> > > > > I just read the code. The drivers/char/random.c do has some fips
> > > > > specific
> > > > > parts(seems not related to crypto). After commit e192be9d9a30 ("random:
> > > > > replace
> > > > > non-blocking pool with a Chacha20-based CRNG") we moved part of chacha
> > > > > code to
> > > > > lib/chacha20.c and make that code out of control.
> > > > > 
> > > > So you are saying that you removed drivers/char/random.c and
> > > > lib/chacha20.c from
> > > > your FIPS module boundary?  Why not do the same for WireGuard?
> > > 
> > > No, I mean this looks like a bug (using not allowed crypto in FIPS mode) and
> > > we should fix it.
> > 
> > The entirety of random.c is not compliant to FIPS rules. ChaCha20 is the least
> > of the problems. SP800-90B is the challenge. This is one of the motivation of
> > the design and architecture of the LRNG allowing different types of crypto and
> > have a different approach to post-process the data.
> > 
> > https://github.com/smuellerDD/lrng
> 
> Thanks Stephan for this info. After offline discussion with Herbert, here is
> what he said:
> 
> """
> This is not a problem in RHEL8 because the Crypto API RNG replaces /dev/random
> in FIPS mode.
> """
> 
> I'm not familiar with this code, not sure how upstream handle this.

It is an open problem upstream.

Simo.

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Jason A. Donenfeld]

On Fri, Apr 9, 2021 at 2:08 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> After offline discussion with Herbert, here is
> what he said:
>
> """
> This is not a problem in RHEL8 because the Crypto API RNG replaces /dev/random
> in FIPS mode.
> """

So far as I can see, this isn't the case in the kernel sources I'm
reading? Maybe you're doing some userspace hack with CUSE? But at
least get_random_bytes doesn't behave this way...

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Hangbin Liu]

On Fri, Apr 09, 2021 at 12:29:42PM -0600, Jason A. Donenfeld wrote:
> On Fri, Apr 9, 2021 at 2:08 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> > After offline discussion with Herbert, here is
> > what he said:
> >
> > """
> > This is not a problem in RHEL8 because the Crypto API RNG replaces /dev/random
> > in FIPS mode.
> > """
> 
> So far as I can see, this isn't the case in the kernel sources I'm
> reading? Maybe you're doing some userspace hack with CUSE? But at
> least get_random_bytes doesn't behave this way...

> > I'm not familiar with this code, not sure how upstream handle this.

Hi Jason,

As I said, I'm not familiar with this part of code. If upstream does not
handle this correctly, sure this is an issue and need to be fixed.

And as Simo said, he is also working on this part. I will talk with him
and Herbert and see if we can have a more proper fix.

Feel free to drop this patch.

Thanks
Hangbin

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Jason A. Donenfeld]

Hi Hangbin,

On Wed, Apr 7, 2021 at 5:39 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not
> FIPS certified, the WireGuard module should be disabled in FIPS mode.

I'm not sure this makes so much sense to do _in wireguard_. If you
feel like the FIPS-allergic part is actually blake, 25519, chacha, and
poly1305, then wouldn't it make most sense to disable _those_ modules
instead? And then the various things that rely on those (such as
wireguard, but maybe there are other things too, like
security/keys/big_key.c) would be naturally disabled transitively?

[As an aside, I don't think any of this fips-flag-in-the-kernel makes
much sense at all for anything, but that seems like a different
discussion, maybe?]

Jason

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Hangbin Liu]

On Wed, Apr 07, 2021 at 03:15:51PM -0600, Jason A. Donenfeld wrote:
> Hi Hangbin,
> 
> On Wed, Apr 7, 2021 at 5:39 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> >
> > As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not
> > FIPS certified, the WireGuard module should be disabled in FIPS mode.
> 
> I'm not sure this makes so much sense to do _in wireguard_. If you
> feel like the FIPS-allergic part is actually blake, 25519, chacha, and
> poly1305, then wouldn't it make most sense to disable _those_ modules
> instead? And then the various things that rely on those (such as
> wireguard, but maybe there are other things too, like
> security/keys/big_key.c) would be naturally disabled transitively?

Hi Jason,

I'm not familiar with the crypto code. From wg_noise_init() it looks the init
part is in header file. So I just disabled wireguard directly.

For disabling the modules. Hi Ondrej, do you know if there is any FIPS policy
in crypto part? There seems no handler when load not allowed crypto modules
in FIPS mode.

BTW, I also has a question, apart from the different RFC standard, what's the
relation/difference between crypto/chacha20poly1305.c and lib/crypto/chacha20poly1305.c?

Thanks
Hangbin

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Ondrej Mosnacek]

On Thu, Apr 8, 2021 at 8:52 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> On Wed, Apr 07, 2021 at 03:15:51PM -0600, Jason A. Donenfeld wrote:
> > Hi Hangbin,
> >
> > On Wed, Apr 7, 2021 at 5:39 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> > >
> > > As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not
> > > FIPS certified, the WireGuard module should be disabled in FIPS mode.
> >
> > I'm not sure this makes so much sense to do _in wireguard_. If you
> > feel like the FIPS-allergic part is actually blake, 25519, chacha, and
> > poly1305, then wouldn't it make most sense to disable _those_ modules
> > instead? And then the various things that rely on those (such as
> > wireguard, but maybe there are other things too, like
> > security/keys/big_key.c) would be naturally disabled transitively?
>
> Hi Jason,
>
> I'm not familiar with the crypto code. From wg_noise_init() it looks the init
> part is in header file. So I just disabled wireguard directly.
>
> For disabling the modules. Hi Ondrej, do you know if there is any FIPS policy
> in crypto part? There seems no handler when load not allowed crypto modules
> in FIPS mode.

If I understand your question correctly, yes, there is a mechanism
that disables not-FIPS-approved algorithms/drivers in FIPS mode (not
kernel modules themselves, AFAIK). So if any part of the kernel tries
to use e.g. chacha20 via the Crypto API (the bits in crypto/...), it
will fail. I'm not sure about the direct library interface (the bits
in lib/crypto/...) though... That's relatively new and I haven't been
following the upstream development in this area that closely for some
time now...

>
> BTW, I also has a question, apart from the different RFC standard, what's the
> relation/difference between crypto/chacha20poly1305.c and lib/crypto/chacha20poly1305.c?
>
> Thanks
> Hangbin
>

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Simo Sorce]

On Wed, 2021-04-07 at 15:15 -0600, Jason A. Donenfeld wrote:
> Hi Hangbin,
> 
> On Wed, Apr 7, 2021 at 5:39 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> > As the cryptos(BLAKE2S, Curve25519, CHACHA20POLY1305) in WireGuard are not
> > FIPS certified, the WireGuard module should be disabled in FIPS mode.
> 
> I'm not sure this makes so much sense to do _in wireguard_. If you
> feel like the FIPS-allergic part is actually blake, 25519, chacha, and
> poly1305, then wouldn't it make most sense to disable _those_ modules
> instead? And then the various things that rely on those (such as
> wireguard, but maybe there are other things too, like
> security/keys/big_key.c) would be naturally disabled transitively?

The reason why it is better to disable the whole module is that it
provide much better feedback to users. Letting init go through and then
just fail operations once someone tries to use it is much harder to
deal with in terms of figuring out what went wrong.
Also wireguard seem to be poking directly into the algorithms
implementations and not use the crypto API, so disabling individual
algorithm via the regular fips_enabled mechanism at runtime doesn't
work.

> [As an aside, I don't think any of this fips-flag-in-the-kernel makes
> much sense at all for anything, but that seems like a different
> discussion, maybe?]

It makes sense, because vendors provide a single kernel that can be
used by both people that are required to be FIPS compliant and people
that don't. For people that are required to be FIPS complaint vendors
want to provide the ability to use a single knob (fips=1 at boot) to
turn off everything that is not FIPS compliant.
Disabling algorithms at compile time would not work for people that do
not care about FIPS compliance.

HTH,
Simo.

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Jason A. Donenfeld]

On Thu, Apr 8, 2021 at 7:55 AM Simo Sorce <simo@redhat.com> wrote:
> > I'm not sure this makes so much sense to do _in wireguard_. If you
> > feel like the FIPS-allergic part is actually blake, 25519, chacha, and
> > poly1305, then wouldn't it make most sense to disable _those_ modules
> > instead? And then the various things that rely on those (such as
> > wireguard, but maybe there are other things too, like
> > security/keys/big_key.c) would be naturally disabled transitively?
>
> The reason why it is better to disable the whole module is that it
> provide much better feedback to users. Letting init go through and then
> just fail operations once someone tries to use it is much harder to
> deal with in terms of figuring out what went wrong.
> Also wireguard seem to be poking directly into the algorithms
> implementations and not use the crypto API, so disabling individual
> algorithm via the regular fips_enabled mechanism at runtime doesn't
> work.

What I'm suggesting _would_ work in basically the exact same way as
this patch. Namely, something like:

diff --git a/lib/crypto/curve25519.c b/lib/crypto/curve25519.c
index 288a62cd29b2..b794f49c291a 100644
--- a/lib/crypto/curve25519.c
+++ b/lib/crypto/curve25519.c
@@ -12,11 +12,15 @@
 #include <crypto/curve25519.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/fips.h>

 bool curve25519_selftest(void);

 static int __init mod_init(void)
 {
+ if (!fips_enabled)
+ return -EOPNOTSUPP;
+
  if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS) &&
      WARN_ON(!curve25519_selftest()))
  return -ENODEV;

Making the various lib/crypto/* modules return EOPNOTSUPP will in turn
mean that wireguard will refuse to load, due to !fips_enabled. It has
the positive effect that all other things that use it will also be
EOPNOTSUPP.

For example, what are you doing about big_key.c? Right now, I assume
nothing. But this way, you'd get all of the various effects for free.
Are you going to continuously audit all uses of non-FIPS crypto and
add `if (!fips_enabled)` to every new use case, always, everywhere,
from now into the boundless future? By adding `if (!fips_enabled)` to
wireguard, that's what you're signing yourself up for. Instead, by
restricting the lib/crypto/* modules to !fips_enabled, you can get all
of those transitive effects without having to do anything additional.

Alternatively, I agree with Eric - why not just consider this outside
your boundary?

Jason

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Simo Sorce]

On Thu, 2021-04-08 at 15:55 -0600, Jason A. Donenfeld wrote:
> On Thu, Apr 8, 2021 at 7:55 AM Simo Sorce <simo@redhat.com> wrote:
> > > I'm not sure this makes so much sense to do _in wireguard_. If you
> > > feel like the FIPS-allergic part is actually blake, 25519, chacha, and
> > > poly1305, then wouldn't it make most sense to disable _those_ modules
> > > instead? And then the various things that rely on those (such as
> > > wireguard, but maybe there are other things too, like
> > > security/keys/big_key.c) would be naturally disabled transitively?
> > 
> > The reason why it is better to disable the whole module is that it
> > provide much better feedback to users. Letting init go through and then
> > just fail operations once someone tries to use it is much harder to
> > deal with in terms of figuring out what went wrong.
> > Also wireguard seem to be poking directly into the algorithms
> > implementations and not use the crypto API, so disabling individual
> > algorithm via the regular fips_enabled mechanism at runtime doesn't
> > work.
> 
> What I'm suggesting _would_ work in basically the exact same way as
> this patch. Namely, something like:
> 
> diff --git a/lib/crypto/curve25519.c b/lib/crypto/curve25519.c
> index 288a62cd29b2..b794f49c291a 100644
> --- a/lib/crypto/curve25519.c
> +++ b/lib/crypto/curve25519.c
> @@ -12,11 +12,15 @@
>  #include <crypto/curve25519.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> +#include <linux/fips.h>
> 
>  bool curve25519_selftest(void);
> 
>  static int __init mod_init(void)
>  {
> + if (!fips_enabled)
> + return -EOPNOTSUPP;
> +
>   if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS) &&
>       WARN_ON(!curve25519_selftest()))
>   return -ENODEV;
> 
> Making the various lib/crypto/* modules return EOPNOTSUPP will in turn
> mean that wireguard will refuse to load, due to !fips_enabled. It has
> the positive effect that all other things that use it will also be
> EOPNOTSUPP.
> 
> For example, what are you doing about big_key.c? Right now, I assume
> nothing. But this way, you'd get all of the various effects for free.
> Are you going to continuously audit all uses of non-FIPS crypto and
> add `if (!fips_enabled)` to every new use case, always, everywhere,
> from now into the boundless future? By adding `if (!fips_enabled)` to
> wireguard, that's what you're signing yourself up for. Instead, by
> restricting the lib/crypto/* modules to !fips_enabled, you can get all
> of those transitive effects without having to do anything additional.

I guess that moving the fips check down at the algorithms level is a
valid option. There are some cases that will be a little iffy though,
like when only certain key sizes cannot be accepted, but for the
wireguard case it would be clean.

> Alternatively, I agree with Eric - why not just consider this outside
> your boundary?

For certification purposes wireguard is not part of the module boundary
(speaking only for my company in this case).

But that is not the issue.

There is an expectation by customers that, when the kernel is in fips
mode, non-approved cryptography is not used (given those customers are
required by law/regulation to use only approved/certified
cryptography).
So we still have a strong desire, where possible, to not allow the
kernel to use non-certified cryptography, regardless of what is the
crypto module boundary (we do the same in user space).

HTH,
Simo.

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Hangbin Liu]

On Thu, Apr 08, 2021 at 03:55:59PM -0600, Jason A. Donenfeld wrote:
> On Thu, Apr 8, 2021 at 7:55 AM Simo Sorce <simo@redhat.com> wrote:
> > > I'm not sure this makes so much sense to do _in wireguard_. If you
> > > feel like the FIPS-allergic part is actually blake, 25519, chacha, and
> > > poly1305, then wouldn't it make most sense to disable _those_ modules
> > > instead? And then the various things that rely on those (such as
> > > wireguard, but maybe there are other things too, like
> > > security/keys/big_key.c) would be naturally disabled transitively?
> >
> > The reason why it is better to disable the whole module is that it
> > provide much better feedback to users. Letting init go through and then
> > just fail operations once someone tries to use it is much harder to
> > deal with in terms of figuring out what went wrong.
> > Also wireguard seem to be poking directly into the algorithms
> > implementations and not use the crypto API, so disabling individual
> > algorithm via the regular fips_enabled mechanism at runtime doesn't
> > work.
> 
> What I'm suggesting _would_ work in basically the exact same way as
> this patch. Namely, something like:

Hi Jason,

I agree that the best way is to disable the crypto modules in FIPS mode.
But the code in lib/crypto looks not the same with crypto/. For modules
in crypto, there is an alg_test() to check if the crytpo is FIPS allowed
when do register.

- crypto_register_alg()
  - crypto_wait_for_test()
    - crypto_probing_notify(CRYPTO_MSG_ALG_REGISTER, larval->adult)
      - cryptomgr_schedule_test()
        - cryptomgr_test()
          - alg_test()

But in lib/crypto the code are more like a library. We can call it anytime
and there is no register. Maybe we should add a similar check in lib/crypto.
But I'm not familiar with crypto code... Not sure if anyone in linux-crypto@
would like help do that.

> 
> diff --git a/lib/crypto/curve25519.c b/lib/crypto/curve25519.c
> index 288a62cd29b2..b794f49c291a 100644
> --- a/lib/crypto/curve25519.c
> +++ b/lib/crypto/curve25519.c
> @@ -12,11 +12,15 @@
>  #include <crypto/curve25519.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> +#include <linux/fips.h>
> 
>  bool curve25519_selftest(void);
> 
>  static int __init mod_init(void)
>  {
> + if (!fips_enabled)
> + return -EOPNOTSUPP;

Question here, why it is !fips_enabled? Shouldn't we return error when
fips_enabled?

Thanks
Hangbin

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Jason A. Donenfeld]

Hi Hangbin,

On Thu, Apr 8, 2021 at 8:41 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
> I agree that the best way is to disable the crypto modules in FIPS mode.
> But the code in lib/crypto looks not the same with crypto/. For modules
> in crypto, there is an alg_test() to check if the crytpo is FIPS allowed
> when do register.
>
> - crypto_register_alg()
>   - crypto_wait_for_test()
>     - crypto_probing_notify(CRYPTO_MSG_ALG_REGISTER, larval->adult)
>       - cryptomgr_schedule_test()
>         - cryptomgr_test()
>           - alg_test()
>
> But in lib/crypto the code are more like a library. We can call it anytime
> and there is no register. Maybe we should add a similar check in lib/crypto.
> But I'm not familiar with crypto code... Not sure if anyone in linux-crypto@
> would like help do that.

Since it's just a normal module library, you can simply do this in the
module_init function, rather than deep within registration
abstractions.

> > diff --git a/lib/crypto/curve25519.c b/lib/crypto/curve25519.c
> > index 288a62cd29b2..b794f49c291a 100644
> > --- a/lib/crypto/curve25519.c
> > +++ b/lib/crypto/curve25519.c
> > @@ -12,11 +12,15 @@
> >  #include <crypto/curve25519.h>
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> > +#include <linux/fips.h>
> >
> >  bool curve25519_selftest(void);
> >
> >  static int __init mod_init(void)
> >  {
> > + if (!fips_enabled)
> > + return -EOPNOTSUPP;
>
> Question here, why it is !fips_enabled? Shouldn't we return error when
> fips_enabled?

Er, just not thinking straight today. `if (fips_enabled)` is probably
what you want indeed.

Jason

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Hangbin Liu]

On Thu, Apr 08, 2021 at 08:44:35PM -0600, Jason A. Donenfeld wrote:
> Since it's just a normal module library, you can simply do this in the
> module_init function, rather than deep within registration
> abstractions.

I did a try but looks it's not that simple. Not sure if it's because wireguard
calls the library directly. Need to check more...

Hangbin

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Jason A. Donenfeld]

On Fri, Apr 09, 2021 at 10:49:07AM +0800, Hangbin Liu wrote:
> On Thu, Apr 08, 2021 at 08:44:35PM -0600, Jason A. Donenfeld wrote:
> > Since it's just a normal module library, you can simply do this in the
> > module_init function, rather than deep within registration
> > abstractions.
> 
> I did a try but looks it's not that simple. Not sure if it's because wireguard
> calls the library directly. Need to check more...

Something like the below should work...

diff --git a/arch/arm/crypto/chacha-glue.c b/arch/arm/crypto/chacha-glue.c
index a408f4bcfd62..47212f9421c1 100644
--- a/arch/arm/crypto/chacha-glue.c
+++ b/arch/arm/crypto/chacha-glue.c
@@ -14,6 +14,7 @@
 #include <linux/jump_label.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/fips.h>

 #include <asm/cputype.h>
 #include <asm/hwcap.h>
@@ -297,6 +298,9 @@ static int __init chacha_simd_mod_init(void)
 {
 	int err = 0;

+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (IS_REACHABLE(CONFIG_CRYPTO_BLKCIPHER)) {
 		err = crypto_register_skciphers(arm_algs, ARRAY_SIZE(arm_algs));
 		if (err)
diff --git a/arch/arm/crypto/curve25519-glue.c b/arch/arm/crypto/curve25519-glue.c
index 31eb75b6002f..d03f810fdaf3 100644
--- a/arch/arm/crypto/curve25519-glue.c
+++ b/arch/arm/crypto/curve25519-glue.c
@@ -14,6 +14,7 @@
 #include <crypto/internal/simd.h>
 #include <linux/types.h>
 #include <linux/module.h>
+#include <linux/fips.h>
 #include <linux/init.h>
 #include <linux/jump_label.h>
 #include <linux/scatterlist.h>
@@ -114,6 +115,9 @@ static struct kpp_alg curve25519_alg = {

 static int __init mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (elf_hwcap & HWCAP_NEON) {
 		static_branch_enable(&have_neon);
 		return IS_REACHABLE(CONFIG_CRYPTO_KPP) ?
diff --git a/arch/arm/crypto/poly1305-glue.c b/arch/arm/crypto/poly1305-glue.c
index 3023c1acfa19..30d6c6de7a27 100644
--- a/arch/arm/crypto/poly1305-glue.c
+++ b/arch/arm/crypto/poly1305-glue.c
@@ -17,6 +17,7 @@
 #include <linux/crypto.h>
 #include <linux/jump_label.h>
 #include <linux/module.h>
+#include <linux/fips.h>

 void poly1305_init_arm(void *state, const u8 *key);
 void poly1305_blocks_arm(void *state, const u8 *src, u32 len, u32 hibit);
@@ -240,6 +241,9 @@ static struct shash_alg arm_poly1305_algs[] = {{

 static int __init arm_poly1305_mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON) &&
 	    (elf_hwcap & HWCAP_NEON))
 		static_branch_enable(&have_neon);
diff --git a/arch/arm64/crypto/chacha-neon-glue.c b/arch/arm64/crypto/chacha-neon-glue.c
index 1d9824c4ae43..1696993326b5 100644
--- a/arch/arm64/crypto/chacha-neon-glue.c
+++ b/arch/arm64/crypto/chacha-neon-glue.c
@@ -26,6 +26,7 @@
 #include <linux/jump_label.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/fips.h>

 #include <asm/hwcap.h>
 #include <asm/neon.h>
@@ -214,6 +215,9 @@ static struct skcipher_alg algs[] = {

 static int __init chacha_simd_mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (!cpu_have_named_feature(ASIMD))
 		return 0;

diff --git a/arch/arm64/crypto/poly1305-glue.c b/arch/arm64/crypto/poly1305-glue.c
index f33ada70c4ed..ac257a52be4d 100644
--- a/arch/arm64/crypto/poly1305-glue.c
+++ b/arch/arm64/crypto/poly1305-glue.c
@@ -17,6 +17,7 @@
 #include <linux/crypto.h>
 #include <linux/jump_label.h>
 #include <linux/module.h>
+#include <linux/fips.h>

 asmlinkage void poly1305_init_arm64(void *state, const u8 *key);
 asmlinkage void poly1305_blocks(void *state, const u8 *src, u32 len, u32 hibit);
@@ -208,6 +209,9 @@ static struct shash_alg neon_poly1305_alg = {

 static int __init neon_poly1305_mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (!cpu_have_named_feature(ASIMD))
 		return 0;

diff --git a/arch/mips/crypto/chacha-glue.c b/arch/mips/crypto/chacha-glue.c
index 90896029d0cd..31f8294f2a31 100644
--- a/arch/mips/crypto/chacha-glue.c
+++ b/arch/mips/crypto/chacha-glue.c
@@ -12,6 +12,7 @@
 #include <crypto/internal/skcipher.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/fips.h>

 asmlinkage void chacha_crypt_arch(u32 *state, u8 *dst, const u8 *src,
 				  unsigned int bytes, int nrounds);
@@ -128,6 +129,9 @@ static struct skcipher_alg algs[] = {

 static int __init chacha_simd_mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	return IS_REACHABLE(CONFIG_CRYPTO_BLKCIPHER) ?
 		crypto_register_skciphers(algs, ARRAY_SIZE(algs)) : 0;
 }
diff --git a/arch/mips/crypto/poly1305-glue.c b/arch/mips/crypto/poly1305-glue.c
index fc881b46d911..f5edec10cef8 100644
--- a/arch/mips/crypto/poly1305-glue.c
+++ b/arch/mips/crypto/poly1305-glue.c
@@ -12,6 +12,7 @@
 #include <linux/cpufeature.h>
 #include <linux/crypto.h>
 #include <linux/module.h>
+#include <linux/fips.h>

 asmlinkage void poly1305_init_mips(void *state, const u8 *key);
 asmlinkage void poly1305_blocks_mips(void *state, const u8 *src, u32 len, u32 hibit);
@@ -173,6 +174,9 @@ static struct shash_alg mips_poly1305_alg = {

 static int __init mips_poly1305_mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	return IS_REACHABLE(CONFIG_CRYPTO_HASH) ?
 		crypto_register_shash(&mips_poly1305_alg) : 0;
 }
diff --git a/arch/x86/crypto/blake2s-glue.c b/arch/x86/crypto/blake2s-glue.c
index 94ac5bdd9f6f..968762fcc8b2 100644
--- a/arch/x86/crypto/blake2s-glue.c
+++ b/arch/x86/crypto/blake2s-glue.c
@@ -11,6 +11,7 @@
 #include <linux/jump_label.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/fips.h>

 #include <asm/cpufeature.h>
 #include <asm/fpu/api.h>
@@ -194,6 +195,9 @@ static struct shash_alg blake2s_algs[] = {{

 static int __init blake2s_mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (!boot_cpu_has(X86_FEATURE_SSSE3))
 		return 0;

diff --git a/arch/x86/crypto/chacha_glue.c b/arch/x86/crypto/chacha_glue.c
index 4c4dc64398cb..15e6cd084598 100644
--- a/arch/x86/crypto/chacha_glue.c
+++ b/arch/x86/crypto/chacha_glue.c
@@ -12,6 +12,7 @@
 #include <crypto/internal/skcipher.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/fips.h>
 #include <asm/simd.h>

 asmlinkage void chacha_block_xor_ssse3(u32 *state, u8 *dst, const u8 *src,
@@ -278,6 +279,9 @@ static struct skcipher_alg algs[] = {

 static int __init chacha_simd_mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (!boot_cpu_has(X86_FEATURE_SSSE3))
 		return 0;

diff --git a/arch/x86/crypto/curve25519-x86_64.c b/arch/x86/crypto/curve25519-x86_64.c
index a9edb6f8a0ba..b840c7e49aa1 100644
--- a/arch/x86/crypto/curve25519-x86_64.c
+++ b/arch/x86/crypto/curve25519-x86_64.c
@@ -11,6 +11,7 @@
 #include <linux/jump_label.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/fips.h>

 #include <asm/cpufeature.h>
 #include <asm/processor.h>
@@ -1488,6 +1489,9 @@ static struct kpp_alg curve25519_alg = {

 static int __init curve25519_mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (boot_cpu_has(X86_FEATURE_BMI2) && boot_cpu_has(X86_FEATURE_ADX))
 		static_branch_enable(&curve25519_use_bmi2_adx);
 	else
diff --git a/arch/x86/crypto/poly1305_glue.c b/arch/x86/crypto/poly1305_glue.c
index b69e362730d0..eb1940c74c7b 100644
--- a/arch/x86/crypto/poly1305_glue.c
+++ b/arch/x86/crypto/poly1305_glue.c
@@ -11,6 +11,7 @@
 #include <linux/jump_label.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/fips.h>
 #include <asm/intel-family.h>
 #include <asm/simd.h>

@@ -258,6 +259,9 @@ static struct shash_alg alg = {

 static int __init poly1305_simd_mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (IS_ENABLED(CONFIG_AS_AVX) && boot_cpu_has(X86_FEATURE_AVX) &&
 	    cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, NULL))
 		static_branch_enable(&poly1305_use_avx);
diff --git a/lib/crypto/blake2s.c b/lib/crypto/blake2s.c
index 41025a30c524..8d244eeb277e 100644
--- a/lib/crypto/blake2s.c
+++ b/lib/crypto/blake2s.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/bug.h>
+#include <linux/fips.h>
 #include <asm/unaligned.h>

 bool blake2s_selftest(void);
@@ -109,6 +110,9 @@ EXPORT_SYMBOL(blake2s256_hmac);

 static int __init mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS) &&
 	    WARN_ON(!blake2s_selftest()))
 		return -ENODEV;
diff --git a/lib/crypto/chacha.c b/lib/crypto/chacha.c
index 65ead6b0c7e0..4f0087717faf 100644
--- a/lib/crypto/chacha.c
+++ b/lib/crypto/chacha.c
@@ -11,6 +11,9 @@
 #include <linux/bitops.h>
 #include <linux/string.h>
 #include <linux/cryptohash.h>
+#include <linux/fips.h>
+#include <linux/errno.h>
+#include <linux/module.h>
 #include <asm/unaligned.h>
 #include <crypto/chacha.h>

@@ -113,3 +116,12 @@ void hchacha_block_generic(const u32 *state, u32 *stream, int nrounds)
 	memcpy(&stream[4], &x[12], 16);
 }
 EXPORT_SYMBOL(hchacha_block_generic);
+
+static int __init mod_init(void)
+{
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+	return 0;
+}
+
+module_init(mod_init);
diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c
index 1fec56e5dd51..d19278c5813d 100644
--- a/lib/crypto/chacha20poly1305.c
+++ b/lib/crypto/chacha20poly1305.c
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/fips.h>

 #define CHACHA_KEY_WORDS	(CHACHA_KEY_SIZE / sizeof(u32))

@@ -358,6 +359,9 @@ EXPORT_SYMBOL(chacha20poly1305_decrypt_sg_inplace);

 static int __init mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS) &&
 	    WARN_ON(!chacha20poly1305_selftest()))
 		return -ENODEV;
diff --git a/lib/crypto/curve25519.c b/lib/crypto/curve25519.c
index 288a62cd29b2..f759d49b0b57 100644
--- a/lib/crypto/curve25519.c
+++ b/lib/crypto/curve25519.c
@@ -12,11 +12,15 @@
 #include <crypto/curve25519.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/fips.h>

 bool curve25519_selftest(void);

 static int __init mod_init(void)
 {
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+
 	if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS) &&
 	    WARN_ON(!curve25519_selftest()))
 		return -ENODEV;
diff --git a/lib/crypto/poly1305.c b/lib/crypto/poly1305.c
index 9d2d14df0fee..ae4255957d31 100644
--- a/lib/crypto/poly1305.c
+++ b/lib/crypto/poly1305.c
@@ -10,6 +10,7 @@
 #include <crypto/internal/poly1305.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/fips.h>
 #include <asm/unaligned.h>

 void poly1305_init_generic(struct poly1305_desc_ctx *desc, const u8 *key)
@@ -73,5 +74,14 @@ void poly1305_final_generic(struct poly1305_desc_ctx *desc, u8 *dst)
 }
 EXPORT_SYMBOL_GPL(poly1305_final_generic);

+static int __init mod_init(void)
+{
+	if (fips_enabled)
+		return -EOPNOTSUPP;
+	return 0;
+}
+
+module_init(mod_init);
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Martin Willi <martin@strongswan.org>");

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Ard Biesheuvel]

On Fri, 9 Apr 2021 at 05:03, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Fri, Apr 09, 2021 at 10:49:07AM +0800, Hangbin Liu wrote:
> > On Thu, Apr 08, 2021 at 08:44:35PM -0600, Jason A. Donenfeld wrote:
> > > Since it's just a normal module library, you can simply do this in the
> > > module_init function, rather than deep within registration
> > > abstractions.
> >
> > I did a try but looks it's not that simple. Not sure if it's because wireguard
> > calls the library directly. Need to check more...
>
> Something like the below should work...
>

The below only works if all the code is modular. initcall return
values are ignored for builtin code, and so the library functions will
happily work regardless of fips_enabled, and there is generally no
guarantee that no library calls can be made before the initcall() is
invoked.

For ordinary crypto API client code, the algorithm in question may be
an a priori unknown, and so the only sensible place to put this check
is where the algorithms are registered or instantiated.

For code such as WireGuard that is hardwired to use a single set of
(forbidden! :-)) algorithms via library calls, the simplest way to do
this securely is to disable the whole thing, even though I agree it is
not the most elegant solution.

If we go with Jason's approach, we would need to mandate each of these
drivers can only be built as a module if the kernel is built with
FIPS-200 support. This is rather trivial by itself, i.e.,

  depends on m || !CRYPTO_FIPS

but I am a bit concerned that the rather intricate kconfig
dependencies between the generic and arch-optimized versions of those
drivers get complicated even further.

-- 
Ard.






> diff --git a/arch/arm/crypto/chacha-glue.c b/arch/arm/crypto/chacha-glue.c
> index a408f4bcfd62..47212f9421c1 100644
> --- a/arch/arm/crypto/chacha-glue.c
> +++ b/arch/arm/crypto/chacha-glue.c
> @@ -14,6 +14,7 @@
>  #include <linux/jump_label.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>
>  #include <asm/cputype.h>
>  #include <asm/hwcap.h>
> @@ -297,6 +298,9 @@ static int __init chacha_simd_mod_init(void)
>  {
>         int err = 0;
>
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (IS_REACHABLE(CONFIG_CRYPTO_BLKCIPHER)) {
>                 err = crypto_register_skciphers(arm_algs, ARRAY_SIZE(arm_algs));
>                 if (err)
> diff --git a/arch/arm/crypto/curve25519-glue.c b/arch/arm/crypto/curve25519-glue.c
> index 31eb75b6002f..d03f810fdaf3 100644
> --- a/arch/arm/crypto/curve25519-glue.c
> +++ b/arch/arm/crypto/curve25519-glue.c
> @@ -14,6 +14,7 @@
>  #include <crypto/internal/simd.h>
>  #include <linux/types.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>  #include <linux/init.h>
>  #include <linux/jump_label.h>
>  #include <linux/scatterlist.h>
> @@ -114,6 +115,9 @@ static struct kpp_alg curve25519_alg = {
>
>  static int __init mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (elf_hwcap & HWCAP_NEON) {
>                 static_branch_enable(&have_neon);
>                 return IS_REACHABLE(CONFIG_CRYPTO_KPP) ?
> diff --git a/arch/arm/crypto/poly1305-glue.c b/arch/arm/crypto/poly1305-glue.c
> index 3023c1acfa19..30d6c6de7a27 100644
> --- a/arch/arm/crypto/poly1305-glue.c
> +++ b/arch/arm/crypto/poly1305-glue.c
> @@ -17,6 +17,7 @@
>  #include <linux/crypto.h>
>  #include <linux/jump_label.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>
>  void poly1305_init_arm(void *state, const u8 *key);
>  void poly1305_blocks_arm(void *state, const u8 *src, u32 len, u32 hibit);
> @@ -240,6 +241,9 @@ static struct shash_alg arm_poly1305_algs[] = {{
>
>  static int __init arm_poly1305_mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (IS_ENABLED(CONFIG_KERNEL_MODE_NEON) &&
>             (elf_hwcap & HWCAP_NEON))
>                 static_branch_enable(&have_neon);
> diff --git a/arch/arm64/crypto/chacha-neon-glue.c b/arch/arm64/crypto/chacha-neon-glue.c
> index 1d9824c4ae43..1696993326b5 100644
> --- a/arch/arm64/crypto/chacha-neon-glue.c
> +++ b/arch/arm64/crypto/chacha-neon-glue.c
> @@ -26,6 +26,7 @@
>  #include <linux/jump_label.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>
>  #include <asm/hwcap.h>
>  #include <asm/neon.h>
> @@ -214,6 +215,9 @@ static struct skcipher_alg algs[] = {
>
>  static int __init chacha_simd_mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (!cpu_have_named_feature(ASIMD))
>                 return 0;
>
> diff --git a/arch/arm64/crypto/poly1305-glue.c b/arch/arm64/crypto/poly1305-glue.c
> index f33ada70c4ed..ac257a52be4d 100644
> --- a/arch/arm64/crypto/poly1305-glue.c
> +++ b/arch/arm64/crypto/poly1305-glue.c
> @@ -17,6 +17,7 @@
>  #include <linux/crypto.h>
>  #include <linux/jump_label.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>
>  asmlinkage void poly1305_init_arm64(void *state, const u8 *key);
>  asmlinkage void poly1305_blocks(void *state, const u8 *src, u32 len, u32 hibit);
> @@ -208,6 +209,9 @@ static struct shash_alg neon_poly1305_alg = {
>
>  static int __init neon_poly1305_mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (!cpu_have_named_feature(ASIMD))
>                 return 0;
>
> diff --git a/arch/mips/crypto/chacha-glue.c b/arch/mips/crypto/chacha-glue.c
> index 90896029d0cd..31f8294f2a31 100644
> --- a/arch/mips/crypto/chacha-glue.c
> +++ b/arch/mips/crypto/chacha-glue.c
> @@ -12,6 +12,7 @@
>  #include <crypto/internal/skcipher.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>
>  asmlinkage void chacha_crypt_arch(u32 *state, u8 *dst, const u8 *src,
>                                   unsigned int bytes, int nrounds);
> @@ -128,6 +129,9 @@ static struct skcipher_alg algs[] = {
>
>  static int __init chacha_simd_mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         return IS_REACHABLE(CONFIG_CRYPTO_BLKCIPHER) ?
>                 crypto_register_skciphers(algs, ARRAY_SIZE(algs)) : 0;
>  }
> diff --git a/arch/mips/crypto/poly1305-glue.c b/arch/mips/crypto/poly1305-glue.c
> index fc881b46d911..f5edec10cef8 100644
> --- a/arch/mips/crypto/poly1305-glue.c
> +++ b/arch/mips/crypto/poly1305-glue.c
> @@ -12,6 +12,7 @@
>  #include <linux/cpufeature.h>
>  #include <linux/crypto.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>
>  asmlinkage void poly1305_init_mips(void *state, const u8 *key);
>  asmlinkage void poly1305_blocks_mips(void *state, const u8 *src, u32 len, u32 hibit);
> @@ -173,6 +174,9 @@ static struct shash_alg mips_poly1305_alg = {
>
>  static int __init mips_poly1305_mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         return IS_REACHABLE(CONFIG_CRYPTO_HASH) ?
>                 crypto_register_shash(&mips_poly1305_alg) : 0;
>  }
> diff --git a/arch/x86/crypto/blake2s-glue.c b/arch/x86/crypto/blake2s-glue.c
> index 94ac5bdd9f6f..968762fcc8b2 100644
> --- a/arch/x86/crypto/blake2s-glue.c
> +++ b/arch/x86/crypto/blake2s-glue.c
> @@ -11,6 +11,7 @@
>  #include <linux/jump_label.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>
>  #include <asm/cpufeature.h>
>  #include <asm/fpu/api.h>
> @@ -194,6 +195,9 @@ static struct shash_alg blake2s_algs[] = {{
>
>  static int __init blake2s_mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (!boot_cpu_has(X86_FEATURE_SSSE3))
>                 return 0;
>
> diff --git a/arch/x86/crypto/chacha_glue.c b/arch/x86/crypto/chacha_glue.c
> index 4c4dc64398cb..15e6cd084598 100644
> --- a/arch/x86/crypto/chacha_glue.c
> +++ b/arch/x86/crypto/chacha_glue.c
> @@ -12,6 +12,7 @@
>  #include <crypto/internal/skcipher.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>  #include <asm/simd.h>
>
>  asmlinkage void chacha_block_xor_ssse3(u32 *state, u8 *dst, const u8 *src,
> @@ -278,6 +279,9 @@ static struct skcipher_alg algs[] = {
>
>  static int __init chacha_simd_mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (!boot_cpu_has(X86_FEATURE_SSSE3))
>                 return 0;
>
> diff --git a/arch/x86/crypto/curve25519-x86_64.c b/arch/x86/crypto/curve25519-x86_64.c
> index a9edb6f8a0ba..b840c7e49aa1 100644
> --- a/arch/x86/crypto/curve25519-x86_64.c
> +++ b/arch/x86/crypto/curve25519-x86_64.c
> @@ -11,6 +11,7 @@
>  #include <linux/jump_label.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>
>  #include <asm/cpufeature.h>
>  #include <asm/processor.h>
> @@ -1488,6 +1489,9 @@ static struct kpp_alg curve25519_alg = {
>
>  static int __init curve25519_mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (boot_cpu_has(X86_FEATURE_BMI2) && boot_cpu_has(X86_FEATURE_ADX))
>                 static_branch_enable(&curve25519_use_bmi2_adx);
>         else
> diff --git a/arch/x86/crypto/poly1305_glue.c b/arch/x86/crypto/poly1305_glue.c
> index b69e362730d0..eb1940c74c7b 100644
> --- a/arch/x86/crypto/poly1305_glue.c
> +++ b/arch/x86/crypto/poly1305_glue.c
> @@ -11,6 +11,7 @@
>  #include <linux/jump_label.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>  #include <asm/intel-family.h>
>  #include <asm/simd.h>
>
> @@ -258,6 +259,9 @@ static struct shash_alg alg = {
>
>  static int __init poly1305_simd_mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (IS_ENABLED(CONFIG_AS_AVX) && boot_cpu_has(X86_FEATURE_AVX) &&
>             cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, NULL))
>                 static_branch_enable(&poly1305_use_avx);
> diff --git a/lib/crypto/blake2s.c b/lib/crypto/blake2s.c
> index 41025a30c524..8d244eeb277e 100644
> --- a/lib/crypto/blake2s.c
> +++ b/lib/crypto/blake2s.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/bug.h>
> +#include <linux/fips.h>
>  #include <asm/unaligned.h>
>
>  bool blake2s_selftest(void);
> @@ -109,6 +110,9 @@ EXPORT_SYMBOL(blake2s256_hmac);
>
>  static int __init mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS) &&
>             WARN_ON(!blake2s_selftest()))
>                 return -ENODEV;
> diff --git a/lib/crypto/chacha.c b/lib/crypto/chacha.c
> index 65ead6b0c7e0..4f0087717faf 100644
> --- a/lib/crypto/chacha.c
> +++ b/lib/crypto/chacha.c
> @@ -11,6 +11,9 @@
>  #include <linux/bitops.h>
>  #include <linux/string.h>
>  #include <linux/cryptohash.h>
> +#include <linux/fips.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
>  #include <asm/unaligned.h>
>  #include <crypto/chacha.h>
>
> @@ -113,3 +116,12 @@ void hchacha_block_generic(const u32 *state, u32 *stream, int nrounds)
>         memcpy(&stream[4], &x[12], 16);
>  }
>  EXPORT_SYMBOL(hchacha_block_generic);
> +
> +static int __init mod_init(void)
> +{
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +       return 0;
> +}
> +
> +module_init(mod_init);
> diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c
> index 1fec56e5dd51..d19278c5813d 100644
> --- a/lib/crypto/chacha20poly1305.c
> +++ b/lib/crypto/chacha20poly1305.c
> @@ -18,6 +18,7 @@
>  #include <linux/init.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>
>  #define CHACHA_KEY_WORDS       (CHACHA_KEY_SIZE / sizeof(u32))
>
> @@ -358,6 +359,9 @@ EXPORT_SYMBOL(chacha20poly1305_decrypt_sg_inplace);
>
>  static int __init mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS) &&
>             WARN_ON(!chacha20poly1305_selftest()))
>                 return -ENODEV;
> diff --git a/lib/crypto/curve25519.c b/lib/crypto/curve25519.c
> index 288a62cd29b2..f759d49b0b57 100644
> --- a/lib/crypto/curve25519.c
> +++ b/lib/crypto/curve25519.c
> @@ -12,11 +12,15 @@
>  #include <crypto/curve25519.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> +#include <linux/fips.h>
>
>  bool curve25519_selftest(void);
>
>  static int __init mod_init(void)
>  {
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +
>         if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS) &&
>             WARN_ON(!curve25519_selftest()))
>                 return -ENODEV;
> diff --git a/lib/crypto/poly1305.c b/lib/crypto/poly1305.c
> index 9d2d14df0fee..ae4255957d31 100644
> --- a/lib/crypto/poly1305.c
> +++ b/lib/crypto/poly1305.c
> @@ -10,6 +10,7 @@
>  #include <crypto/internal/poly1305.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/fips.h>
>  #include <asm/unaligned.h>
>
>  void poly1305_init_generic(struct poly1305_desc_ctx *desc, const u8 *key)
> @@ -73,5 +74,14 @@ void poly1305_final_generic(struct poly1305_desc_ctx *desc, u8 *dst)
>  }
>  EXPORT_SYMBOL_GPL(poly1305_final_generic);
>
> +static int __init mod_init(void)
> +{
> +       if (fips_enabled)
> +               return -EOPNOTSUPP;
> +       return 0;
> +}
> +
> +module_init(mod_init);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Martin Willi <martin@strongswan.org>");
>
>

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Simo Sorce]

On Fri, 2021-04-09 at 08:02 +0200, Ard Biesheuvel wrote:
> On Fri, 9 Apr 2021 at 05:03, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > On Fri, Apr 09, 2021 at 10:49:07AM +0800, Hangbin Liu wrote:
> > > On Thu, Apr 08, 2021 at 08:44:35PM -0600, Jason A. Donenfeld wrote:
> > > > Since it's just a normal module library, you can simply do this in the
> > > > module_init function, rather than deep within registration
> > > > abstractions.
> > > 
> > > I did a try but looks it's not that simple. Not sure if it's because wireguard
> > > calls the library directly. Need to check more...
> > 
> > Something like the below should work...
> > 
> 
> The below only works if all the code is modular. initcall return
> values are ignored for builtin code, and so the library functions will
> happily work regardless of fips_enabled, and there is generally no
> guarantee that no library calls can be made before the initcall() is
> invoked.
> 
> For ordinary crypto API client code, the algorithm in question may be
> an a priori unknown, and so the only sensible place to put this check
> is where the algorithms are registered or instantiated.
> 
> For code such as WireGuard that is hardwired to use a single set of
> (forbidden! :-)) algorithms via library calls, the simplest way to do
> this securely is to disable the whole thing, even though I agree it is
> not the most elegant solution.
> 
> If we go with Jason's approach, we would need to mandate each of these
> drivers can only be built as a module if the kernel is built with
> FIPS-200 support. This is rather trivial by itself, i.e.,
> 
>   depends on m || !CRYPTO_FIPS
> 
> but I am a bit concerned that the rather intricate kconfig
> dependencies between the generic and arch-optimized versions of those
> drivers get complicated even further.

Actually this is the opposite direction we are planning to go for
future fips certifications.

Due to requirements about crypto module naming and versioning in the
new FIPS-140-3 standard we are planning to always build all the CRYPTO
as bultin (and maybe even forbid loading additional crypto modules in
FIPS mode). This is clearly just a vendor choice and has no bearing on
what upstream ultimately will do, but just throwing it here as a data
point.

Plus, as you note, it would overly complicate the interfaces.

As much as the check in wireguard is inelegant, it is much simpler to
understand and is not invasive.

Simo.

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Jason A. Donenfeld]

On Fri, Apr 9, 2021 at 6:47 AM Simo Sorce <simo@redhat.com> wrote:
> >   depends on m || !CRYPTO_FIPS
> >
> > but I am a bit concerned that the rather intricate kconfig
> > dependencies between the generic and arch-optimized versions of those
> > drivers get complicated even further.
>
> Actually this is the opposite direction we are planning to go for
> future fips certifications.
>
> Due to requirements about crypto module naming and versioning in the
> new FIPS-140-3 standard we are planning to always build all the CRYPTO
> as bultin (and maybe even forbid loading additional crypto modules in
> FIPS mode). This is clearly just a vendor choice and has no bearing on
> what upstream ultimately will do, but just throwing it here as a data
> point.

I'm wondering: do you intend to apply similar patches to all the other
uses of "non-FIPS-certified" crypto in the kernel? I've already
brought up big_key.c, for example. Also if you're intent on adding
this check to WireGuard, because it tunnels packets without using
FIPS-certified crypto primitives, do you also plan on adding this
check to other network tunnels that don't tunnel packets using
FIPS-certified crypto primitives? For example, GRE, VXLAN, GENEVE? I'd
be inclined to take this patch more seriously if it was exhaustive and
coherent for your use case. The targeted hit on WireGuard seems
incoherent as a standalone patch, making it hard to even evaluate. So
I think either you should send an exhaustive patch series that forbids
all use of non-FIPS crypto anywhere in the kernel (another example:
net/core/secure_seq.c) in addition to all tunneling modules that don't
use FIPS-certified crypto, or figure out how to disable the lib/crypto
primitives that you want to be disabled in "fips mode". With a
coherent patchset for either of these, we can then evaluate it.

Jason

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Simo Sorce]

On Fri, 2021-04-09 at 12:36 -0600, Jason A. Donenfeld wrote:
> On Fri, Apr 9, 2021 at 6:47 AM Simo Sorce <simo@redhat.com> wrote:
> > >   depends on m || !CRYPTO_FIPS
> > > 
> > > but I am a bit concerned that the rather intricate kconfig
> > > dependencies between the generic and arch-optimized versions of those
> > > drivers get complicated even further.
> > 
> > Actually this is the opposite direction we are planning to go for
> > future fips certifications.
> > 
> > Due to requirements about crypto module naming and versioning in the
> > new FIPS-140-3 standard we are planning to always build all the CRYPTO
> > as bultin (and maybe even forbid loading additional crypto modules in
> > FIPS mode). This is clearly just a vendor choice and has no bearing on
> > what upstream ultimately will do, but just throwing it here as a data
> > point.
> 
> I'm wondering: do you intend to apply similar patches to all the other
> uses of "non-FIPS-certified" crypto in the kernel? I've already
> brought up big_key.c, for example. Also if you're intent on adding
> this check to WireGuard, because it tunnels packets without using
> FIPS-certified crypto primitives, do you also plan on adding this
> check to other network tunnels that don't tunnel packets using
> FIPS-certified crypto primitives? For example, GRE, VXLAN, GENEVE? I'd
> be inclined to take this patch more seriously if it was exhaustive and
> coherent for your use case. The targeted hit on WireGuard seems
> incoherent as a standalone patch, making it hard to even evaluate.

Hi Jason,
I can't speak for Hangbin, we do not work for the same company and I
was not aware of his efforts until this patch landed.
For my part we were already looking at big_key, wireguard and other
areas internally, but were not thinking of sending upstream patches
like these w/o first a good assessment with our teams and lab that they
were proper and sufficient.

>  So
> I think either you should send an exhaustive patch series that forbids
> all use of non-FIPS crypto anywhere in the kernel (another example:
> net/core/secure_seq.c) in addition to all tunneling modules that don't
> use FIPS-certified crypto, or figure out how to disable the lib/crypto
> primitives that you want to be disabled in "fips mode". With a
> coherent patchset for either of these, we can then evaluate it.

Yes a cohesive approach would be ideal, but I do not know if pushing
substantially the same checks we have in the Crypto API down to
lib/crypto is the right way to go, I am not oppose but I guess Herbert
would have to chime in here.

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

Re: [PATCH net-next] [RESEND] wireguard: disable in FIPS mode

[Simo Sorce]

On Fri, 2021-04-09 at 14:56 -0400, Simo Sorce wrote:
> Hi Jason,
> I can't speak for Hangbin, we do not work for the same company and I
> was not aware of his efforts until this patch landed.

Turns out I and Hangbin do work for the same company after all.
Left hand is meeting right hand internally now. :-D
The comments still stand of course.

Simo.

> For my part we were already looking at big_key, wireguard and other
> areas internally, but were not thinking of sending upstream patches
> like these w/o first a good assessment with our teams and lab that they
> were proper and sufficient.
> 
> >  So
> > I think either you should send an exhaustive patch series that forbids
> > all use of non-FIPS crypto anywhere in the kernel (another example:
> > net/core/secure_seq.c) in addition to all tunneling modules that don't
> > use FIPS-certified crypto, or figure out how to disable the lib/crypto
> > primitives that you want to be disabled in "fips mode". With a
> > coherent patchset for either of these, we can then evaluate it.
> 
> Yes a cohesive approach would be ideal, but I do not know if pushing
> substantially the same checks we have in the Crypto API down to
> lib/crypto is the right way to go, I am not oppose but I guess Herbert
> would have to chime in here.
> 

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc