On 1/24/24 11:45, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Wed, 2024-01-24 at 10:31 -0700, David Mosberger-Tang wrote:
>> Alexis,
>>
>> On Wed, 2024-01-24 at 10:01 +0100, Alexis Lothoré wrote:
>>> ==================================================================
>>> BUG: KASAN: slab-use-after-free in wilc_netdev_cleanup+0x294/0x2c0
>>> Read of size 4 at addr c3c91ce8 by task swapper/1
>>
>> OK, I think I see what's going on: it's the list traversal. Here is what
>> wilc_netdev_cleanup() does:
>>
>> list_for_each_entry_rcu(vif, &wilc->vif_list, list) {
>> if (vif->ndev)
>> unregister_netdev(vif->ndev);
>> }
>>
>> The problem is that "vif" is the private part of the netdev, so when the netdev
>> is freed, the vif structure is no longer valid and list_for_each_entry_rcu()
>> ends up dereferencing a dangling pointer.
>>
>> Ajay or Alexis, could you propose a fix for this - this is not something I'm
>> familiar with.
>
> Actually, after staring at the code a little longer, is there anything wrong
> with delaying the unregister_netdev() call until after the vif has been removed
> from the vif-list? Something along the lines of the below.
I think we need to investigate the actual reason for Kasan warning.
First, we need to confirm if this warning exists without the patch(test
by simulating a force error in wilc_bus_probe()). When
wilc_netdev_cleanup() is also called from wilc_bus_remove(), I believe
this warning was not observed. Once it is confirmed, the fix can be done
accordingly.
Avoiding netdev initialization in wilc_cfg80211_init() would require lot
of modification and changes in API call order. IMO the Kasan warning fix
should be independent of netdev initialization order and it should
free-up the resources for all cases. Suppose if the card is not present,
the probe API should clean-up and exit gracefully. For detecting the
chip_id, I have created the below patch considering the above scenarios,
please check if it makes sense.
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -225,6 +225,11 @@ static int wilc_bus_probe(struct spi_device *spi)
if (ret < 0)
goto netdev_cleanup;
+ if (!is_wilc_chip_connected(wilc)) {
+ ret = -ENODEV;
+ goto netdev_cleanup;
+ }
+
wilc->rtc_clk = devm_clk_get_optional(&spi->dev, "rtc");
if (IS_ERR(wilc->rtc_clk)) {
ret = PTR_ERR(wilc->rtc_clk);
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c
b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 6b2f2269ddf8..6f95456b053b 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -1537,3 +1537,22 @@ int wilc_wlan_init(struct net_device *dev)
return ret;
}
+
+bool is_wilc_chip_connected(struct wilc *wl)
+{
+ u32 chipid = 0;
+ int ret;
+
+ acquire_bus(wl, WILC_BUS_ACQUIRE_ONLY);
+ ret = wl->hif_func->hif_init(wl, false);
+ if (!ret) {
+ wl->hif_func->hif_read_reg(wl, WILC_CHIPID, &chipid);
+ wl->hif_func->hif_deinit(wl);
+ }
+ release_bus(wl, WILC_BUS_RELEASE_ONLY);
+ if (ret || !is_wilc1000(chipid))
+ return false;
+
+ return true;
+}
+EXPORT_SYMBOL_GPL(is_wilc_chip_connected);
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h
b/drivers/net/wireless/microchip/wilc1000/wlan.h
index f02775f7e41f..8585dda0790c 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.h
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
@@ -440,4 +440,5 @@ int wilc_send_config_pkt(struct wilc_vif *vif, u8
mode, struct wid *wids,
u32 count);
int wilc_wlan_init(struct net_device *dev);
u32 wilc_get_chipid(struct wilc *wilc, bool update);
+bool is_wilc_chip_connected(struct wilc *wilc);
#endif
--
Regards,
Ajay
