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

[Alexander Duyck]

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.

Thanks.

- Alex