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

[Gui-Dong Han <2045gemini@gmail.com>]

Hi,


Fixed in the patch v3!

Thank you for helpful advice.


Thanks,

Han

On 4/1/2024 下午10:16, Greg KH wrote:
> 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@vger.kernel.org
>> Reported-by: BassCheck <bass@buaa.edu.cn>
>> Signed-off-by: Gui-Dong Han <2045gemini@gmail.com>
>> ---
>> 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