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

[Stewart Hore]

On Thu, Feb 06, 2025 at 08:10:51PM -0500, 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.
>   - 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 efd4affd8218..738891f83310 100644
> --- a/fs/cifs/cifs_debug.c
> +++ b/fs/cifs/cifs_debug.c
> @@ -182,6 +182,8 @@ static int cifs_debug_files_proc_show(struct seq_file *m, void *v)
>  	spin_lock(&cifs_tcp_ses_lock);
>  	list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) {
>  		list_for_each(tmp, &server->smb_ses_list) {
> +			if (cifs_ses_exiting(ses))
> +				continue;
>  			ses = list_entry(tmp, struct cifs_ses, smb_ses_list);
>  			list_for_each(tmp1, &ses->tcon_list) {
>  				tcon = list_entry(tmp1, struct cifs_tcon, tcon_list);
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 2ee67a27020d..7fa69d2a2550 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

The call to `cifs_ses_exiting(ses)` will dereference the `ses` pointer
before it has been assigned.

Suggested change, move the `if(cifs_ses_exiting(ses))` to after `ses`
assignment. E.g.:

```
>  		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) {
```

--
kernel-team mailing list
kernel-team at lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team