diff mbox series

ksmbd: Replace one-element arrays with flexible-array members

Message ID 20240818162136.268325-2-thorsten.blum@toblux.com (mailing list archive)
State Superseded
Headers show
Series ksmbd: Replace one-element arrays with flexible-array members | expand

Commit Message

Thorsten Blum Aug. 18, 2024, 4:21 p.m. UTC 
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(-)

Comments

Namjae Jeon Aug. 20, 2024, 2:11 p.m. UTC | #1 
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
>
Tom Talpey Aug. 20, 2024, 2:52 p.m. UTC | #2 
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
>>
>
Thorsten Blum Aug. 20, 2024, 4:33 p.m. UTC | #3 
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
Tom Talpey Aug. 20, 2024, 6:39 p.m. UTC | #4 
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
diff mbox series

Patch

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 {