[PATCH] ksmbd: Replace one-element arrays with flexible-array members

[PATCH] ksmbd: Replace one-element arrays with flexible-array members

[Thorsten Blum]

Replace the deprecated one-element arrays with flexible-array members
in the structs copychunk_ioctl_req and smb2_ea_info_req.

There are no binary differences after this conversion.

Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
 fs/smb/server/smb2pdu.c | 4 ++--
 fs/smb/server/smb2pdu.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 2df1354288e6..83667cb78fa6 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -4580,7 +4580,7 @@ static int smb2_get_ea(struct ksmbd_work *work, struct ksmbd_file *fp,
 	/* single EA entry is requested with given user.* name */
 	if (req->InputBufferLength) {
 		if (le32_to_cpu(req->InputBufferLength) <
-		    sizeof(struct smb2_ea_info_req))
+		    sizeof(struct smb2_ea_info_req) + 1)
 			return -EINVAL;
 
 		ea_req = (struct smb2_ea_info_req *)((char *)req +
@@ -8083,7 +8083,7 @@ int smb2_ioctl(struct ksmbd_work *work)
 			goto out;
 		}
 
-		if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
+		if (in_buf_len < sizeof(struct copychunk_ioctl_req) + 1) {
 			ret = -EINVAL;
 			goto out;
 		}
diff --git a/fs/smb/server/smb2pdu.h b/fs/smb/server/smb2pdu.h
index 3be7d5ae65a8..73aff20e22d0 100644
--- a/fs/smb/server/smb2pdu.h
+++ b/fs/smb/server/smb2pdu.h
@@ -194,7 +194,7 @@ struct copychunk_ioctl_req {
 	__le64 ResumeKey[3];
 	__le32 ChunkCount;
 	__le32 Reserved;
-	__u8 Chunks[1]; /* array of srv_copychunk */
+	__u8 Chunks[]; /* array of srv_copychunk */
 } __packed;
 
 struct srv_copychunk {
@@ -370,7 +370,7 @@ struct smb2_file_attr_tag_info {
 struct smb2_ea_info_req {
 	__le32 NextEntryOffset;
 	__u8   EaNameLength;
-	char name[1];
+	char name[];
 } __packed; /* level 15 Query */
 
 struct smb2_ea_info {
-- 
2.46.0

Re: [PATCH] ksmbd: Replace one-element arrays with flexible-array members

[Namjae Jeon]

On Mon, Aug 19, 2024 at 1:22 AM Thorsten Blum <thorsten.blum@toblux.com> wrote:
>
> Replace the deprecated one-element arrays with flexible-array members
> in the structs copychunk_ioctl_req and smb2_ea_info_req.
>
> There are no binary differences after this conversion.
>
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> ---
>  fs/smb/server/smb2pdu.c | 4 ++--
>  fs/smb/server/smb2pdu.h | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
> index 2df1354288e6..83667cb78fa6 100644
> --- a/fs/smb/server/smb2pdu.c
> +++ b/fs/smb/server/smb2pdu.c
> @@ -4580,7 +4580,7 @@ static int smb2_get_ea(struct ksmbd_work *work, struct ksmbd_file *fp,
>         /* single EA entry is requested with given user.* name */
>         if (req->InputBufferLength) {
>                 if (le32_to_cpu(req->InputBufferLength) <
> -                   sizeof(struct smb2_ea_info_req))
> +                   sizeof(struct smb2_ea_info_req) + 1)
We can use <= instead of +1.
>                         return -EINVAL;
>
>                 ea_req = (struct smb2_ea_info_req *)((char *)req +
> @@ -8083,7 +8083,7 @@ int smb2_ioctl(struct ksmbd_work *work)
>                         goto out;
>                 }
>
> -               if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
> +               if (in_buf_len < sizeof(struct copychunk_ioctl_req) + 1) {
Ditto.
>                         ret = -EINVAL;
>                         goto out;
>                 }
> diff --git a/fs/smb/server/smb2pdu.h b/fs/smb/server/smb2pdu.h
> index 3be7d5ae65a8..73aff20e22d0 100644
> --- a/fs/smb/server/smb2pdu.h
> +++ b/fs/smb/server/smb2pdu.h
> @@ -194,7 +194,7 @@ struct copychunk_ioctl_req {
>         __le64 ResumeKey[3];
>         __le32 ChunkCount;
>         __le32 Reserved;
> -       __u8 Chunks[1]; /* array of srv_copychunk */
> +       __u8 Chunks[]; /* array of srv_copychunk */
>  } __packed;
>
>  struct srv_copychunk {
> @@ -370,7 +370,7 @@ struct smb2_file_attr_tag_info {
>  struct smb2_ea_info_req {
>         __le32 NextEntryOffset;
>         __u8   EaNameLength;
> -       char name[1];
> +       char name[];
>  } __packed; /* level 15 Query */
>
>  struct smb2_ea_info {
> --
> 2.46.0
>

Re: [PATCH] ksmbd: Replace one-element arrays with flexible-array members

[Tom Talpey]

On 8/20/2024 10:11 AM, Namjae Jeon wrote:
> On Mon, Aug 19, 2024 at 1:22 AM Thorsten Blum <thorsten.blum@toblux.com> wrote:
>>
>> Replace the deprecated one-element arrays with flexible-array members
>> in the structs copychunk_ioctl_req and smb2_ea_info_req.
>>
>> There are no binary differences after this conversion.
>>
>> Link: https://github.com/KSPP/linux/issues/79
>> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
>> ---
>>   fs/smb/server/smb2pdu.c | 4 ++--
>>   fs/smb/server/smb2pdu.h | 4 ++--
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
>> index 2df1354288e6..83667cb78fa6 100644
>> --- a/fs/smb/server/smb2pdu.c
>> +++ b/fs/smb/server/smb2pdu.c
>> @@ -4580,7 +4580,7 @@ static int smb2_get_ea(struct ksmbd_work *work, struct ksmbd_file *fp,
>>          /* single EA entry is requested with given user.* name */
>>          if (req->InputBufferLength) {
>>                  if (le32_to_cpu(req->InputBufferLength) <
>> -                   sizeof(struct smb2_ea_info_req))
>> +                   sizeof(struct smb2_ea_info_req) + 1)
> We can use <= instead of +1.

This is better, but maybe this test was actually not right in
the first place.

I think a strict "<" is correct here, because the ea name
field is a counted array of length EaNameLength. So, it's
a layering issue to fail with EINVAL this early in the
processing. All that should be checked up front is
that a complete smb2_ea_info_req header is present.

>>                          return -EINVAL;
>>
>>                  ea_req = (struct smb2_ea_info_req *)((char *)req +
>> @@ -8083,7 +8083,7 @@ int smb2_ioctl(struct ksmbd_work *work)
>>                          goto out;
>>                  }
>>
>> -               if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
>> +               if (in_buf_len < sizeof(struct copychunk_ioctl_req) + 1) {
> Ditto.

And ditto.

Tom.

>>                          ret = -EINVAL;
>>                          goto out;
>>                  }
>> diff --git a/fs/smb/server/smb2pdu.h b/fs/smb/server/smb2pdu.h
>> index 3be7d5ae65a8..73aff20e22d0 100644
>> --- a/fs/smb/server/smb2pdu.h
>> +++ b/fs/smb/server/smb2pdu.h
>> @@ -194,7 +194,7 @@ struct copychunk_ioctl_req {
>>          __le64 ResumeKey[3];
>>          __le32 ChunkCount;
>>          __le32 Reserved;
>> -       __u8 Chunks[1]; /* array of srv_copychunk */
>> +       __u8 Chunks[]; /* array of srv_copychunk */
>>   } __packed;
>>
>>   struct srv_copychunk {
>> @@ -370,7 +370,7 @@ struct smb2_file_attr_tag_info {
>>   struct smb2_ea_info_req {
>>          __le32 NextEntryOffset;
>>          __u8   EaNameLength;
>> -       char name[1];
>> +       char name[];
>>   } __packed; /* level 15 Query */
>>
>>   struct smb2_ea_info {
>> --
>> 2.46.0
>>
> 

Re: [PATCH] ksmbd: Replace one-element arrays with flexible-array members

[Thorsten Blum]

On 20. Aug 2024, at 16:52, Tom Talpey <tom@talpey.com> wrote:
> On 8/20/2024 10:11 AM, Namjae Jeon wrote:
>> On Mon, Aug 19, 2024 at 1:22 AM Thorsten Blum <thorsten.blum@toblux.com> wrote:
>>> 
>>> Replace the deprecated one-element arrays with flexible-array members
>>> in the structs copychunk_ioctl_req and smb2_ea_info_req.
>>> 
>>> There are no binary differences after this conversion.
>>> 
>>> Link: https://github.com/KSPP/linux/issues/79
>>> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
>>> ---
>>>  fs/smb/server/smb2pdu.c | 4 ++--
>>>  fs/smb/server/smb2pdu.h | 4 ++--
>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
>>> index 2df1354288e6..83667cb78fa6 100644
>>> --- a/fs/smb/server/smb2pdu.c
>>> +++ b/fs/smb/server/smb2pdu.c
>>> @@ -4580,7 +4580,7 @@ static int smb2_get_ea(struct ksmbd_work *work, struct ksmbd_file *fp,
>>>         /* single EA entry is requested with given user.* name */
>>>         if (req->InputBufferLength) {
>>>                 if (le32_to_cpu(req->InputBufferLength) <
>>> -                   sizeof(struct smb2_ea_info_req))
>>> +                   sizeof(struct smb2_ea_info_req) + 1)
>> We can use <= instead of +1.
> 
> This is better, but maybe this test was actually not right in
> the first place.
> 
> I think a strict "<" is correct here, because the ea name
> field is a counted array of length EaNameLength. So, it's
> a layering issue to fail with EINVAL this early in the
> processing. All that should be checked up front is
> that a complete smb2_ea_info_req header is present.

Just to clarify before I submit a v2: Is a strict "<" and without "+1" 
correct?

>>>                         return -EINVAL;
>>> 
>>>                 ea_req = (struct smb2_ea_info_req *)((char *)req +
>>> @@ -8083,7 +8083,7 @@ int smb2_ioctl(struct ksmbd_work *work)
>>>                         goto out;
>>>                 }
>>> 
>>> -               if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
>>> +               if (in_buf_len < sizeof(struct copychunk_ioctl_req) + 1) {
>> Ditto.
> 
> And ditto.

Same here, strict "<" and without "+ 1"? Or just a refactor to "<=" 
without changing the condition?

Thanks,
Thorsten

Re: [PATCH] ksmbd: Replace one-element arrays with flexible-array members

[Tom Talpey]

On 8/20/2024 12:33 PM, Thorsten Blum wrote:
> On 20. Aug 2024, at 16:52, Tom Talpey <tom@talpey.com> wrote:
>> On 8/20/2024 10:11 AM, Namjae Jeon wrote:
>>> On Mon, Aug 19, 2024 at 1:22 AM Thorsten Blum <thorsten.blum@toblux.com> wrote:
>>>>
>>>> Replace the deprecated one-element arrays with flexible-array members
>>>> in the structs copychunk_ioctl_req and smb2_ea_info_req.
>>>>
>>>> There are no binary differences after this conversion.
>>>>
>>>> Link: https://github.com/KSPP/linux/issues/79
>>>> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
>>>> ---
>>>>   fs/smb/server/smb2pdu.c | 4 ++--
>>>>   fs/smb/server/smb2pdu.h | 4 ++--
>>>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
>>>> index 2df1354288e6..83667cb78fa6 100644
>>>> --- a/fs/smb/server/smb2pdu.c
>>>> +++ b/fs/smb/server/smb2pdu.c
>>>> @@ -4580,7 +4580,7 @@ static int smb2_get_ea(struct ksmbd_work *work, struct ksmbd_file *fp,
>>>>          /* single EA entry is requested with given user.* name */
>>>>          if (req->InputBufferLength) {
>>>>                  if (le32_to_cpu(req->InputBufferLength) <
>>>> -                   sizeof(struct smb2_ea_info_req))
>>>> +                   sizeof(struct smb2_ea_info_req) + 1)
>>> We can use <= instead of +1.
>>
>> This is better, but maybe this test was actually not right in
>> the first place.
>>
>> I think a strict "<" is correct here, because the ea name
>> field is a counted array of length EaNameLength. So, it's
>> a layering issue to fail with EINVAL this early in the
>> processing. All that should be checked up front is
>> that a complete smb2_ea_info_req header is present.
> 
> Just to clarify before I submit a v2: Is a strict "<" and without "+1"
> correct?

Maybe safest with "<=", because it changes no behavior.

I do think there is an issue with the test, because it is
just checking for one byte of the EaName and not even checking
EaNameLength, and similar for copychunk.

But these are pre-existing protocol parsing matters, not flex
array ones. We can discuss those later.

Tom.

> 
>>>>                          return -EINVAL;
>>>>
>>>>                  ea_req = (struct smb2_ea_info_req *)((char *)req +
>>>> @@ -8083,7 +8083,7 @@ int smb2_ioctl(struct ksmbd_work *work)
>>>>                          goto out;
>>>>                  }
>>>>
>>>> -               if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
>>>> +               if (in_buf_len < sizeof(struct copychunk_ioctl_req) + 1) {
>>> Ditto.
>>
>> And ditto.
> 
> Same here, strict "<" and without "+ 1"? Or just a refactor to "<="
> without changing the condition?
> 
> Thanks,
> Thorsten

[PATCH] ksmbd: Replace one-element arrays with flexible-array members

[Thorsten Blum]

Replace the deprecated one-element arrays with flexible-array members
in the structs filesystem_attribute_info and filesystem_device_info.

Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
 fs/smb/server/smb_common.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/smb/server/smb_common.h b/fs/smb/server/smb_common.h
index 4a3148b0167f..cc1d6dfe29d5 100644
--- a/fs/smb/server/smb_common.h
+++ b/fs/smb/server/smb_common.h
@@ -213,7 +213,7 @@ struct filesystem_attribute_info {
 	__le32 Attributes;
 	__le32 MaxPathNameComponentLength;
 	__le32 FileSystemNameLen;
-	__le16 FileSystemName[1]; /* do not have to save this - get subset? */
+	__le16 FileSystemName[]; /* do not have to save this - get subset? */
 } __packed;
 
 struct filesystem_device_info {
@@ -226,7 +226,7 @@ struct filesystem_vol_info {
 	__le32 SerialNumber;
 	__le32 VolumeLabelSize;
 	__le16 Reserved;
-	__le16 VolumeLabel[1];
+	__le16 VolumeLabel[];
 } __packed;
 
 struct filesystem_info {
-- 
2.39.2

Re: [PATCH] ksmbd: Replace one-element arrays with flexible-array members

[Gustavo A. R. Silva]

On 16/08/24 07:58, Thorsten Blum wrote:
> Replace the deprecated one-element arrays with flexible-array members
> in the structs filesystem_attribute_info and filesystem_device_info.


Notice that this also affects the size of the involved structs.

I encourage you to study some of the patches that have previously
addressed similar issues.

Thanks
--
Gustavo

> 
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> ---
>   fs/smb/server/smb_common.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/smb/server/smb_common.h b/fs/smb/server/smb_common.h
> index 4a3148b0167f..cc1d6dfe29d5 100644
> --- a/fs/smb/server/smb_common.h
> +++ b/fs/smb/server/smb_common.h
> @@ -213,7 +213,7 @@ struct filesystem_attribute_info {
>   	__le32 Attributes;
>   	__le32 MaxPathNameComponentLength;
>   	__le32 FileSystemNameLen;
> -	__le16 FileSystemName[1]; /* do not have to save this - get subset? */
> +	__le16 FileSystemName[]; /* do not have to save this - get subset? */
>   } __packed;
>   
>   struct filesystem_device_info {
> @@ -226,7 +226,7 @@ struct filesystem_vol_info {
>   	__le32 SerialNumber;
>   	__le32 VolumeLabelSize;
>   	__le16 Reserved;
> -	__le16 VolumeLabel[1];
> +	__le16 VolumeLabel[];
>   } __packed;
>   
>   struct filesystem_info {

Re: [PATCH] ksmbd: Replace one-element arrays with flexible-array members

[Thorsten Blum]

On 16. Aug 2024, at 16:55, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote:
> On 16/08/24 07:58, Thorsten Blum wrote:
>> Replace the deprecated one-element arrays with flexible-array members
>> in the structs filesystem_attribute_info and filesystem_device_info.
> 
> Notice that this also affects the size of the involved structs.
> 
> I encourage you to study some of the patches that have previously
> addressed similar issues.

Thanks. I'll take a look and submit a v2.