[libvirt] [PATCH 1/2] virt-aa-helper: add rules for shmem devices

[Christian Ehrhardt]

On Thu, Nov 14, 2019 at 1:14 AM Cole Robinson <crobinso at 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 at 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 at 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