[04/12] net/ena: fix build with GCC 12
Checks
Commit Message
GCC 12 raises the following warning:
In file included from ../lib/mempool/rte_mempool.h:46,
from ../lib/mbuf/rte_mbuf.h:38,
from ../lib/net/rte_ether.h:22,
from ../drivers/net/ena/ena_ethdev.h:10,
from ../drivers/net/ena/ena_rss.c:6:
../drivers/net/ena/ena_rss.c: In function ‘ena_rss_key_fill’:
../lib/eal/x86/include/rte_memcpy.h:370:9: warning: array subscript 64 is
outside array bounds of ‘uint8_t[40]’ {aka ‘unsigned char[40]’}
[-Warray-bounds]
370 | rte_mov32((uint8_t *)dst + 2 * 32, (const uint8_t *)src + 2 * 32);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../drivers/net/ena/ena_rss.c:51:24: note: while referencing ‘default_key’
51 | static uint8_t default_key[ENA_HASH_KEY_SIZE];
| ^~~~~~~~~~~
This is a false positive because the copied size is checked against
ENA_HASH_KEY_SIZE in a (build) assert.
Silence this warning by calling memcpy with the minimal size.
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
drivers/net/ena/ena_rss.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
Comments
On Wed, 18 May 2022 12:16:49 +0200
David Marchand <david.marchand@redhat.com> wrote:
> + for (i = 0; i < RTE_DIM(default_key); ++i)
> default_key[i] = rte_rand() & 0xff;
We should have rte_random_bytes() functionality if this gets
used often.
Also, worth considering dropping DPDK random number generator
in userspace for security reasons and just using more secure kernel code.
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, 20 May 2022 22.28
>
> On Wed, 18 May 2022 12:16:49 +0200
> David Marchand <david.marchand@redhat.com> wrote:
>
> > + for (i = 0; i < RTE_DIM(default_key); ++i)
> > default_key[i] = rte_rand() & 0xff;
>
> We should have rte_random_bytes() functionality if this gets
> used often.
Since the other pseudorandom functions are called rand, such a function should be named rte_rand_bytes().
>
> Also, worth considering dropping DPDK random number generator
> in userspace for security reasons and just using more secure kernel
> code.
Absolutely not! We need a fast pseudorandom number generator in DPDK.
If anything, we could consider renaming the functions and header file to reflect that they are pseudorandom number generators, and not (cryptographically) random generators. That would cause an API/ABI breakage, so it's probably not going to happen. ;-)
On Sat, 21 May 2022 11:49:47 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > Also, worth considering dropping DPDK random number generator
> > in userspace for security reasons and just using more secure kernel
> > code.
>
> Absolutely not! We need a fast pseudorandom number generator in DPDK.
>
> If anything, we could consider renaming the functions and header file to reflect that they are pseudorandom number generators, and not (cryptographically) random generators. That would cause an API/ABI breakage, so it's probably not going to happen. ;-)
The Linux kernel has received an way more attention on random numbers than
DPDK. If you follow the history, what happens is that a simple dumb LCG
or similar random number generator gets invented, and then gets used for
lots of things that people don't think need a strong generator.
Followed by DoS and other attacks where the weak random number generator
is broken when used for doing things like creating sequence numbers of
TCP port assignment. This is then followed by even more work on the
kernel random number generator to make the default random number generator
stronger.
I bring up this history, so that DPDK won't have to repeat it.
Right now the DPDK random number generator is insecure because it uses
long but weak PRNG and never reseeds itself.
See:
https://lwn.net/Articles/884875/
There is also FIPS to consider.
https://lwn.net/Articles/877607/
Since random number generators are hard, prefer that someone else do it :-)
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, 21 May 2022 18.24
>
> On Sat, 21 May 2022 11:49:47 +0200
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > >
> > > Also, worth considering dropping DPDK random number generator
> > > in userspace for security reasons and just using more secure kernel
> > > code.
> >
> > Absolutely not! We need a fast pseudorandom number generator in DPDK.
> >
> > If anything, we could consider renaming the functions and header file
> to reflect that they are pseudorandom number generators, and not
> (cryptographically) random generators. That would cause an API/ABI
> breakage, so it's probably not going to happen. ;-)
>
>
> The Linux kernel has received an way more attention on random numbers
> than
> DPDK. If you follow the history, what happens is that a simple dumb LCG
> or similar random number generator gets invented, and then gets used
> for
> lots of things that people don't think need a strong generator.
>
> Followed by DoS and other attacks where the weak random number
> generator
> is broken when used for doing things like creating sequence numbers of
> TCP port assignment. This is then followed by even more work on the
> kernel random number generator to make the default random number
> generator
> stronger.
>
> I bring up this history, so that DPDK won't have to repeat it.
>
> Right now the DPDK random number generator is insecure because it uses
> long but weak PRNG and never reseeds itself.
>
> See:
> https://lwn.net/Articles/884875/
>
> There is also FIPS to consider.
> https://lwn.net/Articles/877607/
>
> Since random number generators are hard, prefer that someone else do it
> :-)
First of all, I would like to thank you for the history lesson and references, Stephen, it made my Saturday evening much more nerdy and interesting than expected! Not being a native English speaker, please understand that I mean this sincerely. I really enjoyed reading about this corner of the Linux kernel history.
Overall, I think that RNGs generally fall into two categories: Unsafe (regardless how advanced) and safe for crypto use.
It should be OK for DPDK to provide something blazing fast, but unsafe. The DPDK documentation clearly states that the provided random functions are not safe for crypto, so I would expect the developers to use them accordingly.
Having thought about it, I came to this conclusion: Regardless if we provide unsafe RNG functions in DPDK or not, it is ultimately up to the application developers to choose which RNG category to use for different purposes. If we don't provide something fast, developers will just use the standard rand48() functions or similar. And a blazing fast (but unsafe) RNG is useful for simple things like pseudo-random packet sampling in the data plane.
Who would have thought that using a simple RNG for TCP port assignment could end up being a security problem... The developers will always have a choice between secure and fast, and the risk of a developer making the wrong decision is not affected by DPDK providing some unsafe RNG or not.
At a higher level, I come to think of the RFCs, which all have a Security Considerations chapter. Ideally, all patches had such a chapter, and all reviews considered the security aspects, so someone would catch the use of an unsafe RNG where a safe RNG should be used. Removing the rand() functions from DPDK will not have the desired effect, only raising security awareness will.
And just to leave off where you left off: I 100 % agree that we should not try to invent our own crypto safe RNG!
PS: I assume that safe RNGs cannot generate numbers at the same rate as unsafe RNGs. If this was not generally true, there should be no need to use unsafe RNGs (except for test purposes, where reproducibility is a requirement).
In cases where the safe RNG can generate numbers at a sufficiently high rate, why not use it? This, however, requires that the application developer knows both the required rate and the rate of the safe RNG, which I guess very few developers do.
On Wed, 18 May 2022 12:16:49 +0200
David Marchand <david.marchand@redhat.com> wrote:
> GCC 12 raises the following warning:
>
> In file included from ../lib/mempool/rte_mempool.h:46,
> from ../lib/mbuf/rte_mbuf.h:38,
> from ../lib/net/rte_ether.h:22,
> from ../drivers/net/ena/ena_ethdev.h:10,
> from ../drivers/net/ena/ena_rss.c:6:
> ../drivers/net/ena/ena_rss.c: In function ‘ena_rss_key_fill’:
> ../lib/eal/x86/include/rte_memcpy.h:370:9: warning: array subscript 64 is
> outside array bounds of ‘uint8_t[40]’ {aka ‘unsigned char[40]’}
> [-Warray-bounds]
> 370 | rte_mov32((uint8_t *)dst + 2 * 32, (const uint8_t *)src + 2 * 32);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../drivers/net/ena/ena_rss.c:51:24: note: while referencing ‘default_key’
> 51 | static uint8_t default_key[ENA_HASH_KEY_SIZE];
> | ^~~~~~~~~~~
>
> This is a false positive because the copied size is checked against
> ENA_HASH_KEY_SIZE in a (build) assert.
> Silence this warning by calling memcpy with the minimal size.
>
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
@@ -51,15 +51,14 @@ void ena_rss_key_fill(void *key, size_t size)
static uint8_t default_key[ENA_HASH_KEY_SIZE];
size_t i;
- RTE_ASSERT(size <= ENA_HASH_KEY_SIZE);
-
if (!key_generated) {
- for (i = 0; i < ENA_HASH_KEY_SIZE; ++i)
+ for (i = 0; i < RTE_DIM(default_key); ++i)
default_key[i] = rte_rand() & 0xff;
key_generated = true;
}
- rte_memcpy(key, default_key, size);
+ RTE_ASSERT(size <= sizeof(default_key));
+ rte_memcpy(key, default_key, RTE_MIN(size, sizeof(default_key)));
}
int ena_rss_reta_update(struct rte_eth_dev *dev,