| Message ID | 20241120125944.88095-1-surajsonawane0215@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: sg: fix slab-use-after-free Read in sg_release | expand |
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>
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
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.
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. >
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
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; }