| Message ID | 20190514123439.10524-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rtlwifi: Fix null-pointer dereferences in error handling code of rtl_pci_probe() | expand |
Jia-Ju Bai <baijiaju1990@gmail.com> wrote: > *BUG 1: > In rtl_pci_probe(), when rtlpriv->cfg->ops->init_sw_vars() fails, > rtl_deinit_core() in the error handling code is executed. > rtl_deinit_core() calls rtl_free_entries_from_scan_list(), which uses > rtlpriv->scan_list.list in list_for_each_entry_safe(), but it has been > initialized. Thus a null-pointer dereference occurs. > The reason is that rtlpriv->scan_list.list is initialized by > INIT_LIST_HEAD() in rtl_init_core(), which has not been called. > > To fix this bug, rtl_deinit_core() should not be called when > rtlpriv->cfg->ops->init_sw_vars() fails. > > *BUG 2: > In rtl_pci_probe(), rtl_init_core() can fail when rtl_regd_init() in > this function fails, and rtlpriv->scan_list.list has not been > initialized by INIT_LIST_HEAD(). Then, rtl_deinit_core() in the error > handling code of rtl_pci_probe() is executed. Finally, a null-pointer > dereference occurs due to the same reason of the above bug. > > To fix this bug, the initialization of lists in rtl_init_core() are > performed before the call to rtl_regd_init(). > > These bugs are found by a runtime fuzzing tool named FIZZER written by > us. > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> Ping & Larry, is this ok to take?
On 5/28/19 6:55 AM, Kalle Valo wrote: > Jia-Ju Bai <baijiaju1990@gmail.com> wrote: > >> *BUG 1: >> In rtl_pci_probe(), when rtlpriv->cfg->ops->init_sw_vars() fails, >> rtl_deinit_core() in the error handling code is executed. >> rtl_deinit_core() calls rtl_free_entries_from_scan_list(), which uses >> rtlpriv->scan_list.list in list_for_each_entry_safe(), but it has been >> initialized. Thus a null-pointer dereference occurs. >> The reason is that rtlpriv->scan_list.list is initialized by >> INIT_LIST_HEAD() in rtl_init_core(), which has not been called. >> >> To fix this bug, rtl_deinit_core() should not be called when >> rtlpriv->cfg->ops->init_sw_vars() fails. >> >> *BUG 2: >> In rtl_pci_probe(), rtl_init_core() can fail when rtl_regd_init() in >> this function fails, and rtlpriv->scan_list.list has not been >> initialized by INIT_LIST_HEAD(). Then, rtl_deinit_core() in the error >> handling code of rtl_pci_probe() is executed. Finally, a null-pointer >> dereference occurs due to the same reason of the above bug. >> >> To fix this bug, the initialization of lists in rtl_init_core() are >> performed before the call to rtl_regd_init(). >> >> These bugs are found by a runtime fuzzing tool named FIZZER written by >> us. >> >> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > > Ping & Larry, is this ok to take? > Kalle, Not at the moment. In reviewing the code, I was unable to see how this situation could develop, and his backtrace did not mention any rtlwifi code. For that reason, I asked him to add printk stat4ements to show the last part of rtl_pci that executed correctly. In https://marc.info/?l=linux-wireless&m=155788322631134&w=2, he promised to do that, but I have not seen the result. Larry
Larry Finger <Larry.Finger@lwfinger.net> writes: > On 5/28/19 6:55 AM, Kalle Valo wrote: >> Jia-Ju Bai <baijiaju1990@gmail.com> wrote: >> >>> *BUG 1: >>> In rtl_pci_probe(), when rtlpriv->cfg->ops->init_sw_vars() fails, >>> rtl_deinit_core() in the error handling code is executed. >>> rtl_deinit_core() calls rtl_free_entries_from_scan_list(), which uses >>> rtlpriv->scan_list.list in list_for_each_entry_safe(), but it has been >>> initialized. Thus a null-pointer dereference occurs. >>> The reason is that rtlpriv->scan_list.list is initialized by >>> INIT_LIST_HEAD() in rtl_init_core(), which has not been called. >>> >>> To fix this bug, rtl_deinit_core() should not be called when >>> rtlpriv->cfg->ops->init_sw_vars() fails. >>> >>> *BUG 2: >>> In rtl_pci_probe(), rtl_init_core() can fail when rtl_regd_init() in >>> this function fails, and rtlpriv->scan_list.list has not been >>> initialized by INIT_LIST_HEAD(). Then, rtl_deinit_core() in the error >>> handling code of rtl_pci_probe() is executed. Finally, a null-pointer >>> dereference occurs due to the same reason of the above bug. >>> >>> To fix this bug, the initialization of lists in rtl_init_core() are >>> performed before the call to rtl_regd_init(). >>> >>> These bugs are found by a runtime fuzzing tool named FIZZER written by >>> us. >>> >>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >> >> Ping & Larry, is this ok to take? >> > > Kalle, > > Not at the moment. In reviewing the code, I was unable to see how this > situation could develop, and his backtrace did not mention any rtlwifi > code. For that reason, I asked him to add printk stat4ements to show > the last part of rtl_pci that executed correctly. In > https://marc.info/?l=linux-wireless&m=155788322631134&w=2, he promised > to do that, but I have not seen the result. Ok, thanks. I'll then drop this, please resubmit once everything is understood and the patch is ready to be applied.
On 2019/5/28 21:00, Larry Finger wrote: > On 5/28/19 6:55 AM, Kalle Valo wrote: >> Jia-Ju Bai <baijiaju1990@gmail.com> wrote: >> >>> *BUG 1: >>> In rtl_pci_probe(), when rtlpriv->cfg->ops->init_sw_vars() fails, >>> rtl_deinit_core() in the error handling code is executed. >>> rtl_deinit_core() calls rtl_free_entries_from_scan_list(), which uses >>> rtlpriv->scan_list.list in list_for_each_entry_safe(), but it has been >>> initialized. Thus a null-pointer dereference occurs. >>> The reason is that rtlpriv->scan_list.list is initialized by >>> INIT_LIST_HEAD() in rtl_init_core(), which has not been called. >>> >>> To fix this bug, rtl_deinit_core() should not be called when >>> rtlpriv->cfg->ops->init_sw_vars() fails. >>> >>> *BUG 2: >>> In rtl_pci_probe(), rtl_init_core() can fail when rtl_regd_init() in >>> this function fails, and rtlpriv->scan_list.list has not been >>> initialized by INIT_LIST_HEAD(). Then, rtl_deinit_core() in the error >>> handling code of rtl_pci_probe() is executed. Finally, a null-pointer >>> dereference occurs due to the same reason of the above bug. >>> >>> To fix this bug, the initialization of lists in rtl_init_core() are >>> performed before the call to rtl_regd_init(). >>> >>> These bugs are found by a runtime fuzzing tool named FIZZER written by >>> us. >>> >>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >> >> Ping & Larry, is this ok to take? >> > > Kalle, > > Not at the moment. In reviewing the code, I was unable to see how this > situation could develop, and his backtrace did not mention any rtlwifi > code. For that reason, I asked him to add printk stat4ements to show > the last part of rtl_pci that executed correctly. In > https://marc.info/?l=linux-wireless&m=155788322631134&w=2, he promised > to do that, but I have not seen the result. > Hi Larry, This patch is not related to the message you mentioned. That message is about an occasional crash that I reported. That crash occurred when request_irq() in rtl_pci_intr_mode_legacy() in rtl_pci_intr_mode_decide() fails. I have added printk statements and try to reproduce and debug that crash, but that crash does not always occur, and I still do not know the root cause of that crash. The null-pointer dereferences fixed by this patch are different from that crash, and they always occur when the related functions fail. So please review these null-pointer dereferences, thanks :) Best wishes, Jia-Ju Bai
On 5/29/19 5:30 AM, Jia-Ju Bai wrote: > > > On 2019/5/28 21:00, Larry Finger wrote: >> On 5/28/19 6:55 AM, Kalle Valo wrote: >>> Jia-Ju Bai <baijiaju1990@gmail.com> wrote: >>> >>>> *BUG 1: >>>> In rtl_pci_probe(), when rtlpriv->cfg->ops->init_sw_vars() fails, >>>> rtl_deinit_core() in the error handling code is executed. >>>> rtl_deinit_core() calls rtl_free_entries_from_scan_list(), which uses >>>> rtlpriv->scan_list.list in list_for_each_entry_safe(), but it has been >>>> initialized. Thus a null-pointer dereference occurs. >>>> The reason is that rtlpriv->scan_list.list is initialized by >>>> INIT_LIST_HEAD() in rtl_init_core(), which has not been called. >>>> >>>> To fix this bug, rtl_deinit_core() should not be called when >>>> rtlpriv->cfg->ops->init_sw_vars() fails. >>>> >>>> *BUG 2: >>>> In rtl_pci_probe(), rtl_init_core() can fail when rtl_regd_init() in >>>> this function fails, and rtlpriv->scan_list.list has not been >>>> initialized by INIT_LIST_HEAD(). Then, rtl_deinit_core() in the error >>>> handling code of rtl_pci_probe() is executed. Finally, a null-pointer >>>> dereference occurs due to the same reason of the above bug. >>>> >>>> To fix this bug, the initialization of lists in rtl_init_core() are >>>> performed before the call to rtl_regd_init(). >>>> >>>> These bugs are found by a runtime fuzzing tool named FIZZER written by >>>> us. >>>> >>>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >>> >>> Ping & Larry, is this ok to take? >>> >> >> Kalle, >> >> Not at the moment. In reviewing the code, I was unable to see how this >> situation could develop, and his backtrace did not mention any rtlwifi code. >> For that reason, I asked him to add printk stat4ements to show the last part >> of rtl_pci that executed correctly. In >> https://marc.info/?l=linux-wireless&m=155788322631134&w=2, he promised to do >> that, but I have not seen the result. >> > > Hi Larry, > > This patch is not related to the message you mentioned. > That message is about an occasional crash that I reported. > That crash occurred when request_irq() in rtl_pci_intr_mode_legacy() in > rtl_pci_intr_mode_decide() fails. > I have added printk statements and try to reproduce and debug that crash, but > that crash does not always occur, and I still do not know the root cause of that > crash. > > The null-pointer dereferences fixed by this patch are different from that crash, > and they always occur when the related functions fail. > So please review these null-pointer dereferences, thanks :) > > > Best wishes, > Jia-Ju Bai Sorry if I got confused. Kalle has dropped this patch, thus you will need to submit a new version. Larry
diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c index 217d2a7a43c7..b3f341ec3710 100644 --- a/drivers/net/wireless/realtek/rtlwifi/base.c +++ b/drivers/net/wireless/realtek/rtlwifi/base.c @@ -526,8 +526,14 @@ int rtl_init_core(struct ieee80211_hw *hw) /* <2> rate control register */ hw->rate_control_algorithm = "rtl_rc"; + /* <3> init list */ + INIT_LIST_HEAD(&rtlpriv->entry_list); + INIT_LIST_HEAD(&rtlpriv->scan_list.list); + skb_queue_head_init(&rtlpriv->tx_report.queue); + skb_queue_head_init(&rtlpriv->c2hcmd_queue); + /* - * <3> init CRDA must come after init + * <4> init CRDA must come after init * mac80211 hw in _rtl_init_mac80211. */ if (rtl_regd_init(hw, rtl_reg_notifier)) { @@ -535,7 +541,7 @@ int rtl_init_core(struct ieee80211_hw *hw) return 1; } - /* <4> locks */ + /* <5> locks */ mutex_init(&rtlpriv->locks.conf_mutex); mutex_init(&rtlpriv->locks.ips_mutex); mutex_init(&rtlpriv->locks.lps_mutex); @@ -550,11 +556,6 @@ int rtl_init_core(struct ieee80211_hw *hw) spin_lock_init(&rtlpriv->locks.cck_and_rw_pagea_lock); spin_lock_init(&rtlpriv->locks.fw_ps_lock); spin_lock_init(&rtlpriv->locks.iqk_lock); - /* <5> init list */ - INIT_LIST_HEAD(&rtlpriv->entry_list); - INIT_LIST_HEAD(&rtlpriv->scan_list.list); - skb_queue_head_init(&rtlpriv->tx_report.queue); - skb_queue_head_init(&rtlpriv->c2hcmd_queue); rtlmac->link_state = MAC80211_NOLINK; diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.c b/drivers/net/wireless/realtek/rtlwifi/pci.c index 48ca52102cef..864cb76230c4 100644 --- a/drivers/net/wireless/realtek/rtlwifi/pci.c +++ b/drivers/net/wireless/realtek/rtlwifi/pci.c @@ -2267,7 +2267,7 @@ int rtl_pci_probe(struct pci_dev *pdev, if (rtlpriv->cfg->ops->init_sw_vars(hw)) { pr_err("Can't init_sw_vars\n"); err = -ENODEV; - goto fail3; + goto fail2; } rtlpriv->cfg->ops->init_sw_leds(hw); -- 2.17.0
*BUG 1: In rtl_pci_probe(), when rtlpriv->cfg->ops->init_sw_vars() fails, rtl_deinit_core() in the error handling code is executed. rtl_deinit_core() calls rtl_free_entries_from_scan_list(), which uses rtlpriv->scan_list.list in list_for_each_entry_safe(), but it has been initialized. Thus a null-pointer dereference occurs. The reason is that rtlpriv->scan_list.list is initialized by INIT_LIST_HEAD() in rtl_init_core(), which has not been called. To fix this bug, rtl_deinit_core() should not be called when rtlpriv->cfg->ops->init_sw_vars() fails. *BUG 2: In rtl_pci_probe(), rtl_init_core() can fail when rtl_regd_init() in this function fails, and rtlpriv->scan_list.list has not been initialized by INIT_LIST_HEAD(). Then, rtl_deinit_core() in the error handling code of rtl_pci_probe() is executed. Finally, a null-pointer dereference occurs due to the same reason of the above bug. To fix this bug, the initialization of lists in rtl_init_core() are performed before the call to rtl_regd_init(). These bugs are found by a runtime fuzzing tool named FIZZER written by us. Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- drivers/net/wireless/realtek/rtlwifi/base.c | 15 ++++++++------- drivers/net/wireless/realtek/rtlwifi/pci.c | 4 ++-- 2 files changed, 10 insertions(+), 9 deletions(-)