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 <[email protected]>
---
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(-)
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",
--
2.17.1
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 <[email protected]>
> ---
>
> 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 <[email protected]>
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
> 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