Cmnt: [SRU][J][PATCH v2 1/2] smb: client: fix potential UAF in cifs_debug_files_proc_show()

[Yuxuan Luo]

On Thu, Feb 13, 2025 at 04:40:52PM +0900, Koichiro Den wrote:
> On Tue, Feb 11, 2025 at 09:12:29PM GMT, Yuxuan Luo wrote:
> > From: Paulo Alcantara <pc at manguebit.com>
> > 
> > Skip sessions that are being teared down (status == SES_EXITING) to
> > avoid UAF.
> > 
> > Cc: stable at vger.kernel.org
> > Signed-off-by: Paulo Alcantara (Red Hat) <pc at manguebit.com>
> > Signed-off-by: Steve French <stfrench at microsoft.com>
> > (backported from commit ca545b7f0823f19db0f1148d59bc5e1a56634502)
> > [yuxuan.luo:
> > - cifs_debug.c: ignored context conflicts and added new lines.
> > - cifsglob.h:
> >   - Use GlobalMid_Lock instead of ses_lock.
> 
> I wondered if it might be helpful to note that Jammy does not include
> upstream commit:
> 080dc5e5656c ("cifs: take cifs_tcp_ses_lock for statuschecks")(v5.17-rc1~57^2~6)
> and here we should choose GlobalMid_Lock, in a similar manner to e.g.:
> 0060a4f28a9e ("cifs: fix missing spinlock around update to ses->status")(v5.14-rc1~137^2~1)
> to align with the code base. Correct me if I'm missing something.
> 

What you note here totally makes sense, and I concur with your approach.
To who might apply this patch and dissatisfy with this cover letter,
ping me and I'll include these two commits in the next revision.

> >   - Use status instead of ses_status.
> >   - Use CifsExiting instead of SES_EXITING.
> > ]
> > CVE-2024-35864/CVE-2024-26928
> > Signed-off-by: Yuxuan Luo <yuxuan.luo at canonical.com>
> > ---
> >  fs/cifs/cifs_debug.c |  2 ++
> >  fs/cifs/cifsglob.h   | 10 ++++++++++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
> > index efd4affd8218a..87c9fc98dfdec 100644
> > --- a/fs/cifs/cifs_debug.c
> > +++ b/fs/cifs/cifs_debug.c
> > @@ -183,6 +183,8 @@ static int cifs_debug_files_proc_show(struct seq_file *m, void *v)
> >  	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
> >  		list_for_each(tmp, &server->smb_ses_list) {
> >  			ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
> > +			if (cifs_ses_exiting(ses))
> > +				continue;
> >  			list_for_each(tmp1, &ses->tcon_list) {
> >  				tcon = list_entry(tmp1, struct cifs_tcon, tcon_list);
> >  				spin_lock(&tcon->open_file_lock);
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index 2ee67a27020d9..7fa69d2a25502 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -2041,4 +2041,14 @@ static inline struct scatterlist *cifs_sg_set_buf(struct scatterlist *sg,
> >  	return sg;
> >  }
> >  
> > +static inline bool cifs_ses_exiting(struct cifs_ses *ses)
> > +{
> > +	bool ret;
> > +
> > +	spin_lock(&GlobalMid_Lock);
> > +	ret = ses->status == CifsExiting;
> > +	spin_unlock(&GlobalMid_Lock);
> > +	return ret;
> > +}
> > +
> >  #endif	/* _CIFS_GLOB_H */
> > -- 
> > 2.43.0
> > 
> > 
> > -- 
> > kernel-team mailing list
> > kernel-team at lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team