scsi: sg: fix slab-use-after-free Read in sg_release

[PATCH] scsi: sg: fix slab-use-after-free Read in sg_release

Posted by Suraj Sonawane 1 year, 5 months ago

Fix a use-after-free bug in `sg_release`,
detected by syzbot with KASAN:

BUG: KASAN: slab-use-after-free in lock_release+0x151/0xa30
kernel/locking/lockdep.c:5838
__mutex_unlock_slowpath+0xe2/0x750 kernel/locking/mutex.c:912
sg_release+0x1f4/0x2e0 drivers/scsi/sg.c:407

Root Cause:
In `sg_release`, the function `kref_put(&sfp->f_ref, sg_remove_sfp)`
is called before releasing the `open_rel_lock` mutex. The `kref_put`
call may decrement the reference count of `sfp` to zero, triggering
its cleanup through `sg_remove_sfp`. This cleanup includes scheduling
deferred work via `sg_remove_sfp_usercontext`, which ultimately frees
`sfp`.

After `kref_put`, `sg_release` continues to unlock `open_rel_lock` and
may reference `sfp` or `sdp`. If `sfp` has already been freed, this
results in a slab-use-after-free error.

Fix:
The `kref_put(&sfp->f_ref, sg_remove_sfp)` call is moved after unlocking
the `open_rel_lock` mutex. This ensures:
- No references to `sfp` or `sdp` occur after the reference count is  
  decremented.  
- Cleanup functions such as sg_remove_sfp and sg_remove_sfp_usercontext  
  can safely execute without impacting the mutex handling in `sg_release`. 

The fix has been tested and validated by syzbot. This patch closes the bug
reported at the following syzkaller link and ensures proper sequencing of
resource cleanup and mutex operations, eliminating the risk of
use-after-free errors in `sg_release`.

Reported-by: syzbot+7efb5850a17ba6ce098b@syzkaller.appspotmail.com 
Closes: https://syzkaller.appspot.com/bug?extid=7efb5850a17ba6ce098b 
Tested-by: syzbot+7efb5850a17ba6ce098b@syzkaller.appspotmail.com 
Fixes: cc833acbee9d ("sg: O_EXCL and other lock handling ")
Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
---
 drivers/scsi/sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index f86be197f..457d54171 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -393,7 +393,6 @@ sg_release(struct inode *inode, struct file *filp)
 
 	mutex_lock(&sdp->open_rel_lock);
 	scsi_autopm_put_device(sdp->device);
-	kref_put(&sfp->f_ref, sg_remove_sfp);
 	sdp->open_cnt--;
 
 	/* possibly many open()s waiting on exlude clearing, start many;
@@ -405,6 +404,7 @@ sg_release(struct inode *inode, struct file *filp)
 		wake_up_interruptible(&sdp->open_wait);
 	}
 	mutex_unlock(&sdp->open_rel_lock);
+	kref_put(&sfp->f_ref, sg_remove_sfp);
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH] scsi: sg: fix slab-use-after-free Read in sg_release

Posted by Martin K. Petersen 1 year, 5 months ago

On Wed, 20 Nov 2024 18:29:44 +0530, Suraj Sonawane wrote:

> Fix a use-after-free bug in `sg_release`,
> detected by syzbot with KASAN:
> 
> BUG: KASAN: slab-use-after-free in lock_release+0x151/0xa30
> kernel/locking/lockdep.c:5838
> __mutex_unlock_slowpath+0xe2/0x750 kernel/locking/mutex.c:912
> sg_release+0x1f4/0x2e0 drivers/scsi/sg.c:407
> 
> [...]

Applied to 6.13/scsi-fixes, thanks!

[1/1] scsi: sg: fix slab-use-after-free Read in sg_release
      https://git.kernel.org/mkp/scsi/c/f10593ad9bc3

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: sg: fix slab-use-after-free Read in sg_release

Posted by Suraj Sonawane 1 year, 5 months ago

On 12/5/24 07:47, Martin K. Petersen wrote:
> On Wed, 20 Nov 2024 18:29:44 +0530, Suraj Sonawane wrote:
> 
>> Fix a use-after-free bug in `sg_release`,
>> detected by syzbot with KASAN:
>>
>> BUG: KASAN: slab-use-after-free in lock_release+0x151/0xa30
>> kernel/locking/lockdep.c:5838
>> __mutex_unlock_slowpath+0xe2/0x750 kernel/locking/mutex.c:912
>> sg_release+0x1f4/0x2e0 drivers/scsi/sg.c:407
>>
>> [...]
> 
> Applied to 6.13/scsi-fixes, thanks!
> 
> [1/1] scsi: sg: fix slab-use-after-free Read in sg_release
>        https://git.kernel.org/mkp/scsi/c/f10593ad9bc3
> 

Thank you for applying the patch and for the update.

Best regards,
Suraj Sonawane

Re: [PATCH] scsi: sg: fix slab-use-after-free Read in sg_release

Posted by Suraj Sonawane 1 year, 5 months ago

On 11/20/24 18:29, Suraj Sonawane wrote:
> Fix a use-after-free bug in `sg_release`,
> detected by syzbot with KASAN:
> 
> BUG: KASAN: slab-use-after-free in lock_release+0x151/0xa30
> kernel/locking/lockdep.c:5838
> __mutex_unlock_slowpath+0xe2/0x750 kernel/locking/mutex.c:912
> sg_release+0x1f4/0x2e0 drivers/scsi/sg.c:407
> 
> Root Cause:
> In `sg_release`, the function `kref_put(&sfp->f_ref, sg_remove_sfp)`
> is called before releasing the `open_rel_lock` mutex. The `kref_put`
> call may decrement the reference count of `sfp` to zero, triggering
> its cleanup through `sg_remove_sfp`. This cleanup includes scheduling
> deferred work via `sg_remove_sfp_usercontext`, which ultimately frees
> `sfp`.
> 
> After `kref_put`, `sg_release` continues to unlock `open_rel_lock` and
> may reference `sfp` or `sdp`. If `sfp` has already been freed, this
> results in a slab-use-after-free error.
> 
> Fix:
> The `kref_put(&sfp->f_ref, sg_remove_sfp)` call is moved after unlocking
> the `open_rel_lock` mutex. This ensures:
> - No references to `sfp` or `sdp` occur after the reference count is
>    decremented.
> - Cleanup functions such as sg_remove_sfp and sg_remove_sfp_usercontext
>    can safely execute without impacting the mutex handling in `sg_release`.
> 
> The fix has been tested and validated by syzbot. This patch closes the bug
> reported at the following syzkaller link and ensures proper sequencing of
> resource cleanup and mutex operations, eliminating the risk of
> use-after-free errors in `sg_release`.
> 
> Reported-by: syzbot+7efb5850a17ba6ce098b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7efb5850a17ba6ce098b
> Tested-by: syzbot+7efb5850a17ba6ce098b@syzkaller.appspotmail.com
> Fixes: cc833acbee9d ("sg: O_EXCL and other lock handling ")
> Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com>
> ---
>   drivers/scsi/sg.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index f86be197f..457d54171 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -393,7 +393,6 @@ sg_release(struct inode *inode, struct file *filp)
>   
>   	mutex_lock(&sdp->open_rel_lock);
>   	scsi_autopm_put_device(sdp->device);
> -	kref_put(&sfp->f_ref, sg_remove_sfp);
>   	sdp->open_cnt--;
>   
>   	/* possibly many open()s waiting on exlude clearing, start many;
> @@ -405,6 +404,7 @@ sg_release(struct inode *inode, struct file *filp)
>   		wake_up_interruptible(&sdp->open_wait);
>   	}
>   	mutex_unlock(&sdp->open_rel_lock);
> +	kref_put(&sfp->f_ref, sg_remove_sfp);
>   	return 0;
>   }
>   

Hello!

I wanted to follow up on the patch I submitted. I was wondering if you 
had a chance to review it and if there are any comments or feedback.

Thank you for your time and consideration. I look forward to your response.

Best regards,
Suraj Sonawane

Re: [PATCH] scsi: sg: fix slab-use-after-free Read in sg_release

Posted by Bart Van Assche 1 year, 5 months ago

On 11/25/24 6:30 AM, Suraj Sonawane wrote:
> Hello!

Which person are you addressing with this email?

> I wanted to follow up on the patch I submitted. I was wondering if you 
> had a chance to review it and if there are any comments or feedback.

Sending a ping after 5 days is too quick. I think that you should wait
at least a week before sending a ping.

Bart.

Re: [PATCH] scsi: sg: fix slab-use-after-free Read in sg_release

Posted by Suraj Sonawane 1 year, 5 months ago

On 11/25/24 22:33, Bart Van Assche wrote:
> On 11/25/24 6:30 AM, Suraj Sonawane wrote:
>> Hello!
> 
> Which person are you addressing with this email?
> 
I apologize for not clarifying earlier—my email was intended for 
everyone who might be reviewing the patch.

>> I wanted to follow up on the patch I submitted. I was wondering if you 
>> had a chance to review it and if there are any comments or feedback.
> 
> Sending a ping after 5 days is too quick. I think that you should wait
> at least a week before sending a ping.
> 

Thank you for pointing out that sending a follow-up after five days 
might be too soon. I truly appreciate your guidance on this. The reason 
for my follow-up is that, as a mentee in the Linux Kernel Bug Fixing 
Program, I need to include updates on my contributions in my progress 
report. I’ll make sure to wait at least a week before sending any future 
follow-ups.

> Bart.
>

Re: [PATCH] scsi: sg: fix slab-use-after-free Read in sg_release

Posted by Bart Van Assche 1 year, 5 months ago

On 11/20/24 4:59 AM, Suraj Sonawane wrote:
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index f86be197f..457d54171 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -393,7 +393,6 @@ sg_release(struct inode *inode, struct file *filp)
>   
>   	mutex_lock(&sdp->open_rel_lock);
>   	scsi_autopm_put_device(sdp->device);
> -	kref_put(&sfp->f_ref, sg_remove_sfp);
>   	sdp->open_cnt--;
>   
>   	/* possibly many open()s waiting on exlude clearing, start many;
> @@ -405,6 +404,7 @@ sg_release(struct inode *inode, struct file *filp)
>   		wake_up_interruptible(&sdp->open_wait);
>   	}
>   	mutex_unlock(&sdp->open_rel_lock);
> +	kref_put(&sfp->f_ref, sg_remove_sfp);
>   	return 0;
>   }

Reviewed-by: Bart Van Assche <bvanassche@acm.org>