Re: [PATCH] cifs: fix bad fids sent over wire

[Tom Talpey <tom@xxxxxxxxxx>]

On 3/20/2022 8:20 PM, Paulo Alcantara wrote:
> The client used to partially convert the fids to le64, while storing
> or sending them by using host endianness.  This broke the client on
> big-endian machines.  Instead of converting them to le64, store them
> verbatim and then avoid byteswapping when sending them over wire.
>
> Signed-off-by: Paulo Alcantara (SUSE) <pc@xxxxxx>
> ---
>   fs/cifs/smb2misc.c |  4 ++--
>   fs/cifs/smb2ops.c  |  8 +++----
>   fs/cifs/smb2pdu.c  | 53 ++++++++++++++++++++--------------------------
>   3 files changed, 29 insertions(+), 36 deletions(-)
>
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index b25623e3fe3d..3b7c636be377 100644
> --- a/fs/cifs/smb2misc.c
> +++ b/fs/cifs/smb2misc.c
> @@ -832,8 +832,8 @@ smb2_handle_cancelled_mid(struct mid_q_entry *mid, struct TCP_Server_Info *serve
>   	rc = __smb2_handle_cancelled_cmd(tcon,
>   					 le16_to_cpu(hdr->Command),
>   					 le64_to_cpu(hdr->MessageId),
> -					 le64_to_cpu(rsp->PersistentFileId),
> -					 le64_to_cpu(rsp->VolatileFileId));
> +					 rsp->PersistentFileId,
> +					 rsp->VolatileFileId);

This conflicts with the statement "store them verbatim". Because the
rsp->{Persistent,Volatile}FileId fields are u64 (integer) types,
they are not being stored verbatim, they are being manipulated
by the CPU load/store instructions. Storing them into a u8[8]
array is more to the point.

If course, if the rsp structure is purely private to the code, then
the structure element type is similarly private. But a debugger, or
a future structure reference, may once again get it wrong

Are you rejecting the idea of using a byte array?

>   	if (rc)
>   		cifs_put_tcon(tcon);
>   
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 0ecd6e1832a1..c122530e5043 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -900,8 +900,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>   	atomic_inc(&tcon->num_remote_opens);
>   
>   	o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
> -	oparms.fid->persistent_fid = le64_to_cpu(o_rsp->PersistentFileId);
> -	oparms.fid->volatile_fid = le64_to_cpu(o_rsp->VolatileFileId);
> +	oparms.fid->persistent_fid = o_rsp->PersistentFileId;
> +	oparms.fid->volatile_fid = o_rsp->VolatileFileId;
>   #ifdef CONFIG_CIFS_DEBUG2
>   	oparms.fid->mid = le64_to_cpu(o_rsp->hdr.MessageId);
>   #endif /* CIFS_DEBUG2 */
> @@ -2410,8 +2410,8 @@ smb2_query_dir_first(const unsigned int xid, struct cifs_tcon *tcon,
>   		cifs_dbg(FYI, "query_dir_first: open failed rc=%d\n", rc);
>   		goto qdf_free;
>   	}
> -	fid->persistent_fid = le64_to_cpu(op_rsp->PersistentFileId);
> -	fid->volatile_fid = le64_to_cpu(op_rsp->VolatileFileId);
> +	fid->persistent_fid = op_rsp->PersistentFileId;
> +	fid->volatile_fid = op_rsp->VolatileFileId;
>   
>   	/* Anything else than ENODATA means a genuine error */
>   	if (rc && rc != -ENODATA) {
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 7e7909b1ae11..178af70331f8 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -2734,13 +2734,10 @@ int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
>   		goto err_free_req;
>   	}
>   
> -	trace_smb3_posix_mkdir_done(xid, le64_to_cpu(rsp->PersistentFileId),
> -				    tcon->tid,
> -				    ses->Suid, CREATE_NOT_FILE,
> -				    FILE_WRITE_ATTRIBUTES);
> +	trace_smb3_posix_mkdir_done(xid, rsp->PersistentFileId, tcon->tid, ses->Suid,
> +				    CREATE_NOT_FILE, FILE_WRITE_ATTRIBUTES);
>   
> -	SMB2_close(xid, tcon, le64_to_cpu(rsp->PersistentFileId),
> -		   le64_to_cpu(rsp->VolatileFileId));
> +	SMB2_close(xid, tcon, rsp->PersistentFileId, rsp->VolatileFileId);
>   
>   	/* Eventually save off posix specific response info and timestaps */
>   
> @@ -3009,14 +3006,12 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path,
>   	} else if (rsp == NULL) /* unlikely to happen, but safer to check */
>   		goto creat_exit;
>   	else
> -		trace_smb3_open_done(xid, le64_to_cpu(rsp->PersistentFileId),
> -				     tcon->tid,
> -				     ses->Suid, oparms->create_options,
> -				     oparms->desired_access);
> +		trace_smb3_open_done(xid, rsp->PersistentFileId, tcon->tid, ses->Suid,
> +				     oparms->create_options, oparms->desired_access);
>   
>   	atomic_inc(&tcon->num_remote_opens);
> -	oparms->fid->persistent_fid = le64_to_cpu(rsp->PersistentFileId);
> -	oparms->fid->volatile_fid = le64_to_cpu(rsp->VolatileFileId);
> +	oparms->fid->persistent_fid = rsp->PersistentFileId;
> +	oparms->fid->volatile_fid = rsp->VolatileFileId;
>   	oparms->fid->access = oparms->desired_access;
>   #ifdef CONFIG_CIFS_DEBUG2
>   	oparms->fid->mid = le64_to_cpu(rsp->hdr.MessageId);
> @@ -3313,8 +3308,8 @@ SMB2_close_init(struct cifs_tcon *tcon, struct TCP_Server_Info *server,
>   	if (rc)
>   		return rc;
>   
> -	req->PersistentFileId = cpu_to_le64(persistent_fid);
> -	req->VolatileFileId = cpu_to_le64(volatile_fid);
> +	req->PersistentFileId = persistent_fid;
> +	req->VolatileFileId = volatile_fid;
>   	if (query_attrs)
>   		req->Flags = SMB2_CLOSE_FLAG_POSTQUERY_ATTRIB;
>   	else
> @@ -3677,8 +3672,8 @@ SMB2_notify_init(const unsigned int xid, struct smb_rqst *rqst,
>   	if (rc)
>   		return rc;
>   
> -	req->PersistentFileId = cpu_to_le64(persistent_fid);
> -	req->VolatileFileId = cpu_to_le64(volatile_fid);
> +	req->PersistentFileId = persistent_fid;
> +	req->VolatileFileId = volatile_fid;
>   	/* See note 354 of MS-SMB2, 64K max */
>   	req->OutputBufferLength =
>   		cpu_to_le32(SMB2_MAX_BUFFER_SIZE - MAX_SMB2_HDR_SIZE);
> @@ -3951,8 +3946,8 @@ SMB2_flush_init(const unsigned int xid, struct smb_rqst *rqst,
>   	if (rc)
>   		return rc;
>   
> -	req->PersistentFileId = cpu_to_le64(persistent_fid);
> -	req->VolatileFileId = cpu_to_le64(volatile_fid);
> +	req->PersistentFileId = persistent_fid;
> +	req->VolatileFileId = volatile_fid;
>   
>   	iov[0].iov_base = (char *)req;
>   	iov[0].iov_len = total_len;
> @@ -4033,8 +4028,8 @@ smb2_new_read_req(void **buf, unsigned int *total_len,
>   	shdr = &req->hdr;
>   	shdr->Id.SyncId.ProcessId = cpu_to_le32(io_parms->pid);
>   
> -	req->PersistentFileId = cpu_to_le64(io_parms->persistent_fid);
> -	req->VolatileFileId = cpu_to_le64(io_parms->volatile_fid);
> +	req->PersistentFileId = io_parms->persistent_fid;
> +	req->VolatileFileId = io_parms->volatile_fid;
>   	req->ReadChannelInfoOffset = 0; /* reserved */
>   	req->ReadChannelInfoLength = 0; /* reserved */
>   	req->Channel = 0; /* reserved */
> @@ -4094,8 +4089,8 @@ smb2_new_read_req(void **buf, unsigned int *total_len,
>   			 */
>   			shdr->SessionId = cpu_to_le64(0xFFFFFFFFFFFFFFFF);
>   			shdr->Id.SyncId.TreeId = cpu_to_le32(0xFFFFFFFF);
> -			req->PersistentFileId = cpu_to_le64(0xFFFFFFFFFFFFFFFF);
> -			req->VolatileFileId = cpu_to_le64(0xFFFFFFFFFFFFFFFF);
> +			req->PersistentFileId = (u64)-1;
> +			req->VolatileFileId = (u64)-1;
>   		}
>   	}
>   	if (remaining_bytes > io_parms->length)
> @@ -4312,10 +4307,8 @@ SMB2_read(const unsigned int xid, struct cifs_io_parms *io_parms,
>   					    io_parms->offset, io_parms->length,
>   					    rc);
>   		} else
> -			trace_smb3_read_done(xid,
> -					     le64_to_cpu(req->PersistentFileId),
> -					     io_parms->tcon->tid, ses->Suid,
> -					     io_parms->offset, 0);
> +			trace_smb3_read_done(xid, req->PersistentFileId, io_parms->tcon->tid,
> +					     ses->Suid, io_parms->offset, 0);
>   		free_rsp_buf(resp_buftype, rsp_iov.iov_base);
>   		cifs_small_buf_release(req);
>   		return rc == -ENODATA ? 0 : rc;
> @@ -4463,8 +4456,8 @@ smb2_async_writev(struct cifs_writedata *wdata,
>   	shdr = (struct smb2_hdr *)req;
>   	shdr->Id.SyncId.ProcessId = cpu_to_le32(wdata->cfile->pid);
>   
> -	req->PersistentFileId = cpu_to_le64(wdata->cfile->fid.persistent_fid);
> -	req->VolatileFileId = cpu_to_le64(wdata->cfile->fid.volatile_fid);
> +	req->PersistentFileId = wdata->cfile->fid.persistent_fid;
> +	req->VolatileFileId = wdata->cfile->fid.volatile_fid;
>   	req->WriteChannelInfoOffset = 0;
>   	req->WriteChannelInfoLength = 0;
>   	req->Channel = 0;
> @@ -4615,8 +4608,8 @@ SMB2_write(const unsigned int xid, struct cifs_io_parms *io_parms,
>   
>   	req->hdr.Id.SyncId.ProcessId = cpu_to_le32(io_parms->pid);
>   
> -	req->PersistentFileId = cpu_to_le64(io_parms->persistent_fid);
> -	req->VolatileFileId = cpu_to_le64(io_parms->volatile_fid);
> +	req->PersistentFileId = io_parms->persistent_fid;
> +	req->VolatileFileId = io_parms->volatile_fid;
>   	req->WriteChannelInfoOffset = 0;
>   	req->WriteChannelInfoLength = 0;
>   	req->Channel = 0;