The ACPI index of a device in a running guest can't be modified, and libvirt doesn't actually attempt to modify it, but it was possible for a user to request such a modification, and libvirt wouldn't complain, thus misleading the user into thinking that it had actually been changed. Resolves: https://bugzilla.redhat.com/1998920 Signed-off-by: Laine Stump <laine(a)redhat.com&gt; --- src/conf/domain_conf.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fefc527901..7ff890d8b7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28485,6 +28485,12 @@ virDomainDefCompatibleDevice(virDomainDef *def, _("changing device alias is not allowed")); return -1; } + + if (data.newInfo->acpiIndex != data.oldInfo->acpiIndex) { + virReportError(VIR_ERR_OPERATION_DENIED, "%s", + _("changing device 'acpi index' is not allowed")); + return -1; + }

On 9/10/21 9:30 AM, Peter Krempa wrote: > On Thu, Sep 09, 2021 at 13:46:41 -0400, Laine Stump wrote: >> The ACPI index of a device in a running guest can't be modified, and >> libvirt doesn't actually attempt to modify it, but it was possible for >> a user to request such a modification, and libvirt wouldn't complain, >> thus misleading the user into thinking that it had actually been changed. >> >> Resolves: https://bugzilla.redhat.com/1998920 >> >> Signed-off-by: Laine Stump <laine(a)redhat.com&gt; >> --- >> src/conf/domain_conf.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index fefc527901..7ff890d8b7 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -28485,6 +28485,12 @@ virDomainDefCompatibleDevice(virDomainDef *def, >> _("changing device alias is not allowed")); >> return -1; >> } >> + >> + if (data.newInfo->acpiIndex != data.oldInfo->acpiIndex) { >> + virReportError(VIR_ERR_OPERATION_DENIED, "%s", >> + _("changing device 'acpi index' is not allowed")); >> + return -1; >> + } > > I don't remember any more what the intended semantics of the 'update' > API are in regards to asking the user to provide a complete definition > of the device, but according to the 'alias' check above we seem to be > specifically ignoring the situation when the new definition didn't > provide an alias. This seems to be hinting that we ignore stuff that the > used didn't provide. > > If that's the case you'll need to specifically exclude index '0' from > the above check. > While I agree that it would be user friendly I think we mandate users to provide full device XML and looking into the doc we really do: * virDomainUpdateDeviceFlags: * @domain: pointer to domain object * @xml: pointer to XML description of one device * @flags: bitwise-OR of virDomainDeviceModifyFlags * * The supplied XML description of the device should contain all * the information that is found in the corresponding domain XML. * Leaving out any piece of information may be treated as a * request for its removal, which may be denied. We can special case acpiIndex as you suggest but I think that's the farthest we should go. Otherwise it'll be hard to distinguish user's laziness to provide full XML from genuine request for removal.

On 9/10/21 3:30 AM, Peter Krempa wrote: > On Thu, Sep 09, 2021 at 13:46:41 -0400, Laine Stump wrote: >> The ACPI index of a device in a running guest can't be modified, and >> libvirt doesn't actually attempt to modify it, but it was possible for >> a user to request such a modification, and libvirt wouldn't complain, >> thus misleading the user into thinking that it had actually been >> changed. >> >> Resolves: https://bugzilla.redhat.com/1998920 >> >> Signed-off-by: Laine Stump <laine(a)redhat.com&gt; >> --- >>   src/conf/domain_conf.c | 6 ++++++ >>   1 file changed, 6 insertions(+) >> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index fefc527901..7ff890d8b7 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -28485,6 +28485,12 @@ virDomainDefCompatibleDevice(virDomainDef *def, >>                              _("changing device alias is not allowed")); >>               return -1; >>           } >> + >> +        if (data.newInfo->acpiIndex != data.oldInfo->acpiIndex) { >> +            virReportError(VIR_ERR_OPERATION_DENIED, "%s", >> +                           _("changing device 'acpi index' is not >> allowed")); >> +            return -1; >> +        } > > I don't remember any more what the intended semantics of the 'update' > API are in regards to asking the user to provide a complete definition > of the device, but according to the 'alias' check above we seem to be > specifically ignoring the situation when the new definition didn't > provide an alias. This seems to be hinting that we ignore stuff that the > used didn't provide. > > If that's the case you'll need to specifically exclude index '0' from > the above check. The difference here is that alias is something that normally is not set by the user, but is instead auto-generated by libvirt when the guest starts and not stored/reported in the persistent config. So in the common case of "grab config XML, extract device element, modify it, send it to update-device" the alias wouldn't be in the XML, but if the user instead grabs the *status* XML then the alias *would* be there. (However, if the alias had been set by the user (with the prefix "ua-"), the alias would be present in the config XML.) So we allow for either blank (in the case that config XML was used) or no-change (in case alias was user-set and/or the status XML is used). The acpi index, on the other hand, is always unset unless the user specifies it, and so is never auto-generated and always matches in both the config and status XMLs. So whichever way the user gets the original XML (status or config) they are always going to have the actual setting. There are many other attributes that are checked to assure they match in (e.g.) qemuDomainChangeNet() (for example, network device model name, virtio options, upscript name,...) and (with one exception) none of those check for "not specified or not changed" - they all just check for "not changed". The one exception is ifname, which is another attribute that (like alias) is autogenerated every time the guest starts, and is never output in config XML (unless it was user-specified); for ifname if it's been left unspecified in the update XML, then the value from the status is copied over, making it also effectively ("unspecified or unchanged"). But again, this special handling isn't done for any attribute that isn't both auto-generated and ephemeral (as alias and ifname are). So in the end, I think it's correct that validation of acpi index *shouldn't* ignore an "apparently missing" value in the update. ====== BTW, something forgot to mention in the commit log - in the case of, e.g., network interfaces which is the only type of device where an alias is useful, most of the checks are in qemuDomainChangeNet() rather than in this virDomainDefCompatibleDevice() function. I was originally going to add this check into qemuDomainChangeNet() as well, but it is an attribute that's technically valid for any PCI device, not just interfaces; searching around, I found that virDomainDefCompatibleDevice() is called on update for all devices, and is already used to pretect against alias change (another attribute that's valid for all devices), so in the interest of consistency I put the check there. That said, although I used it based on precendent, I can't say that I really like virDomainDefCompatibleDevice(). It is used for multiple unrelated things - the bit at the beginning of the function is only executed if action is UPDATE and live is true, and all the rest of the function is only useful if action is ATTACH - it's checking things that could verifiably never happen during a live device update (e.g. checking if the device is USB but the domain doesn't support USB devices, or checking attributes for a device type that isn't allowed to be updated at all.). So it's really 2 completely independent functions in one, where the final two args act as an extension to the function name. I notice its action and live args previously weren't there - I did the spelunking into git history to find when/how; essentially there was at one time an unused action arg which was removed, but then re-added in order to add the check for alias). I didn't dig much deeper, but it seems like this might be an unnecessary tangle that should be split up into multiple functions. I didn't want to mess with that for a simple bugfix, though.

On Fri, Sep 10, 2021 at 11:18:20 -0400, Laine Stump wrote: > On 9/10/21 3:30 AM, Peter Krempa wrote: >> On Thu, Sep 09, 2021 at 13:46:41 -0400, Laine Stump wrote: >>> The ACPI index of a device in a running guest can't be modified, and >>> libvirt doesn't actually attempt to modify it, but it was possible for >>> a user to request such a modification, and libvirt wouldn't complain, >>> thus misleading the user into thinking that it had actually been changed. >>> >>> Resolves: https://bugzilla.redhat.com/1998920 >>> >>> Signed-off-by: Laine Stump <laine(a)redhat.com&gt; >>> --- >>> src/conf/domain_conf.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >>> index fefc527901..7ff890d8b7 100644 >>> --- a/src/conf/domain_conf.c >>> +++ b/src/conf/domain_conf.c >>> @@ -28485,6 +28485,12 @@ virDomainDefCompatibleDevice(virDomainDef *def, >>> _("changing device alias is not allowed")); >>> return -1; >>> } >>> + >>> + if (data.newInfo->acpiIndex != data.oldInfo->acpiIndex) { >>> + virReportError(VIR_ERR_OPERATION_DENIED, "%s", >>> + _("changing device 'acpi index' is not allowed")); >>> + return -1; >>> + } >> >> I don't remember any more what the intended semantics of the 'update' >> API are in regards to asking the user to provide a complete definition >> of the device, but according to the 'alias' check above we seem to be >> specifically ignoring the situation when the new definition didn't >> provide an alias. This seems to be hinting that we ignore stuff that the >> used didn't provide. >> >> If that's the case you'll need to specifically exclude index '0' from >> the above check. > > The difference here is that alias is something that normally is not set by > the user, but is instead auto-generated by libvirt when the guest starts and > not stored/reported in the persistent config. So in the common case of "grab I strongly disagree with this statement. Firstly we now have user aliases and secondly there's always runtime related stuff in the XML.

> config XML, extract device element, modify it, send it to update-device" the > alias wouldn't be in the XML, but if the user instead grabs the *status* XML > then the alias *would* be there. (However, if the alias had been set by the > user (with the prefix "ua-"), the alias would be present in the config XML.) > So we allow for either blank (in the case that config XML was used) or > no-change (in case alias was user-set and/or the status XML is used). Picking config XML is semantically as wrong as generating a partial XML. It can simply be a completely different device. > If we want the users to use the full XML when updating the device it _must_ be the full XML snippet from the running config. Otherwise we are back at a "partial XML" and all the problems connected to that.