[PATCH] security: AppArmor allow write when os loader readonly=no
Since libvirt commit 3ef9b51b10e52886e8fe8d75e36d0714957616b7, the pflash storage for the os loader file follows its read-only flag, and qemu tries to open the file for writing if set so. This patches virt-aa-helper to generate the VM's AppArmor rules that allow this, using the same domain definition flag and default. Signed-off-by: Miroslav Los <mirlos@cisco.com> --- src/security/virt-aa-helper.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 0374581f07..2f57664a4c 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1001,9 +1001,14 @@ get_files(vahControl * ctl) if (vah_add_file(&buf, ctl->def->os.slic_table, "r") != 0) goto cleanup; - if (ctl->def->os.loader && ctl->def->os.loader->path) - if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0) + if (ctl->def->os.loader && ctl->def->os.loader->path) { + bool readonly = false; + virTristateBoolToBool(ctl->def->os.loader->readonly, &readonly); + if (vah_add_file(&buf, + ctl->def->os.loader->path, + readonly ? "rk" : "rwk") != 0) goto cleanup; + } if (ctl->def->os.loader && ctl->def->os.loader->nvram) { if (storage_source_add_files(ctl->def->os.loader->nvram, &buf, 0) < 0) -- 2.25.1
On Tue, Jun 4, 2024 at 1:17 PM Miroslav Los via Devel <devel@lists.libvirt.org> wrote:
Since libvirt commit 3ef9b51b10e52886e8fe8d75e36d0714957616b7, the pflash storage for the os loader file follows its read-only flag, and qemu tries to open the file for writing if set so.
This patches virt-aa-helper to generate the VM's AppArmor rules that allow this, using the same domain definition flag and default.
Signed-off-by: Miroslav Los <mirlos@cisco.com> --- src/security/virt-aa-helper.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 0374581f07..2f57664a4c 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1001,9 +1001,14 @@ get_files(vahControl * ctl) if (vah_add_file(&buf, ctl->def->os.slic_table, "r") != 0) goto cleanup;
- if (ctl->def->os.loader && ctl->def->os.loader->path) - if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0) + if (ctl->def->os.loader && ctl->def->os.loader->path) { + bool readonly = false; + virTristateBoolToBool(ctl->def->os.loader->readonly, &readonly); + if (vah_add_file(&buf, + ctl->def->os.loader->path, + readonly ? "rk" : "rwk") != 0)
Not tested, but the approach looks totally reasonable and in line with the usual virt-aa-helper handling of such cases. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
goto cleanup; + }
if (ctl->def->os.loader && ctl->def->os.loader->nvram) { if (storage_source_add_files(ctl->def->os.loader->nvram, &buf, 0) < 0) -- 2.25.1
-- Christian Ehrhardt Director of Engineering, Ubuntu Server Canonical Ltd
On Tue, Jun 04, 2024 at 02:20:14PM GMT, Christian Ehrhardt wrote:
On Tue, Jun 4, 2024 at 1:17 PM Miroslav Los via Devel <devel@lists.libvirt.org> wrote:
- if (ctl->def->os.loader && ctl->def->os.loader->path) - if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0) + if (ctl->def->os.loader && ctl->def->os.loader->path) { + bool readonly = false; + virTristateBoolToBool(ctl->def->os.loader->readonly, &readonly); + if (vah_add_file(&buf, + ctl->def->os.loader->path, + readonly ? "rk" : "rwk") != 0)
Not tested, but the approach looks totally reasonable and in line with the usual virt-aa-helper handling of such cases.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
It looks reasonable to me too, but I'd like to see someone other than the author take it for a spin. Christian, can you please give it a shot? Once we have your Tested-by, I'll happily throw in my Reviewed-by and push the patch. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jun 07, 2024 at 09:39:30AM GMT, Andrea Bolognani wrote:
On Tue, Jun 04, 2024 at 02:20:14PM GMT, Christian Ehrhardt wrote:
On Tue, Jun 4, 2024 at 1:17 PM Miroslav Los via Devel <devel@lists.libvirt.org> wrote:
- if (ctl->def->os.loader && ctl->def->os.loader->path) - if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0) + if (ctl->def->os.loader && ctl->def->os.loader->path) { + bool readonly = false; + virTristateBoolToBool(ctl->def->os.loader->readonly, &readonly); + if (vah_add_file(&buf, + ctl->def->os.loader->path, + readonly ? "rk" : "rwk") != 0)
Not tested, but the approach looks totally reasonable and in line with the usual virt-aa-helper handling of such cases.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
It looks reasonable to me too, but I'd like to see someone other than the author take it for a spin. Christian, can you please give it a shot? Once we have your Tested-by, I'll happily throw in my Reviewed-by and push the patch.
Unfortunately this has missed 10.5.0 now. Can we get it tested and merged? Thanks! -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Jul 2, 2024 at 5:02 PM Andrea Bolognani <abologna@redhat.com> wrote:
On Tue, Jun 04, 2024 at 02:20:14PM GMT, Christian Ehrhardt wrote:
On Tue, Jun 4, 2024 at 1:17 PM Miroslav Los via Devel < devel@lists.libvirt.org> wrote:
- if (ctl->def->os.loader && ctl->def->os.loader->path) - if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") !=
On Fri, Jun 07, 2024 at 09:39:30AM GMT, Andrea Bolognani wrote: 0)
+ if (ctl->def->os.loader && ctl->def->os.loader->path) { + bool readonly = false; + virTristateBoolToBool(ctl->def->os.loader->readonly, &readonly); + if (vah_add_file(&buf, + ctl->def->os.loader->path, + readonly ? "rk" : "rwk") != 0)
Not tested, but the approach looks totally reasonable and in line with the usual virt-aa-helper handling of such cases.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
It looks reasonable to me too, but I'd like to see someone other than the author take it for a spin. Christian, can you please give it a shot? Once we have your Tested-by, I'll happily throw in my Reviewed-by and push the patch.
I can currently only easily test that as a patch on top of Ubuntu. There I'm still on 10.0 planning for the merge of 10.6. I can apply and build the patch there fine there though. With that I was trying to look into starting a domain with loader + readonly set to yes/no. When setting readable="no" I also remove the nvram entry which is then no more needed and would cause conflicts. # Before the patch readonly root@o:~# virsh dumpxml testdomain | grep OVMF; uuid=$(virsh domuuid testdomain); cat /etc/apparmor.d/libvirt/libvirt-${uuid}.files | grep OVM <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE_4M.fd</loader> <nvram template='/usr/share/OVMF/OVMF_VARS_4M.fd'>/var/lib/libvirt/nvram/testdomain_VARS.fd</nvram> "/usr/share/OVMF/OVMF_CODE_4M.fd" rk, deny "/usr/share/OVMF/OVMF_CODE_4M.fd" w, writable <loader readonly='no' type='pflash'>/usr/share/OVMF/OVMF_CODE_4M.fd</loader> <!-- <nvram template='/usr/share/OVMF/OVMF_VARS_4M.fd'>/var/lib/libvirt/nvram/testdomain_VARS.fd</nvram> --> => error: internal error: process exited while connecting to monitor: 2024-07-05T10:09:54.914293Z qemu-system-x86_64: Could not open '/usr/share/OVMF/OVMF_CODE_4M.fd': Permission denied As expected as it would be blocked by appamor # After the patch readonly root@o:~# virsh dumpxml testdomain | grep OVMF; uuid=$(virsh domuuid testdomain); cat /etc/apparmor.d/libvirt/libvirt-${uuid}.files | grep OVM <loader readonly='yes' type='pflash'>/usr/share/OVMF/OVMF_CODE_4M.fd</loader> <nvram template='/usr/share/OVMF/OVMF_VARS_4M.fd'>/var/lib/libvirt/nvram/testdomain_VARS.fd</nvram> "/usr/share/OVMF/OVMF_CODE_4M.fd" rk, deny "/usr/share/OVMF/OVMF_CODE_4M.fd" w, writable <loader readonly='no' type='pflash'>/usr/share/OVMF/OVMF_CODE_4M.fd</loader> <!-- <nvram template='/usr/share/OVMF/OVMF_VARS_4M.fd'>/var/lib/libvirt/nvram/testdomain_VARS.fd</nvram> --> => error: internal error: cannot load AppArmor profile 'libvirt-0801d8c1-d29a-41b4-afc3-aff78bfe0bb6' So something is broken in the generated profile now, that is ... not good. Sadly if it breaks on loading it cleans up and does not leave it behind. Regenerating the content it would have created along the way ... $ virsh dumpxml testdomain > verify.xml /usr/lib/libvirt/virt-aa-helper -r -d -u 'libvirt-0801d8c1-d29a-41b4-afc3-aff78bfe0bb6' < verify.xml virt-aa-helper: error: /usr/share/OVMF/OVMF_CODE_4M.fd virt-aa-helper: error: skipped restricted file virt-aa-helper: error: invalid VM definition That is due to the check for valid_path having /usr/share/OVMF/ in restricted_rw and therefore blocking this. Now the question is, was this meant to allow using the system files RW - then I think it is rightfully blocked. Instead I'm going to assume this was meant for separate files. # TEST ONLY - DO NOT DO THIS $ cp /usr/share/OVMF/OVMF_CODE_4M.fd /opt/OVMF_CODE_4M.fd $ chmod 666 /opt/OVMF_CODE_4M.fd root@o:~# virsh dumpxml testdomain | grep OVMF; uuid=$(virsh domuuid testdomain); cat /etc/apparmor.d/libvirt/libvirt-${uuid}.files | grep OVM <loader readonly='no' type='pflash'>/opt/OVMF_CODE_4M.fd</loader> "/opt/OVMF_CODE_4M.fd" rwk, So we can see the code achieves what it is intended to do while not regressing the former flow. But it can not be used on the normal paths, that is probably good to avoid accidentally overwriting these. But I think along the patch we should hear a little bit more about the use case to ensure that it has a practical use? @Miroslav: how exactly would you make use of that now? Valid once the question above about the use case is answered: Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Unfortunately this has missed 10.5.0 now. Can we get it tested and merged? Thanks!
-- Andrea Bolognani / Red Hat / Virtualization
-- Christian Ehrhardt Director of Engineering, Ubuntu Server Canonical Ltd
On Fri, Jul 05, 2024 at 12:38:36PM GMT, Christian Ehrhardt wrote:
That is due to the check for valid_path having /usr/share/OVMF/ in restricted_rw and therefore blocking this. Now the question is, was this meant to allow using the system files RW - then I think it is rightfully blocked. Instead I'm going to assume this was meant for separate files.
# TEST ONLY - DO NOT DO THIS $ cp /usr/share/OVMF/OVMF_CODE_4M.fd /opt/OVMF_CODE_4M.fd $ chmod 666 /opt/OVMF_CODE_4M.fd
root@o:~# virsh dumpxml testdomain | grep OVMF; uuid=$(virsh domuuid testdomain); cat /etc/apparmor.d/libvirt/libvirt-${uuid}.files | grep OVM <loader readonly='no' type='pflash'>/opt/OVMF_CODE_4M.fd</loader> "/opt/OVMF_CODE_4M.fd" rwk,
So we can see the code achieves what it is intended to do while not regressing the former flow.
But it can not be used on the normal paths, that is probably good to avoid accidentally overwriting these.
We definitely don't want the shared CODE files to be used in read/write mode, as that would certainly cause corruption. In fact, I would have expected such a request on the user's part to be outright rejected by libvirt, but when I tried I realized that wasn't the case, at least in some scenarios. I've just posted some patches[1] that should improve the situation.
But I think along the patch we should hear a little bit more about the use case to ensure that it has a practical use?
@Miroslav: how exactly would you make use of that now?
Yes, it would be nice to get confirmation. [1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/2NR3N... -- Andrea Bolognani / Red Hat / Virtualization
My reply by email has not arrived by now, hence I'll post it via the archive site. Sorry for the potential double post. Older bootloaders were not split into separate _CODE.fd and _VARS.fd, i.e. there was no separate nvram for the latter file to create. The guest could write to the single bootloader, which then must not be shared. You mark such bootloaders readonly=no and make a copy of the pflash, e.g. next to the VM's disk files, as you did in your TEST ONLY run. It is a mode of operation supported by the formatdomain documentation on the loader element and intended to work as described. This patch makes such combination of parameters actually succeed on Ubuntu, which I find should be useful to the project. In the VMs we use this for, they do not actually write anything to the loader. This meant that we never noticed the bug, which was present in focal and configured qemu to open the loader read-only anyway. But it failed on AA in noble since the bug is now fixed in newer libvirt. As a workaround, we've in the mean time switched to marking the loader stateless and read-only, which is now allowed in libvirt, and also works without requiring a separate nvram. Of course, this only works because the VM does not make any writes and would fail in case it needed to.
On Tue, Jul 09, 2024 at 08:41:17AM GMT, mirlos--- via Devel wrote:
My reply by email has not arrived by now, hence I'll post it via the archive site. Sorry for the potential double post.
No double post as far as I can tell :)
Older bootloaders were not split into separate _CODE.fd and _VARS.fd, i.e. there was no separate nvram for the latter file to create. The guest could write to the single bootloader, which then must not be shared.
You mark such bootloaders readonly=no and make a copy of the pflash, e.g. next to the VM's disk files, as you did in your TEST ONLY run.
It is a mode of operation supported by the formatdomain documentation on the loader element and intended to work as described. This patch makes such combination of parameters actually succeed on Ubuntu, which I find should be useful to the project.
In the VMs we use this for, they do not actually write anything to the loader. This meant that we never noticed the bug, which was present in focal and configured qemu to open the loader read-only anyway. But it failed on AA in noble since the bug is now fixed in newer libvirt.
So the behavior changed between libvirt 6.0.0 in Ubuntu 20.04 and libvirt 10.0.0 in Ubuntu 24.04. That's not surprising, since I've done a lot of work to fix and improve firmware handling around the 9.2.0 timeframe. You seem to identify the change of behavior as a bug fix, which matches my understanding too.
As a workaround, we've in the mean time switched to marking the loader stateless and read-only, which is now allowed in libvirt, and also works without requiring a separate nvram. Of course, this only works because the VM does not make any writes and would fail in case it needed to.
Right, that ought to do the trick. As far as I'm concerned, I'm satisfied with the explanation you've provided so the patch gets my Reviewed-by: Andrea Bolognani <abologna@redhat.com> Christian, any objection to pushing it? -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Jul 11, 2024 at 06:26:31AM GMT, Andrea Bolognani wrote:
On Tue, Jul 09, 2024 at 08:41:17AM GMT, mirlos--- via Devel wrote:
My reply by email has not arrived by now, hence I'll post it via the archive site. Sorry for the potential double post.
No double post as far as I can tell :)
Older bootloaders were not split into separate _CODE.fd and _VARS.fd, i.e. there was no separate nvram for the latter file to create. The guest could write to the single bootloader, which then must not be shared.
You mark such bootloaders readonly=no and make a copy of the pflash, e.g. next to the VM's disk files, as you did in your TEST ONLY run.
It is a mode of operation supported by the formatdomain documentation on the loader element and intended to work as described. This patch makes such combination of parameters actually succeed on Ubuntu, which I find should be useful to the project.
In the VMs we use this for, they do not actually write anything to the loader. This meant that we never noticed the bug, which was present in focal and configured qemu to open the loader read-only anyway. But it failed on AA in noble since the bug is now fixed in newer libvirt.
So the behavior changed between libvirt 6.0.0 in Ubuntu 20.04 and libvirt 10.0.0 in Ubuntu 24.04. That's not surprising, since I've done a lot of work to fix and improve firmware handling around the 9.2.0 timeframe.
You seem to identify the change of behavior as a bug fix, which matches my understanding too.
As a workaround, we've in the mean time switched to marking the loader stateless and read-only, which is now allowed in libvirt, and also works without requiring a separate nvram. Of course, this only works because the VM does not make any writes and would fail in case it needed to.
Right, that ought to do the trick.
As far as I'm concerned, I'm satisfied with the explanation you've provided so the patch gets my
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Christian, any objection to pushing it?
Since I got no reply, I'm going to assume Christian is okay with me pushing the patch. We still have a bit of time to revert it before 10.6.0 is tagged if needs be. -- Andrea Bolognani / Red Hat / Virtualization
participants (4)
-
Andrea Bolognani -
Christian Ehrhardt -
mirlos＠cisco.com -
Miroslav Los