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 <[email protected]>
---
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;
}
static int qed_ilt_blk_alloc(struct qed_hwfn *p_hwfn,
--
2.17.1
>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 <[email protected]>
>---
> 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 <[email protected]>
> >---
> > 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