RE: [PATCH] net: rtase: Fix NULL pointer dereference in NAPI cleanup on probe failure

[Justin Lai]

Yang Zi <2959243019@xxxxxx> wrote:
> 
> External mail : This email originated from outside the organization. Do not
> reply, click links, or open attachments unless you recognize the sender and
> know the content is safe.
> 
> 
> 
> rtase_init_napi() (which adds the NAPI contexts and sets napi->dev) is
> called after rtase_alloc_interrupt().  If interrupt allocation fails,
> rtase_init_one() jumps to err_out_del_napi and calls netif_napi_del() on
> NAPI contexts that were never added, so napi->dev is still NULL.
> netif_napi_del() -> __netif_napi_del() -> netdev_lock(napi->dev)
> dereferences that NULL pointer.
> 
> KASAN report:
> 
>     BUG: KASAN: null-ptr-deref in __mutex_lock_common
> kernel/locking/mutex.c:625 [inline]
>     BUG: KASAN: null-ptr-deref in __mutex_lock+0x8f/0xce0
> kernel/locking/mutex.c:820
>     Read of size 8 at addr 0000000000000cd8 by task syz.0.1/1147
> 
>     __mutex_lock+0x8f/0xce0 kernel/locking/mutex.c:820
>     netdev_lock include/linux/netdevice.h:2818 [inline] [rtase]
>     __netif_napi_del include/linux/netdevice.h:2942 [inline] [rtase]
>     netif_napi_del include/linux/netdevice.h:2961 [inline] [rtase]
>     rtase_init_one+0x5c3d/0x5fd0
> drivers/net/ethernet/realtek/rtase/rtase_main.c:2295 [rtase]
> 
> Only call netif_napi_del() on NAPI contexts that were actually added
> (napi->dev != NULL), so the interrupt-allocation failure path skips them
> while the post-napi-init failure paths still remove them.
> 
> Signed-off-by: Yang Zi <2959243019@xxxxxx>
> ---
>  drivers/net/ethernet/realtek/rtase/rtase_main.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> index e3cd4f7c1380..b4d8828ca87d 100644
> --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> @@ -2493,9 +2493,14 @@ static int rtase_init_one(struct pci_dev *pdev,
>      }
> 
>  err_out_del_napi:
> +    /* rtase_init_napi() runs after rtase_alloc_interrupt(); if interrupt
> +     * allocation failed the NAPI contexts were never added and napi->dev
> is
> +     * still NULL, so netif_napi_del() must not be called on them.
> +     */
>      for (i = 0; i < tp->int_nums; i++) {
>          ivec = &tp->int_vector[i];
> -        netif_napi_del(&ivec->napi);
> +        if (ivec->napi.dev)
> +            netif_napi_del(&ivec->napi);
>      }
> 
>  err_out_release_board:

Thanks for the patch.
 
Since rtase_init_napi() is only called after
rtase_alloc_interrupt() succeeds, I think the interrupt allocation
failure path can simply jump to err_out_release_board instead of
checking napi->dev in the NAPI cleanup path. The
err_out_del_napi label can then be removed.
 
Justin