linux-kernel - Re: [PATCH v1 1/2] fsi: sbefifo: Remove write's max command length

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <0b177938-2b73-4331-b7c3-cd5a707aac78@linux.ibm.com>
Date:   Mon, 11 Sep 2023 17:42:08 -0500
From:   Ninad Palsule <ninad@...ux.ibm.com>
To:     Joel Stanley <joel@....id.au>
Cc:     jk@...abs.org, alistair@...ple.id.au, eajames@...ux.ibm.com,
        linux-fsi@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] fsi: sbefifo: Remove write's max command length

Hi Joel,

On 9/11/23 01:03, Joel Stanley wrote:
> On Thu, 7 Sept 2023 at 22:10, Ninad Palsule <ninad@...ux.ibm.com> wrote:
>> This commit removes max command length check in the user write path.
>> This is required to support images larger than 1MB. This should not
>> create any issues as read path does not have this check either.
>>
>> As per the original design cronus server was suppose to break up the
>> image into 1MB pieces but it requires restructuring of the driver.
> When you say "driver" you mean the kernel driver, or userspace?
>
> This isn't a great justification for removing a bounds check.
I have improved the comment. Added length check back.
>
>> Today driver sends EOT message on each write request so we will have to
>> send it only after all pieces are sent which requires large change hence
>> we decided to remove this check.
> This paragraph could be clearer. Could you try rephrasing?
>
> Assuming we want to make this change, what is the expected maximum
> transfer? Could we instead make the check be that value (3MB?).
Added length check back. I am using 4MB for some cushion.

Thanks for the review.

~Ninad

>> Testing:
>>    Loaded 3 MB image through cronus server.
>>
>> Signed-off-by: Ninad Palsule <ninad@...ux.ibm.com>
>> ---
>>   drivers/fsi/fsi-sbefifo.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
>> index 9912b7a6a4b9..b771dff27f7f 100644
>> --- a/drivers/fsi/fsi-sbefifo.c
>> +++ b/drivers/fsi/fsi-sbefifo.c
>> @@ -113,7 +113,6 @@ enum sbe_state
>>   #define SBEFIFO_TIMEOUT_IN_RSP         1000
>>
>>   /* Other constants */
>> -#define SBEFIFO_MAX_USER_CMD_LEN       (0x100000 + PAGE_SIZE)
>>   #define SBEFIFO_RESET_MAGIC            0x52534554 /* "RSET" */
>>
>>   struct sbefifo {
>> @@ -870,8 +869,6 @@ static ssize_t sbefifo_user_write(struct file *file, const char __user *buf,
>>          if (!user)
>>                  return -EINVAL;
>>          sbefifo = user->sbefifo;
>> -       if (len > SBEFIFO_MAX_USER_CMD_LEN)
>> -               return -EINVAL;
>>          if (len & 3)
>>                  return -EINVAL;
>>
>> --
>> 2.39.2
>>