mbox series

[0/17] crypto: api - Change completion callback argument to void star

Message ID Y+DUkqe1sagWaErA@gondor.apana.org.au
Headers show
Series crypto: api - Change completion callback argument to void star | expand

Message

Herbert Xu Feb. 6, 2023, 10:21 a.m. UTC 
Hi:

The crypto completion function currently takes a pointer to a
struct crypto_async_request object.  However, in reality the API
does not allow the use of any part of the object apart from the
data field.  For example, ahash/shash will create a fake object
on the stack to pass along a different data field.

This leads to potential bugs where the user may try to dereference
or otherwise use the crypto_async_request object.

This series changes the completion function to take a void *
argument instead of crypto_async_request.

This series touches code in a number of different subsystems.
Most of them are trivial except for tls which was actually buggy
as it did exactly what was described above.

I'd like to pull all the changes through the crypto tree.  But
feel free to object if you'd like the relevant patches to go
through your trees instead and I'll split this up.

Thanks,

Comments

Jakub Kicinski Feb. 7, 2023, 6:51 p.m. UTC | #1 
On Tue, 7 Feb 2023 16:03:52 +0800 Herbert Xu wrote:
> > Buggy means bug could be hit in real light or buggy == did not use 
> > the API right?  
> 
> Yes this bug is real.  If you hit a driver/algorithm that returns
> a different request object (of which there are many in the API) then
> you will be dereferencing random pointers.

Any aes-gcm or chacha-poly implementations which would do that come 
to mind? I'm asking 'cause we probably want to do stable if we know
of a combination which would be broken, or the chances of one existing
are high.

Otherwise no objections for the patches to go via the crypto tree,
there should be no conflicts AFAIK. Feel free to add my ack on the
networking changes if needed.
Herbert Xu Feb. 8, 2023, 4:02 a.m. UTC | #2 
On Tue, Feb 07, 2023 at 10:51:46AM -0800, Jakub Kicinski wrote:
.
> Any aes-gcm or chacha-poly implementations which would do that come 
> to mind? I'm asking 'cause we probably want to do stable if we know
> of a combination which would be broken, or the chances of one existing
> are high.

Good point.  I had a quick look at tls_sw.c and it *appears* to be
safe with the default software code.  As tls_sw only uses the generic
AEAD algorithms (rather than the IPsec-specific variants which aren't
safe), the software-only paths *should* be OK.

However, drivers that support these algorithms may require fallbacks
for esoteric reasons.  For example, drivers/crypto/amcc appears to
require a fallback for certain input parameters which may or may not
be possible with TLS.

To be on the safe side I would do a backport once this has been
in mainline for a little bit.

Cheers,