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

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, 7:10 a.m. UTC | #1

On Mon, 6 Feb 2023 18:21:06 +0800 Herbert Xu wrote:
> 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.

"different data field" == copy the value to a different structure?
A bit hard to parse TBH.

> 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.

Buggy means bug could be hit in real light or buggy == did not use 
the API right?

> 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.

--
dm-devel mailing list
[e-mail]
https://listman.redhat.com/mailman/listinfo/dm-devel

Jakub Kicinski Feb. 7, 2023, 7:16 a.m. UTC | #2

On Mon, 6 Feb 2023 23:10:08 -0800 Jakub Kicinski wrote:
> Buggy means bug could be hit in real light or buggy == did not use 
> the API right?

"in real light"... time to sign off for the night. s/light/life/

--
dm-devel mailing list
[e-mail]
https://listman.redhat.com/mailman/listinfo/dm-devel

Herbert Xu Feb. 7, 2023, 8:03 a.m. UTC | #3

On Mon, Feb 06, 2023 at 11:10:08PM -0800, Jakub Kicinski wrote:
> On Mon, 6 Feb 2023 18:21:06 +0800 Herbert Xu wrote:
> > 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.
> 
> "different data field" == copy the value to a different structure?
> A bit hard to parse TBH.

The word data here refers to the data field in struct crypto_async_request.
 
> 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.

Cheers,

Jakub Kicinski Feb. 7, 2023, 6:51 p.m. UTC | #4

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.

--
dm-devel mailing list
[e-mail]
https://listman.redhat.com/mailman/listinfo/dm-devel

Herbert Xu Feb. 8, 2023, 4:02 a.m. UTC | #5

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,