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
