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

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

[Gui-Dong Han]

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);
 	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;
 	return 0;
-- 
2.34.1

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@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

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

[Gui-Dong Han]

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