| Message ID | aa1a31a25c2d0121e039f34ee58a996ea9a130ad.1694018970.git.sd@queasysnail.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tls: fix some issues with async encryption | expand |
On Wed, 6 Sep 2023 19:08:32 +0200 Sabrina Dubroca wrote: > @@ -221,7 +222,8 @@ static void tls_decrypt_done(void *data, int err) > /* Free the destination pages if skb was not decrypted inplace */ > if (sgout != sgin) { This check is always true now, right? Should we replace it with dctx->put_outsg? > /* Skip the first S/G entry as it points to AAD */ > for_each_sg(sg_next(sgout), sg, UINT_MAX, pages) { > if (!sg) > break; > - put_page(sg_page(sg)); > + if (dctx->put_outsg) > + put_page(sg_page(sg)); > } > }
2023-09-06, 19:05:29 -0700, Jakub Kicinski wrote: > On Wed, 6 Sep 2023 19:08:32 +0200 Sabrina Dubroca wrote: > > @@ -221,7 +222,8 @@ static void tls_decrypt_done(void *data, int err) > > /* Free the destination pages if skb was not decrypted inplace */ > > if (sgout != sgin) { > > This check is always true now, right? > Should we replace it with dctx->put_outsg? Ugh, I noticed that when I was debugging, and didn't think about replacing the test. Yeah, I'll do that in v2.
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 4f3dd0403efb..f23cceaceb36 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -63,6 +63,7 @@ struct tls_decrypt_ctx { u8 iv[MAX_IV_SIZE]; u8 aad[TLS_MAX_AAD_SIZE]; u8 tail; + bool put_outsg; struct scatterlist sg[]; }; @@ -221,7 +222,8 @@ static void tls_decrypt_done(void *data, int err) for_each_sg(sg_next(sgout), sg, UINT_MAX, pages) { if (!sg) break; - put_page(sg_page(sg)); + if (dctx->put_outsg) + put_page(sg_page(sg)); } } @@ -1549,6 +1551,8 @@ static int tls_decrypt_sg(struct sock *sk, struct iov_iter *out_iov, if (err < 0) goto exit_free; + dctx->put_outsg = false; + if (clear_skb) { sg_init_table(sgout, n_sgout); sg_set_buf(&sgout[0], dctx->aad, prot->aad_size); @@ -1558,6 +1562,8 @@ static int tls_decrypt_sg(struct sock *sk, struct iov_iter *out_iov, if (err < 0) goto exit_free; } else if (out_iov) { + dctx->put_outsg = true; + sg_init_table(sgout, n_sgout); sg_set_buf(&sgout[0], dctx->aad, prot->aad_size);
tls_decrypt_sg doesn't take a reference on the pages from clear_skb, so the put_page() in tls_decrypt_done releases them, and we trigger a use-after-free in process_rx_list when we try to read from the partially-read skb. This can be seen with the recv_and_splice test case. Fixes: fd31f3996af2 ("tls: rx: decrypt into a fresh skb") Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- net/tls/tls_sw.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)