[libvirt] [PATCH 0/2] virt-aa-helper for shmem device
Cole was recently adding a few of the usual apparmor suspects to BZ 1761645 and I was taking a look at the low hanging fruits of it today. It isn't perfect, but would resolve the reported issue - so I'd appreciate a review. Limitations: - One could break the path creating elements in qemuBuildShmemBackendMemProps and qemuDomainPrepareShmemChardev into extra functions and then use those from virt-aa-helper. But I haven't done so yet and unless it is strictly required consider it too much for what we want/need to achieve here. - I haven't covered hotplug of shmem devices yet, it seems there is no infrastructure for their labels yet and I wasn't sure how important shmem-hotplug would even be to anyone. Christian Ehrhardt (2): virt-aa-helper: add rules for shmem devices virt-aa-helper: testcase for shmem devices src/security/virt-aa-helper.c | 35 +++++++++++++++++++++++++++++++++++ tests/virt-aa-helper-test | 15 +++++++++++++++ 2 files changed, 50 insertions(+) -- 2.23.0
Shared memory devices need qemu to be able to access certain paths either for the shared memory directly (mostly ivshmem-plain) or for a socket (mostly ivshmem-doorbell). Add logic to virt-aa-helper to render those apparmor rules based on the domain configuration. https://bugzilla.redhat.com/show_bug.cgi?id=1761645 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7d7262ca39..8c261f0010 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -958,6 +958,7 @@ get_files(vahControl * ctl) int rc = -1; size_t i; char *uuid; + char *mem_path = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; bool needsVfio = false, needsvhost = false, needsgl = false; @@ -1224,6 +1225,39 @@ get_files(vahControl * ctl) } } + for (i = 0; i < ctl->def->nshmems; i++) { + if (ctl->def->shmems[i]) { + virDomainShmemDef *shmem = ctl->def->shmems[i]; + /* server path can be on any type and overwrites defaults */ + if (shmem->server.enabled && + shmem->server.chr.data.nix.path) { + if (vah_add_file(&buf, shmem->server.chr.data.nix.path, + "rw") != 0) + goto cleanup; + } else { + switch (shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + /* until exposed, recreate qemuBuildShmemBackendMemProps */ + if (virAsprintf(&mem_path, "/dev/shm/%s", shmem->name) < 0) + goto cleanup; + break; + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + /* until exposed, recreate qemuDomainPrepareShmemChardev */ + if (virAsprintf(&mem_path, "/var/lib/libvirt/shmem-%s-sock", + shmem->name) < 0) + goto cleanup; + break; + } + if (mem_path != NULL) { + if (vah_add_file(&buf, mem_path, "rw") != 0) + goto cleanup; + } + } + } + } + + if (ctl->def->tpm) { char *shortName = NULL; const char *tpmpath = NULL; @@ -1324,6 +1358,7 @@ get_files(vahControl * ctl) ctl->files = virBufferContentAndReset(&buf); cleanup: + VIR_FREE(mem_path); VIR_FREE(uuid); return rc; } -- 2.23.0
On 10/22/19 8:18 AM, Christian Ehrhardt wrote:
Shared memory devices need qemu to be able to access certain paths either for the shared memory directly (mostly ivshmem-plain) or for a socket (mostly ivshmem-doorbell).
Add logic to virt-aa-helper to render those apparmor rules based on the domain configuration.
https://bugzilla.redhat.com/show_bug.cgi?id=1761645
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7d7262ca39..8c261f0010 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -958,6 +958,7 @@ get_files(vahControl * ctl) int rc = -1; size_t i; char *uuid; + char *mem_path = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; bool needsVfio = false, needsvhost = false, needsgl = false;
@@ -1224,6 +1225,39 @@ get_files(vahControl * ctl) } }
+ for (i = 0; i < ctl->def->nshmems; i++) { + if (ctl->def->shmems[i]) {
shmems[i] should never be NULL here except in the case of a serious and obvious bug elsewhere in libvirt. So this should be removed IMO. Same goes for all other device list handling in this function, but that's a separate issue. Also style comment, IMO in cases where this type of pattern _is_ relevant, it's nicer to do if (!condition) continue; Which saves a lot of indenting
+ virDomainShmemDef *shmem = ctl->def->shmems[i]; + /* server path can be on any type and overwrites defaults */ + if (shmem->server.enabled && + shmem->server.chr.data.nix.path) { + if (vah_add_file(&buf, shmem->server.chr.data.nix.path, + "rw") != 0) + goto cleanup; + } else { + switch (shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + /* until exposed, recreate qemuBuildShmemBackendMemProps */ + if (virAsprintf(&mem_path, "/dev/shm/%s", shmem->name) < 0) + goto cleanup;
virAsprintf is gone in git, you can use g_strdup_printf, which also means dropping the error checking. See the other conversions patches With those changes, for the series: Reviewed-by: Cole Robinson <crobinso@redhat.com>
+ break; + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + /* until exposed, recreate qemuDomainPrepareShmemChardev */ + if (virAsprintf(&mem_path, "/var/lib/libvirt/shmem-%s-sock", + shmem->name) < 0) + goto cleanup; + break; + } + if (mem_path != NULL) { + if (vah_add_file(&buf, mem_path, "rw") != 0) + goto cleanup; + } + } + } + } + + if (ctl->def->tpm) { char *shortName = NULL; const char *tpmpath = NULL; @@ -1324,6 +1358,7 @@ get_files(vahControl * ctl) ctl->files = virBufferContentAndReset(&buf);
cleanup: + VIR_FREE(mem_path); VIR_FREE(uuid); return rc; }
- Cole
On Thu, Nov 14, 2019 at 1:14 AM Cole Robinson <crobinso@redhat.com> wrote:
On 10/22/19 8:18 AM, Christian Ehrhardt wrote:
Shared memory devices need qemu to be able to access certain paths either for the shared memory directly (mostly ivshmem-plain) or for a socket (mostly ivshmem-doorbell).
Add logic to virt-aa-helper to render those apparmor rules based on the domain configuration.
https://bugzilla.redhat.com/show_bug.cgi?id=1761645
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7d7262ca39..8c261f0010 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -958,6 +958,7 @@ get_files(vahControl * ctl) int rc = -1; size_t i; char *uuid; + char *mem_path = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; bool needsVfio = false, needsvhost = false, needsgl = false;
@@ -1224,6 +1225,39 @@ get_files(vahControl * ctl) } }
+ for (i = 0; i < ctl->def->nshmems; i++) { + if (ctl->def->shmems[i]) {
shmems[i] should never be NULL here except in the case of a serious and obvious bug elsewhere in libvirt. So this should be removed IMO. Same goes for all other device list handling in this function, but that's a separate issue.
Hi Cole and thanks for the review! I continued the pattern which existed for ages, but I agree and thank you for pointing this one out! I have reworked it and will append a patch that generally removes those as I agree these days this should be safe since this is no interim structure but a freshly parsed definition. OTOH you could say that is defensive programming, we will see if there is different feedback on the explicit patch about it.
Also style comment, IMO in cases where this type of pattern _is_ relevant, it's nicer to do
if (!condition) continue;
Which saves a lot of indenting
+ virDomainShmemDef *shmem = ctl->def->shmems[i]; + /* server path can be on any type and overwrites defaults */ + if (shmem->server.enabled && + shmem->server.chr.data.nix.path) { + if (vah_add_file(&buf, shmem->server.chr.data.nix.path, + "rw") != 0) + goto cleanup; + } else { + switch (shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + /* until exposed, recreate qemuBuildShmemBackendMemProps */ + if (virAsprintf(&mem_path, "/dev/shm/%s", shmem->name) < 0) + goto cleanup;
virAsprintf is gone in git, you can use g_strdup_printf, which also means dropping the error checking. See the other conversions patches
Yeah I knew this was coming, but wanted to wait for general reviews to not proliferate this series with v1..v99 :-) Done in v2
With those changes, for the series:
Reviewed-by: Cole Robinson <crobinso@redhat.com>
+ break; + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + /* until exposed, recreate qemuDomainPrepareShmemChardev */ + if (virAsprintf(&mem_path, "/var/lib/libvirt/shmem-%s-sock", + shmem->name) < 0) + goto cleanup; + break; + } + if (mem_path != NULL) { + if (vah_add_file(&buf, mem_path, "rw") != 0) + goto cleanup; + } + } + } + } + + if (ctl->def->tpm) { char *shortName = NULL; const char *tpmpath = NULL; @@ -1324,6 +1358,7 @@ get_files(vahControl * ctl) ctl->files = virBufferContentAndReset(&buf);
cleanup: + VIR_FREE(mem_path); VIR_FREE(uuid); return rc; }
- Cole
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
On Tue, 22 Oct 2019, Christian Ehrhardt wrote:
Shared memory devices need qemu to be able to access certain paths either for the shared memory directly (mostly ivshmem-plain) or for a socket (mostly ivshmem-doorbell).
Add logic to virt-aa-helper to render those apparmor rules based on the domain configuration.
https://bugzilla.redhat.com/show_bug.cgi?id=1761645
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7d7262ca39..8c261f0010 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -958,6 +958,7 @@ get_files(vahControl * ctl) int rc = -1; size_t i; char *uuid; + char *mem_path = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; bool needsVfio = false, needsvhost = false, needsgl = false;
@@ -1224,6 +1225,39 @@ get_files(vahControl * ctl) } }
+ for (i = 0; i < ctl->def->nshmems; i++) { + if (ctl->def->shmems[i]) { + virDomainShmemDef *shmem = ctl->def->shmems[i]; + /* server path can be on any type and overwrites defaults */ + if (shmem->server.enabled && + shmem->server.chr.data.nix.path) { + if (vah_add_file(&buf, shmem->server.chr.data.nix.path, + "rw") != 0) + goto cleanup;
I'll let others comment on the code changes, but this apparmor rule looks ok.
+ } else {
That said, I wonder about the logic here since up above we have: if (shmem->server.enabled && shmem->server.chr.data.nix.path) but here we just have 'else'. What happens if server.enabled is false and server.chr.data.nix.path is set or vice versa? Does this 'else' clause correctly handle those scenarios?
+ switch (shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + /* until exposed, recreate qemuBuildShmemBackendMemProps */ + if (virAsprintf(&mem_path, "/dev/shm/%s", shmem->name) < 0) + goto cleanup; + break; + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + /* until exposed, recreate qemuDomainPrepareShmemChardev */ + if (virAsprintf(&mem_path, "/var/lib/libvirt/shmem-%s-sock", + shmem->name) < 0) + goto cleanup; + break; + } + if (mem_path != NULL) { + if (vah_add_file(&buf, mem_path, "rw") != 0) + goto cleanup; + } + } + } + } +
-- Jamie Strandboge | http://www.canonical.com
On Tue, Nov 19, 2019 at 10:25 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Tue, 22 Oct 2019, Christian Ehrhardt wrote:
Shared memory devices need qemu to be able to access certain paths either for the shared memory directly (mostly ivshmem-plain) or for a socket (mostly ivshmem-doorbell).
Add logic to virt-aa-helper to render those apparmor rules based on the domain configuration.
https://bugzilla.redhat.com/show_bug.cgi?id=1761645
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7d7262ca39..8c261f0010 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -958,6 +958,7 @@ get_files(vahControl * ctl) int rc = -1; size_t i; char *uuid; + char *mem_path = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; bool needsVfio = false, needsvhost = false, needsgl = false;
@@ -1224,6 +1225,39 @@ get_files(vahControl * ctl) } }
+ for (i = 0; i < ctl->def->nshmems; i++) { + if (ctl->def->shmems[i]) { + virDomainShmemDef *shmem = ctl->def->shmems[i]; + /* server path can be on any type and overwrites defaults */ + if (shmem->server.enabled && + shmem->server.chr.data.nix.path) { + if (vah_add_file(&buf, shmem->server.chr.data.nix.path, + "rw") != 0) + goto cleanup;
I'll let others comment on the code changes, but this apparmor rule looks ok.
+ } else {
That said, I wonder about the logic here since up above we have:
if (shmem->server.enabled && shmem->server.chr.data.nix.path)
but here we just have 'else'. What happens if server.enabled is false and server.chr.data.nix.path is set or vice versa? Does this 'else' clause correctly handle those scenarios?
Yes if either of the above isn't fulfilled it will fall back to use the default paths. So a single else without other checks should be correct. The following switch then differs the default paths used base on the model.
+ switch (shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + /* until exposed, recreate qemuBuildShmemBackendMemProps */ + if (virAsprintf(&mem_path, "/dev/shm/%s", shmem->name) < 0) + goto cleanup; + break; + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + /* until exposed, recreate qemuDomainPrepareShmemChardev */ + if (virAsprintf(&mem_path, "/var/lib/libvirt/shmem-%s-sock", + shmem->name) < 0) + goto cleanup; + break; + } + if (mem_path != NULL) { + if (vah_add_file(&buf, mem_path, "rw") != 0) + goto cleanup; + } + } + } + } +
-- Jamie Strandboge | http://www.canonical.com
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
On Wed, 20 Nov 2019, Christian Ehrhardt wrote:
On Tue, Nov 19, 2019 at 10:25 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Tue, 22 Oct 2019, Christian Ehrhardt wrote:
+ for (i = 0; i < ctl->def->nshmems; i++) { + if (ctl->def->shmems[i]) { + virDomainShmemDef *shmem = ctl->def->shmems[i]; + /* server path can be on any type and overwrites defaults */ + if (shmem->server.enabled && + shmem->server.chr.data.nix.path) { + if (vah_add_file(&buf, shmem->server.chr.data.nix.path, + "rw") != 0) + goto cleanup;
I'll let others comment on the code changes, but this apparmor rule looks ok.
+ } else {
That said, I wonder about the logic here since up above we have:
if (shmem->server.enabled && shmem->server.chr.data.nix.path)
but here we just have 'else'. What happens if server.enabled is false and server.chr.data.nix.path is set or vice versa? Does this 'else' clause correctly handle those scenarios?
Yes if either of the above isn't fulfilled it will fall back to use the default paths. So a single else without other checks should be correct. The following switch then differs the default paths used base on the model.
Thanks! We agreed on irc a small code comment would clarify this. LGTM provided you add a code comment similar to what we discussed on IRC. -- Jamie Strandboge | http://www.canonical.com
Adding build time self tests for basic (deprecated), doorbell and plain mode. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- tests/virt-aa-helper-test | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 6e674bfe5c..6a6703ecf5 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -384,6 +384,21 @@ testme "0" "dri egl" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl1.*rw,$" sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<graphics type='spice'><gl enable='yes' rendernode='/dev/dri/testegl2'/></graphics></devices>,g" "$template_xml" > "$test_xml" testme "0" "dri spice" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl2.*rw,$" +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='my_shmem0'><model type='ivshmem'/><size unit='M'>4</size></shmem></devices>,g" "$template_xml" > "$test_xml" +testme "0" "shmem" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/shmem-my_shmem0-sock\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='my_shmem0'><model type='ivshmem'/><size unit='M'>4</size><server path='/var/lib/libvirt/ivshmem_socket'/></shmem></devices>,g" "$template_xml" > "$test_xml" +testme "0" "shmem serverpath" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ivshmem_socket\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='my_shmem0'><model type='ivshmem-plain'/><size unit='M'>4</size></shmem></devices>,g" "$template_xml" > "$test_xml" +testme "0" "shmem plain" "-r -u $valid_uuid" "$test_xml" "\"/dev/shm/my_shmem0\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='shmem_server'><model type='ivshmem-doorbell'/></shmem></devices>,g" "$template_xml" > "$test_xml" +testme "0" "shmem doorbell" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/shmem-shmem_server-sock\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='shmem_server'><model type='ivshmem-doorbell'/><server path='/var/lib/libvirt/ivshmem_socket'/></shmem></devices>,g" "$template_xml" > "$test_xml" +testme "0" "shmem doorbell serverpath" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ivshmem_socket\"\s*rw,$" + testme "0" "help" "-h" echo "" >$output -- 2.23.0
Cole was recently adding a few of the usual apparmor suspects to BZ 1761645 and I was taking a look at the low hanging fruits of it today. It isn't perfect, but would resolve the reported issue - so I'd appreciate a review. Limitations: - One could break the path creating elements in qemuBuildShmemBackendMemProps and qemuDomainPrepareShmemChardev into extra functions and then use those from virt-aa-helper. But I haven't done so yet and unless it is strictly required consider it too much for what we want/need to achieve here. - I haven't covered hotplug of shmem devices yet, it seems there is no infrastructure for their labels yet and I wasn't sure how important shmem-hotplug would even be to anyone. Updates in v2: - rebase latest master - drop checking shmems[i] - switch to use g_strdup_printf - added an extra patch removing checks like ctl->def->mems[i] - add reviewed-by tag Christian Ehrhardt (3): virt-aa-helper: add rules for shmem devices virt-aa-helper: testcase for shmem devices virt-aa-helper: drop pointer checks in get_files src/security/virt-aa-helper.c | 166 +++++++++++++++++++--------------- tests/virt-aa-helper-test | 15 +++ 2 files changed, 109 insertions(+), 72 deletions(-) -- 2.24.0
Shared memory devices need qemu to be able to access certain paths either for the shared memory directly (mostly ivshmem-plain) or for a socket (mostly ivshmem-doorbell). Add logic to virt-aa-helper to render those apparmor rules based on the domain configuration. https://bugzilla.redhat.com/show_bug.cgi?id=1761645 Reviewed-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5ac9a9eeb8..c6c4bb9bd0 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -929,6 +929,7 @@ get_files(vahControl * ctl) int rc = -1; size_t i; char *uuid; + char *mem_path = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; bool needsVfio = false, needsvhost = false, needsgl = false; @@ -1192,6 +1193,35 @@ get_files(vahControl * ctl) } } + for (i = 0; i < ctl->def->nshmems; i++) { + virDomainShmemDef *shmem = ctl->def->shmems[i]; + /* server path can be on any type and overwrites defaults */ + if (shmem->server.enabled && + shmem->server.chr.data.nix.path) { + if (vah_add_file(&buf, shmem->server.chr.data.nix.path, + "rw") != 0) + goto cleanup; + } else { + switch (shmem->model) { + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: + /* until exposed, recreate qemuBuildShmemBackendMemProps */ + mem_path = g_strdup_printf("/dev/shm/%s", shmem->name); + break; + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: + /* until exposed, recreate qemuDomainPrepareShmemChardev */ + mem_path = g_strdup_printf("/var/lib/libvirt/shmem-%s-sock", + shmem->name); + break; + } + if (mem_path != NULL) { + if (vah_add_file(&buf, mem_path, "rw") != 0) + goto cleanup; + } + } + } + + if (ctl->def->tpm) { char *shortName = NULL; const char *tpmpath = NULL; @@ -1286,6 +1316,7 @@ get_files(vahControl * ctl) ctl->files = virBufferContentAndReset(&buf); cleanup: + VIR_FREE(mem_path); VIR_FREE(uuid); return rc; } -- 2.24.0
Adding build time self tests for basic (deprecated), doorbell and plain mode. Reviewed-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- tests/virt-aa-helper-test | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 6e674bfe5c..6a6703ecf5 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -384,6 +384,21 @@ testme "0" "dri egl" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl1.*rw,$" sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<graphics type='spice'><gl enable='yes' rendernode='/dev/dri/testegl2'/></graphics></devices>,g" "$template_xml" > "$test_xml" testme "0" "dri spice" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl2.*rw,$" +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='my_shmem0'><model type='ivshmem'/><size unit='M'>4</size></shmem></devices>,g" "$template_xml" > "$test_xml" +testme "0" "shmem" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/shmem-my_shmem0-sock\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='my_shmem0'><model type='ivshmem'/><size unit='M'>4</size><server path='/var/lib/libvirt/ivshmem_socket'/></shmem></devices>,g" "$template_xml" > "$test_xml" +testme "0" "shmem serverpath" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ivshmem_socket\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='my_shmem0'><model type='ivshmem-plain'/><size unit='M'>4</size></shmem></devices>,g" "$template_xml" > "$test_xml" +testme "0" "shmem plain" "-r -u $valid_uuid" "$test_xml" "\"/dev/shm/my_shmem0\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='shmem_server'><model type='ivshmem-doorbell'/></shmem></devices>,g" "$template_xml" > "$test_xml" +testme "0" "shmem doorbell" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/shmem-shmem_server-sock\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='shmem_server'><model type='ivshmem-doorbell'/><server path='/var/lib/libvirt/ivshmem_socket'/></shmem></devices>,g" "$template_xml" > "$test_xml" +testme "0" "shmem doorbell serverpath" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ivshmem_socket\"\s*rw,$" + testme "0" "help" "-h" echo "" >$output -- 2.24.0
It was mentioned that the pointers in loops like: for (i = 0; i < ctl->def->nserials; i++) if (ctl->def->serials[i] ... will always be !NULL or we would have a much more serious problem. Simplify the if chains in get_files by dropping these checks. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 135 ++++++++++++++++------------------ 1 file changed, 63 insertions(+), 72 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index c6c4bb9bd0..17f49a6259 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -965,8 +965,7 @@ get_files(vahControl * ctl) } for (i = 0; i < ctl->def->nserials; i++) - if (ctl->def->serials[i] && - (ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -980,8 +979,7 @@ get_files(vahControl * ctl) goto cleanup; for (i = 0; i < ctl->def->nconsoles; i++) - if (ctl->def->consoles[i] && - (ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -993,8 +991,7 @@ get_files(vahControl * ctl) goto cleanup; for (i = 0; i < ctl->def->nparallels; i++) - if (ctl->def->parallels[i] && - (ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -1008,8 +1005,7 @@ get_files(vahControl * ctl) goto cleanup; for (i = 0; i < ctl->def->nchannels; i++) - if (ctl->def->channels[i] && - (ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -1082,76 +1078,74 @@ get_files(vahControl * ctl) "r") != 0) goto cleanup; - for (i = 0; i < ctl->def->nhostdevs; i++) - if (ctl->def->hostdevs[i]) { - virDomainHostdevDefPtr dev = ctl->def->hostdevs[i]; - virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb; - switch (dev->source.subsys.type) { - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { - virUSBDevicePtr usb = - virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL); + for (i = 0; i < ctl->def->nhostdevs; i++) { + virDomainHostdevDefPtr dev = ctl->def->hostdevs[i]; + virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb; + switch (dev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { + virUSBDevicePtr usb = + virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL); - if (usb == NULL) - continue; - - if (virHostdevFindUSBDevice(dev, true, &usb) < 0) - continue; + if (usb == NULL) + continue; - rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf); - virUSBDeviceFree(usb); - if (rc != 0) - goto cleanup; - break; - } + if (virHostdevFindUSBDevice(dev, true, &usb) < 0) + continue; - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: { - virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev; - switch ((virMediatedDeviceModelType) mdevsrc->model) { - case VIR_MDEV_MODEL_TYPE_VFIO_PCI: - case VIR_MDEV_MODEL_TYPE_VFIO_AP: - case VIR_MDEV_MODEL_TYPE_VFIO_CCW: - needsVfio = true; - break; - case VIR_MDEV_MODEL_TYPE_LAST: - default: - virReportEnumRangeError(virMediatedDeviceModelType, - mdevsrc->model); - break; - } - break; - } - - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { - virPCIDevicePtr pci = virPCIDeviceNew( - dev->source.subsys.u.pci.addr.domain, - dev->source.subsys.u.pci.addr.bus, - dev->source.subsys.u.pci.addr.slot, - dev->source.subsys.u.pci.addr.function); + rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf); + virUSBDeviceFree(usb); + if (rc != 0) + goto cleanup; + break; + } - virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend; - if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO || - backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: { + virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev; + switch ((virMediatedDeviceModelType) mdevsrc->model) { + case VIR_MDEV_MODEL_TYPE_VFIO_PCI: + case VIR_MDEV_MODEL_TYPE_VFIO_AP: + case VIR_MDEV_MODEL_TYPE_VFIO_CCW: needsVfio = true; - } + break; + case VIR_MDEV_MODEL_TYPE_LAST: + default: + virReportEnumRangeError(virMediatedDeviceModelType, + mdevsrc->model); + break; + } + break; + } - if (pci == NULL) - continue; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { + virPCIDevicePtr pci = virPCIDeviceNew( + dev->source.subsys.u.pci.addr.domain, + dev->source.subsys.u.pci.addr.bus, + dev->source.subsys.u.pci.addr.slot, + dev->source.subsys.u.pci.addr.function); + + virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend; + if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO || + backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) { + needsVfio = true; + } - rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf); - virPCIDeviceFree(pci); + if (pci == NULL) + continue; - break; - } + rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf); + virPCIDeviceFree(pci); - default: - rc = 0; - break; - } /* switch */ + break; } + default: + rc = 0; + break; + } /* switch */ + } + for (i = 0; i < ctl->def->nfss; i++) { - if (ctl->def->fss[i] && - ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT && + if (ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT && (ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH || ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) && ctl->def->fss[i]->src) { @@ -1166,16 +1160,14 @@ get_files(vahControl * ctl) } for (i = 0; i < ctl->def->ninputs; i++) { - if (ctl->def->inputs[i] && - ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { + if (ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { if (vah_add_file(&buf, ctl->def->inputs[i]->source.evdev, "rw") != 0) goto cleanup; } } for (i = 0; i < ctl->def->nnets; i++) { - if (ctl->def->nets[i] && - ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && + if (ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && ctl->def->nets[i]->data.vhostuser) { virDomainChrSourceDefPtr vhu = ctl->def->nets[i]->data.vhostuser; @@ -1186,8 +1178,7 @@ get_files(vahControl * ctl) } for (i = 0; i < ctl->def->nmems; i++) { - if (ctl->def->mems[i] && - ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { if (vah_add_file(&buf, ctl->def->mems[i]->nvdimmPath, "rw") != 0) goto cleanup; } -- 2.24.0
On 11/14/19 6:20 AM, Christian Ehrhardt wrote:
It was mentioned that the pointers in loops like: for (i = 0; i < ctl->def->nserials; i++) if (ctl->def->serials[i] ... will always be !NULL or we would have a much more serious problem.
Simplify the if chains in get_files by dropping these checks.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 135 ++++++++++++++++------------------ 1 file changed, 63 insertions(+), 72 deletions(-)
Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
On Thu, 14 Nov 2019, Christian Ehrhardt wrote:
It was mentioned that the pointers in loops like: for (i = 0; i < ctl->def->nserials; i++) if (ctl->def->serials[i] ... will always be !NULL or we would have a much more serious problem.
Simplify the if chains in get_files by dropping these checks.
I don't really see why a NULL check is a problem so this feels a bit like busy work and it seems short-circuiting and not doing is exactly what you would want to do in the event of a 'much more serious problem'. Is this really required? +0 to apply on principle (I've not reviewed the changes closely).
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 135 ++++++++++++++++------------------ 1 file changed, 63 insertions(+), 72 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index c6c4bb9bd0..17f49a6259 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -965,8 +965,7 @@ get_files(vahControl * ctl) }
for (i = 0; i < ctl->def->nserials; i++) - if (ctl->def->serials[i] && - (ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -980,8 +979,7 @@ get_files(vahControl * ctl) goto cleanup;
for (i = 0; i < ctl->def->nconsoles; i++) - if (ctl->def->consoles[i] && - (ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -993,8 +991,7 @@ get_files(vahControl * ctl) goto cleanup;
for (i = 0; i < ctl->def->nparallels; i++) - if (ctl->def->parallels[i] && - (ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -1008,8 +1005,7 @@ get_files(vahControl * ctl) goto cleanup;
for (i = 0; i < ctl->def->nchannels; i++) - if (ctl->def->channels[i] && - (ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -1082,76 +1078,74 @@ get_files(vahControl * ctl) "r") != 0) goto cleanup;
- for (i = 0; i < ctl->def->nhostdevs; i++) - if (ctl->def->hostdevs[i]) { - virDomainHostdevDefPtr dev = ctl->def->hostdevs[i]; - virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb; - switch (dev->source.subsys.type) { - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { - virUSBDevicePtr usb = - virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL); + for (i = 0; i < ctl->def->nhostdevs; i++) { + virDomainHostdevDefPtr dev = ctl->def->hostdevs[i]; + virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb; + switch (dev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { + virUSBDevicePtr usb = + virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL);
- if (usb == NULL) - continue; - - if (virHostdevFindUSBDevice(dev, true, &usb) < 0) - continue; + if (usb == NULL) + continue;
- rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf); - virUSBDeviceFree(usb); - if (rc != 0) - goto cleanup; - break; - } + if (virHostdevFindUSBDevice(dev, true, &usb) < 0) + continue;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: { - virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev; - switch ((virMediatedDeviceModelType) mdevsrc->model) { - case VIR_MDEV_MODEL_TYPE_VFIO_PCI: - case VIR_MDEV_MODEL_TYPE_VFIO_AP: - case VIR_MDEV_MODEL_TYPE_VFIO_CCW: - needsVfio = true; - break; - case VIR_MDEV_MODEL_TYPE_LAST: - default: - virReportEnumRangeError(virMediatedDeviceModelType, - mdevsrc->model); - break; - } - break; - } - - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { - virPCIDevicePtr pci = virPCIDeviceNew( - dev->source.subsys.u.pci.addr.domain, - dev->source.subsys.u.pci.addr.bus, - dev->source.subsys.u.pci.addr.slot, - dev->source.subsys.u.pci.addr.function); + rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf); + virUSBDeviceFree(usb); + if (rc != 0) + goto cleanup; + break; + }
- virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend; - if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO || - backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: { + virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev; + switch ((virMediatedDeviceModelType) mdevsrc->model) { + case VIR_MDEV_MODEL_TYPE_VFIO_PCI: + case VIR_MDEV_MODEL_TYPE_VFIO_AP: + case VIR_MDEV_MODEL_TYPE_VFIO_CCW: needsVfio = true; - } + break; + case VIR_MDEV_MODEL_TYPE_LAST: + default: + virReportEnumRangeError(virMediatedDeviceModelType, + mdevsrc->model); + break; + } + break; + }
- if (pci == NULL) - continue; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { + virPCIDevicePtr pci = virPCIDeviceNew( + dev->source.subsys.u.pci.addr.domain, + dev->source.subsys.u.pci.addr.bus, + dev->source.subsys.u.pci.addr.slot, + dev->source.subsys.u.pci.addr.function); + + virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend; + if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO || + backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) { + needsVfio = true; + }
- rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf); - virPCIDeviceFree(pci); + if (pci == NULL) + continue;
- break; - } + rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf); + virPCIDeviceFree(pci);
- default: - rc = 0; - break; - } /* switch */ + break; }
+ default: + rc = 0; + break; + } /* switch */ + } + for (i = 0; i < ctl->def->nfss; i++) { - if (ctl->def->fss[i] && - ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT && + if (ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT && (ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH || ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) && ctl->def->fss[i]->src) { @@ -1166,16 +1160,14 @@ get_files(vahControl * ctl) }
for (i = 0; i < ctl->def->ninputs; i++) { - if (ctl->def->inputs[i] && - ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { + if (ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { if (vah_add_file(&buf, ctl->def->inputs[i]->source.evdev, "rw") != 0) goto cleanup; } }
for (i = 0; i < ctl->def->nnets; i++) { - if (ctl->def->nets[i] && - ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && + if (ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && ctl->def->nets[i]->data.vhostuser) { virDomainChrSourceDefPtr vhu = ctl->def->nets[i]->data.vhostuser;
@@ -1186,8 +1178,7 @@ get_files(vahControl * ctl) }
for (i = 0; i < ctl->def->nmems; i++) { - if (ctl->def->mems[i] && - ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { if (vah_add_file(&buf, ctl->def->mems[i]->nvdimmPath, "rw") != 0) goto cleanup; } -- 2.24.0
-- Jamie Strandboge | http://www.canonical.com
On Tue, Nov 19, 2019 at 10:31 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Thu, 14 Nov 2019, Christian Ehrhardt wrote:
It was mentioned that the pointers in loops like: for (i = 0; i < ctl->def->nserials; i++) if (ctl->def->serials[i] ... will always be !NULL or we would have a much more serious problem.
Simplify the if chains in get_files by dropping these checks.
I don't really see why a NULL check is a problem so this feels a bit like busy work and it seems short-circuiting and not doing is exactly what you would want to do in the event of a 'much more serious problem'. Is this really required? +0 to apply on principle (I've not reviewed the changes closely).
Well Cole brought it up and I intentionally wanted to have the discussion separate to the shmem changes. As I said in the mail on patch 2/2 in v1 of this I consider it "defensive programming" and not strictly required to be removed. I'm happy and fine to keep the checks as-is since they are not on a performance critical path. I agree as you say "in the event of a 'much more serious problem'" this would be exactly what we want. I'll withdraw this patch from the series then unless somebody really shows a reason to why we should drop the checks.
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 135 ++++++++++++++++------------------ 1 file changed, 63 insertions(+), 72 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index c6c4bb9bd0..17f49a6259 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -965,8 +965,7 @@ get_files(vahControl * ctl) }
for (i = 0; i < ctl->def->nserials; i++) - if (ctl->def->serials[i] && - (ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -980,8 +979,7 @@ get_files(vahControl * ctl) goto cleanup;
for (i = 0; i < ctl->def->nconsoles; i++) - if (ctl->def->consoles[i] && - (ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -993,8 +991,7 @@ get_files(vahControl * ctl) goto cleanup;
for (i = 0; i < ctl->def->nparallels; i++) - if (ctl->def->parallels[i] && - (ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -1008,8 +1005,7 @@ get_files(vahControl * ctl) goto cleanup;
for (i = 0; i < ctl->def->nchannels; i++) - if (ctl->def->channels[i] && - (ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || + if ((ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY || ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV || ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE || ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX || @@ -1082,76 +1078,74 @@ get_files(vahControl * ctl) "r") != 0) goto cleanup;
- for (i = 0; i < ctl->def->nhostdevs; i++) - if (ctl->def->hostdevs[i]) { - virDomainHostdevDefPtr dev = ctl->def->hostdevs[i]; - virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb; - switch (dev->source.subsys.type) { - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { - virUSBDevicePtr usb = - virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL); + for (i = 0; i < ctl->def->nhostdevs; i++) { + virDomainHostdevDefPtr dev = ctl->def->hostdevs[i]; + virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb; + switch (dev->source.subsys.type) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { + virUSBDevicePtr usb = + virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL);
- if (usb == NULL) - continue; - - if (virHostdevFindUSBDevice(dev, true, &usb) < 0) - continue; + if (usb == NULL) + continue;
- rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf); - virUSBDeviceFree(usb); - if (rc != 0) - goto cleanup; - break; - } + if (virHostdevFindUSBDevice(dev, true, &usb) < 0) + continue;
- case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: { - virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev; - switch ((virMediatedDeviceModelType) mdevsrc->model) { - case VIR_MDEV_MODEL_TYPE_VFIO_PCI: - case VIR_MDEV_MODEL_TYPE_VFIO_AP: - case VIR_MDEV_MODEL_TYPE_VFIO_CCW: - needsVfio = true; - break; - case VIR_MDEV_MODEL_TYPE_LAST: - default: - virReportEnumRangeError(virMediatedDeviceModelType, - mdevsrc->model); - break; - } - break; - } - - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { - virPCIDevicePtr pci = virPCIDeviceNew( - dev->source.subsys.u.pci.addr.domain, - dev->source.subsys.u.pci.addr.bus, - dev->source.subsys.u.pci.addr.slot, - dev->source.subsys.u.pci.addr.function); + rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf); + virUSBDeviceFree(usb); + if (rc != 0) + goto cleanup; + break; + }
- virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend; - if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO || - backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: { + virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev; + switch ((virMediatedDeviceModelType) mdevsrc->model) { + case VIR_MDEV_MODEL_TYPE_VFIO_PCI: + case VIR_MDEV_MODEL_TYPE_VFIO_AP: + case VIR_MDEV_MODEL_TYPE_VFIO_CCW: needsVfio = true; - } + break; + case VIR_MDEV_MODEL_TYPE_LAST: + default: + virReportEnumRangeError(virMediatedDeviceModelType, + mdevsrc->model); + break; + } + break; + }
- if (pci == NULL) - continue; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: { + virPCIDevicePtr pci = virPCIDeviceNew( + dev->source.subsys.u.pci.addr.domain, + dev->source.subsys.u.pci.addr.bus, + dev->source.subsys.u.pci.addr.slot, + dev->source.subsys.u.pci.addr.function); + + virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend; + if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO || + backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) { + needsVfio = true; + }
- rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf); - virPCIDeviceFree(pci); + if (pci == NULL) + continue;
- break; - } + rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf); + virPCIDeviceFree(pci);
- default: - rc = 0; - break; - } /* switch */ + break; }
+ default: + rc = 0; + break; + } /* switch */ + } + for (i = 0; i < ctl->def->nfss; i++) { - if (ctl->def->fss[i] && - ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT && + if (ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT && (ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH || ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) && ctl->def->fss[i]->src) { @@ -1166,16 +1160,14 @@ get_files(vahControl * ctl) }
for (i = 0; i < ctl->def->ninputs; i++) { - if (ctl->def->inputs[i] && - ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { + if (ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) { if (vah_add_file(&buf, ctl->def->inputs[i]->source.evdev, "rw") != 0) goto cleanup; } }
for (i = 0; i < ctl->def->nnets; i++) { - if (ctl->def->nets[i] && - ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && + if (ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && ctl->def->nets[i]->data.vhostuser) { virDomainChrSourceDefPtr vhu = ctl->def->nets[i]->data.vhostuser;
@@ -1186,8 +1178,7 @@ get_files(vahControl * ctl) }
for (i = 0; i < ctl->def->nmems; i++) { - if (ctl->def->mems[i] && - ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { if (vah_add_file(&buf, ctl->def->mems[i]->nvdimmPath, "rw") != 0) goto cleanup; } -- 2.24.0
-- Jamie Strandboge | http://www.canonical.com
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
On 11/19/19 4:31 PM, Jamie Strandboge wrote:
On Thu, 14 Nov 2019, Christian Ehrhardt wrote:
It was mentioned that the pointers in loops like: for (i = 0; i < ctl->def->nserials; i++) if (ctl->def->serials[i] ... will always be !NULL or we would have a much more serious problem.
Simplify the if chains in get_files by dropping these checks.
I don't really see why a NULL check is a problem so this feels a bit like busy work and it seems short-circuiting and not doing is exactly what you would want to do in the event of a 'much more serious problem'. Is this really required? +0 to apply on principle (I've not reviewed the changes closely).
If for example def->nserials and def->serials[i] disagree, then there is a serious bug somewhere. The internal API contract is that it should never happen, so code shouldn't check for the condition. I'm pretty sure if the XML parser is creating that situation, the qemu driver would crash in a dozen different places. So the patch isn't strictly required, but it is an improvement IMO: it reduces code, improves readability, and helps simplify review for other libvirt devs who may be confused by this uncommon idiom (I was). But I will leave it up to you guys to decide whether to push or not Thanks, Cole
On Wed, 20 Nov 2019, Cole Robinson wrote:
On 11/19/19 4:31 PM, Jamie Strandboge wrote:
On Thu, 14 Nov 2019, Christian Ehrhardt wrote:
It was mentioned that the pointers in loops like: for (i = 0; i < ctl->def->nserials; i++) if (ctl->def->serials[i] ... will always be !NULL or we would have a much more serious problem.
Simplify the if chains in get_files by dropping these checks.
I don't really see why a NULL check is a problem so this feels a bit like busy work and it seems short-circuiting and not doing is exactly what you would want to do in the event of a 'much more serious problem'. Is this really required? +0 to apply on principle (I've not reviewed the changes closely).
If for example def->nserials and def->serials[i] disagree, then there is a serious bug somewhere. The internal API contract is that it should never happen, so code shouldn't check for the condition. I'm pretty sure if the XML parser is creating that situation, the qemu driver would crash in a dozen different places.
So the patch isn't strictly required, but it is an improvement IMO: it reduces code, improves readability, and helps simplify review for other libvirt devs who may be confused by this uncommon idiom (I was). But I will leave it up to you guys to decide whether to push or not
To be clear, I am intentionally not blocking on this. If, as upstream, you'd prefer this to be a certain way for reasons that outweigh my POV, by all means feel free to do that. The changes are mechanical and IMHO don't need an apparmor-focused review (though if you'd prefer I go through the full patch, I can). -- Jamie Strandboge | http://www.canonical.com
On 11/20/19 12:25 PM, Jamie Strandboge wrote:
On Wed, 20 Nov 2019, Cole Robinson wrote:
On 11/19/19 4:31 PM, Jamie Strandboge wrote:
On Thu, 14 Nov 2019, Christian Ehrhardt wrote:
It was mentioned that the pointers in loops like: for (i = 0; i < ctl->def->nserials; i++) if (ctl->def->serials[i] ... will always be !NULL or we would have a much more serious problem.
Simplify the if chains in get_files by dropping these checks.
I don't really see why a NULL check is a problem so this feels a bit like busy work and it seems short-circuiting and not doing is exactly what you would want to do in the event of a 'much more serious problem'. Is this really required? +0 to apply on principle (I've not reviewed the changes closely).
If for example def->nserials and def->serials[i] disagree, then there is a serious bug somewhere. The internal API contract is that it should never happen, so code shouldn't check for the condition. I'm pretty sure if the XML parser is creating that situation, the qemu driver would crash in a dozen different places.
So the patch isn't strictly required, but it is an improvement IMO: it reduces code, improves readability, and helps simplify review for other libvirt devs who may be confused by this uncommon idiom (I was). But I will leave it up to you guys to decide whether to push or not
To be clear, I am intentionally not blocking on this. If, as upstream, you'd prefer this to be a certain way for reasons that outweigh my POV, by all means feel free to do that. The changes are mechanical and IMHO don't need an apparmor-focused review (though if you'd prefer I go through the full patch, I can).
This patch moves the code to be more consistent with the rest of the libvirt code base, so I think it's good to push. I thought Christian was waiting on you for review before pushing apparmor patches though, I didn't want to step on any toes :) Thanks, Cole
On Thu, Nov 14, 2019 at 12:20 PM Christian Ehrhardt <christian.ehrhardt@canonical.com> wrote:
Cole was recently adding a few of the usual apparmor suspects to BZ 1761645 and I was taking a look at the low hanging fruits of it today. It isn't perfect, but would resolve the reported issue - so I'd appreciate a review.
Limitations: - One could break the path creating elements in qemuBuildShmemBackendMemProps and qemuDomainPrepareShmemChardev into extra functions and then use those from virt-aa-helper. But I haven't done so yet and unless it is strictly required consider it too much for what we want/need to achieve here. - I haven't covered hotplug of shmem devices yet, it seems there is no infrastructure for their labels yet and I wasn't sure how important shmem-hotplug would even be to anyone.
Updates in v2: - rebase latest master - drop checking shmems[i] - switch to use g_strdup_printf - added an extra patch removing checks like ctl->def->mems[i] - add reviewed-by tag
Thanks everyone for the further review and discussion. Pushing with Ack/Review Tags after a rebuild/recheck
Christian Ehrhardt (3): virt-aa-helper: add rules for shmem devices virt-aa-helper: testcase for shmem devices virt-aa-helper: drop pointer checks in get_files
src/security/virt-aa-helper.c | 166 +++++++++++++++++++--------------- tests/virt-aa-helper-test | 15 +++ 2 files changed, 109 insertions(+), 72 deletions(-)
-- 2.24.0
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
participants (3)
-
Christian Ehrhardt -
Cole Robinson -
Jamie Strandboge