| Message ID | 20231206064531.6089-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | qed: Fix a potential double-free in qed_cxt_tables_alloc | expand |
>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() frees this pointer again on failure of >qed_ilt_shadow_alloc() through calling qed_cxt_mngr_free(), which may >lead to double-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> >--- > drivers/net/ethernet/qlogic/qed/qed_cxt.c | 1 + > 1 file changed, 1 insertion(+) > >diff --git a/drivers/net/ethernet/qlogic/qed/qed_cxt.c >b/drivers/net/ethernet/qlogic/qed/qed_cxt.c >index 65e20693c549..26e247517394 100644 >--- a/drivers/net/ethernet/qlogic/qed/qed_cxt.c >+++ b/drivers/net/ethernet/qlogic/qed/qed_cxt.c >@@ -933,6 +933,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; [Suman] Hi Dinghao, I am not sure how this will help prevent the double free here? If qed_ilt_shadow_alloc() fails to allocate memory, then still qed_cxt_mngr_free() will be called and kfree() will try to free the NULL pointer. Shouldn't it be like below? if (p_mngr->ilt_shadow) Kfree(p_mngr->ilt_shadow); > } > > static int qed_ilt_blk_alloc(struct qed_hwfn *p_hwfn, >-- >2.17.1 >
Hi Suman, > >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() frees this pointer again on failure of > >qed_ilt_shadow_alloc() through calling qed_cxt_mngr_free(), which may > >lead to double-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> > >--- > > drivers/net/ethernet/qlogic/qed/qed_cxt.c | 1 + > > 1 file changed, 1 insertion(+) > > > >diff --git a/drivers/net/ethernet/qlogic/qed/qed_cxt.c > >b/drivers/net/ethernet/qlogic/qed/qed_cxt.c > >index 65e20693c549..26e247517394 100644 > >--- a/drivers/net/ethernet/qlogic/qed/qed_cxt.c > >+++ b/drivers/net/ethernet/qlogic/qed/qed_cxt.c > >@@ -933,6 +933,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; > [Suman] Hi Dinghao, > > I am not sure how this will help prevent the double free here? If qed_ilt_shadow_alloc() fails to allocate memory, then still qed_cxt_mngr_free() will be called and kfree() > will try to free the NULL pointer. Shouldn't it be like below? > > if (p_mngr->ilt_shadow) > Kfree(p_mngr->ilt_shadow); > > } > > > > static int qed_ilt_blk_alloc(struct qed_hwfn *p_hwfn, > >-- > >2.17.1 > > kfree(NULL) is safe in kernel. But kfree() will not set the freed pointer to NULL. Therefore, checking p_mngr->ilt_shadow will not prevent the kfree() for the second time. Many double-free bugs are fixed by setting the freed pointer to NULL (e.g., 6b0d0477fce7 ("media: dvb-core: Fix double free in dvb_register_device()")), so I just fix this bug in the same way. Regards, Dinghao
>> >+++ b/drivers/net/ethernet/qlogic/qed/qed_cxt.c >> >@@ -933,6 +933,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; >> [Suman] Hi Dinghao, >> >> I am not sure how this will help prevent the double free here? If >> qed_ilt_shadow_alloc() fails to allocate memory, then still >qed_cxt_mngr_free() will be called and kfree() will try to free the NULL >pointer. Shouldn't it be like below? >> >> if (p_mngr->ilt_shadow) >> Kfree(p_mngr->ilt_shadow); >> > } >> > >> > static int qed_ilt_blk_alloc(struct qed_hwfn *p_hwfn, >> >-- >> >2.17.1 >> > > >kfree(NULL) is safe in kernel. But kfree() will not set the freed >pointer to NULL. Therefore, checking p_mngr->ilt_shadow will not prevent >the kfree() for the second time. Many double-free bugs are fixed by >setting the freed pointer to NULL (e.g., >6b0d0477fce7 ("media: dvb-core: Fix double free in >dvb_register_device()")), so I just fix this bug in the same way. > >Regards, >Dinghao [Suman] Okay, I understand. Along with this change, I have couple of suggestion. But it is up to you to make them. 1. In the beginning of the function qed_ilt_shadow_free() should we add a check and return if ilt_shadew == NULL? So, that the control does not reach to the end of the function? 2. I see in qed_ilt_shadow_alloc() we are calling "goto ilt_shadow_fail" even if the kcalloc() is failing. If kcalloc() fails, then there is nothing to free, and we can directly return from there, right?
> >> >+++ b/drivers/net/ethernet/qlogic/qed/qed_cxt.c > >> >@@ -933,6 +933,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; > >> [Suman] Hi Dinghao, > >> > >> I am not sure how this will help prevent the double free here? If > >> qed_ilt_shadow_alloc() fails to allocate memory, then still > >qed_cxt_mngr_free() will be called and kfree() will try to free the NULL > >pointer. Shouldn't it be like below? > >> > >> if (p_mngr->ilt_shadow) > >> Kfree(p_mngr->ilt_shadow); > >> > } > >> > > >> > static int qed_ilt_blk_alloc(struct qed_hwfn *p_hwfn, > >> >-- > >> >2.17.1 > >> > > > > >kfree(NULL) is safe in kernel. But kfree() will not set the freed > >pointer to NULL. Therefore, checking p_mngr->ilt_shadow will not prevent > >the kfree() for the second time. Many double-free bugs are fixed by > >setting the freed pointer to NULL (e.g., > >6b0d0477fce7 ("media: dvb-core: Fix double free in > >dvb_register_device()")), so I just fix this bug in the same way. > > > >Regards, > >Dinghao > [Suman] Okay, I understand. Along with this change, I have couple of suggestion. But it is up to you to make them. > 1. In the beginning of the function qed_ilt_shadow_free() should we add a check and return if ilt_shadew == NULL? So, that the control does not reach to the end of the function? > 2. I see in qed_ilt_shadow_alloc() we are calling "goto ilt_shadow_fail" even if the kcalloc() is failing. If kcalloc() fails, then there is nothing to free, and we can directly return from there, right? Thanks for your suggestions! For the first suggestion, ilt_shadew is checked in the beginning of the for loop, but it seems that we need not to check this pointer at each iteration. I will move it to the beginning of the function. I also rechecked the loop and found the bug here should be a use-after-free instead of double-free. There is a pointer dereference &p_mngr->ilt_shadow[i], which will be triggered before kfree(). I will resend a new patch to fix this. For the second suggestion, I agree that directly returning would be better. I will fix this in the next patch version, 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..26e247517394 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_cxt.c +++ b/drivers/net/ethernet/qlogic/qed/qed_cxt.c @@ -933,6 +933,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,
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() frees this pointer again on failure of qed_ilt_shadow_alloc() through calling qed_cxt_mngr_free(), which may lead to double-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> --- drivers/net/ethernet/qlogic/qed/qed_cxt.c | 1 + 1 file changed, 1 insertion(+)