[PATCH] cifs: fix bad fids sent over wire

[PATCH] cifs: fix bad fids sent over wire

[Paulo Alcantara]

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@cjr.nz>
---
 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);
 	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;
-- 
2.35.1

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

[Tom Talpey]

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@cjr.nz>
> ---
>   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;

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

[Paulo Alcantara]

Tom Talpey <tom@talpey.com> writes:

> 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@cjr.nz>
>> ---
>>   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.

Yes, makes sense.

> 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?

No.  That would work, too.  I was just trying to avoid changing a lot of
places and eventually making it harder to backport.

I'll go with the byte array then.

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

[Tom Talpey]

On 3/21/2022 8:30 AM, Paulo Alcantara wrote:
> Tom Talpey <tom@talpey.com> writes:
> 
>> 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@cjr.nz>
>>> ---
>>>    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.
> 
> Yes, makes sense.
> 
>> 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?
> 
> No.  That would work, too.  I was just trying to avoid changing a lot of
> places and eventually making it harder to backport.
> 
> I'll go with the byte array then.

If you want to reduce a bit of code change, you could let the
compiler generate the loads and stores via memcpy, with (perhaps)
a struct { u8[8] } instead of the bare array.

Tom.

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

[Paulo Alcantara]

Tom Talpey <tom@talpey.com> writes:

> If you want to reduce a bit of code change, you could let the
> compiler generate the loads and stores via memcpy, with (perhaps)
> a struct { u8[8] } instead of the bare array.

Thanks for the suggestions.  It turned out to be more changes than I
expected.  In this case, I'm gonna fix some remaining sparse warnings
from my last patch and fix the commit message as you suggested.

Of course, we can refactor this out later and start with something like:

	struct smb2_fid {
	        __u8 Persistent[8];
	        __u8 Volatile[8];
	} __packed;

and then replace u64 integers with the above.

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

[Tom Talpey]

On 3/21/2022 11:01 AM, Steve French wrote:
> Wouldn't u64 for these with no conversion (the original code was 
> consistent before half of use of fields converted to le64) be faster, 
> easier?

I guess that's what Paulo is going to do. It's certainly faster and
easier, but I predict it won't close the door on future code issues.
Someone is going to try to byteswap, or treat it as an integer somehow.
I'm advocating for correctness. But fast and easy are your call.

Tom.

> On Mon, Mar 21, 2022, 07:55 Tom Talpey <tom@talpey.com 
> <mailto:tom@talpey.com>> wrote:
> 
>     On 3/21/2022 8:30 AM, Paulo Alcantara wrote:
>      > Tom Talpey <tom@talpey.com <mailto:tom@talpey.com>> writes:
>      >
>      >> 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@cjr.nz
>     <mailto:pc@cjr.nz>>
>      >>> ---
>      >>>    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.
>      >
>      > Yes, makes sense.
>      >
>      >> 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?
>      >
>      > No.  That would work, too.  I was just trying to avoid changing a
>     lot of
>      > places and eventually making it harder to backport.
>      >
>      > I'll go with the byte array then.
> 
>     If you want to reduce a bit of code change, you could let the
>     compiler generate the loads and stores via memcpy, with (perhaps)
>     a struct { u8[8] } instead of the bare array.
> 
>     Tom.
> 

[PATCH] cifs: fix bad fids sent over wire

[Steve French]

Any comments on Paulo's patch? It fixes an endian conversion problem
that can affect file ids used on big endian archs.  I will add cc:
stable

https://git.cjr.nz/linux.git/commit/?h=cifs-bad-fid-fixes&id=a857bb6b15646a7946fafb281878ddf498107dc3

-- 
Thanks,

Steve

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

[Tom Talpey]

On 3/19/2022 12:23 AM, Steve French wrote:
> Any comments on Paulo's patch? It fixes an endian conversion problem
> that can affect file ids used on big endian archs.  I will add cc:
> stable
> 
> https://git.cjr.nz/linux.git/commit/?h=cifs-bad-fid-fixes&id=a857bb6b15646a7946fafb281878ddf498107dc3

It seems a bad idea to be storing opaque fields in le64 integers, then 
byteswapping them when they go back on the wire. Wouldn't it be better 
to make them u8[8] arrays and just use memcpy/memcmp?

Tom.

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

[Steve French]

They probably should be always 'u64' everywhere (not le64) and change
the code back in fs/smbfs_common/smb2pdu.h (was this due to ksmbd
using the file and converting these fields in fs/smbfs_common) rather
than the ones you changed


On Sat, Mar 19, 2022 at 11:01 AM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Yes, I agreed. Why not simply store them as le64 and avoid the byteswapping altogether?
>
> On 19 March 2022 09:06:55 GMT-03:00, Tom Talpey <tom@talpey.com> wrote:
>>
>>
>> On 3/19/2022 12:23 AM, Steve French wrote:
>>>
>>> Any comments on Paulo's patch? It fixes an endian conversion problem
>>> that can affect file ids used on big endian archs.  I will add cc:
>>> stable
>>>
>>> https://git.cjr.nz/linux.git/commit/?h=cifs-bad-fid-fixes&id=a857bb6b15646a7946fafb281878ddf498107dc3
>>
>>
>> It seems a bad idea to be storing opaque fields in le64 integers, then
>> byteswapping them when they go back on the wire. Wouldn't it be better
>> to make them u8[8] arrays and just use memcpy/memcmp?
>>
>> Tom.



-- 
Thanks,

Steve

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

[Namjae Jeon]

2022-03-20 3:34 GMT+09:00, Steve French <smfrench@gmail.com>:
> They probably should be always 'u64' everywhere (not le64) and change
> the code back in fs/smbfs_common/smb2pdu.h (was this due to ksmbd
> using the file and converting these fields in fs/smbfs_common) rather
> than the ones you changed
I don't understand why only FileId fields should be declared as u64 not le64.
It means that FileID doesn't need byteswap in client? samba seems to
stores them in little-endian byte order.

#define SBVAL(p, ofs, v) PUSH_LE_U64(p, ofs, v)

        SBVAL(outbody.data, 0x40,
              out_file_id_persistent);          /* file id (persistent) */
        SBVAL(outbody.data, 0x48,
              out_file_id_volatile);            /* file id (volatile) */

Am I missing something ?
>
>
> On Sat, Mar 19, 2022 at 11:01 AM Paulo Alcantara <pc@cjr.nz> wrote:
>>
>> Yes, I agreed. Why not simply store them as le64 and avoid the
>> byteswapping altogether?
>>
>> On 19 March 2022 09:06:55 GMT-03:00, Tom Talpey <tom@talpey.com> wrote:
>>>
>>>
>>> On 3/19/2022 12:23 AM, Steve French wrote:
>>>>
>>>> Any comments on Paulo's patch? It fixes an endian conversion problem
>>>> that can affect file ids used on big endian archs.  I will add cc:
>>>> stable
>>>>
>>>> https://git.cjr.nz/linux.git/commit/?h=cifs-bad-fid-fixes&id=a857bb6b15646a7946fafb281878ddf498107dc3
>>>
>>>
>>> It seems a bad idea to be storing opaque fields in le64 integers, then
>>> byteswapping them when they go back on the wire. Wouldn't it be better
>>> to make them u8[8] arrays and just use memcpy/memcmp?
>>>
>>> Tom.
>
>
>
> --
> Thanks,
>
> Steve
>

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

[Tom Talpey]

On 3/19/2022 9:22 PM, Namjae Jeon wrote:
> 2022-03-20 3:34 GMT+09:00, Steve French <smfrench@gmail.com>:
>> They probably should be always 'u64' everywhere (not le64) and change
>> the code back in fs/smbfs_common/smb2pdu.h (was this due to ksmbd
>> using the file and converting these fields in fs/smbfs_common) rather
>> than the ones you changed
> I don't understand why only FileId fields should be declared as u64 not le64.

Because they're opaque to the client.

> It means that FileID doesn't need byteswap in client?

Correct. They are tested only for equality, or are placed in a packet
verbatim and sent back to the server.

> samba seems to
> stores them in little-endian byte order.

Again, unnecessary and dangerous, IMO.

Tom.

> 
> #define SBVAL(p, ofs, v) PUSH_LE_U64(p, ofs, v)
> 
>          SBVAL(outbody.data, 0x40,
>                out_file_id_persistent);          /* file id (persistent) */
>          SBVAL(outbody.data, 0x48,
>                out_file_id_volatile);            /* file id (volatile) */
> 
> Am I missing something ?
>>
>>
>> On Sat, Mar 19, 2022 at 11:01 AM Paulo Alcantara <pc@cjr.nz> wrote:
>>>
>>> Yes, I agreed. Why not simply store them as le64 and avoid the
>>> byteswapping altogether?
>>>
>>> On 19 March 2022 09:06:55 GMT-03:00, Tom Talpey <tom@talpey.com> wrote:
>>>>
>>>>
>>>> On 3/19/2022 12:23 AM, Steve French wrote:
>>>>>
>>>>> Any comments on Paulo's patch? It fixes an endian conversion problem
>>>>> that can affect file ids used on big endian archs.  I will add cc:
>>>>> stable
>>>>>
>>>>> https://git.cjr.nz/linux.git/commit/?h=cifs-bad-fid-fixes&id=a857bb6b15646a7946fafb281878ddf498107dc3
>>>>
>>>>
>>>> It seems a bad idea to be storing opaque fields in le64 integers, then
>>>> byteswapping them when they go back on the wire. Wouldn't it be better
>>>> to make them u8[8] arrays and just use memcpy/memcmp?
>>>>
>>>> Tom.
>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
>>
> 

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

[Steve French]

On Sat, Mar 19, 2022 at 8:45 PM Tom Talpey <tom@talpey.com> wrote:
>
> On 3/19/2022 9:22 PM, Namjae Jeon wrote:
> > 2022-03-20 3:34 GMT+09:00, Steve French <smfrench@gmail.com>:
> >> They probably should be always 'u64' everywhere (not le64) and change
> >> the code back in fs/smbfs_common/smb2pdu.h (was this due to ksmbd
> >> using the file and converting these fields in fs/smbfs_common) rather
> >> than the ones you changed
> > I don't understand why only FileId fields should be declared as u64 not le64.
>
> Because they're opaque to the client.
>
> > It means that FileID doesn't need byteswap in client?
>
> Correct. They are tested only for equality, or are placed in a packet
> verbatim and sent back to the server.
>
> > samba seems to
> > stores them in little-endian byte order.
>
> Again, unnecessary and dangerous, IMO.

Agree - safer to have opaque fields to be host endian, and a tiny bit faster.


-- 
Thanks,

Steve

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

[ronnie sahlberg]

On Sun, Mar 20, 2022 at 7:24 PM Steve French <smfrench@gmail.com> wrote:
>
> They probably should be always 'u64' everywhere (not le64) and change
> the code back in fs/smbfs_common/smb2pdu.h (was this due to ksmbd
> using the file and converting these fields in fs/smbfs_common) rather
> than the ones you changed

I agree, they should not be le64 as that implies there is a byteorder
for this field, so u64 seems better.

(Technically, it should probably not even be an integer and instead be
unsigned char[8]
but that seems like overkill.)


>
>
> On Sat, Mar 19, 2022 at 11:01 AM Paulo Alcantara <pc@cjr.nz> wrote:
> >
> > Yes, I agreed. Why not simply store them as le64 and avoid the byteswapping altogether?
> >
> > On 19 March 2022 09:06:55 GMT-03:00, Tom Talpey <tom@talpey.com> wrote:
> >>
> >>
> >> On 3/19/2022 12:23 AM, Steve French wrote:
> >>>
> >>> Any comments on Paulo's patch? It fixes an endian conversion problem
> >>> that can affect file ids used on big endian archs.  I will add cc:
> >>> stable
> >>>
> >>> https://git.cjr.nz/linux.git/commit/?h=cifs-bad-fid-fixes&id=a857bb6b15646a7946fafb281878ddf498107dc3
> >>
> >>
> >> It seems a bad idea to be storing opaque fields in le64 integers, then
> >> byteswapping them when they go back on the wire. Wouldn't it be better
> >> to make them u8[8] arrays and just use memcpy/memcmp?
> >>
> >> Tom.
>
>
>
> --
> Thanks,
>
> Steve

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

[Tom Talpey]

On 3/20/2022 10:13 PM, ronnie sahlberg wrote:
> On Sun, Mar 20, 2022 at 7:24 PM Steve French <smfrench@gmail.com> wrote:
>>
>> They probably should be always 'u64' everywhere (not le64) and change
>> the code back in fs/smbfs_common/smb2pdu.h (was this due to ksmbd
>> using the file and converting these fields in fs/smbfs_common) rather
>> than the ones you changed
> 
> I agree, they should not be le64 as that implies there is a byteorder
> for this field, so u64 seems better.
> 
> (Technically, it should probably not even be an integer and instead be
> unsigned char[8]
> but that seems like overkill.)

So that was my original suggestion. The FID is not an integer at all.
I don't think it's overkill personally.

Tom.

>> On Sat, Mar 19, 2022 at 11:01 AM Paulo Alcantara <pc@cjr.nz> wrote:
>>>
>>> Yes, I agreed. Why not simply store them as le64 and avoid the byteswapping altogether?
>>>
>>> On 19 March 2022 09:06:55 GMT-03:00, Tom Talpey <tom@talpey.com> wrote:
>>>>
>>>>
>>>> On 3/19/2022 12:23 AM, Steve French wrote:
>>>>>
>>>>> Any comments on Paulo's patch? It fixes an endian conversion problem
>>>>> that can affect file ids used on big endian archs.  I will add cc:
>>>>> stable
>>>>>
>>>>> https://git.cjr.nz/linux.git/commit/?h=cifs-bad-fid-fixes&id=a857bb6b15646a7946fafb281878ddf498107dc3
>>>>
>>>>
>>>> It seems a bad idea to be storing opaque fields in le64 integers, then
>>>> byteswapping them when they go back on the wire. Wouldn't it be better
>>>> to make them u8[8] arrays and just use memcpy/memcmp?
>>>>
>>>> Tom.
>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
>