qemu: Add audit entries for suspend and resume

[PATCH] qemu: Add audit entries for suspend and resume

Posted by Jim Fehlig via Devel 1 year, 1 month ago

We recently received a request from certification auditors to provide
audit entries for suspend and resume. This small patch uses the existing
virtDomainAudit{Start,Stop} functions with new reasons "suspended" and
"resumed".

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---

For suspend, I initially wrote the following

  virDomainAuditStart(vm, virDomainPausedReasonTypeToString(reason), true);

but I'm not sure it makes sense in resume, where we have reasons such as
VIR_DOMAIN_CRASHED_PANICKED. For symmetry, it seemed best to go with
"suspended" and "resumed".

 src/qemu/qemu_driver.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f1a633fdd3..c670bb681e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1682,6 +1682,7 @@ static int qemuDomainSuspend(virDomainPtr dom)
             goto endjob;
     }
     qemuDomainSaveStatus(vm);
+    virDomainAuditStart(vm, "suspended", true);
     ret = 0;
 
  endjob:
@@ -1738,6 +1739,7 @@ static int qemuDomainResume(virDomainPtr dom)
         }
     }
     qemuDomainSaveStatus(vm);
+    virDomainAuditStop(vm, "resumed");
     ret = 0;
 
  endjob:
-- 
2.43.0

Re: [PATCH] qemu: Add audit entries for suspend and resume

Posted by Michal Prívozník 1 year, 1 month ago

On 12/17/24 00:56, Jim Fehlig via Devel wrote:
> We recently received a request from certification auditors to provide
> audit entries for suspend and resume. This small patch uses the existing
> virtDomainAudit{Start,Stop} functions with new reasons "suspended" and
> "resumed".
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
> 
> For suspend, I initially wrote the following
> 
>   virDomainAuditStart(vm, virDomainPausedReasonTypeToString(reason), true);
> 
> but I'm not sure it makes sense in resume, where we have reasons such as
> VIR_DOMAIN_CRASHED_PANICKED. For symmetry, it seemed best to go with
> "suspended" and "resumed".
> 
>  src/qemu/qemu_driver.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

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

Re: [PATCH] qemu: Add audit entries for suspend and resume

Posted by Daniel P. Berrangé 1 year, 1 month ago

On Tue, Jan 07, 2025 at 12:06:59PM +0100, Michal Prívozník wrote:
> On 12/17/24 00:56, Jim Fehlig via Devel wrote:
> > We recently received a request from certification auditors to provide
> > audit entries for suspend and resume. This small patch uses the existing
> > virtDomainAudit{Start,Stop} functions with new reasons "suspended" and
> > "resumed".
> > 
> > Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> > ---
> > 
> > For suspend, I initially wrote the following
> > 
> >   virDomainAuditStart(vm, virDomainPausedReasonTypeToString(reason), true);
> > 
> > but I'm not sure it makes sense in resume, where we have reasons such as
> > VIR_DOMAIN_CRASHED_PANICKED. For symmetry, it seemed best to go with
> > "suspended" and "resumed".
> > 
> >  src/qemu/qemu_driver.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Actually, I'm not convinced it makese sense to call virDomainAuditStart
/ virDomainAuditStop for these cases.

Start is used when a domain is created (eg QEMU spawned) and records all
the host resources that are now used.

Stop is used when a domain is destroyed (eg QEMU killed) and thus indicates
that host resources are no longer in use.

Resume / suspend are not creating/destroying a domain, they are merely
changing the CPU running state.

I'm not really convinced that these operations are compelling to audit,
since they're not changing what host resources are in use. Even when
guest CPUs stopped, you still have incidental host CPU usage by the
emulator itself, and all the other host resources remain open by the
emulator.

If we really do need to audit this, I'd suggest completely distinct
audit events from stop/start, but personally I'd push back against
this auditors request first, as it doesn't fit with the rationale
for auditing IMHO.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] qemu: Add audit entries for suspend and resume

Posted by Jim Fehlig via Devel 1 year, 1 month ago

On 1/7/25 04:22, Daniel P. Berrangé wrote:
> On Tue, Jan 07, 2025 at 12:06:59PM +0100, Michal Prívozník wrote:
>> On 12/17/24 00:56, Jim Fehlig via Devel wrote:
>>> We recently received a request from certification auditors to provide
>>> audit entries for suspend and resume. This small patch uses the existing
>>> virtDomainAudit{Start,Stop} functions with new reasons "suspended" and
>>> "resumed".
>>>
>>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>>> ---
>>>
>>> For suspend, I initially wrote the following
>>>
>>>    virDomainAuditStart(vm, virDomainPausedReasonTypeToString(reason), true);
>>>
>>> but I'm not sure it makes sense in resume, where we have reasons such as
>>> VIR_DOMAIN_CRASHED_PANICKED. For symmetry, it seemed best to go with
>>> "suspended" and "resumed".
>>>
>>>   src/qemu/qemu_driver.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>
>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> Actually, I'm not convinced it makese sense to call virDomainAuditStart
> / virDomainAuditStop for these cases.
> 
> Start is used when a domain is created (eg QEMU spawned) and records all
> the host resources that are now used.
> 
> Stop is used when a domain is destroyed (eg QEMU killed) and thus indicates
> that host resources are no longer in use.

Thanks for pointing that out. I was lazy and didn't look at the impl of 
Audit{Start,Stop} :-/. AuditStart is definitely not correct for suspend!

> 
> Resume / suspend are not creating/destroying a domain, they are merely
> changing the CPU running state.
> 
> I'm not really convinced that these operations are compelling to audit,
> since they're not changing what host resources are in use. Even when
> guest CPUs stopped, you still have incidental host CPU usage by the
> emulator itself, and all the other host resources remain open by the
> emulator.
> 
> If we really do need to audit this, I'd suggest completely distinct
> audit events from stop/start, but personally I'd push back against
> this auditors request first, as it doesn't fit with the rationale
> for auditing IMHO.

I attempted pushing back prior to writing this patch. From a non-public bug comment:

"IMO, it's hard to classify suspend and resume as lifecycle operations. Suspend 
simply suspends execution of the VM's vcpus. All VM resources are still 
allocated and in use. In practice, I wonder if these operations are even used..."

I've made another attempt and referenced this thread :-).

Regards,
Jim

Re: [PATCH] qemu: Add audit entries for suspend and resume

Posted by Jim Fehlig via Devel 1 year, 1 month ago

Happy new year everyone!

Any comments on this small patch?

Regards,
Jim

On 12/16/24 16:56, Jim Fehlig wrote:
> We recently received a request from certification auditors to provide
> audit entries for suspend and resume. This small patch uses the existing
> virtDomainAudit{Start,Stop} functions with new reasons "suspended" and
> "resumed".
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
> 
> For suspend, I initially wrote the following
> 
>    virDomainAuditStart(vm, virDomainPausedReasonTypeToString(reason), true);
> 
> but I'm not sure it makes sense in resume, where we have reasons such as
> VIR_DOMAIN_CRASHED_PANICKED. For symmetry, it seemed best to go with
> "suspended" and "resumed".
> 
>   src/qemu/qemu_driver.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f1a633fdd3..c670bb681e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -1682,6 +1682,7 @@ static int qemuDomainSuspend(virDomainPtr dom)
>               goto endjob;
>       }
>       qemuDomainSaveStatus(vm);
> +    virDomainAuditStart(vm, "suspended", true);
>       ret = 0;
>   
>    endjob:
> @@ -1738,6 +1739,7 @@ static int qemuDomainResume(virDomainPtr dom)
>           }
>       }
>       qemuDomainSaveStatus(vm);
> +    virDomainAuditStop(vm, "resumed");
>       ret = 0;
>   
>    endjob: