[PATCH] qemu: do not allow /dev/rtc or /dev/hpet access via the devices cgroup

Paolo Bonzini posted 1 patch 1 week ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200518230659.24510-1-pbonzini@redhat.com
docs/drvqemu.html.in               | 1 -
src/qemu/qemu.conf                 | 1 -
src/qemu/qemu_cgroup.c             | 1 -
src/qemu/test_libvirtd_qemu.aug.in | 2 --
4 files changed, 5 deletions(-)

[PATCH] qemu: do not allow /dev/rtc or /dev/hpet access via the devices cgroup

Posted by Paolo Bonzini 1 week ago
The RTC and HPET modes for the QEMU emulation tick have been dropped almost 9 years
ago, in commit 25f3151ece1d5881826232bebccc21b588d4e03e.  Do not allow them in the
devices cgroup policy.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 docs/drvqemu.html.in               | 1 -
 src/qemu/qemu.conf                 | 1 -
 src/qemu/qemu_cgroup.c             | 1 -
 src/qemu/test_libvirtd_qemu.aug.in | 2 --
 4 files changed, 5 deletions(-)

diff --git a/docs/drvqemu.html.in b/docs/drvqemu.html.in
index afc4ddf56d..b6d731bb59 100644
--- a/docs/drvqemu.html.in
+++ b/docs/drvqemu.html.in
@@ -484,7 +484,6 @@ chmod o+x /path/to/directory
 /dev/null, /dev/full, /dev/zero,
 /dev/random, /dev/urandom,
 /dev/ptmx, /dev/kvm,
-/dev/rtc, /dev/hpet
 </pre>
 
     <p>
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index abdbf07fec..d7a3f40e78 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -495,7 +495,6 @@
 #    "/dev/null", "/dev/full", "/dev/zero",
 #    "/dev/random", "/dev/urandom",
 #    "/dev/ptmx", "/dev/kvm",
-#    "/dev/rtc","/dev/hpet"
 #]
 #
 # RDMA migration requires the following extra files to be added to the list:
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 2e019b64af..d92202f847 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -47,7 +47,6 @@ const char *const defaultDeviceACL[] = {
     "/dev/null", "/dev/full", "/dev/zero",
     "/dev/random", "/dev/urandom",
     "/dev/ptmx", "/dev/kvm",
-    "/dev/rtc", "/dev/hpet",
     NULL,
 };
 #define DEVICE_PTY_MAJOR 136
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index 19da591aae..e533b9f551 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -61,8 +61,6 @@ module Test_libvirtd_qemu =
     { "5" = "/dev/urandom" }
     { "6" = "/dev/ptmx" }
     { "7" = "/dev/kvm" }
-    { "8" = "/dev/rtc" }
-    { "9" = "/dev/hpet" }
 }
 { "save_image_format" = "raw" }
 { "dump_image_format" = "raw" }
-- 
2.25.4

Re: [PATCH] qemu: do not allow /dev/rtc or /dev/hpet access via the devices cgroup

Posted by Michal Privoznik 1 week ago
On 5/19/20 1:06 AM, Paolo Bonzini wrote:
> The RTC and HPET modes for the QEMU emulation tick have been dropped almost 9 years
> ago, in commit 25f3151ece1d5881826232bebccc21b588d4e03e.  Do not allow them in the

qemu.git $ git describe --tags 25f3151ece1d5881826232bebccc21b588d4e03e
v0.14.0-rc0-1169-g25f3151ece

and the minimum supported version is 1.5.0 so this is safe to merge from 
min version POV.

> devices cgroup policy.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   docs/drvqemu.html.in               | 1 -
>   src/qemu/qemu.conf                 | 1 -
>   src/qemu/qemu_cgroup.c             | 1 -
>   src/qemu/test_libvirtd_qemu.aug.in | 2 --
>   4 files changed, 5 deletions(-)

It's not only QEMU that might use these but also a library that is 
linking with. However, quick strace showed no access to either of the 
files so:

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

And pushed.

Michal

Re: [PATCH] qemu: do not allow /dev/rtc or /dev/hpet access via the devices cgroup

Posted by Daniel P. Berrangé 1 week ago
On Tue, May 19, 2020 at 10:10:54AM +0200, Michal Privoznik wrote:
> On 5/19/20 1:06 AM, Paolo Bonzini wrote:
> > The RTC and HPET modes for the QEMU emulation tick have been dropped almost 9 years
> > ago, in commit 25f3151ece1d5881826232bebccc21b588d4e03e.  Do not allow them in the
> 
> qemu.git $ git describe --tags 25f3151ece1d5881826232bebccc21b588d4e03e
> v0.14.0-rc0-1169-g25f3151ece
> 
> and the minimum supported version is 1.5.0 so this is safe to merge from min
> version POV.
> 
> > devices cgroup policy.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   docs/drvqemu.html.in               | 1 -
> >   src/qemu/qemu.conf                 | 1 -
> >   src/qemu/qemu_cgroup.c             | 1 -
> >   src/qemu/test_libvirtd_qemu.aug.in | 2 --
> >   4 files changed, 5 deletions(-)
> 
> It's not only QEMU that might use these but also a library that is linking
> with. However, quick strace showed no access to either of the files so:
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> And pushed.

This broke make check

https://ci.centos.org/view/libvirt/job/libvirt-check/systems=libvirt-fedora-32/1170/console

though I don't understand why as it looks like it removed all the
right pieces. I wonder if we had a bad dependancy in make rules
meaning we didn't regenerate

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: do not allow /dev/rtc or /dev/hpet access via the devices cgroup

Posted by Michal Privoznik 1 week ago
On 5/19/20 10:55 AM, Daniel P. Berrangé wrote:
> On Tue, May 19, 2020 at 10:10:54AM +0200, Michal Privoznik wrote:
>> On 5/19/20 1:06 AM, Paolo Bonzini wrote:
>>> The RTC and HPET modes for the QEMU emulation tick have been dropped almost 9 years
>>> ago, in commit 25f3151ece1d5881826232bebccc21b588d4e03e.  Do not allow them in the
>>
>> qemu.git $ git describe --tags 25f3151ece1d5881826232bebccc21b588d4e03e
>> v0.14.0-rc0-1169-g25f3151ece
>>
>> and the minimum supported version is 1.5.0 so this is safe to merge from min
>> version POV.
>>
>>> devices cgroup policy.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>    docs/drvqemu.html.in               | 1 -
>>>    src/qemu/qemu.conf                 | 1 -
>>>    src/qemu/qemu_cgroup.c             | 1 -
>>>    src/qemu/test_libvirtd_qemu.aug.in | 2 --
>>>    4 files changed, 5 deletions(-)
>>
>> It's not only QEMU that might use these but also a library that is linking
>> with. However, quick strace showed no access to either of the files so:
>>
>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>>
>> And pushed.
> 
> This broke make check
> 
> https://ci.centos.org/view/libvirt/job/libvirt-check/systems=libvirt-fedora-32/1170/console
> 
> though I don't understand why as it looks like it removed all the
> right pieces. I wonder if we had a bad dependancy in make rules
> meaning we didn't regenerate

Ah, could it be because of the stray comma? From qemu.conf:

#cgroup_device_acl = [
#    "/dev/null", "/dev/full", "/dev/zero",
#    "/dev/random", "/dev/urandom",
#    "/dev/ptmx", "/dev/kvm",
#]

Let me check if removing the comma after /dev/kvm fixes the build.

Michal

Re: [PATCH] qemu: do not allow /dev/rtc or /dev/hpet access via the devices cgroup

Posted by Paolo Bonzini 1 week ago
On 19/05/20 10:58, Michal Privoznik wrote:
> 
> Ah, could it be because of the stray comma? From qemu.conf:
> 
> #cgroup_device_acl = [
> #    "/dev/null", "/dev/full", "/dev/zero",
> #    "/dev/random", "/dev/urandom",
> #    "/dev/ptmx", "/dev/kvm",
> #]
> 
> Let me check if removing the comma after /dev/kvm fixes the build.

It does.  I forgot to commit it, sorry. :(

Though perhaps accepting the trailing comma is better, which would be
something like the following untested patch:

diff --git a/src/bhyve/libvirtd_bhyve.aug b/src/bhyve/libvirtd_bhyve.aug
index 66079376c4..9f4e582ab9 100644
--- a/src/bhyve/libvirtd_bhyve.aug
+++ b/src/bhyve/libvirtd_bhyve.aug
@@ -15,7 +15,7 @@ module Libvirtd_bhyve =
    let bool_val = store /0|1/
    let int_val = store /[0-9]+/
    let str_array_element = [ seq "el" . str_val ] . del /[ \t\n]*/ ""
-   let str_array_val = counter "el" . array_start . ( str_array_element . ( array_sep . str_array_element ) * ) ? . array_end
+   let str_array_val = counter "el" . array_start . ( ( str_array_element . array_sep ) * . str_array_element ? ) ? . array_end
 
    let str_entry       (kw:string) = [ key kw . value_sep . str_val ]
    let bool_entry      (kw:string) = [ key kw . value_sep . bool_val ]
diff --git a/src/libxl/libvirtd_libxl.aug b/src/libxl/libvirtd_libxl.aug
index 58b9af3707..39049cf139 100644
--- a/src/libxl/libvirtd_libxl.aug
+++ b/src/libxl/libvirtd_libxl.aug
@@ -15,7 +15,7 @@ module Libvirtd_libxl =
    let bool_val = store /0|1/
    let int_val = store /[0-9]+/
    let str_array_element = [ seq "el" . str_val ] . del /[ \t\n]*/ ""
-   let str_array_val = counter "el" . array_start . ( str_array_element . ( array_sep . str_array_element ) * ) ? . array_end
+   let str_array_val = counter "el" . array_start . ( ( str_array_element . array_sep ) * . str_array_element ? ) ? . array_end
 
    let str_entry       (kw:string) = [ key kw . value_sep . str_val ]
    let bool_entry      (kw:string) = [ key kw . value_sep . bool_val ]
diff --git a/src/locking/virtlockd.aug b/src/locking/virtlockd.aug
index 06d508e6e5..66eb4125ad 100644
--- a/src/locking/virtlockd.aug
+++ b/src/locking/virtlockd.aug
@@ -15,7 +15,7 @@ module Virtlockd =
    let bool_val = store /0|1/
    let int_val = store /[0-9]+/
    let str_array_element = [ seq "el" . str_val ] . del /[ \t\n]*/ ""
-   let str_array_val = counter "el" . array_start . ( str_array_element . ( array_sep . str_array_element ) * ) ? . array_end
+   let str_array_val = counter "el" . array_start . ( ( str_array_element . array_sep ) * . str_array_element ? ) ? . array_end
 
    let str_entry       (kw:string) = [ key kw . value_sep . str_val ]
    let bool_entry      (kw:string) = [ key kw . value_sep . bool_val ]
diff --git a/src/logging/virtlogd.aug b/src/logging/virtlogd.aug
index 04580734d6..60bbf44305 100644
--- a/src/logging/virtlogd.aug
+++ b/src/logging/virtlogd.aug
@@ -15,7 +15,7 @@ module Virtlogd =
    let bool_val = store /0|1/
    let int_val = store /[0-9]+/
    let str_array_element = [ seq "el" . str_val ] . del /[ \t\n]*/ ""
-   let str_array_val = counter "el" . array_start . ( str_array_element . ( array_sep . str_array_element ) * ) ? . array_end
+   let str_array_val = counter "el" . array_start . ( ( str_array_element . array_sep ) * . str_array_element ? ) ? . array_end
 
    let str_entry       (kw:string) = [ key kw . value_sep . str_val ]
    let bool_entry      (kw:string) = [ key kw . value_sep . bool_val ]
diff --git a/src/lxc/libvirtd_lxc.aug b/src/lxc/libvirtd_lxc.aug
index be6402cc01..e6ab5f0dde 100644
--- a/src/lxc/libvirtd_lxc.aug
+++ b/src/lxc/libvirtd_lxc.aug
@@ -15,7 +15,7 @@ module Libvirtd_lxc =
    let bool_val = store /0|1/
    let int_val = store /[0-9]+/
    let str_array_element = [ seq "el" . str_val ] . del /[ \t\n]*/ ""
-   let str_array_val = counter "el" . array_start . ( str_array_element . ( array_sep . str_array_element ) * ) ? . array_end
+   let str_array_val = counter "el" . array_start . ( ( str_array_element . array_sep ) * . str_array_element ? ) ? . array_end
 
    let str_entry       (kw:string) = [ key kw . value_sep . str_val ]
    let bool_entry      (kw:string) = [ key kw . value_sep . bool_val ]
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 404498b611..aceace7d86 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -15,7 +15,7 @@ module Libvirtd_qemu =
    let bool_val = store /0|1/
    let int_val = store /[0-9]+/
    let str_array_element = [ seq "el" . str_val ] . del /[ \t\n]*/ ""
-   let str_array_val = counter "el" . array_start . ( str_array_element . ( array_sep . str_array_element ) * ) ? . array_end
+   let str_array_val = counter "el" . array_start . ( ( str_array_element . array_sep ) * . str_array_element ? ) ? . array_end
 
    let str_entry       (kw:string) = [ key kw . value_sep . str_val ]
    let bool_entry      (kw:string) = [ key kw . value_sep . bool_val ]