lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <dc134856-8735-4169-89fd-5b7359e446cf@talpey.com>
Date: Tue, 20 Aug 2024 14:39:00 -0400
From: Tom Talpey <tom@...pey.com>
To: Thorsten Blum <thorsten.blum@...lux.com>
Cc: Namjae Jeon <linkinjeon@...nel.org>, sfrench@...ba.org,
 senozhatsky@...omium.org, linux-cifs@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] ksmbd: Replace one-element arrays with flexible-array
 members

On 8/20/2024 12:33 PM, Thorsten Blum wrote:
> On 20. Aug 2024, at 16:52, Tom Talpey <tom@...pey.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@...lux.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@...lux.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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ