netdev - Re: [net-next 09/17] can: length: can_fd_len2dlc(): simplify length calculcation

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]

Date:   Thu, 14 Jan 2021 09:23:10 +0100
From:   Oliver Hartkopp <socketcan@...tkopp.net>
To:     Vincent MAILHOL <mailhol.vincent@...adoo.fr>,
        Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        linux-can <linux-can@...r.kernel.org>, kernel@...gutronix.de
Subject: Re: [net-next 09/17] can: length: can_fd_len2dlc(): simplify length
 calculcation



On 14.01.21 02:59, Vincent MAILHOL wrote:
> On Tue. 14 Jan 2021 at 06:14, Marc Kleine-Budde <mkl@...gutronix.de> wrote:
>>
>> If the length paramter in len2dlc() exceeds the size of the len2dlc array, we
>> return 0xF. This is equal to the last 16 members of the array.
>>
>> This patch removes these members from the array, uses ARRAY_SIZE() for the
>> length check, and returns CANFD_MAX_DLC (which is 0xf).
>>
>> Reviewed-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
>> Link: https://lore.kernel.org/r/20210111141930.693847-9-mkl@pengutronix.de
>> Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
>> ---
>>   drivers/net/can/dev/length.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/can/dev/length.c b/drivers/net/can/dev/length.c
>> index 5e7d481717ea..d695a3bee1ed 100644
>> --- a/drivers/net/can/dev/length.c
>> +++ b/drivers/net/can/dev/length.c
>> @@ -27,15 +27,13 @@ static const u8 len2dlc[] = {
>>          13, 13, 13, 13, 13, 13, 13, 13, /* 25 - 32 */
>>          14, 14, 14, 14, 14, 14, 14, 14, /* 33 - 40 */
>>          14, 14, 14, 14, 14, 14, 14, 14, /* 41 - 48 */
>> -       15, 15, 15, 15, 15, 15, 15, 15, /* 49 - 56 */
>> -       15, 15, 15, 15, 15, 15, 15, 15  /* 57 - 64 */
>>   };
>>
>>   /* map the sanitized data length to an appropriate data length code */
>>   u8 can_fd_len2dlc(u8 len)
>>   {
>> -       if (unlikely(len > 64))
>> -               return 0xF;
>> +       if (len > ARRAY_SIZE(len2dlc))
> 
> Sorry but I missed an of-by-one issue when I did my first
> review. Don't know why but it popped to my eyes this morning when
> casually reading the emails.

Oh, yes.

The fist line is 0 .. 8 which has 9 bytes.

I also looked on it (from the back), and wondered if it was correct. But 
didn't see it either at first sight.

> 
> ARRAY_SIZE(len2dlc) is 49. If len is between 0 and 48, use the
> array, if len is greater *or equal* return CANFD_MAX_DLC.

All these changes and discussions make it very obviously more tricky to 
understand that code.

I don't really like this kind of improvement ...

Before that it was pretty clear that we only catch an out of bounds 
value and usually grab the value from the table.

Regards,
Oliver

> 
> In short, replace > by >=:
> +       if (len >= ARRAY_SIZE(len2dlc))
> 
>> +               return CANFD_MAX_DLC;
>>
>>          return len2dlc[len];
>>   }
> 
> Yours sincerely,
> Vincent
>