Re: [PATCH 6.1 149/150] smb3: Replace smb2pdu 1-element arrays with flex-arrays

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2024-01-09 0:36 GMT+09:00, Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>:
> 6.1-stable review patch.  If anyone has any objections, please let me know.
This patch incorrectly changed ksmbd part. This will cause problem
that can not connect windows client. I will send you an updated patch
soon.

Thanks!
>
> ------------------
>
> From: Kees Cook <keescook@xxxxxxxxxxxx>
>
> commit eb3e28c1e89b4984308777231887e41aa8a0151f upstream.
>
> The kernel is globally removing the ambiguous 0-length and 1-element
> arrays in favor of flexible arrays, so that we can gain both compile-time
> and run-time array bounds checking[1].
>
> Replace the trailing 1-element array with a flexible array in the
> following structures:
>
> 	struct smb2_err_rsp
> 	struct smb2_tree_connect_req
> 	struct smb2_negotiate_rsp
> 	struct smb2_sess_setup_req
> 	struct smb2_sess_setup_rsp
> 	struct smb2_read_req
> 	struct smb2_read_rsp
> 	struct smb2_write_req
> 	struct smb2_write_rsp
> 	struct smb2_query_directory_req
> 	struct smb2_query_directory_rsp
> 	struct smb2_set_info_req
> 	struct smb2_change_notify_rsp
> 	struct smb2_create_rsp
> 	struct smb2_query_info_req
> 	struct smb2_query_info_rsp
>
> Replace the trailing 1-element array with a flexible array, but leave
> the existing structure padding:
>
> 	struct smb2_file_all_info
> 	struct smb2_lock_req
>
> Adjust all related size calculations to match the changes to sizeof().
>
> No machine code output or .data section differences are produced after
> these changes.
>
> [1] For lots of details, see both:
>
> https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
>     https://people.kernel.org/kees/bounded-flexible-arrays-in-c
>
> Cc: Steve French <sfrench@xxxxxxxxx>
> Cc: Paulo Alcantara <pc@xxxxxx>
> Cc: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> Cc: Shyam Prasad N <sprasad@xxxxxxxxxxxxx>
> Cc: Tom Talpey <tom@xxxxxxxxxx>
> Cc: Namjae Jeon <linkinjeon@xxxxxxxxxx>
> Cc: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
> Cc: linux-cifs@xxxxxxxxxxxxxxx
> Cc: samba-technical@xxxxxxxxxxxxxxx
> Reviewed-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
>  fs/smb/client/smb2file.c |    2 +-
>  fs/smb/client/smb2misc.c |    2 +-
>  fs/smb/client/smb2ops.c  |   14 +++++++-------
>  fs/smb/client/smb2pdu.c  |   16 +++++++---------
>  fs/smb/client/smb2pdu.h  |    2 +-
>  fs/smb/common/smb2pdu.h  |   42 ++++++++++++++++++++++++------------------
>  fs/smb/server/smb2ops.c  |    8 ++++----
>  fs/smb/server/smb2pdu.c  |    5 ++---
>  8 files changed, 47 insertions(+), 44 deletions(-)
>
> --- a/fs/smb/client/smb2file.c
> +++ b/fs/smb/client/smb2file.c
> @@ -34,7 +34,7 @@ static struct smb2_symlink_err_rsp *syml
>  		len = (u32)err->ErrorContextCount * (offsetof(struct
> smb2_error_context_rsp,
>  							      ErrorContextData) +
>  						     sizeof(struct smb2_symlink_err_rsp));
> -		if (le32_to_cpu(err->ByteCount) < len || iov->iov_len < len +
> sizeof(*err))
> +		if (le32_to_cpu(err->ByteCount) < len || iov->iov_len < len +
> sizeof(*err) + 1)
>  			return ERR_PTR(-EINVAL);
>
>  		p = (struct smb2_error_context_rsp *)err->ErrorData;
> --- a/fs/smb/client/smb2misc.c
> +++ b/fs/smb/client/smb2misc.c
> @@ -113,7 +113,7 @@ static __u32 get_neg_ctxt_len(struct smb
>  	} else if (nc_offset + 1 == non_ctxlen) {
>  		cifs_dbg(FYI, "no SPNEGO security blob in negprot rsp\n");
>  		size_of_pad_before_neg_ctxts = 0;
> -	} else if (non_ctxlen == SMB311_NEGPROT_BASE_SIZE)
> +	} else if (non_ctxlen == SMB311_NEGPROT_BASE_SIZE + 1)
>  		/* has padding, but no SPNEGO blob */
>  		size_of_pad_before_neg_ctxts = nc_offset - non_ctxlen + 1;
>  	else
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -5764,7 +5764,7 @@ struct smb_version_values smb20_values =
>  	.header_size = sizeof(struct smb2_hdr),
>  	.header_preamble_size = 0,
>  	.max_header_size = MAX_SMB2_HDR_SIZE,
> -	.read_rsp_size = sizeof(struct smb2_read_rsp) - 1,
> +	.read_rsp_size = sizeof(struct smb2_read_rsp),
>  	.lock_cmd = SMB2_LOCK,
>  	.cap_unix = 0,
>  	.cap_nt_find = SMB2_NT_FIND,
> @@ -5786,7 +5786,7 @@ struct smb_version_values smb21_values =
>  	.header_size = sizeof(struct smb2_hdr),
>  	.header_preamble_size = 0,
>  	.max_header_size = MAX_SMB2_HDR_SIZE,
> -	.read_rsp_size = sizeof(struct smb2_read_rsp) - 1,
> +	.read_rsp_size = sizeof(struct smb2_read_rsp),
>  	.lock_cmd = SMB2_LOCK,
>  	.cap_unix = 0,
>  	.cap_nt_find = SMB2_NT_FIND,
> @@ -5807,7 +5807,7 @@ struct smb_version_values smb3any_values
>  	.header_size = sizeof(struct smb2_hdr),
>  	.header_preamble_size = 0,
>  	.max_header_size = MAX_SMB2_HDR_SIZE,
> -	.read_rsp_size = sizeof(struct smb2_read_rsp) - 1,
> +	.read_rsp_size = sizeof(struct smb2_read_rsp),
>  	.lock_cmd = SMB2_LOCK,
>  	.cap_unix = 0,
>  	.cap_nt_find = SMB2_NT_FIND,
> @@ -5828,7 +5828,7 @@ struct smb_version_values smbdefault_val
>  	.header_size = sizeof(struct smb2_hdr),
>  	.header_preamble_size = 0,
>  	.max_header_size = MAX_SMB2_HDR_SIZE,
> -	.read_rsp_size = sizeof(struct smb2_read_rsp) - 1,
> +	.read_rsp_size = sizeof(struct smb2_read_rsp),
>  	.lock_cmd = SMB2_LOCK,
>  	.cap_unix = 0,
>  	.cap_nt_find = SMB2_NT_FIND,
> @@ -5849,7 +5849,7 @@ struct smb_version_values smb30_values =
>  	.header_size = sizeof(struct smb2_hdr),
>  	.header_preamble_size = 0,
>  	.max_header_size = MAX_SMB2_HDR_SIZE,
> -	.read_rsp_size = sizeof(struct smb2_read_rsp) - 1,
> +	.read_rsp_size = sizeof(struct smb2_read_rsp),
>  	.lock_cmd = SMB2_LOCK,
>  	.cap_unix = 0,
>  	.cap_nt_find = SMB2_NT_FIND,
> @@ -5870,7 +5870,7 @@ struct smb_version_values smb302_values
>  	.header_size = sizeof(struct smb2_hdr),
>  	.header_preamble_size = 0,
>  	.max_header_size = MAX_SMB2_HDR_SIZE,
> -	.read_rsp_size = sizeof(struct smb2_read_rsp) - 1,
> +	.read_rsp_size = sizeof(struct smb2_read_rsp),
>  	.lock_cmd = SMB2_LOCK,
>  	.cap_unix = 0,
>  	.cap_nt_find = SMB2_NT_FIND,
> @@ -5891,7 +5891,7 @@ struct smb_version_values smb311_values
>  	.header_size = sizeof(struct smb2_hdr),
>  	.header_preamble_size = 0,
>  	.max_header_size = MAX_SMB2_HDR_SIZE,
> -	.read_rsp_size = sizeof(struct smb2_read_rsp) - 1,
> +	.read_rsp_size = sizeof(struct smb2_read_rsp),
>  	.lock_cmd = SMB2_LOCK,
>  	.cap_unix = 0,
>  	.cap_nt_find = SMB2_NT_FIND,
> --- a/fs/smb/client/smb2pdu.c
> +++ b/fs/smb/client/smb2pdu.c
> @@ -1386,7 +1386,7 @@ SMB2_sess_sendreceive(struct SMB2_sess_d
>
>  	/* Testing shows that buffer offset must be at location of Buffer[0] */
>  	req->SecurityBufferOffset =
> -		cpu_to_le16(sizeof(struct smb2_sess_setup_req) - 1 /* pad */);
> +		cpu_to_le16(sizeof(struct smb2_sess_setup_req));
>  	req->SecurityBufferLength = cpu_to_le16(sess_data->iov[1].iov_len);
>
>  	memset(&rqst, 0, sizeof(struct smb_rqst));
> @@ -1905,8 +1905,7 @@ SMB2_tcon(const unsigned int xid, struct
>  	iov[0].iov_len = total_len - 1;
>
>  	/* Testing shows that buffer offset must be at location of Buffer[0] */
> -	req->PathOffset = cpu_to_le16(sizeof(struct smb2_tree_connect_req)
> -			- 1 /* pad */);
> +	req->PathOffset = cpu_to_le16(sizeof(struct smb2_tree_connect_req));
>  	req->PathLength = cpu_to_le16(unc_path_len - 2);
>  	iov[1].iov_base = unc_path;
>  	iov[1].iov_len = unc_path_len;
> @@ -3796,7 +3795,7 @@ SMB2_change_notify(const unsigned int xi
>  			ses->Suid, (u8)watch_tree, completion_filter);
>  		/* validate that notify information is plausible */
>  		if ((rsp_iov.iov_base == NULL) ||
> -		    (rsp_iov.iov_len < sizeof(struct smb2_change_notify_rsp)))
> +		    (rsp_iov.iov_len < sizeof(struct smb2_change_notify_rsp) + 1))
>  			goto cnotify_exit;
>
>  		smb_rsp = (struct smb2_change_notify_rsp *)rsp_iov.iov_base;
> @@ -5009,7 +5008,7 @@ int SMB2_query_directory_init(const unsi
>  	memcpy(bufptr, &asteriks, len);
>
>  	req->FileNameOffset =
> -		cpu_to_le16(sizeof(struct smb2_query_directory_req) - 1);
> +		cpu_to_le16(sizeof(struct smb2_query_directory_req));
>  	req->FileNameLength = cpu_to_le16(len);
>  	/*
>  	 * BB could be 30 bytes or so longer if we used SMB2 specific
> @@ -5205,8 +5204,7 @@ SMB2_set_info_init(struct cifs_tcon *tco
>  	req->VolatileFileId = volatile_fid;
>  	req->AdditionalInformation = cpu_to_le32(additional_info);
>
> -	req->BufferOffset =
> -			cpu_to_le16(sizeof(struct smb2_set_info_req) - 1);
> +	req->BufferOffset = cpu_to_le16(sizeof(struct smb2_set_info_req));
>  	req->BufferLength = cpu_to_le32(*size);
>
>  	memcpy(req->Buffer, *data, *size);
> @@ -5440,9 +5438,9 @@ build_qfs_info_req(struct kvec *iov, str
>  	req->VolatileFileId = volatile_fid;
>  	/* 1 for pad */
>  	req->InputBufferOffset =
> -			cpu_to_le16(sizeof(struct smb2_query_info_req) - 1);
> +			cpu_to_le16(sizeof(struct smb2_query_info_req));
>  	req->OutputBufferLength = cpu_to_le32(
> -		outbuf_len + sizeof(struct smb2_query_info_rsp) - 1);
> +		outbuf_len + sizeof(struct smb2_query_info_rsp));
>
>  	iov->iov_base = (char *)req;
>  	iov->iov_len = total_len;
> --- a/fs/smb/client/smb2pdu.h
> +++ b/fs/smb/client/smb2pdu.h
> @@ -57,7 +57,7 @@ struct smb2_rdma_crypto_transform {
>  #define COMPOUND_FID 0xFFFFFFFFFFFFFFFFULL
>
>  #define SMB2_SYMLINK_STRUCT_SIZE \
> -	(sizeof(struct smb2_err_rsp) - 1 + sizeof(struct smb2_symlink_err_rsp))
> +	(sizeof(struct smb2_err_rsp) + sizeof(struct smb2_symlink_err_rsp))
>
>  #define SYMLINK_ERROR_TAG 0x4c4d5953
>
> --- a/fs/smb/common/smb2pdu.h
> +++ b/fs/smb/common/smb2pdu.h
> @@ -189,7 +189,7 @@ struct smb2_err_rsp {
>  	__u8   ErrorContextCount;
>  	__u8   Reserved;
>  	__le32 ByteCount;  /* even if zero, at least one byte follows */
> -	__u8   ErrorData[1];  /* variable length */
> +	__u8   ErrorData[];  /* variable length */
>  } __packed;
>
>  #define SMB3_AES_CCM_NONCE 11
> @@ -330,7 +330,7 @@ struct smb2_tree_connect_req {
>  	__le16 Flags;		/* Flags in SMB3.1.1 */
>  	__le16 PathOffset;
>  	__le16 PathLength;
> -	__u8   Buffer[1];	/* variable length */
> +	__u8   Buffer[];	/* variable length */
>  } __packed;
>
>  /* Possible ShareType values */
> @@ -617,7 +617,7 @@ struct smb2_negotiate_rsp {
>  	__le16 SecurityBufferOffset;
>  	__le16 SecurityBufferLength;
>  	__le32 NegotiateContextOffset;	/* Pre:SMB3.1.1 was reserved/ignored */
> -	__u8   Buffer[1];	/* variable length GSS security buffer */
> +	__u8   Buffer[];	/* variable length GSS security buffer */
>  } __packed;
>
>
> @@ -638,7 +638,7 @@ struct smb2_sess_setup_req {
>  	__le16 SecurityBufferOffset;
>  	__le16 SecurityBufferLength;
>  	__le64 PreviousSessionId;
> -	__u8   Buffer[1];	/* variable length GSS security buffer */
> +	__u8   Buffer[];	/* variable length GSS security buffer */
>  } __packed;
>
>  /* Currently defined SessionFlags */
> @@ -655,7 +655,7 @@ struct smb2_sess_setup_rsp {
>  	__le16 SessionFlags;
>  	__le16 SecurityBufferOffset;
>  	__le16 SecurityBufferLength;
> -	__u8   Buffer[1];	/* variable length GSS security buffer */
> +	__u8   Buffer[];	/* variable length GSS security buffer */
>  } __packed;
>
>
> @@ -737,7 +737,7 @@ struct smb2_read_req {
>  	__le32 RemainingBytes;
>  	__le16 ReadChannelInfoOffset;
>  	__le16 ReadChannelInfoLength;
> -	__u8   Buffer[1];
> +	__u8   Buffer[];
>  } __packed;
>
>  /* Read flags */
> @@ -752,7 +752,7 @@ struct smb2_read_rsp {
>  	__le32 DataLength;
>  	__le32 DataRemaining;
>  	__le32 Flags;
> -	__u8   Buffer[1];
> +	__u8   Buffer[];
>  } __packed;
>
>
> @@ -776,7 +776,7 @@ struct smb2_write_req {
>  	__le16 WriteChannelInfoOffset;
>  	__le16 WriteChannelInfoLength;
>  	__le32 Flags;
> -	__u8   Buffer[1];
> +	__u8   Buffer[];
>  } __packed;
>
>  struct smb2_write_rsp {
> @@ -787,7 +787,7 @@ struct smb2_write_rsp {
>  	__le32 DataLength;
>  	__le32 DataRemaining;
>  	__u32  Reserved2;
> -	__u8   Buffer[1];
> +	__u8   Buffer[];
>  } __packed;
>
>
> @@ -834,7 +834,10 @@ struct smb2_lock_req {
>  	__u64  PersistentFileId;
>  	__u64  VolatileFileId;
>  	/* Followed by at least one */
> -	struct smb2_lock_element locks[1];
> +	union {
> +		struct smb2_lock_element lock;
> +		DECLARE_FLEX_ARRAY(struct smb2_lock_element, locks);
> +	};
>  } __packed;
>
>  struct smb2_lock_rsp {
> @@ -888,7 +891,7 @@ struct smb2_query_directory_req {
>  	__le16 FileNameOffset;
>  	__le16 FileNameLength;
>  	__le32 OutputBufferLength;
> -	__u8   Buffer[1];
> +	__u8   Buffer[];
>  } __packed;
>
>  struct smb2_query_directory_rsp {
> @@ -896,7 +899,7 @@ struct smb2_query_directory_rsp {
>  	__le16 StructureSize; /* Must be 9 */
>  	__le16 OutputBufferOffset;
>  	__le32 OutputBufferLength;
> -	__u8   Buffer[1];
> +	__u8   Buffer[];
>  } __packed;
>
>  /*
> @@ -919,7 +922,7 @@ struct smb2_set_info_req {
>  	__le32 AdditionalInformation;
>  	__u64  PersistentFileId;
>  	__u64  VolatileFileId;
> -	__u8   Buffer[1];
> +	__u8   Buffer[];
>  } __packed;
>
>  struct smb2_set_info_rsp {
> @@ -974,7 +977,7 @@ struct smb2_change_notify_rsp {
>  	__le16	StructureSize;  /* Must be 9 */
>  	__le16	OutputBufferOffset;
>  	__le32	OutputBufferLength;
> -	__u8	Buffer[1]; /* array of file notify structs */
> +	__u8	Buffer[]; /* array of file notify structs */
>  } __packed;
>
>
> @@ -1180,7 +1183,7 @@ struct smb2_create_rsp {
>  	__u64  VolatileFileId;
>  	__le32 CreateContextsOffset;
>  	__le32 CreateContextsLength;
> -	__u8   Buffer[1];
> +	__u8   Buffer[];
>  } __packed;
>
>  struct create_posix {
> @@ -1524,7 +1527,7 @@ struct smb2_query_info_req {
>  	__le32 Flags;
>  	__u64  PersistentFileId;
>  	__u64  VolatileFileId;
> -	__u8   Buffer[1];
> +	__u8   Buffer[];
>  } __packed;
>
>  struct smb2_query_info_rsp {
> @@ -1532,7 +1535,7 @@ struct smb2_query_info_rsp {
>  	__le16 StructureSize; /* Must be 9 */
>  	__le16 OutputBufferOffset;
>  	__le32 OutputBufferLength;
> -	__u8   Buffer[1];
> +	__u8   Buffer[];
>  } __packed;
>
>  /*
> @@ -1593,7 +1596,10 @@ struct smb2_file_all_info { /* data bloc
>  	__le32 Mode;
>  	__le32 AlignmentRequirement;
>  	__le32 FileNameLength;
> -	char   FileName[1];
> +	union {
> +		char __pad;	/* Legacy structure padding */
> +		DECLARE_FLEX_ARRAY(char, FileName);
> +	};
>  } __packed; /* level 18 Query */
>
>  struct smb2_file_eof_info { /* encoding of request for level 10 */
> --- a/fs/smb/server/smb2ops.c
> +++ b/fs/smb/server/smb2ops.c
> @@ -26,7 +26,7 @@ static struct smb_version_values smb21_s
>  	.unlock_lock_type = SMB2_LOCKFLAG_UNLOCK,
>  	.header_size = sizeof(struct smb2_hdr),
>  	.max_header_size = MAX_SMB2_HDR_SIZE,
> -	.read_rsp_size = sizeof(struct smb2_read_rsp) - 1,
> +	.read_rsp_size = sizeof(struct smb2_read_rsp),
>  	.lock_cmd = SMB2_LOCK,
>  	.cap_unix = 0,
>  	.cap_nt_find = SMB2_NT_FIND,
> @@ -52,7 +52,7 @@ static struct smb_version_values smb30_s
>  	.unlock_lock_type = SMB2_LOCKFLAG_UNLOCK,
>  	.header_size = sizeof(struct smb2_hdr),
>  	.max_header_size = MAX_SMB2_HDR_SIZE,
> -	.read_rsp_size = sizeof(struct smb2_read_rsp) - 1,
> +	.read_rsp_size = sizeof(struct smb2_read_rsp),
>  	.lock_cmd = SMB2_LOCK,
>  	.cap_unix = 0,
>  	.cap_nt_find = SMB2_NT_FIND,
> @@ -79,7 +79,7 @@ static struct smb_version_values smb302_
>  	.unlock_lock_type = SMB2_LOCKFLAG_UNLOCK,
>  	.header_size = sizeof(struct smb2_hdr),
>  	.max_header_size = MAX_SMB2_HDR_SIZE,
> -	.read_rsp_size = sizeof(struct smb2_read_rsp) - 1,
> +	.read_rsp_size = sizeof(struct smb2_read_rsp),
>  	.lock_cmd = SMB2_LOCK,
>  	.cap_unix = 0,
>  	.cap_nt_find = SMB2_NT_FIND,
> @@ -106,7 +106,7 @@ static struct smb_version_values smb311_
>  	.unlock_lock_type = SMB2_LOCKFLAG_UNLOCK,
>  	.header_size = sizeof(struct smb2_hdr),
>  	.max_header_size = MAX_SMB2_HDR_SIZE,
> -	.read_rsp_size = sizeof(struct smb2_read_rsp) - 1,
> +	.read_rsp_size = sizeof(struct smb2_read_rsp),
>  	.lock_cmd = SMB2_LOCK,
>  	.cap_unix = 0,
>  	.cap_nt_find = SMB2_NT_FIND,
> --- a/fs/smb/server/smb2pdu.c
> +++ b/fs/smb/server/smb2pdu.c
> @@ -295,7 +295,7 @@ int init_smb2_neg_rsp(struct ksmbd_work
>  		rsp->SecurityMode |= SMB2_NEGOTIATE_SIGNING_REQUIRED_LE;
>  	err = ksmbd_iov_pin_rsp(work, rsp,
>  				sizeof(struct smb2_negotiate_rsp) -
> -				sizeof(rsp->Buffer) + AUTH_GSS_LENGTH);
> +				sizeof(struct smb2_hdr) + AUTH_GSS_LENGTH);
>  	if (err)
>  		return err;
>  	conn->use_spnego = true;
> @@ -1264,8 +1264,7 @@ err_out:
>  	if (!rc)
>  		rc = ksmbd_iov_pin_rsp(work, rsp,
>  				       sizeof(struct smb2_negotiate_rsp) -
> -					sizeof(rsp->Buffer) +
> -					AUTH_GSS_LENGTH + neg_ctxt_len);
> +				       sizeof(struct smb2_hdr) + AUTH_GSS_LENGTH + neg_ctxt_len);
>  	if (rc < 0)
>  		smb2_set_err_rsp(work);
>  	return rc;
>
>
>




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux