Re: [PATCH v2] usb: mon: Fix atomicity violation in mon_bin_vma_fault

[Greg KH]

On Fri, Dec 22, 2023 at 02:04:50PM +0800, Gui-Dong Han wrote:
> In mon_bin_vma_fault():
> 	offset = vmf->pgoff << PAGE_SHIFT;
> 	if (offset >= rp->b_size)
> 		return VM_FAULT_SIGBUS;
> 	chunk_idx = offset / CHUNK_SIZE;
> 	pageptr = rp->b_vec[chunk_idx].pg;
> The code is executed without holding any lock.
> 
> In mon_bin_vma_close():
> 	spin_lock_irqsave(&rp->b_lock, flags);
> 	rp->mmap_active--;
> 	spin_unlock_irqrestore(&rp->b_lock, flags);
> 
> In mon_bin_ioctl():
> 	spin_lock_irqsave(&rp->b_lock, flags);
> 	if (rp->mmap_active) {
> 		...
> 	} else {
> 		...
> 		kfree(rp->b_vec);
> 		rp->b_vec  = vec;
> 		rp->b_size = size;
> 		...
> 	}
> 	spin_unlock_irqrestore(&rp->b_lock, flags);
> 
> Concurrent execution of mon_bin_vma_fault() with mon_bin_vma_close() and
> mon_bin_ioctl() could lead to atomicity violations. mon_bin_vma_fault()
> accesses rp->b_size and rp->b_vec without locking, risking array
> out-of-bounds access or use-after-free bugs due to possible modifications
> in mon_bin_ioctl().
> 
> This possible bug is found by an experimental static analysis tool
> developed by our team, BassCheck[1]. This tool analyzes the locking APIs
> to extract function pairs that can be concurrently executed, and then
> analyzes the instructions in the paired functions to identify possible
> concurrency bugs including data races and atomicity violations. The above
> possible bug is reported when our tool analyzes the source code of
> Linux 6.2.
> 
> To address this issue, it is proposed to add a spin lock pair in
> mon_bin_vma_fault() to ensure atomicity. With this patch applied, our tool
> never reports the possible bug, with the kernel configuration allyesconfig
> for x86_64. Due to the lack of associated hardware, we cannot test the
> patch in runtime testing, and just verify it according to the code logic.
> 
> [1] https://sites.google.com/view/basscheck/
> 
> Fixes: 19e6317d24c25 ("usb: mon: Fix a deadlock in usbmon between ...")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: BassCheck <bass@xxxxxxxxxxx>
> Signed-off-by: Gui-Dong Han <2045gemini@xxxxxxxxx>
> ---
> v2:
> * In this patch v2, we've added some information of the static analysis
> tool used, as per the researcher guidelines. Also, we've added a cc in the
> signed-off-by area, according to the stable-kernel-rules.
>   Thank Greg KH for helpful advice.
> ---
>  drivers/usb/mon/mon_bin.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index 9ca9305243fe..509cd1b8ff13 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -1250,12 +1250,16 @@ static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
>  	struct mon_reader_bin *rp = vmf->vma->vm_private_data;
>  	unsigned long offset, chunk_idx;
>  	struct page *pageptr;
> -
> +	unsigned long flags;
> +	spin_lock_irqsave(&rp->b_lock, flags);

Nit, you still need the blank line before spin_lock_irqsave() here,
right?

>  	offset = vmf->pgoff << PAGE_SHIFT;
> -	if (offset >= rp->b_size)
> +	if (offset >= rp->b_size) {
> +		spin_unlock_irqrestore(&rp->b_lock, flags);
>  		return VM_FAULT_SIGBUS;
> +	}
>  	chunk_idx = offset / CHUNK_SIZE;
>  	pageptr = rp->b_vec[chunk_idx].pg;
> +	spin_unlock_irqrestore(&rp->b_lock, flags);
>  	get_page(pageptr);
>  	vmf->page = pageptr;

Shouldn't the unlock go here, not 2 lines above as you are still
modifying things touched by rp.

thanks,

greg k-h