Re: [Intel-wired-lan] [PATCH] e1000: fix OOB in e1000_tbi_should_accept()

[Guangshuo Li]

Hi Tony, all,

Thanks for the review. As suggested by Tony, I’ll keep the declarations at
the top and place the bounds checks before assigning last_byte. I’ll send a
v2 with the following change:

static bool e1000_tbi_should_accept(struct e1000_adapter *adapter,
                                    u8 status, u8 errors,
                                    u32 length, const u8 *data)
{
    struct e1000_hw *hw = &adapter->hw;
    u8 last_byte;

    /* Guard against OOB on data[length - 1] */
    if (unlikely(!length))
        return false;

    /* Upper bound: length must not exceed rx_buffer_len */
    if (unlikely(length > adapter->rx_buffer_len))
        return false;

    last_byte = data[length - 1];

    /* existing logic follows ... */
}
Please let me know if further adjustments are preferred.

Best regards,
Guangshuo Li


Tony Nguyen <anthony.l.ngu...@intel.com> 于2025年11月18日周二 07:24写道：

>
>
> On 11/4/2025 12:28 AM, Guangshuo Li wrote:
> > In e1000_tbi_should_accept() we read the last byte of the frame via
> > 'data[length - 1]' to evaluate the TBI workaround. If the descriptor-
> > reported length is zero or larger than the actual RX buffer size, this
> > read goes out of bounds and can hit unrelated slab objects. The issue
> > is observed from the NAPI receive path (e1000_clean_rx_irq):
>
> ...
>
> > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> > @@ -4090,6 +4090,12 @@ static bool e1000_tbi_should_accept(struct
> e1000_adapter *adapter,
> >                                   u8 status, u8 errors,
> >                                   u32 length, const u8 *data)
> >   {
> > +     /* Guard against OOB on data[length - 1] */
> > +     if (unlikely(!length))
> > +             return false;
> > +     /* Upper bound: length must not exceed rx_buffer_len */
> > +     if (unlikely(length > adapter->rx_buffer_len))
> > +             return false;
>
> The change itself looks fine, however, the declarations should be at the
> beginning of the function so this should be moved to be after that.
>
> >       struct e1000_hw *hw = &adapter->hw;
> >       u8 last_byte = *(data + length - 1);
>
> Also, since last_byte uses length, this should be broken up and the
> assignment moved after the added checks.
>
> Thanks,
> Tony
>