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

On Wed, Sep 11, 2024 at 18:24:07 -0400, 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, > > (my understanding of the purpose of the two has always been that the > PostParse callback is intended for performing post-parse > modifications/fixups to the domain def, while the Validate only checks for > correctness, without modifying anything. There are multiple cases of having > validation in the postparse function, but I think those are the 1) leftovers > from before the introduction of the validate callbacks, and 2) the result of > misunderstanding and/or sloth (e.g. in cases where you want to validate > something, but the driver you're adding this validation to doesn't already > have a deviceValidateCallback) The main difference between the two is that 'postParse' is called on every parse of a XML. That means also for parsing the XMLs of persistently defined domains. If you reject parsing of a XML in a 'postParse' it fails to load the persistent definition so the VM "vanishes", which is something we don't want to do.

Thus doing validation in postParse is really valid only when it's a new attribute or configuration that can't exist in "defined" state. In contrast the validate callback is applied in a limited set of XML entrypoints which then don't make the VM to vanish and certain other situations. For that reason the 'Validate' callback/step needs to be re-tried when starting the VM.