Re: [PATCH net v2 1/3] i40e: skip unnecessary VF reset when setting trust

[Jose Ignacio Tornos Martinez <jtornosm@redhat.com>]

Hi Jacob,

> Since the usual case is going to be a situation where we don't expect
> to exceed these limits, couldn't this check be expanded so we would
> still reset in the case where the filters need to be removed? I guess
> its overkill and maybe we accept the extra filters.

I think accepting the extra filters in this rare case is a reasonable
trade-off. In the usual case the VF will be well within the untrusted
limits (bonding setup typically adds 1-2 MAC filters). The reset adds
~10 second delay, so we only want it when truly necessary.

> This doesn't address the case where filters have MAC addresses
> different from the admin-set MAC? I am not sure if that still leaves
> a security hole or not.

The PF enforces the admin-set MAC restriction on every new add request
from an untrusted VF (i40e_check_vf_permission() rejects it). Existing
filters with different MACs remain in hardware, but the VF cannot add
new ones. I think this is not a security hole: the filters were
legitimately installed while the VF was trusted, and the VF cannot
escalate from this state.

> This is an intentional change that I think should at least be called
> out in the commit message. This *does* mean a VF which was previously
> trusted could remain exceeding its untrusted limit in perpetuity
> until it resets or self-removes the filters.
>
> That does mean the VF will technically violate the trusted boundary,
> but it can't get worse, and it was previously accepted while under
> trust. I think.
>
> I think this is acceptable, but we may want to clarify the
> functionally changed behavior here so that it is clear.

Agreed, this is an intentional trade-off that was considered during
development. As you say, the VF cannot make it worse, and the filters
were accepted while trusted. And of course, a VF reset cleans
everything up completely. If you think it would be helpful, I can send
a follow-up adding a comment in the code documenting this trade-off.

Thanks

Best regards
José Ignacio