[v1] smb: client: correctly handle ErrorContextData as a flexible array

[PATCH] smb: client: correctly handle ErrorContextData as a flexible array

Posted by Liang Jie 11 months ago

From: Liang Jie <liangjie@lixiang.com>

The `smb2_symlink_err_rsp` structure was previously defined with
`ErrorContextData` as a single `__u8` byte. However, the `ErrorContextData`
field is intended to be a variable-length array based on `ErrorDataLength`.
This mismatch leads to incorrect pointer arithmetic and potential memory
access issues when processing error contexts.

Updates the `ErrorContextData` field to be a flexible array
(`__u8 ErrorContextData[]`). Additionally, it modifies the corresponding
casts in the `symlink_data()` function to properly handle the flexible
array, ensuring correct memory calculations and data handling.

These changes improve the robustness of SMB2 symlink error processing.

Signed-off-by: Liang Jie <liangjie@lixiang.com>
---
 fs/smb/client/smb2file.c | 4 ++--
 fs/smb/client/smb2pdu.h  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/smb2file.c b/fs/smb/client/smb2file.c
index e836bc2193dd..9ec44eab8dbc 100644
--- a/fs/smb/client/smb2file.c
+++ b/fs/smb/client/smb2file.c
@@ -42,14 +42,14 @@ static struct smb2_symlink_err_rsp *symlink_data(const struct kvec *iov)
 		end = (struct smb2_error_context_rsp *)((u8 *)err + iov->iov_len);
 		do {
 			if (le32_to_cpu(p->ErrorId) == SMB2_ERROR_ID_DEFAULT) {
-				sym = (struct smb2_symlink_err_rsp *)&p->ErrorContextData;
+				sym = (struct smb2_symlink_err_rsp *)p->ErrorContextData;
 				break;
 			}
 			cifs_dbg(FYI, "%s: skipping unhandled error context: 0x%x\n",
 				 __func__, le32_to_cpu(p->ErrorId));
 
 			len = ALIGN(le32_to_cpu(p->ErrorDataLength), 8);
-			p = (struct smb2_error_context_rsp *)((u8 *)&p->ErrorContextData + len);
+			p = (struct smb2_error_context_rsp *)(p->ErrorContextData + len);
 		} while (p < end);
 	} else if (le32_to_cpu(err->ByteCount) >= sizeof(*sym) &&
 		   iov->iov_len >= SMB2_SYMLINK_STRUCT_SIZE) {
diff --git a/fs/smb/client/smb2pdu.h b/fs/smb/client/smb2pdu.h
index 076d9e83e1a0..ba2696352634 100644
--- a/fs/smb/client/smb2pdu.h
+++ b/fs/smb/client/smb2pdu.h
@@ -79,7 +79,7 @@ struct smb2_symlink_err_rsp {
 struct smb2_error_context_rsp {
 	__le32 ErrorDataLength;
 	__le32 ErrorId;
-	__u8  ErrorContextData; /* ErrorDataLength long array */
+	__u8  ErrorContextData[]; /* ErrorDataLength long array */
 } __packed;
 
 /* ErrorId values */
-- 
2.25.1

Re: [PATCH] smb: client: correctly handle ErrorContextData as a flexible array

Posted by Tom Talpey 11 months ago

On 1/16/2025 2:29 AM, Liang Jie wrote:
> From: Liang Jie <liangjie@lixiang.com>
> 
> The `smb2_symlink_err_rsp` structure was previously defined with
> `ErrorContextData` as a single `__u8` byte. However, the `ErrorContextData`
> field is intended to be a variable-length array based on `ErrorDataLength`.
> This mismatch leads to incorrect pointer arithmetic and potential memory
> access issues when processing error contexts.
> 
> Updates the `ErrorContextData` field to be a flexible array
> (`__u8 ErrorContextData[]`). Additionally, it modifies the corresponding
> casts in the `symlink_data()` function to properly handle the flexible
> array, ensuring correct memory calculations and data handling.

Is there some reason you didn't also add the __counted_by_le attribute
to reference the ErrorDataLength protocol field?

Tom.

> 
> These changes improve the robustness of SMB2 symlink error processing.
> 
> Signed-off-by: Liang Jie <liangjie@lixiang.com>
> ---
>   fs/smb/client/smb2file.c | 4 ++--
>   fs/smb/client/smb2pdu.h  | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/smb/client/smb2file.c b/fs/smb/client/smb2file.c
> index e836bc2193dd..9ec44eab8dbc 100644
> --- a/fs/smb/client/smb2file.c
> +++ b/fs/smb/client/smb2file.c
> @@ -42,14 +42,14 @@ static struct smb2_symlink_err_rsp *symlink_data(const struct kvec *iov)
>   		end = (struct smb2_error_context_rsp *)((u8 *)err + iov->iov_len);
>   		do {
>   			if (le32_to_cpu(p->ErrorId) == SMB2_ERROR_ID_DEFAULT) {
> -				sym = (struct smb2_symlink_err_rsp *)&p->ErrorContextData;
> +				sym = (struct smb2_symlink_err_rsp *)p->ErrorContextData;
>   				break;
>   			}
>   			cifs_dbg(FYI, "%s: skipping unhandled error context: 0x%x\n",
>   				 __func__, le32_to_cpu(p->ErrorId));
>   
>   			len = ALIGN(le32_to_cpu(p->ErrorDataLength), 8);
> -			p = (struct smb2_error_context_rsp *)((u8 *)&p->ErrorContextData + len);
> +			p = (struct smb2_error_context_rsp *)(p->ErrorContextData + len);
>   		} while (p < end);
>   	} else if (le32_to_cpu(err->ByteCount) >= sizeof(*sym) &&
>   		   iov->iov_len >= SMB2_SYMLINK_STRUCT_SIZE) {
> diff --git a/fs/smb/client/smb2pdu.h b/fs/smb/client/smb2pdu.h
> index 076d9e83e1a0..ba2696352634 100644
> --- a/fs/smb/client/smb2pdu.h
> +++ b/fs/smb/client/smb2pdu.h
> @@ -79,7 +79,7 @@ struct smb2_symlink_err_rsp {
>   struct smb2_error_context_rsp {
>   	__le32 ErrorDataLength;
>   	__le32 ErrorId;
> -	__u8  ErrorContextData; /* ErrorDataLength long array */
> +	__u8  ErrorContextData[]; /* ErrorDataLength long array */
>   } __packed;
>   
>   /* ErrorId values */

Re: [PATCH] smb: client: correctly handle ErrorContextData as a flexible array

Posted by Liang Jie 10 months, 4 weeks ago

On Thu, 16 Jan 2025 13:02:51 -0500, Tom Talpey <tom@talpey.com> wrote:
> On 1/16/2025 2:29 AM, Liang Jie wrote:
> > From: Liang Jie <liangjie@lixiang.com>
> > 
> > The `smb2_symlink_err_rsp` structure was previously defined with
> > `ErrorContextData` as a single `__u8` byte. However, the `ErrorContextData`
> > field is intended to be a variable-length array based on `ErrorDataLength`.
> > This mismatch leads to incorrect pointer arithmetic and potential memory
> > access issues when processing error contexts.
> > 
> > Updates the `ErrorContextData` field to be a flexible array
> > (`__u8 ErrorContextData[]`). Additionally, it modifies the corresponding
> > casts in the `symlink_data()` function to properly handle the flexible
> > array, ensuring correct memory calculations and data handling.
> 
> Is there some reason you didn't also add the __counted_by_le attribute
> to reference the ErrorDataLength protocol field?
> 
> Tom.
> 

Hi Tom,

You're right; `__counted_by_le` would help to clearly associate `ErrorContextData`
with `ErrorDataLength`. It appears I overlooked this, and it should be added to
improve clarity. I will prepare an update [PATCH v2] to include the `__counted_by_le`
attribute.

Thanks for pointing it out.

Best regards,
Liang Jie

> > 
> > These changes improve the robustness of SMB2 symlink error processing.
> > 
> > Signed-off-by: Liang Jie <liangjie@lixiang.com>
> > ---
> >   fs/smb/client/smb2file.c | 4 ++--
> >   fs/smb/client/smb2pdu.h  | 2 +-
> >   2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/smb/client/smb2file.c b/fs/smb/client/smb2file.c
> > index e836bc2193dd..9ec44eab8dbc 100644
> > --- a/fs/smb/client/smb2file.c
> > +++ b/fs/smb/client/smb2file.c
> > @@ -42,14 +42,14 @@ static struct smb2_symlink_err_rsp *symlink_data(const struct kvec *iov)
> >   		end = (struct smb2_error_context_rsp *)((u8 *)err + iov->iov_len);
> >   		do {
> >   			if (le32_to_cpu(p->ErrorId) == SMB2_ERROR_ID_DEFAULT) {
> > -				sym = (struct smb2_symlink_err_rsp *)&p->ErrorContextData;
> > +				sym = (struct smb2_symlink_err_rsp *)p->ErrorContextData;
> >   				break;
> >   			}
> >   			cifs_dbg(FYI, "%s: skipping unhandled error context: 0x%x\n",
> >   				 __func__, le32_to_cpu(p->ErrorId));
> >   
> >   			len = ALIGN(le32_to_cpu(p->ErrorDataLength), 8);
> > -			p = (struct smb2_error_context_rsp *)((u8 *)&p->ErrorContextData + len);
> > +			p = (struct smb2_error_context_rsp *)(p->ErrorContextData + len);
> >   		} while (p < end);
> >   	} else if (le32_to_cpu(err->ByteCount) >= sizeof(*sym) &&
> >   		   iov->iov_len >= SMB2_SYMLINK_STRUCT_SIZE) {
> > diff --git a/fs/smb/client/smb2pdu.h b/fs/smb/client/smb2pdu.h
> > index 076d9e83e1a0..ba2696352634 100644
> > --- a/fs/smb/client/smb2pdu.h
> > +++ b/fs/smb/client/smb2pdu.h
> > @@ -79,7 +79,7 @@ struct smb2_symlink_err_rsp {
> >   struct smb2_error_context_rsp {
> >   	__le32 ErrorDataLength;
> >   	__le32 ErrorId;
> > -	__u8  ErrorContextData; /* ErrorDataLength long array */
> > +	__u8  ErrorContextData[]; /* ErrorDataLength long array */
> >   } __packed;
> >   
> >   /* ErrorId values */