[libvirt] [PATCH] virt-aa-helper: resolve yet to be created paths

[Christian Ehrhardt]

On Wed, Mar 7, 2018 at 12:45 PM, Michal Privoznik <mprivozn at redhat.com>
wrote:

> On 03/01/2018 03:53 PM, Christian Ehrhardt wrote:
> > In certain cases a xml contains paths that do not yet exist, but
> > are valid as qemu will create them later on - for example
> > vhostuser mode=server sockets.
> >
> > In any such cases so far the check to virFileExists failed and due to
> > that the paths stayed non-resolved in regard to symlinks.
> >
> > But for apparmor those non-resolved rules are non functional as they
> > are evaluated after resolving any symlinks.
> >
> > Therefore for non-existent files and partially non-existent paths
> > resolve as much as possible to get valid rules.
> >
> > Example:
> >    <interface type='vhostuser'>
> >        <model type='virtio'/>
> >        <source type='unix'
> >                path='/var/run/symlinknet'
> >                mode='server'/>
>
> Don't be afraid to write this on one line. Firstly, the 80 character
> line is just a soft limit, secondly I like to see verbatim text
> unchanged. But feel free to keep it as it is. Your call.
>
> >    </interface>
> >
> > Got rendered as:
> >   "/var/run/symlinknet" rw,
> >
> > But correct with "/var/run" being a symlink to "/run" is:
> >   "/run/symlinknet" rw,
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt at canonical.com>
> > ---
> >  src/security/virt-aa-helper.c | 45 ++++++++++++++++++++++++++++++
> ++++++-------
> >  1 file changed, 38 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/security/virt-aa-helper.c
> b/src/security/virt-aa-helper.c
> > index ff0068c..91bc339 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -41,6 +41,7 @@
> >  #include "viralloc.h"
> >  #include "vircommand.h"
> >  #include "virlog.h"
> > +#include "dirname.h"
> >  #include "driver.h"
> >
> >  #include "security_driver.h"
> > @@ -752,6 +753,9 @@ vah_add_path(virBufferPtr buf, const char *path,
> const char *perms, bool recursi
> >      bool explicit_deny_rule = true;
> >      char *sub = NULL;
> >      char *perms_new = NULL;
> > +    char *pathdir = NULL;
> > +    char *pathtmp = NULL;
> > +    char *pathreal = NULL;
> >
> >      if (path == NULL)
> >          return rc;
> > @@ -766,14 +770,38 @@ vah_add_path(virBufferPtr buf, const char *path,
> const char *perms, bool recursi
> >          return 0;
> >      }
> >
> > -    if (virFileExists(path)) {
> > -        if ((tmp = realpath(path, NULL)) == NULL) {
> > -            vah_error(NULL, 0, path);
> > -            vah_error(NULL, 0, _("could not find realpath for disk"));
> > -            return rc;
> > +    /* files might be created by qemu later on and not exist right now.
> > +     * But realpath needs a valid path to work on, therefore:
> > +     * 1. walk the path to find longest valid path
> > +     * 2. get the realpath of that valid path
> > +     * 3. re-combine the realpath with the remaining suffix
> > +     * Note: A totally non existent path is used as-is
> > +     */
> > +    if ((pathdir = mdir_name(path)) == NULL)
> > +        goto cleanup;
> > +    while (!virFileExists(pathdir)) {
> > +        if (VIR_STRDUP_QUIET(pathtmp, pathdir) < 0)
> > +            goto cleanup;
> > +        VIR_FREE(pathdir);
> > +        if ((pathdir = mdir_name(pathtmp)) == NULL)
> > +            goto cleanup;
> > +        VIR_FREE(pathtmp);
> > +    }
>
> I guess this can be written simpler:
>
>   if ((pathdir = mdir_name(path)) == NULL)
>       goto cleanup;
>   while (!virFileExists(pathdir)) {
>       if ((pathtmp = mdir_name(pathdir)) == NULL)
>           goto cleanup;
>       VIR_FREE(pathdir);
>       VIR_STEAL_PTR(pathdir, pathtmp);
>   }
>
>
> ACK whether you decide to go with my way (untested though) or what you
> have. Sorry for the delay.
>

Thanks, the usage of steal makes this indeed much nicer.
I played through all the cases in my mind and think it is safe against
leaks just as the former version was.
Also tests with various XMLs were good.

Also another round of checks was good, so pushing in the new version with
your ack.

Thanks for the review Michal!


> Michal
>



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180307/882996ff/attachment-0001.htm>