Message ID | 20220526100227.483609-1-niejianglei2021@163.com |
---|---|
State | New |
Headers | show |
Series | ath11k: mhi: fix potential memory leak in ath11k_mhi_register() | expand |
On 5/26/2022 3:02 AM, Jianglei Nie wrote: > mhi_alloc_controller() allocates a memory space for mhi_ctrl. When some > errors occur, mhi_ctrl should be freed by mhi_free_controller(). But > when ath11k_mhi_read_addr_from_dt() fails, the function returns without > calling mhi_free_controller(), which will lead to a memory leak. > > We can fix it by calling mhi_free_controller() when > ath11k_mhi_read_addr_from_dt() fails. > > Signed-off-by: Jianglei Nie <niejianglei2021@163.com> Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com> > --- > drivers/net/wireless/ath/ath11k/mhi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c > index fc3524e83e52..3318c7c2b32b 100644 > --- a/drivers/net/wireless/ath/ath11k/mhi.c > +++ b/drivers/net/wireless/ath/ath11k/mhi.c > @@ -376,8 +376,10 @@ int ath11k_mhi_register(struct ath11k_pci *ab_pci) > > if (test_bit(ATH11K_FLAG_FIXED_MEM_RGN, &ab->dev_flags)) { > ret = ath11k_mhi_read_addr_from_dt(mhi_ctrl); > - if (ret < 0) > + if (ret < 0) { > + mhi_free_controller(mhi_ctrl); > return ret; > + } > } else { > mhi_ctrl->iova_start = 0; > mhi_ctrl->iova_stop = 0xFFFFFFFF; This patch looks good to me. However there is an additional issue that IMO should be addressed in this function with a separate patch. Just after mhi_ctrl is allocated the pointer is saved: ab_pci->mhi_ctrl = mhi_ctrl; When there is an error, mhi_ctrl is freed, but ab_pci->mhi_ctrl still has a dangling pointer to the freed memory. This has the potential to result in a subsequent use-after-free or double-free error. So IMO we should do one of two things (I haven't looked at the code in detail to determine which is better): 1) don't set ab_pci->mhi_ctrl = mhi_ctrl until the end of the function after success from mhi_register_controller(). This requires that none of the functions called between the current assignment point and the proposed assignment point dereference ab_pci->mhi_ctrl 2) set ab_pci->mhi_ctrl = NULL in all of the places where we call mhi_free_controller() 2a) have a central error exit label and have all of the error conditions goto that label where there is a central code: mhi_free_controller(mhi_ctrl); ab_pci->mhi_ctrl = NULL; return ret;
diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c index fc3524e83e52..3318c7c2b32b 100644 --- a/drivers/net/wireless/ath/ath11k/mhi.c +++ b/drivers/net/wireless/ath/ath11k/mhi.c @@ -376,8 +376,10 @@ int ath11k_mhi_register(struct ath11k_pci *ab_pci) if (test_bit(ATH11K_FLAG_FIXED_MEM_RGN, &ab->dev_flags)) { ret = ath11k_mhi_read_addr_from_dt(mhi_ctrl); - if (ret < 0) + if (ret < 0) { + mhi_free_controller(mhi_ctrl); return ret; + } } else { mhi_ctrl->iova_start = 0; mhi_ctrl->iova_stop = 0xFFFFFFFF;
mhi_alloc_controller() allocates a memory space for mhi_ctrl. When some errors occur, mhi_ctrl should be freed by mhi_free_controller(). But when ath11k_mhi_read_addr_from_dt() fails, the function returns without calling mhi_free_controller(), which will lead to a memory leak. We can fix it by calling mhi_free_controller() when ath11k_mhi_read_addr_from_dt() fails. Signed-off-by: Jianglei Nie <niejianglei2021@163.com> --- drivers/net/wireless/ath/ath11k/mhi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)