[libvirt PATCH 2/2] conf: log error on attempts to modify ACPI index of active device

[Michal Prívozník]

On 9/10/21 5:18 PM, 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 at redhat.com>
>>> ---
>>>   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.
> 

Thank you Laine for your detailed analysis. I think you can merge this
patch.

Reviewed-by: Michal Privoznik <mprivozn at redhat.com>

Michal