Re: [PATCH net] net: dpaa: Pad packets to ETH_ZLEN

[Sean Anderson]

On 9/9/24 12:46, Eric Dumazet wrote:
> On Mon, Sep 9, 2024 at 6:06 PM Sean Anderson <sean.anderson@xxxxxxxxx> wrote:
>>
>> When sending packets under 60 bytes, up to three bytes of the buffer following
>> the data may be leaked. Avoid this by extending all packets to ETH_ZLEN,
>> ensuring nothing is leaked in the padding. This bug can be reproduced by
>> running
>>
>>         $ ping -s 11 destination
>>
>> Fixes: 9ad1a3749333 ("dpaa_eth: add support for DPAA Ethernet")
>> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx>
>> ---
>>
>>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
>> index cfe6b57b1da0..e4e8ee8b7356 100644
>> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
>> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
>> @@ -2322,6 +2322,12 @@ dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev)
>>         }
>>  #endif
>>
>> +       /* Packet data is always read as 32-bit words, so zero out any part of
>> +        * the skb which might be sent if we have to pad the packet
>> +        */
>> +       if (__skb_put_padto(skb, ETH_ZLEN, false))
>> +               goto enomem;
>> +
> 
> This call might linearize the packet.
> 
> @nonlinear variable might be wrong after this point.
> 
>>         if (nonlinear) {
>>                 /* Just create a S/G fd based on the skb */
>>                 err = skb_to_sg_fd(priv, skb, &fd);
>> --
>> 2.35.1.1320.gc452695387.dirty
>>
> 
> Perhaps this instead ?
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index cfe6b57b1da0e45613ac1bbf32ddd6ace329f4fd..5763d2f1bf8dd31b80fda0681361514dad1dc307
> 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2272,12 +2272,12 @@ static netdev_tx_t
>  dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev)
>  {
>         const int queue_mapping = skb_get_queue_mapping(skb);
> -       bool nonlinear = skb_is_nonlinear(skb);
>         struct rtnl_link_stats64 *percpu_stats;
>         struct dpaa_percpu_priv *percpu_priv;
>         struct netdev_queue *txq;
>         struct dpaa_priv *priv;
>         struct qm_fd fd;
> +       bool nonlinear;
>         int offset = 0;
>         int err = 0;
> 
> @@ -2287,6 +2287,10 @@ dpaa_start_xmit(struct sk_buff *skb, struct
> net_device *net_dev)
> 
>         qm_fd_clear_fd(&fd);
> 
> +       if (__skb_put_padto(skb, ETH_ZLEN, false))
> +               goto enomem;
> +
> +       nonlinear = skb_is_nonlinear(skb);
>         if (!nonlinear) {
>                 /* We're going to store the skb backpointer at the beginning
>                  * of the data buffer, so we need a privately owned skb

Thanks for the suggestion; I was having a hard time figuring out where
to call this.

Do you have any hints for how to test this for correctness? I'm not sure
how to generate a non-linear packet under 60 bytes.

--Sean