Re: [PATCH net-next v17 07/12] virtio_net: Implement layer 2 ethtool flow rules

[Dan Jurgens <danielj@nvidia.com>]

On 2/3/26 3:19 AM, Paolo Abeni wrote:
> Hi,
> 
> The AI review reported a possible issue that looks valid to me.
> Reporting the feedback manually because I think only one to the AI
> remarks is valid, see below.
> 
> On 2/2/26 6:05 PM, Daniel Jurgens wrote:
>> +static bool validate_eth_mask(const struct virtnet_ff *ff,
>> +			      const struct virtio_net_ff_selector *sel,
>> +			      const struct virtio_net_ff_selector *sel_cap)
>> +{
>> +	bool partial_mask = !!(sel_cap->flags & VIRTIO_NET_FF_MASK_F_PARTIAL_MASK);
>> +	struct ethhdr *cap, *mask;
>> +	struct ethhdr zeros = {};
>> +
>> +	cap = (struct ethhdr *)&sel_cap->mask;
>> +	mask = (struct ethhdr *)&sel->mask;
> 
> This function casts sel_cap->mask to struct ethhdr * and accesses fields
> at offsets 0, 6, and 12. Shouldn't there be validation that
> sel_cap->length is at least sizeof(struct ethhdr) = 14 bytes?
> 
> Looking at virtnet_ff_init() at line 6291, it only checks that
> sel->length <= MAX_SEL_LEN (40 bytes) but doesn't enforce a minimum
> length for the ETH selector type. If a device provides an ETH selector
> capability with length < 14 bytes, won't validate_eth_mask() read beyond
> the allocated mask array?

It won't read beyond the end of the array. When we retrieve the selector
caps we make sure the that the size of all the selectors and their data
is less than what we allocated. So I don't think this is really a bug.

I'll change the check here to be sel->length !=
get_mask_size(sel->type). It'll fail in a clearer way if this is the case.

                if (sel->length > MAX_SEL_LEN ||
                    test_and_set_bit(sel->type, &sel_types)) {
                        WARN_ON_ONCE(true);
                        err = -EINVAL;
                        goto err_ff_action;
                }



> ---
> 
> Note that the AI review additionally reported a possible leak on xarray,
> but I think it got confused possibly because it run out of token and
> mixed-up the patch context.
> 

Yes, I saw this and it didn't make sense.

> /P
>