Re: [PATCH net 1/2] bonding: reject frames with insufficient headroom in bond_header_create

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Qihang wrote:
> From: Qihang Tang <q.h.hack.winter@gmail.com>

Thanks for working on this Qihang.

Minor: a patch series should generally have a cover letter

> AF_PACKET SOCK_DGRAM sends reserve skb headroom from a snapshot of
> bond_dev->hard_header_len.  A concurrent bond type change can switch the
> active slave to one with a larger hard_header_len between that snapshot
> and bond_header_create(), so the slave's create() pushes or writes past
> skb->head.

Are we certain that bond and team are the only devices that implement
header_ops->create?
 
> The hard_header_len snapshot series that fixed the SOCK_RAW send paths
> deferred this SOCK_DGRAM race: dev->header_ops is the stable
> bond_header_ops, so snapshotting header_ops in the caller does not help.
> 
> Reject the frame if skb headroom is smaller than the active slave's
> hard_header_len, before delegating under the existing rcu_read_lock.
> 
> Fixes: 950803f72547 ("bonding: fix type confusion in bond_setup_by_slave()")

What's the rationale behind blaming this SHA1?

> Cc: stable@vger.kernel.org
> Cc: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Qihang Tang <q.h.hack.winter@gmail.com>
> ---
>  drivers/net/bonding/bond_main.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 522eab060f9e..9ec663610dfd 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1524,10 +1524,24 @@ static int bond_header_create(struct sk_buff *skb, struct net_device *bond_dev,
>  	slave = rcu_dereference(bond->curr_active_slave);
>  	if (slave) {
>  		slave_ops = READ_ONCE(slave->dev->header_ops);
> -		if (slave_ops && slave_ops->create)
> +		if (slave_ops && slave_ops->create) {
> +			unsigned int hlen = READ_ONCE(slave->dev->hard_header_len);
> +
> +			/* Headroom was reserved from a snapshot of
> +			 * bond_dev->hard_header_len that may predate this
> +			 * slave (concurrent bond type change); reject if
> +			 * insufficient for the slave's create(), which
> +			 * pushes its own hlen.
> +			 */
> +			if (skb_headroom(skb) < hlen) {
> +				ret = -EINVAL;
> +				goto unlock;
> +			}
>  			ret = slave_ops->create(skb, slave->dev,
>  						type, daddr, saddr, len);
> +		}

Ideally we could fix this in dev_hard_header or PF_PACKET itself,
rather than in each implementation that's in scope.

But there seems no way around holding this rcu reference on the slave
device while re-checking.



>  	}
> +unlock:
>  	rcu_read_unlock();
>  	return ret;
>  }
> -- 
> 2.50.1 (Apple Git-155)
>