Re: [PATCH 2/6] mwifiex: usb: urb->context sanity check in complete handler

[Arend Van Spriel <arend.vanspriel@broadcom.com>]

On 3-5-2017 13:48, Xinming Hu wrote:
> From: Xinming Hu <huxm@marvell.com>
> 
> urb/context might be freed in cornel case, add sanity check to avoid
> use-after-free.
> 
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/usb.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
> index 2f7705c..ee5f488 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -169,6 +169,11 @@ static void mwifiex_usb_rx_complete(struct urb *urb)
>  	int recv_length = urb->actual_length;
>  	int size, status;
>  
> +	if (!urb || !urb->context) {
> +		pr_err("URB or URB context is not valid in USB Rx complete\n");
> +		return;
> +	}

Something is really off if you need !urb here. Furthermore, you are
already initializing stack variables using fields inside the urb before
this check causing a null-deref so it is bogus anyway. The completion
function is a member in struct urb. If your driver has a list of these
urb's somewhere and they are free in parallel then your design is wrong
and this does not solve the use-after-free. The urb here will not be
NULL and still points to the old memory location as it was handed to the
usb subsystem. That piece of memory might already been handed out to
some other piece of the kernel making any access to it invalid and
potentially crashing the kernel.

Regards,
Arend