Tony Nguyen <[email protected]> 于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
>

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

Reply via email to