[Intel-wired-lan] [PATCH] ixgbe: add array bounds check

[Tantilov, Emil S]

>-----Original Message-----
>From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
>Behalf Of Alexander Duyck
>Sent: Tuesday, October 24, 2017 11:49 AM
>To: Tantilov, Emil S <emil.s.tantilov at intel.com>
>Cc: intel-wired-lan <intel-wired-lan at lists.osuosl.org>
>Subject: Re: [Intel-wired-lan] [PATCH] ixgbe: add array bounds check
>
>On Tue, Oct 24, 2017 at 10:44 AM, Tantilov, Emil S
><emil.s.tantilov at intel.com> wrote:
>>>-----Original Message-----
>>>From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
>>>Behalf Of Alexander Duyck
>>>Sent: Tuesday, October 24, 2017 9:20 AM
>>>To: Greenwalt, Paul <paul.greenwalt at intel.com>
>>>Cc: intel-wired-lan <intel-wired-lan at lists.osuosl.org>
>>>Subject: Re: [Intel-wired-lan] [PATCH] ixgbe: add array bounds check
>>>
>>>On Mon, Oct 23, 2017 at 8:21 AM, Paul Greenwalt
>>><paul.greenwalt at intel.com> wrote:
>>>> Add bounds check for index fcoe_i of rx_ring array.
>>>>
>>>> Signed-off-by: Paul Greenwalt <paul.greenwalt at intel.com>
>>>> ---
>>>>  drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
>>>b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
>>>> index a23c2b5..5772fe2 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
>>>> @@ -698,6 +698,8 @@ void ixgbe_configure_fcoe(struct ixgbe_adapter
>>>*adapter)
>>>>                 }
>>>>
>>>>                 fcoe_i = fcoe->offset + (i % fcoe->indices);
>>>> +               if (fcoe_i >= MAX_RX_QUEUES)
>>>> +                       fcoe_i = MAX_RX_QUEUES - 1;
>>>
>>>Instead of using MAX_RX_QUEUES why use fcoe->limit? If I am not
>>>mistake the upper limit is either 8 or 32 for the FCoE redirection
>>>table anyway so odds are you can't even use 64 queues if you wanted
>>>to. From what I can tell the only advantage of the approach you are
>>>using now is that you don't potentially zero out the value when it
>>>gets hit with the mask below.
>>>
>>>>                 fcoe_i &= IXGBE_FCRETA_ENTRY_MASK;
>>
>> Actually the check is added to protect from accessing rx_ring[fcoe_i]
>beyond
>> it's boundaries. The above line was supposed to be removed. This is
>because
>> IXGBE_FCRETA_ENTRY_MASK = 0x7f, and static checkers report possible
>out of
>> bounds issue.
>>
>> Thanks,
>> Emil
>
>So is this actually fixing a bug, or just making some static analysis
>tool happy? If it is about a static analysis tool I would stay just
>identify it as a false hit since this value should be configured and
>limited by the code in ixgbe_lib.c that is configuring the offset and
>indices.
It's a bit of both in my opinion since the line limiting the value of fcoe_i:

	fcoe_i &= IXGBE_FCRETA_ENTRY_MASK;

Is clearly wrong since IXGBE_FCRETA_ENTRY_MASK exceeds MAX_RX_QUEUES.

Why even do this if we are sure that the limits are enforced in some other part
of the code?

Thanks,
Emil