[v2,7/8] tests/9pfs: add local Tlink test

This test case uses a Tlink request to create a hard link to a regular
file using the 9pfs 'local' fs driver.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 tests/qtest/virtio-9p-test.c | 71 ++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

On Mittwoch, 21. Oktober 2020 14:51:09 CEST Christian Schoenebeck wrote:
> This test case uses a Tlink request to create a hard link to a regular
> file using the 9pfs 'local' fs driver.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  tests/qtest/virtio-9p-test.c | 71 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 33cba24b18..460fa49fe3 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
>          id == P9_RMKDIR ? "RMKDIR" :
>          id == P9_RLCREATE ? "RLCREATE" :
>          id == P9_RSYMLINK ? "RSYMLINK" :
> +        id == P9_RLINK ? "RLINK" :
>          id == P9_RUNLINKAT ? "RUNLINKAT" :
>          id == P9_RFLUSH ? "RFLUSH" :
>          id == P9_RREADDIR ? "READDIR" :
> @@ -767,6 +768,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
>      v9fs_req_free(req);
>  }
> 
> +/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
> +static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
> +                         const char *name, uint16_t tag)
> +{
> +    P9Req *req;
> +
> +    uint32_t body_size = 4 + 4;
> +    uint16_t string_size = v9fs_string_size(name);
> +
> +    g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
> +    body_size += string_size;
> +
> +    req = v9fs_req_init(v9p, body_size, P9_TLINK, tag);
> +    v9fs_uint32_write(req, dfid);
> +    v9fs_uint32_write(req, fid);
> +    v9fs_string_write(req, name);
> +    v9fs_req_send(req);
> +    return req;
> +}
> +
> +/* size[4] Rlink tag[2] */
> +static void v9fs_rlink(P9Req *req)
> +{
> +    v9fs_req_recv(req, P9_RLINK);
> +    v9fs_req_free(req);
> +}
> +
>  /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
>  static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char
> *name, uint32_t flags, uint16_t tag)
> @@ -1142,6 +1170,21 @@ static void do_symlink(QVirtio9P *v9p, const char
> *path, const char *clink, g_free(name);
>  }
> 
> +/* create a hard link named @a clink in directory @a path pointing to @a to
> */ +static void do_hardlink(QVirtio9P *v9p, const char *path, const char
> *clink, +                        const char *to)
> +{
> +    uint32_t dfid, fid;
> +    P9Req *req;
> +
> +    dfid = do_walk(v9p, path);
> +    fid = do_walk(v9p, to);
> +
> +    req = v9fs_tlink(v9p, dfid, fid, clink, 0);
> +    v9fs_req_wait_for_reply(req, NULL);
> +    v9fs_rlink(req);
> +}
> +
>  static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char
> *rpath, uint32_t flags)
>  {
> @@ -1321,6 +1364,33 @@ static void fs_unlinkat_symlink(void *obj, void
> *data, g_free(real_file);
>  }
> 
> +static void fs_hardlink_file(void *obj, void *data, QGuestAllocator
> *t_alloc) +{
> +    QVirtio9P *v9p = obj;
> +    alloc = t_alloc;
> +    struct stat st_real, st_link;
> +    char *real_file = virtio_9p_test_path("07/real_file");
> +    char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
> +
> +    do_attach(v9p);
> +    do_mkdir(v9p, "/", "07");
> +    do_lcreate(v9p, "07", "real_file");
> +    g_assert(stat(real_file, &st_real) == 0);
> +    g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
> +
> +    do_hardlink(v9p, "07", "hardlink_file", "07/real_file");
> +
> +    /* check if link exists now ... */
> +    g_assert(stat(hardlink_file, &st_link) == 0);
> +    /* ... and it's a hard link, right? */
> +    g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
> +    g_assert(st_link.st_dev == st_real.st_dev);
> +    g_assert(st_link.st_ino == st_real.st_ino);
> +
> +    g_free(hardlink_file);
> +    g_free(real_file);
> +}
> +
>  static void *assign_9p_local_driver(GString *cmd_line, void *arg)
>  {
>      virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> @@ -1367,6 +1437,7 @@ static void register_virtio_9p_test(void)
>      qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file,
> &opts); qos_add_test("local/unlinkat_symlink", "virtio-9p",
> fs_unlinkat_symlink, &opts);
> +    qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file,
> &opts); }
> 
>  libqos_init(register_virtio_9p_test);

Ok, I found the problem on the mentioned box that failed to create hard links 
with 9p: it is libvirt auto generating AppArmor policy rules for 9p export 
pathes, which libvirt generates with "rw" AA (AppArmor) permissions. Once I 
manually adjusted the AA rule to "rwl", creating hard links worked again.

I guess I'll send a patch for libvirt to change that. At least IMO creating 
hard links with 9pfs is a very basic operation and should be enabled for the 
respective 9p export path by default.

Actually I think it should also include "k" which means "file locking", as 
file locking is also a fundamental operation with 9pfs, right?

Best regards,
Christian Schoenebeck

On Wed, 21 Oct 2020 14:51:09 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This test case uses a Tlink request to create a hard link to a regular
> file using the 9pfs 'local' fs driver.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/qtest/virtio-9p-test.c | 71 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> index 33cba24b18..460fa49fe3 100644
> --- a/tests/qtest/virtio-9p-test.c
> +++ b/tests/qtest/virtio-9p-test.c
> @@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
>          id == P9_RMKDIR ? "RMKDIR" :
>          id == P9_RLCREATE ? "RLCREATE" :
>          id == P9_RSYMLINK ? "RSYMLINK" :
> +        id == P9_RLINK ? "RLINK" :
>          id == P9_RUNLINKAT ? "RUNLINKAT" :
>          id == P9_RFLUSH ? "RFLUSH" :
>          id == P9_RREADDIR ? "READDIR" :
> @@ -767,6 +768,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
>      v9fs_req_free(req);
>  }
>  
> +/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
> +static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
> +                         const char *name, uint16_t tag)
> +{
> +    P9Req *req;
> +
> +    uint32_t body_size = 4 + 4;
> +    uint16_t string_size = v9fs_string_size(name);
> +
> +    g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
> +    body_size += string_size;
> +
> +    req = v9fs_req_init(v9p, body_size, P9_TLINK, tag);
> +    v9fs_uint32_write(req, dfid);
> +    v9fs_uint32_write(req, fid);
> +    v9fs_string_write(req, name);
> +    v9fs_req_send(req);
> +    return req;
> +}
> +
> +/* size[4] Rlink tag[2] */
> +static void v9fs_rlink(P9Req *req)
> +{
> +    v9fs_req_recv(req, P9_RLINK);
> +    v9fs_req_free(req);
> +}
> +
>  /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
>  static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char *name,
>                               uint32_t flags, uint16_t tag)
> @@ -1142,6 +1170,21 @@ static void do_symlink(QVirtio9P *v9p, const char *path, const char *clink,
>      g_free(name);
>  }
>  
> +/* create a hard link named @a clink in directory @a path pointing to @a to */
> +static void do_hardlink(QVirtio9P *v9p, const char *path, const char *clink,
> +                        const char *to)
> +{
> +    uint32_t dfid, fid;
> +    P9Req *req;
> +
> +    dfid = do_walk(v9p, path);
> +    fid = do_walk(v9p, to);
> +
> +    req = v9fs_tlink(v9p, dfid, fid, clink, 0);
> +    v9fs_req_wait_for_reply(req, NULL);
> +    v9fs_rlink(req);
> +}
> +
>  static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char *rpath,
>                          uint32_t flags)
>  {
> @@ -1321,6 +1364,33 @@ static void fs_unlinkat_symlink(void *obj, void *data,
>      g_free(real_file);
>  }
>  
> +static void fs_hardlink_file(void *obj, void *data, QGuestAllocator *t_alloc)
> +{
> +    QVirtio9P *v9p = obj;
> +    alloc = t_alloc;
> +    struct stat st_real, st_link;
> +    char *real_file = virtio_9p_test_path("07/real_file");
> +    char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
> +
> +    do_attach(v9p);
> +    do_mkdir(v9p, "/", "07");
> +    do_lcreate(v9p, "07", "real_file");
> +    g_assert(stat(real_file, &st_real) == 0);
> +    g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
> +
> +    do_hardlink(v9p, "07", "hardlink_file", "07/real_file");
> +
> +    /* check if link exists now ... */
> +    g_assert(stat(hardlink_file, &st_link) == 0);
> +    /* ... and it's a hard link, right? */
> +    g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
> +    g_assert(st_link.st_dev == st_real.st_dev);
> +    g_assert(st_link.st_ino == st_real.st_ino);
> +
> +    g_free(hardlink_file);
> +    g_free(real_file);
> +}
> +
>  static void *assign_9p_local_driver(GString *cmd_line, void *arg)
>  {
>      virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> @@ -1367,6 +1437,7 @@ static void register_virtio_9p_test(void)
>      qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file, &opts);
>      qos_add_test("local/unlinkat_symlink", "virtio-9p", fs_unlinkat_symlink,
>                   &opts);
> +    qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file, &opts);
>  }
>  
>  libqos_init(register_virtio_9p_test);

On Wed, 21 Oct 2020 20:20:08 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Mittwoch, 21. Oktober 2020 14:51:09 CEST Christian Schoenebeck wrote:
> > This test case uses a Tlink request to create a hard link to a regular
> > file using the 9pfs 'local' fs driver.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >  tests/qtest/virtio-9p-test.c | 71 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> > 
> > diff --git a/tests/qtest/virtio-9p-test.c b/tests/qtest/virtio-9p-test.c
> > index 33cba24b18..460fa49fe3 100644
> > --- a/tests/qtest/virtio-9p-test.c
> > +++ b/tests/qtest/virtio-9p-test.c
> > @@ -260,6 +260,7 @@ static const char *rmessage_name(uint8_t id)
> >          id == P9_RMKDIR ? "RMKDIR" :
> >          id == P9_RLCREATE ? "RLCREATE" :
> >          id == P9_RSYMLINK ? "RSYMLINK" :
> > +        id == P9_RLINK ? "RLINK" :
> >          id == P9_RUNLINKAT ? "RUNLINKAT" :
> >          id == P9_RFLUSH ? "RFLUSH" :
> >          id == P9_RREADDIR ? "READDIR" :
> > @@ -767,6 +768,33 @@ static void v9fs_rsymlink(P9Req *req, v9fs_qid *qid)
> >      v9fs_req_free(req);
> >  }
> > 
> > +/* size[4] Tlink tag[2] dfid[4] fid[4] name[s] */
> > +static P9Req *v9fs_tlink(QVirtio9P *v9p, uint32_t dfid, uint32_t fid,
> > +                         const char *name, uint16_t tag)
> > +{
> > +    P9Req *req;
> > +
> > +    uint32_t body_size = 4 + 4;
> > +    uint16_t string_size = v9fs_string_size(name);
> > +
> > +    g_assert_cmpint(body_size, <=, UINT32_MAX - string_size);
> > +    body_size += string_size;
> > +
> > +    req = v9fs_req_init(v9p, body_size, P9_TLINK, tag);
> > +    v9fs_uint32_write(req, dfid);
> > +    v9fs_uint32_write(req, fid);
> > +    v9fs_string_write(req, name);
> > +    v9fs_req_send(req);
> > +    return req;
> > +}
> > +
> > +/* size[4] Rlink tag[2] */
> > +static void v9fs_rlink(P9Req *req)
> > +{
> > +    v9fs_req_recv(req, P9_RLINK);
> > +    v9fs_req_free(req);
> > +}
> > +
> >  /* size[4] Tunlinkat tag[2] dirfd[4] name[s] flags[4] */
> >  static P9Req *v9fs_tunlinkat(QVirtio9P *v9p, uint32_t dirfd, const char
> > *name, uint32_t flags, uint16_t tag)
> > @@ -1142,6 +1170,21 @@ static void do_symlink(QVirtio9P *v9p, const char
> > *path, const char *clink, g_free(name);
> >  }
> > 
> > +/* create a hard link named @a clink in directory @a path pointing to @a to
> > */ +static void do_hardlink(QVirtio9P *v9p, const char *path, const char
> > *clink, +                        const char *to)
> > +{
> > +    uint32_t dfid, fid;
> > +    P9Req *req;
> > +
> > +    dfid = do_walk(v9p, path);
> > +    fid = do_walk(v9p, to);
> > +
> > +    req = v9fs_tlink(v9p, dfid, fid, clink, 0);
> > +    v9fs_req_wait_for_reply(req, NULL);
> > +    v9fs_rlink(req);
> > +}
> > +
> >  static void do_unlinkat(QVirtio9P *v9p, const char *atpath, const char
> > *rpath, uint32_t flags)
> >  {
> > @@ -1321,6 +1364,33 @@ static void fs_unlinkat_symlink(void *obj, void
> > *data, g_free(real_file);
> >  }
> > 
> > +static void fs_hardlink_file(void *obj, void *data, QGuestAllocator
> > *t_alloc) +{
> > +    QVirtio9P *v9p = obj;
> > +    alloc = t_alloc;
> > +    struct stat st_real, st_link;
> > +    char *real_file = virtio_9p_test_path("07/real_file");
> > +    char *hardlink_file = virtio_9p_test_path("07/hardlink_file");
> > +
> > +    do_attach(v9p);
> > +    do_mkdir(v9p, "/", "07");
> > +    do_lcreate(v9p, "07", "real_file");
> > +    g_assert(stat(real_file, &st_real) == 0);
> > +    g_assert((st_real.st_mode & S_IFMT) == S_IFREG);
> > +
> > +    do_hardlink(v9p, "07", "hardlink_file", "07/real_file");
> > +
> > +    /* check if link exists now ... */
> > +    g_assert(stat(hardlink_file, &st_link) == 0);
> > +    /* ... and it's a hard link, right? */
> > +    g_assert((st_link.st_mode & S_IFMT) == S_IFREG);
> > +    g_assert(st_link.st_dev == st_real.st_dev);
> > +    g_assert(st_link.st_ino == st_real.st_ino);
> > +
> > +    g_free(hardlink_file);
> > +    g_free(real_file);
> > +}
> > +
> >  static void *assign_9p_local_driver(GString *cmd_line, void *arg)
> >  {
> >      virtio_9p_assign_local_driver(cmd_line, "security_model=mapped-xattr");
> > @@ -1367,6 +1437,7 @@ static void register_virtio_9p_test(void)
> >      qos_add_test("local/symlink_file", "virtio-9p", fs_symlink_file,
> > &opts); qos_add_test("local/unlinkat_symlink", "virtio-9p",
> > fs_unlinkat_symlink, &opts);
> > +    qos_add_test("local/hardlink_file", "virtio-9p", fs_hardlink_file,
> > &opts); }
> > 
> >  libqos_init(register_virtio_9p_test);
> 
> Ok, I found the problem on the mentioned box that failed to create hard links 
> with 9p: it is libvirt auto generating AppArmor policy rules for 9p export 
> pathes, which libvirt generates with "rw" AA (AppArmor) permissions. Once I 
> manually adjusted the AA rule to "rwl", creating hard links worked again.
> 
> I guess I'll send a patch for libvirt to change that. At least IMO creating 
> hard links with 9pfs is a very basic operation and should be enabled for the 
> respective 9p export path by default.
> 
> Actually I think it should also include "k" which means "file locking", as 
> file locking is also a fundamental operation with 9pfs, right?
> 

Well, I don't know exactly why libvirt is generating a strict AppArmor policy
but I'm not that surprised. If the user can _easily_ change the policy to
fit its needs, it's fine to have a "better safe than sorry" default.

> Best regards,
> Christian Schoenebeck
> 
>

On Donnerstag, 22. Oktober 2020 11:07:49 CEST Greg Kurz wrote:
> > Ok, I found the problem on the mentioned box that failed to create hard
> > links with 9p: it is libvirt auto generating AppArmor policy rules for 9p
> > export pathes, which libvirt generates with "rw" AA (AppArmor)
> > permissions. Once I manually adjusted the AA rule to "rwl", creating hard
> > links worked again.
> > 
> > I guess I'll send a patch for libvirt to change that. At least IMO
> > creating
> > hard links with 9pfs is a very basic operation and should be enabled for
> > the respective 9p export path by default.
> > 
> > Actually I think it should also include "k" which means "file locking", as
> > file locking is also a fundamental operation with 9pfs, right?
> 
> Well, I don't know exactly why libvirt is generating a strict AppArmor
> policy but I'm not that surprised. If the user can _easily_ change the
> policy to fit its needs, it's fine to have a "better safe than sorry"
> default.

Alone identifying the root cause of this is not that easy I would say. And if
it takes me quite some time to find it, then I imagine that other people would
struggle with this even more.

A large portion of software, even scripts, rely on being able to create hard
links and locking files. Right now they typically error out on guest with no
helpful error message.

So you start looking into the logs, don't find something obvious, then strace
on guest side to find the most relevant failing syscall on guest side and see
it was probably link(). Then you have to check several security layers: file
permissions on guest, file permissions on host, effective UID of qemu process.
You try creating hard links directly on host with that UID, works. Next you
check is it qemu's seccomp? Is it host's SELinux? Is it AppArmor?

Even for an experienced sysadmin I doubt it'll be a matter of minutes to
resolve this issue. Now imagine a regular user who just wants to sandbox
something on a workstation.

Looking at libvirt git log, it seems this security policy exists more or less
since day one (SHA-1 29ea8a9b64aac60251d283f74d57690e4edb5a6b, Mar 9 2014).
And I don't see an explanation that would suggest a specific reason for
exactly "rw".

I think something has to be improved here, so I'll challenge by sending a
simple libvirt patch, CCing involved authors, and seeing the feedback:

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 12429278fb..ce243e304b 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1142,7 +1142,7 @@ get_files(vahControl * ctl)
             /* We don't need to add deny rw rules for readonly mounts,
              * this can only lead to troubles when mounting / readonly.
              */
-            if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rw", true) != 0)
+            if (vah_add_path(&buf, fs->src->path, fs->readonly ? "R" : "rwlk", true) != 0)
                 goto cleanup;
         }
     }

Even after this change, this is not a global policy. Allowing hard links and
file locks would only be lifted for the 9p export path.

There would be other options as well of course: e.g. detecting on 9pfs side
whether AppArmor and co are enabled, and log a warning to the user a syscall
failed for that reason. But that would be much more complicated and I wonder
whether it would be worth it.

Best regards,
Christian Schoenebeck