Re: [Intel-wired-lan] [PATCH RESEND] igbvf: Fix rx_buffer_len

[Akihiko Odaki]

On 2023/01/24 19:49, Paul Menzel wrote:
> Dear Akihiko,
>
>
> Thank you for your patch.
>
> Am 24.01.23 um 05:39 schrieb Akihiko Odaki:
>
> Maybe improve the commit message summary to be more specific:
>
> igbvf: Align rx_buffer_len to fix memory corrption
>
> > When rx_buffer_len is not aligned by 1024, igbvf sets the aligned size
> > to SRRCTL while the buffer is allocated with the unaligned size. This
> > allows the device to write more data than rx_buffer_len, resulting in
> > memory corruption. Align rx_buffer_len itself so that the buffer will
> > be allocated with the aligned size.
> >
> > The condition to split RX packet header, which uses rx_buffer_len, is
> > also modified so that it doesn't change the behavior for the same
> > actual (unaligned) packet size. Actually the new condition is not
> > identical with the old one as it will no longer request splitting when
> > the actual packet size is exactly 2048, but that should be negligible.
>
> Is there an easy way to reproduce it?
>
>
> Kind regards,
>
> Paul

I withdraw this patch. While igbvf sets a value greater than the actual 
buffer size to SRRCTL.BSIZEPKT, such a long packet should be dropped 
according to VMOLR.RLPML.

Regards,
Akihiko Odaki

> > Fixes: d4e0fe01a38a ("igbvf: add new driver to support 82576 virtual 
> > functions")
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@xxxxxxxxxx>
> > ---
> >   drivers/net/ethernet/intel/igbvf/netdev.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c 
> > b/drivers/net/ethernet/intel/igbvf/netdev.c
> > index 3a32809510fc..b6bca78198fa 100644
> > --- a/drivers/net/ethernet/intel/igbvf/netdev.c
> > +++ b/drivers/net/ethernet/intel/igbvf/netdev.c
> > @@ -1341,10 +1341,9 @@ static void igbvf_setup_srrctl(struct 
> > igbvf_adapter *adapter)
> >       srrctl |= E1000_SRRCTL_DROP_EN;
> >       /* Setup buffer sizes */
> > -    srrctl |= ALIGN(adapter->rx_buffer_len, 1024) >>
> > -          E1000_SRRCTL_BSIZEPKT_SHIFT;
> > +    srrctl |= adapter->rx_buffer_len >> E1000_SRRCTL_BSIZEPKT_SHIFT;
> > -    if (adapter->rx_buffer_len < 2048) {
> > +    if (adapter->rx_buffer_len <= 2048) {
> >           adapter->rx_ps_hdr_size = 0;
> >           srrctl |= E1000_SRRCTL_DESCTYPE_ADV_ONEBUF;
> >       } else {
> > @@ -1625,7 +1624,7 @@ static int igbvf_sw_init(struct igbvf_adapter 
> > *adapter)
> >       struct net_device *netdev = adapter->netdev;
> >       s32 rc;
> > -    adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN;
> > +    adapter->rx_buffer_len = ALIGN(ETH_FRAME_LEN + VLAN_HLEN + 
> > ETH_FCS_LEN, 1024);
> >       adapter->rx_ps_hdr_size = 0;
> >       adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
> >       adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
> > @@ -2429,6 +2428,8 @@ static int igbvf_change_mtu(struct net_device 
> > *netdev, int new_mtu)
> >           adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN +
> >                        ETH_FCS_LEN;
> > +    adapter->rx_buffer_len = ALIGN(adapter->rx_buffer_len, 1024);
> > +
> >       netdev_dbg(netdev, "changing MTU from %d to %d\n",
> >              netdev->mtu, new_mtu);
> >       netdev->mtu = new_mtu;