[Intel-wired-lan] [PATCH] ixgbe: add array bounds check
Alexander Duyck
alexander.duyck at gmail.com
Tue Oct 24 18:49:06 UTC 2017
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
More information about the Intel-wired-lan
mailing list