[PATCH 0/3] 9pfs: fix invalid union V9fsFidOpenState access

[PATCH 0/3] 9pfs: fix invalid union V9fsFidOpenState access

[Christian Schoenebeck]

The individual FID types (P9_FID_NONE, P9_FID_FILE, P9_FID_DIR, P9_FID_XATTR)
share union V9fsFidOpenState with FID-type specific fields. Accessing any of
the union fields must comply with the FID-type to avoid undefined behaviour
or information disclosure.

This series fixes invalid access of this union type at several locations.

* Patch 1 and Patch 2 are the core fixes checking the FID type on protocol
  level (9p.c) before allowing access to a FID-type specific union field.

* Patch 3 adds another safety layer by returning -1 from local_fid_fd() if
  the FID type would not have a valid file descriptor.

Christian Schoenebeck (3):
  hw/9pfs: fix invalid union access by v9fs_co_fsync()
  hw/9pfs: fix invalid union access by v9fs_co_fstat()
  hw/9pfs/local: harden local_fid_fd() on FID types

 hw/9pfs/9p-local.c |  5 ++++-
 hw/9pfs/9p.c       | 17 +++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

-- 
2.47.3

[PATCH 3/3] hw/9pfs/local: harden local_fid_fd() on FID types

[Christian Schoenebeck]

local_fid_fd() returns fs->fd for any FID type that is not P9_FID_DIR.

Since P9_FID_XATTR and P9_FID_NONE share union V9fsFidOpenState, calling
local_fid_fd() on these types misinterprets xattr state as a file
descriptor, potentially leading to undefined behaviour or information
disclosure.

Even though we are catching these FID type mismatches on protocol level
in 9p.c already, previous patches proofed this to be error prone.

So let's add another safety layer in local_fid_fd() that would return -1
if the FID type would not possess a valid file descriptor, to prevent
wrong file descriptors from reaching fs backend calls.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p-local.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index aa48306b0e..724f57dc3d 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -775,8 +775,11 @@ static int local_fid_fd(int fid_type, V9fsFidOpenState *fs)
 {
     if (fid_type == P9_FID_DIR) {
         return dirfd(fs->dir.stream);
-    } else {
+    } else if (fid_type == P9_FID_FILE) {
         return fs->fd;
+    } else {
+        errno = EBADF;
+        return -1;
     }
 }
 
-- 
2.47.3

[PATCH 2/3] hw/9pfs: fix invalid union access by v9fs_co_fstat()

[Christian Schoenebeck]

The individual FID types (P9_FID_NONE, P9_FID_FILE, P9_FID_DIR, P9_FID_XATTR)
share union V9fsFidOpenState with FID-type specific fields. Accessing any of
the union fields must comply with the FID-type to avoid undefined behaviour
or information disclosure.

Fix this in v9fs_lock() and v9fs_getlock() by checking if FID has a valid
file descriptor before calling v9fs_co_fstat().

Fixes: 10b468bdc533 ("virtio-9p: Implement TXATTRCREATE")
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 6dd6a57e7a..c83df57698 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3852,6 +3852,10 @@ static void coroutine_fn v9fs_lock(void *opaque)
         err = -ENOENT;
         goto out_nofid;
     }
+    if (!fid_has_valid_file_handle(pdu->s, fidp)) {
+        err = -EBADF;
+        goto out;
+    }
     err = v9fs_co_fstat(pdu, fidp, &stbuf);
     if (err < 0) {
         goto out;
@@ -3897,6 +3901,10 @@ static void coroutine_fn v9fs_getlock(void *opaque)
         err = -ENOENT;
         goto out_nofid;
     }
+    if (!fid_has_valid_file_handle(pdu->s, fidp)) {
+        err = -EBADF;
+        goto out;
+    }
     err = v9fs_co_fstat(pdu, fidp, &stbuf);
     if (err < 0) {
         goto out;
-- 
2.47.3

[PATCH 1/3] hw/9pfs: fix invalid union access by v9fs_co_fsync()

[Christian Schoenebeck]

The individual FID types (P9_FID_NONE, P9_FID_FILE, P9_FID_DIR, P9_FID_XATTR)
share union V9fsFidOpenState with FID-type specific fields. Accessing any of
the union fields must comply with the FID-type to avoid undefined behaviour
or information disclosure.

Fix this in v9fs_fsync() and v9fs_wstat() by checking if FID has a valid file
descriptor before calling v9fs_co_fsync().

Fixes: 10b468bdc533 ("virtio-9p: Implement TXATTRCREATE")
Reported-by: Feifan Qian <bea1e@proton.me>
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index b4314d2549..6dd6a57e7a 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2247,10 +2247,15 @@ static void coroutine_fn v9fs_fsync(void *opaque)
         err = -ENOENT;
         goto out_nofid;
     }
+    if (!fid_has_valid_file_handle(pdu->s, fidp)) {
+        err = -EBADF;
+        goto out;
+    }
     err = v9fs_co_fsync(pdu, fidp, datasync);
     if (!err) {
         err = offset;
     }
+out:
     put_fid(pdu, fidp);
 out_nofid:
     pdu_complete(pdu, err);
@@ -3584,6 +3589,10 @@ static void coroutine_fn v9fs_wstat(void *opaque)
     }
     /* do we need to sync the file? */
     if (donttouch_stat(&v9stat)) {
+        if (!fid_has_valid_file_handle(s, fidp)) {
+            err = -EBADF;
+            goto out;
+        }
         err = v9fs_co_fsync(pdu, fidp, 0);
         goto out;
     }
-- 
2.47.3