Re: [PATCH 2/2] libxl: Reject VM config referencing nwfilters

On Wed, Sep 11, 2024 at 05:09:03PM -0600, Jim Fehlig wrote: > On 9/11/24 16:24, Laine Stump wrote: >> On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote: >>> The Xen libxl driver does not support nwfilter. Add a check for nwfilters >>> to the devicesPostParseCallback, returning VIR_ERR_CONFIG_UNSUPPORTED if >>> any are found. >>> >>> It's generally preferred for drivers to ignore unsupported XML features, >> >> I would instead characterize it as "drivers generally ignore >> *unrecognized* XML", but it's quite common for a bit of XML that's >> understood and supported in one context within libvirt to generate an >> UNSUPPORTED error when attempting to use it in a place where it isn't >> supported. >> >>> but ignoring a user's request to filter VM network traffic can be viewed >>> as a security issue. >> >> Definitely. >> >>> >>> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; >>> --- >>>   src/libxl/libxl_domain.c | 7 +++++++ >>>   1 file changed, 7 insertions(+) >>> >>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c >>> index 0f129ec69c..2f6cebb8ae 100644 >>> --- a/src/libxl/libxl_domain.c >>> +++ b/src/libxl/libxl_domain.c >>> @@ -131,6 +131,13 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev, >>>                                 void *opaque G_GNUC_UNUSED, >>>                                 void *parseOpaque G_GNUC_UNUSED) >>>   { >>> +    if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->filter) { >>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >>> +                       _("filterref is not supported in %1$s"), >>> +                       virDomainVirtTypeToString(def->virtType)); >>> +        return -1; >>> +    } >>> + >> >> This more properly should be in a function called >> libxlValidateDomainDeviceDef(), which would look something like >> qemuValidateDomainDeviceDef() and be added into >> libxlDomainDefParserConfig with this initialization: >> >>         .deviceValidateCallback = libxlValidateDomainDeviceDef, > > Yes, good point! The libxl driver already has domainValidateCallback, but > now needs a deviceValidateCallback for this code. I'll make that change in > V2. > > Before sending another version, I'd like to hear opinions on Demi's question > about the other hypervisor drivers. Do they need a similar change?

I'm not familiar with how libvirt works internally, but to me it seems like one option would be to have a flag set on the drivers that support network filtering. Generic code would then check the flag and fail if filtering is requested with a driver that doesn't support it. This has the advantage of not requiring changes to each and every driver.