| Message ID | 20231207093606.17868-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] qed: Fix a potential use-after-free in qed_cxt_tables_alloc | expand |
On 12/7/23 10:36, Dinghao Liu wrote: > qed_ilt_shadow_alloc() will call qed_ilt_shadow_free() to > free p_hwfn->p_cxt_mngr->ilt_shadow on error. However, > qed_cxt_tables_alloc() accesses the freed pointer on failure > of qed_ilt_shadow_alloc() through calling qed_cxt_mngr_free(), > which may lead to use-after-free. Fix this issue by setting > p_hwfn->p_cxt_mngr->ilt_shadow to NULL in qed_ilt_shadow_free(). > > Fixes: fe56b9e6a8d9 ("qed: Add module with basic common support") > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > --- > > Changelog: For future submissions please also provide links to previous versions (would be to v1 in this case). No need to add this now for this one. > > v2: -Change the bug type from double-free to use-after-free. > -Move the null check against p_mngr->ilt_shadow to the beginning > of the function qed_ilt_shadow_free(). > -When kcalloc() fails in qed_ilt_shadow_alloc(), just return > because there is nothing to free. > --- > drivers/net/ethernet/qlogic/qed/qed_cxt.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
On Thu, 7 Dec 2023 17:36:06 +0800 Dinghao Liu wrote: > v2: -Change the bug type from double-free to use-after-free. > -Move the null check against p_mngr->ilt_shadow to the beginning > of the function qed_ilt_shadow_free(). > -When kcalloc() fails in qed_ilt_shadow_alloc(), just return > because there is nothing to free. This refactoring is not acceptable as part of a fix, sorry. > @@ -933,6 +936,7 @@ static void qed_ilt_shadow_free(struct qed_hwfn *p_hwfn) > p_dma->virt_addr = NULL; > } > kfree(p_mngr->ilt_shadow); > + p_hwfn->p_cxt_mngr->ilt_shadow = NULL; Why do you dereference p_hwfn here? Seems more natural to use: p_mngr->ilt_shadow = NULL; since that's the exact pointer that was passed to free.
> On Thu, 7 Dec 2023 17:36:06 +0800 Dinghao Liu wrote: > > v2: -Change the bug type from double-free to use-after-free. > > -Move the null check against p_mngr->ilt_shadow to the beginning > > of the function qed_ilt_shadow_free(). > > -When kcalloc() fails in qed_ilt_shadow_alloc(), just return > > because there is nothing to free. > > This refactoring is not acceptable as part of a fix, sorry. > > > @@ -933,6 +936,7 @@ static void qed_ilt_shadow_free(struct qed_hwfn *p_hwfn) > > p_dma->virt_addr = NULL; > > } > > kfree(p_mngr->ilt_shadow); > > + p_hwfn->p_cxt_mngr->ilt_shadow = NULL; > > Why do you dereference p_hwfn here? > Seems more natural to use: > > p_mngr->ilt_shadow = NULL; > > since that's the exact pointer that was passed to free. > -- > pw-bot: cr I will resend a new patch to fix this, thanks! Regards, Dinghao
diff --git a/drivers/net/ethernet/qlogic/qed/qed_cxt.c b/drivers/net/ethernet/qlogic/qed/qed_cxt.c index 65e20693c549..911e0c0d3563 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_cxt.c +++ b/drivers/net/ethernet/qlogic/qed/qed_cxt.c @@ -921,9 +921,12 @@ static void qed_ilt_shadow_free(struct qed_hwfn *p_hwfn) struct qed_cxt_mngr *p_mngr = p_hwfn->p_cxt_mngr; u32 ilt_size, i; + if (!p_mngr->ilt_shadow) + return; + ilt_size = qed_cxt_ilt_shadow_size(p_cli); - for (i = 0; p_mngr->ilt_shadow && i < ilt_size; i++) { + for (i = 0; i < ilt_size; i++) { struct phys_mem_desc *p_dma = &p_mngr->ilt_shadow[i]; if (p_dma->virt_addr) @@ -933,6 +936,7 @@ static void qed_ilt_shadow_free(struct qed_hwfn *p_hwfn) p_dma->virt_addr = NULL; } kfree(p_mngr->ilt_shadow); + p_hwfn->p_cxt_mngr->ilt_shadow = NULL; } static int qed_ilt_blk_alloc(struct qed_hwfn *p_hwfn, @@ -995,10 +999,8 @@ static int qed_ilt_shadow_alloc(struct qed_hwfn *p_hwfn) size = qed_cxt_ilt_shadow_size(clients); p_mngr->ilt_shadow = kcalloc(size, sizeof(struct phys_mem_desc), GFP_KERNEL); - if (!p_mngr->ilt_shadow) { - rc = -ENOMEM; - goto ilt_shadow_fail; - } + if (!p_mngr->ilt_shadow) + return -ENOMEM; DP_VERBOSE(p_hwfn, QED_MSG_ILT, "Allocated 0x%x bytes for ilt shadow\n",
qed_ilt_shadow_alloc() will call qed_ilt_shadow_free() to free p_hwfn->p_cxt_mngr->ilt_shadow on error. However, qed_cxt_tables_alloc() accesses the freed pointer on failure of qed_ilt_shadow_alloc() through calling qed_cxt_mngr_free(), which may lead to use-after-free. Fix this issue by setting p_hwfn->p_cxt_mngr->ilt_shadow to NULL in qed_ilt_shadow_free(). Fixes: fe56b9e6a8d9 ("qed: Add module with basic common support") Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> --- Changelog: v2: -Change the bug type from double-free to use-after-free. -Move the null check against p_mngr->ilt_shadow to the beginning of the function qed_ilt_shadow_free(). -When kcalloc() fails in qed_ilt_shadow_alloc(), just return because there is nothing to free. --- drivers/net/ethernet/qlogic/qed/qed_cxt.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)