| Message ID | 20230224102839.26538-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: tls: fix possible info leak in tls_set_device_offload() | expand |
On Fri, 24 Feb 2023 18:28:39 +0800 Hangyu Hua wrote: > After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But > tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails > before kmalloc cctx->iv. It is better to Set them to NULL to avoid any > potential info leak. Please show clear chain of events which can lead to a use-after-free or info leak. And if you can't please don't send the patch.
2023-02-24, 10:57:29 -0800, Jakub Kicinski wrote: > On Fri, 24 Feb 2023 18:28:39 +0800 Hangyu Hua wrote: > > After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But > > tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails > > before kmalloc cctx->iv. It is better to Set them to NULL to avoid any > > potential info leak. > > Please show clear chain of events which can lead to a use-after-free > or info leak. And if you can't please don't send the patch. Sorry, I thought in this morning's discussion Hangyu had agreed to remove all mentions of possible info leak while sending v2, since we agreed [1] that this patch didn't fix any issue, just that it looked more consistent, as tls_set_sw_offload NULLs iv and rec_seq on failure. We can also drop the patch completely. Anyway since net-next is closed, I should have told Hangyu to wait for 2 weeks. [1] https://lore.kernel.org/all/310391ea-7c71-395e-5dcb-b0a983e6fc93@gmail.com/
On 25/2/2023 04:22, Sabrina Dubroca wrote: > 2023-02-24, 10:57:29 -0800, Jakub Kicinski wrote: >> On Fri, 24 Feb 2023 18:28:39 +0800 Hangyu Hua wrote: >>> After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But >>> tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails >>> before kmalloc cctx->iv. It is better to Set them to NULL to avoid any >>> potential info leak. >> >> Please show clear chain of events which can lead to a use-after-free >> or info leak. And if you can't please don't send the patch. > > Sorry, I thought in this morning's discussion Hangyu had agreed to > remove all mentions of possible info leak while sending v2, since we > agreed [1] that this patch didn't fix any issue, just that it looked > more consistent, as tls_set_sw_offload NULLs iv and rec_seq on > failure. We can also drop the patch completely. Anyway since net-next > is closed, I should have told Hangyu to wait for 2 weeks. > > [1] https://lore.kernel.org/all/310391ea-7c71-395e-5dcb-b0a983e6fc93@gmail.com/ > Oops. I will make a whole new patch without mentions of info leak in a few weeks. Please ignore this patch.
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index 6c593788dc25..a63f6f727f58 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -1241,8 +1241,10 @@ int tls_set_device_offload(struct sock *sk, struct tls_context *ctx) kfree(start_marker_record); free_rec_seq: kfree(ctx->tx.rec_seq); + ctx->tx.rec_seq = NULL; free_iv: kfree(ctx->tx.iv); + ctx->tx.iv = NULL; release_netdev: dev_put(netdev); return rc;
After tls_set_device_offload() fails, we enter tls_set_sw_offload(). But tls_set_sw_offload can't set cctx->iv and cctx->rec_seq to NULL if it fails before kmalloc cctx->iv. It is better to Set them to NULL to avoid any potential info leak. Fixes: e8f69799810c ("net/tls: Add generic NIC offload infrastructure") Signed-off-by: Hangyu Hua <hbh25y@gmail.com> --- v2: change commit log. The original issue will be fixed in another patch. net/tls/tls_device.c | 2 ++ 1 file changed, 2 insertions(+)