| Message ID | Y+DUkqe1sagWaErA@gondor.apana.org.au (mailing list archive) |
|---|---|
| Headers | show |
| Series | crypto: api - Change completion callback argument to void star | expand |
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.
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/
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,
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.
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,