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

[Paolo Abeni <pabeni@redhat.com>]

Hi,

On 2/3/26 10:40 PM, Dan Jurgens wrote:
> 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.

[sorry for the latency]

I'm possibly lost, but I do see such check for classifier->selectors,
but I do not see them for ff->ff_mask->selectors, neither in
get_selector_cap() nor in virtio_admin_cap_get().

> 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;
>                 }

If you mean:

		if (sel->length != get_mask_size(sel->type) ||
                      test_and_set_bit(sel->type, &sel_types))

LGTM!

Thanks,

Paolo