Re: [PATCH] smb: client: reject lease breaks with reserved NewLeaseState bits set

On this one, Sashiko calls attention to a potential problem...

On 8/17/26 12:16 PM, Frank Sorenson wrote:
> smb2_tcon_has_lease() and smb2_tcon_find_pending_open_lease() assign the
> server-supplied __le32 NewLeaseState field directly to a __u8 local
> variable, silently dropping bits 8-31.  Valid SMB2 lease-state values use
> only the lower three bits (READ_CACHING=0x01, HANDLE_CACHING=0x02,
> WRITE_CACHING=0x04); a server that sets reserved upper bits would have
> those bits silently discarded, causing the client to record an incorrect
> lease state and potentially miss cache invalidations.
>
> Decode NewLeaseState to a u32 first and reject the response if any
> reserved bits are set before narrowing to __u8.

> -	lease_state = le32_to_cpu(rsp->NewLeaseState);
> +	ls = le32_to_cpu(rsp->NewLeaseState);
> +	if (ls & ~(SMB2_LEASE_READ_CACHING_HE | SMB2_LEASE_HANDLE_CACHING_HE |
> +		   SMB2_LEASE_WRITE_CACHING_HE)) {
> +		cifs_dbg(VFS | ONCE, "%s: invalid NewLeaseState 0x%x\n",
> +			 __func__, ls);
> +		return false;

 From Sashiko:
======================
If we return false here, will the demultiplex thread in receive_fd()
treat this lease break as an unknown frame and drop it without sending
an acknowledgment?
Looking at the caller smb2_is_valid_lease_break(), returning false causes
the packet to fall through to the unhandled frame path in receive_fd():
fs/smb/client/connect.c:receive_fd() {
     ...
     } else {
         cifs_server_dbg(VFS, "No task to wake, unknown frame received! ...
         cifs_dump_mem("Received Data is: ", bufs[i], ...
     }
     ...
}
Since the packet is dropped entirely, does this mean the client never
downgrades its lease and never sends the required lease break acknowledgment
to the server?
Could this lead to a split-brain scenario where the server forcefully 
revokes
the lease after a timeout and grants access to other clients, while this
client incorrectly assumes it still holds the caching lease?
Would it be safer to mask out the invalid bits and process the valid ones,
or downgrade the lease to SMB2_LEASE_NONE (0) and acknowledge it, rather
than completely dropping the protocol message?

========================

There's a similar complaint later as well...


I can resend with either masking out the invalid bits or downgrading the 
lease...  Masking off the invalid bits makes the most sense to me.  
What's the preference?


> +	}
> +	lease_state = (__u8)ls;
>   
>   	list_for_each_entry(cfile, &tcon->openFileList, tlist) {
>   		cinode = CIFS_I(d_inode(cfile->dentry));
> @@ -609,11 +617,20 @@ static struct cifs_pending_open *
>   smb2_tcon_find_pending_open_lease(struct cifs_tcon *tcon,
>   				  struct smb2_lease_break *rsp)
>   {
> -	__u8 lease_state = le32_to_cpu(rsp->NewLeaseState);
> +	u32 ls = le32_to_cpu(rsp->NewLeaseState);
>   	int ack_req = le32_to_cpu(rsp->Flags &
>   				  SMB2_NOTIFY_BREAK_LEASE_FLAG_ACK_REQUIRED);
>   	struct cifs_pending_open *open;
>   	struct cifs_pending_open *found = NULL;
> +	__u8 lease_state;
> +
> +	if (ls & ~(SMB2_LEASE_READ_CACHING_HE | SMB2_LEASE_HANDLE_CACHING_HE |
> +		   SMB2_LEASE_WRITE_CACHING_HE)) {
> +		cifs_dbg(VFS | ONCE, "%s: invalid NewLeaseState 0x%x\n",
> +			 __func__, ls);
> +		return NULL;

sashiko again

Does this validation logic successfully reject invalid lease breaks for
cached directories?
If this function and smb2_tcon_has_lease() both fail to find or reject the
lease (returning NULL and false respectively), the caller
smb2_is_valid_lease_break() falls through to check cached directories:
fs/smb/client/smb2misc.c:smb2_is_valid_lease_break() {
     ...
     if (smb2_tcon_has_lease(tcon, rsp)) {
         ...
     }
     open = smb2_tcon_find_pending_open_lease(tcon, rsp);
     if (open) {
         ...
     }
     if (cached_dir_lease_break(tcon, rsp->LeaseKey)) {
         spin_unlock(&cifs_tcp_ses_lock);
         return true;
     }
     ...
}
Since cached_dir_lease_break() does not check NewLeaseState and simply
drops the lease if the key matches, will an invalid lease break directed
at a cached directory bypass the new rejection checks and be successfully
processed?
Should the validation be placed at the top of smb2_is_valid_lease_break()
to consistently cover all lease types?
=============================




> +	}
> +	lease_state = (__u8)ls;
>   
>   	list_for_each_entry(open, &tcon->pending_opens, olist) {
>   		if (memcmp(open->lease_key, rsp->LeaseKey,



-- 
Frank Sorenson
[email protected]
Principal Software Maintenance Engineer, filesystems
Red Hat