<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 7, 2018 at 12:45 PM, Michal Privoznik <span dir="ltr"><<a href="mailto:mprivozn@redhat.com" target="_blank">mprivozn@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On 03/01/2018 03:53 PM, Christian Ehrhardt wrote:<br>
> In certain cases a xml contains paths that do not yet exist, but<br>
> are valid as qemu will create them later on - for example<br>
> vhostuser mode=server sockets.<br>
><br>
> In any such cases so far the check to virFileExists failed and due to<br>
> that the paths stayed non-resolved in regard to symlinks.<br>
><br>
> But for apparmor those non-resolved rules are non functional as they<br>
> are evaluated after resolving any symlinks.<br>
><br>
> Therefore for non-existent files and partially non-existent paths<br>
> resolve as much as possible to get valid rules.<br>
><br>
> Example:<br>
> <interface type='vhostuser'><br>
> <model type='virtio'/><br>
> <source type='unix'<br>
> path='/var/run/symlinknet'<br>
> mode='server'/><br>
<br>
</span>Don't be afraid to write this on one line. Firstly, the 80 character<br>
line is just a soft limit, secondly I like to see verbatim text<br>
unchanged. But feel free to keep it as it is. Your call.<br>
<div><div class="m_-5177979950369013633h5"><br>
> </interface><br>
><br>
> Got rendered as:<br>
> "/var/run/symlinknet" rw,<br>
><br>
> But correct with "/var/run" being a symlink to "/run" is:<br>
> "/run/symlinknet" rw,<br>
><br>
> Signed-off-by: Christian Ehrhardt <<a href="mailto:christian.ehrhardt@canonical.com" target="_blank">christian.ehrhardt@canonical.<wbr>com</a>><br>
> ---<br>
> src/security/virt-aa-helper.c | 45 ++++++++++++++++++++++++++++++<wbr>++++++-------<br>
> 1 file changed, 38 insertions(+), 7 deletions(-)<br>
><br>
> diff --git a/src/security/virt-aa-helper.<wbr>c b/src/security/virt-aa-helper.<wbr>c<br>
> index ff0068c..91bc339 100644<br>
> --- a/src/security/virt-aa-helper.<wbr>c<br>
> +++ b/src/security/virt-aa-helper.<wbr>c<br>
> @@ -41,6 +41,7 @@<br>
> #include "viralloc.h"<br>
> #include "vircommand.h"<br>
> #include "virlog.h"<br>
> +#include "dirname.h"<br>
> #include "driver.h"<br>
><br>
> #include "security_driver.h"<br>
> @@ -752,6 +753,9 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi<br>
> bool explicit_deny_rule = true;<br>
> char *sub = NULL;<br>
> char *perms_new = NULL;<br>
> + char *pathdir = NULL;<br>
> + char *pathtmp = NULL;<br>
> + char *pathreal = NULL;<br>
><br>
> if (path == NULL)<br>
> return rc;<br>
> @@ -766,14 +770,38 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi<br>
> return 0;<br>
> }<br>
><br>
> - if (virFileExists(path)) {<br>
> - if ((tmp = realpath(path, NULL)) == NULL) {<br>
> - vah_error(NULL, 0, path);<br>
> - vah_error(NULL, 0, _("could not find realpath for disk"));<br>
> - return rc;<br>
> + /* files might be created by qemu later on and not exist right now.<br>
> + * But realpath needs a valid path to work on, therefore:<br>
> + * 1. walk the path to find longest valid path<br>
> + * 2. get the realpath of that valid path<br>
> + * 3. re-combine the realpath with the remaining suffix<br>
> + * Note: A totally non existent path is used as-is<br>
> + */<br>
> + if ((pathdir = mdir_name(path)) == NULL)<br>
> + goto cleanup;<br>
> + while (!virFileExists(pathdir)) {<br>
> + if (VIR_STRDUP_QUIET(pathtmp, pathdir) < 0)<br>
> + goto cleanup;<br>
> + VIR_FREE(pathdir);<br>
> + if ((pathdir = mdir_name(pathtmp)) == NULL)<br>
> + goto cleanup;<br>
> + VIR_FREE(pathtmp);<br>
> + }<br>
<br>
</div></div>I guess this can be written simpler:<br>
<span><br>
if ((pathdir = mdir_name(path)) == NULL)<br>
</span> goto cleanup;<br>
while (!virFileExists(pathdir)) {<br>
if ((pathtmp = mdir_name(pathdir)) == NULL)<br>
goto cleanup;<br>
VIR_FREE(pathdir);<br>
VIR_STEAL_PTR(pathdir, pathtmp);<br>
}<br>
<br>
<br>
ACK whether you decide to go with my way (untested though) or what you<br>
have. Sorry for the delay.<br></blockquote><div><br></div><div>Thanks, the usage of steal makes this indeed much nicer.</div><div>I played through all the cases in my mind and think it is safe against leaks just as the former version was.</div><div>Also tests with various XMLs were good.</div><div><br></div><div>Also another round of checks was good, so pushing in the new version with your ack.</div><div><br></div><div>Thanks for the review Michal!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="m_-5177979950369013633HOEnZb"><font color="#888888">
Michal<br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="m_-5177979950369013633gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><span style="color:rgb(136,136,136);font-size:12.8px">Christian Ehrhardt</span><div style="color:rgb(136,136,136);font-size:12.8px">Software Engineer, Ubuntu Server</div><div style="color:rgb(136,136,136);font-size:12.8px">Canonical Ltd</div></div></div></div></div>
</div></div>