RE: [PATCH v3] ksmbd: fix use-after-free and NULL deref in smb_grant_oplock()

[Werner Kasselman]

Hi Steve,

Thanks for flagging. I reviewed the Sashiko comments, here's a summary:

1) UAF risk on fp from earlier assignment, not a concern. The caller (smb2_open) holds a reference to fp throughout smb_grant_oplock(), so it cannot be freed during execution.

2) RCU locking in lease list iteration (find_same_lease_key, lookup_lease_in_table dropping rcu_read_unlock mid-traversal). This is a pre-existing pattern, not introduced by this patch. Worth a separate audit but out of scope for this fix imho.

3) NULL deref window on opinfo->o_fp: This is exactly the bug this patch fixes, i.e. concurrent readers could see opinfo on the lease list before o_fp was assigned.

4) Reference count management in error paths: opinfo_put() usage is unchanged by this patch. The reordering moves o_fp assignment before list publication but doesn't change refcount semantics.

Points 1, 2, and 4 are either false positives or pre-existing patterns outside the scope of this fix, happy to be corrected though.

Werner

-----Original Message-----
From: Steve French <smfrench@xxxxxxxxx> 
Sent: Wednesday, 18 March 2026 7:27 AM
To: ChenXiaoSong <chenxiaosong@xxxxxxxxxxxxxxxx>
Cc: Werner Kasselman <werner@xxxxxxxxxx>; linux-cifs@xxxxxxxxxxxxxxx; linkinjeon@xxxxxxxxxx; senozhatsky@xxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v3] ksmbd: fix use-after-free and NULL deref in smb_grant_oplock()

I see some AI review comments from Sashiko:
https://sashiko.dev/#/patchset/20260317065253.1743552-1-werner%40verivus.com

On Tue, Mar 17, 2026 at 3:17 AM ChenXiaoSong <chenxiaosong@xxxxxxxxxxxxxxxx> wrote:
>
> Looks good to me so far. Others can continue the review.
>
> Thanks,
> ChenXiaoSong <chenxiaosong@xxxxxxxxxx>
>
> On 3/17/26 14:52, Werner Kasselman wrote:
> > smb_grant_oplock() has two issues in the oplock publication sequence:
> >
> > 1) opinfo is linked into ci->m_op_list (via opinfo_add) before
> >     add_lease_global_list() is called.  If add_lease_global_list()
> >     fails (kmalloc returns NULL), the error path frees the opinfo
> >     via __free_opinfo() while it is still linked in ci->m_op_list.
> >     Concurrent m_op_list readers (opinfo_get_list, or direct iteration
> >     in smb_break_all_levII_oplock) dereference the freed node.
> >
> > 2) opinfo->o_fp is assigned after add_lease_global_list() publishes
> >     the opinfo on the global lease list.  A concurrent
> >     find_same_lease_key() can walk the lease list and dereference
> >     opinfo->o_fp->f_ci while o_fp is still NULL.
> >
> > Fix by restructuring the publication sequence to eliminate 
> > post-publish
> > failure:
> >
> > - Set opinfo->o_fp before any list publication (fixes NULL deref).
> > - Preallocate lease_table via alloc_lease_table() before opinfo_add()
> >    so add_lease_global_list() becomes infallible after publication.
> > - Keep the original m_op_list publication order (opinfo_add before
> >    lease list) so concurrent opens via same_client_has_lease() and
> >    opinfo_get_list() still see the in-flight grant.
> > - Use opinfo_put() instead of __free_opinfo() on err_out so that
> >    the RCU-deferred free path is used.
> >
> > This also requires splitting add_lease_global_list() to take a 
> > preallocated lease_table and changing its return type from int to 
> > void, since it can no longer fail.
> >
> > Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
> > Fixes: 1dfd062caa16 ("ksmbd: fix use-after-free by using call_rcu() 
> > for oplock_info")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Werner Kasselman <werner@xxxxxxxxxxx>



--
Thanks,

Steve