[Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Sassan Panahinejad]

v9fs_fsync and possibly others break when asked to operate on a directory.
It does not check fid_type to see if it is operating on a directory and therefore accesses the wrong element of the fs union.
This error can result in guest applications failing (in my case it was dpkg).
This patch fixes the issue, although there may be other, similar bugs in virtio-9p.
---
 hw/virtio-9p.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7e29535..09fb5da 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1875,7 +1875,10 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
         v9fs_post_do_fsync(s, pdu, err);
         return;
     }
-    err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
+    if (fidp->fid_type == P9_FID_DIR)
+        err = v9fs_do_fsync(s, dirfd(fidp->fs.dir), datasync);
+    else
+        err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
     v9fs_post_do_fsync(s, pdu, err);
 }
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Stefan Hajnoczi]

On Mon, Apr 25, 2011 at 6:54 PM, Sassan Panahinejad <sassan@sassan.me.uk> wrote:

Thanks for finding and fixing this.  Please see this wiki page on
contributing patches to QEMU:
http://wiki.qemu.org/Contribute/SubmitAPatch

> v9fs_fsync and possibly others break when asked to operate on a directory.
> It does not check fid_type to see if it is operating on a directory and therefore accesses the wrong element of the fs union.
> This error can result in guest applications failing (in my case it was dpkg).
> This patch fixes the issue, although there may be other, similar bugs in virtio-9p.
> ---
>  hw/virtio-9p.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)

Missing Signed-off-by:.

> diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
> index 7e29535..09fb5da 100644
> --- a/hw/virtio-9p.c
> +++ b/hw/virtio-9p.c
> @@ -1875,7 +1875,10 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
>         v9fs_post_do_fsync(s, pdu, err);
>         return;
>     }
> -    err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
> +    if (fidp->fid_type == P9_FID_DIR)
> +        err = v9fs_do_fsync(s, dirfd(fidp->fs.dir), datasync);
> +    else
> +        err = v9fs_do_fsync(s, fidp->fs.fd, datasync);

Please follow QEMU coding style and always use {} with if ... else.

Stefan

[Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Sassan Panahinejad]

v9fs_fsync and possibly others break when asked to operate on a directory.
It does not check fid_type to see if it is operating on a directory and therefore accesses the wrong element of the fs union.
This error can result in guest applications failing (in my case it was dpkg).
This patch fixes the issue, although there may be other, similar bugs in virtio-9p.

Signed-off-by: Sassan Panahinejad <sassan@sassan.me.uk>
---
 hw/virtio-9p.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7e29535..cc4fdc8 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1875,7 +1875,11 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
         v9fs_post_do_fsync(s, pdu, err);
         return;
     }
-    err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
+    if (fidp->fid_type == P9_FID_DIR) {
+        err = v9fs_do_fsync(s, dirfd(fidp->fs.dir), datasync);
+    } else {
+        err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
+    }
     v9fs_post_do_fsync(s, pdu, err);
 }
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Stefan Hajnoczi]

On Tue, Apr 26, 2011 at 1:14 PM, Sassan Panahinejad <sassan@sassan.me.uk> wrote:
> v9fs_fsync and possibly others break when asked to operate on a directory.
> It does not check fid_type to see if it is operating on a directory and therefore accesses the wrong element of the fs union.
> This error can result in guest applications failing (in my case it was dpkg).
> This patch fixes the issue, although there may be other, similar bugs in virtio-9p.
>
> Signed-off-by: Sassan Panahinejad <sassan@sassan.me.uk>
> ---
>  hw/virtio-9p.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
> index 7e29535..cc4fdc8 100644
> --- a/hw/virtio-9p.c
> +++ b/hw/virtio-9p.c
> @@ -1875,7 +1875,11 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
>         v9fs_post_do_fsync(s, pdu, err);
>         return;
>     }
> -    err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
> +    if (fidp->fid_type == P9_FID_DIR) {
> +        err = v9fs_do_fsync(s, dirfd(fidp->fs.dir), datasync);
> +    } else {
> +        err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
> +    }

What about P9_FID_XATTR, seems like we have the same issue there too?

wstat, lock, and getlock need closer auditing and perhaps fixing.

Stefan

Re: [Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Sassan Panahinejad]

[-- Attachment #1: Type: text/plain, Size: 695 bytes --]

On 26 April 2011 13:58, Stefan Hajnoczi <stefanha@gmail.com> wrote:

> What about P9_FID_XATTR, seems like we have the same issue there too?
>
> wstat, lock, and getlock need closer auditing and perhaps fixing.
>
> Stefan
>

Sorry, forgot to hit reply-to-all.

Yes, it is probable that those functions will suffer from the same bug.
I will have to study XATTR and see how that will be affected. I don't know
whether it is possible for these functions to be called for XATTR, and if it
is then I do not know the proper way to handle it.
Perhaps we should have some function or macro to obtain the correct FD from
an fidp structure, which could be used for fsync, wstat, lock and getlock?

Sassan

[-- Attachment #2: Type: text/html, Size: 1076 bytes --]

Re: [Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Venkateswararao Jujjuri]

On 04/26/2011 06:29 AM, Sassan Panahinejad wrote:
> I will have to study XATTR and see how that will be affected. I don't 
> know whether it is possible for these functions to be called for 
> XATTR, and if it is then I do not know the proper way to handle it.
> Perhaps we should have some function or macro to obtain the correct FD 
> from an fidp structure, which could be used for fsync, wstat, lock and 
> getlock?
I agree; we need some level of macro for this. How about doing that as 
part of this patch itself?

Thanks,
JV

[Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Sassan Panahinejad]

v9fs_fsync and possibly others break when asked to operate on a directory.
It does not check fid_type to see if it is operating on a directory and therefore accesses the wrong element of the fs union.
This error can result in guest applications failing (in my case it was dpkg).
This patch fixes the issue, although there may be other, similar bugs in virtio-9p.

Signed-off-by: Sassan Panahinejad <sassan@sassan.me.uk>
---

Here I've implemented it as a function. If you'd prefer a macro or inline function then let me know.

Thanks
Sassan

 hw/virtio-9p.c |   43 +++++++++++++++++++++++++++++++++++++------
 1 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7e29535..26be0fc 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1860,6 +1860,18 @@ static void v9fs_post_do_fsync(V9fsState *s, V9fsPDU *pdu, int err)
     complete_pdu(s, pdu, err);
 }
 
+static int v9fs_get_fd(V9fsFidState *fidp) 
+{
+    switch (fidp->fid_type) {
+        case P9_FID_FILE: 
+            return fidp->fs.fd;
+        case P9_FID_DIR: 
+            return dirfd(fidp->fs.dir);
+        default:
+            return -1;
+    }
+}
+
 static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
 {
     int32_t fid;
@@ -1867,6 +1879,7 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
     V9fsFidState *fidp;
     int datasync;
     int err;
+    int fd;
 
     pdu_unmarshal(pdu, offset, "dd", &fid, &datasync);
     fidp = lookup_fid(s, fid);
@@ -1875,7 +1888,11 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
         v9fs_post_do_fsync(s, pdu, err);
         return;
     }
-    err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
+    if ((fd = v9fs_get_fd(fidp)) >= 0) {
+        err = v9fs_do_fsync(s, fd, datasync);
+    } else {
+	err = -EINVAL;
+    }
     v9fs_post_do_fsync(s, pdu, err);
 }
 
@@ -2984,6 +3001,7 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
     int32_t fid;
     V9fsWstatState *vs;
     int err = 0;
+    int fd;
 
     vs = qemu_malloc(sizeof(*vs));
     vs->pdu = pdu;
@@ -2999,7 +3017,10 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
 
     /* do we need to sync the file? */
     if (donttouch_stat(&vs->v9stat)) {
-        err = v9fs_do_fsync(s, vs->fidp->fs.fd, 0);
+        if ((fd = v9fs_get_fd(vs->fidp)) >= 0) {
+            err = v9fs_do_fsync(s, fd, 0);
+        } else
+            err = -EINVAL;
         v9fs_wstat_post_fsync(s, vs, err);
         return;
     }
@@ -3176,6 +3197,7 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu)
 {
     int32_t fid, err = 0;
     V9fsLockState *vs;
+    int fd;
 
     vs = qemu_mallocz(sizeof(*vs));
     vs->pdu = pdu;
@@ -3198,8 +3220,12 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu)
         err = -ENOENT;
         goto out;
     }
-
-    err = v9fs_do_fstat(s, vs->fidp->fs.fd, &vs->stbuf);
+    if ((fd = v9fs_get_fd(vs->fidp)) >= 0) {
+        err = v9fs_do_fstat(s, fd, &vs->stbuf);
+    } else {
+        err = -EINVAL;
+        goto out;
+    }
     if (err < 0) {
         err = -errno;
         goto out;
@@ -3221,6 +3247,7 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu)
 {
     int32_t fid, err = 0;
     V9fsGetlockState *vs;
+    int fd;
 
     vs = qemu_mallocz(sizeof(*vs));
     vs->pdu = pdu;
@@ -3236,8 +3263,12 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu)
         err = -ENOENT;
         goto out;
     }
-
-    err = v9fs_do_fstat(s, vs->fidp->fs.fd, &vs->stbuf);
+    if ((fd = v9fs_get_fd(vs->fidp)) >= 0) {
+        err = v9fs_do_fstat(s, fd, &vs->stbuf);
+    } else {
+        err = -EINVAL;
+        goto out;
+    }
     if (err < 0) {
         err = -errno;
         goto out;
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Venkateswararao Jujjuri]

[-- Attachment #1: Type: text/plain, Size: 4554 bytes --]

On 04/26/2011 09:51 AM, Sassan Panahinejad wrote:
> v9fs_fsync and possibly others break when asked to operate on a directory.
> It does not check fid_type to see if it is operating on a directory and therefore accesses the wrong element of the fs union.
> This error can result in guest applications failing (in my case it was dpkg).
> This patch fixes the issue, although there may be other, similar bugs in virtio-9p.
>
> Signed-off-by: Sassan Panahinejad<sassan@sassan.me.uk>
> ---
>
> Here I've implemented it as a function. If you'd prefer a macro or inline function then let me know.
>
> Thanks
> Sassan

Please put your commentary on the top. After "---" you should only 
expect code diff.
>   hw/virtio-9p.c |   43 +++++++++++++++++++++++++++++++++++++------
>   1 files changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
> index 7e29535..26be0fc 100644
> --- a/hw/virtio-9p.c
> +++ b/hw/virtio-9p.c
> @@ -1860,6 +1860,18 @@ static void v9fs_post_do_fsync(V9fsState *s, V9fsPDU *pdu, int err)
>       complete_pdu(s, pdu, err);
>   }
>
> +static int v9fs_get_fd(V9fsFidState *fidp)
> +{
> +    switch (fidp->fid_type) {
> +        case P9_FID_FILE:
> +            return fidp->fs.fd;
> +        case P9_FID_DIR:
> +            return dirfd(fidp->fs.dir);
> +        default:
> +            return -1;
> +    }
> +}
> +
>   static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
>   {
>       int32_t fid;
> @@ -1867,6 +1879,7 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
>       V9fsFidState *fidp;
>       int datasync;
>       int err;
> +    int fd;
>
>       pdu_unmarshal(pdu, offset, "dd",&fid,&datasync);
>       fidp = lookup_fid(s, fid);
> @@ -1875,7 +1888,11 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
>           v9fs_post_do_fsync(s, pdu, err);
>           return;
>       }
> -    err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
> +    if ((fd = v9fs_get_fd(fidp))>= 0) {
> +        err = v9fs_do_fsync(s, fd, datasync);
> +    } else {
> +	err = -EINVAL;
> +    }
>       v9fs_post_do_fsync(s, pdu, err);
>   }
>
> @@ -2984,6 +3001,7 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
>       int32_t fid;
>       V9fsWstatState *vs;
>       int err = 0;
> +    int fd;
>
>       vs = qemu_malloc(sizeof(*vs));
>       vs->pdu = pdu;
> @@ -2999,7 +3017,10 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
>
>       /* do we need to sync the file? */
>       if (donttouch_stat(&vs->v9stat)) {
> -        err = v9fs_do_fsync(s, vs->fidp->fs.fd, 0);
> +        if ((fd = v9fs_get_fd(vs->fidp))>= 0) {
> +            err = v9fs_do_fsync(s, fd, 0);
> +        } else
> +            err = -EINVAL;
>           v9fs_wstat_post_fsync(s, vs, err);
>           return;
>       }
> @@ -3176,6 +3197,7 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu)
>   {
>       int32_t fid, err = 0;
>       V9fsLockState *vs;
> +    int fd;
>
>       vs = qemu_mallocz(sizeof(*vs));
>       vs->pdu = pdu;
> @@ -3198,8 +3220,12 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu)
>           err = -ENOENT;
>           goto out;
>       }
> -
> -    err = v9fs_do_fstat(s, vs->fidp->fs.fd,&vs->stbuf);
> +    if ((fd = v9fs_get_fd(vs->fidp))>= 0) {
> +        err = v9fs_do_fstat(s, fd,&vs->stbuf);
> +    } else {
> +        err = -EINVAL;
> +        goto out;
> +    }

I think we need a different handle for lock and getlock.
If the operation is on a dir we need to return

EISDIR

    The /cmd/ parameter is F_GETLK, F_GETLK64, F_SETLK, F_SETLK64,
    F_SETLKW, or F_SETLKW64, and /fildes/ refers to a directory.

Since the handling is different, I am not sure if the separate function 
makes any sense now.
    May be go back to your initial check for sync and for locks, if the
    fid is a dir type then EISDIR
otherwise EINVAL.

Thanks,
JV



>       if (err<  0) {
>           err = -errno;
>           goto out;
> @@ -3221,6 +3247,7 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu)
>   {
>       int32_t fid, err = 0;
>       V9fsGetlockState *vs;
> +    int fd;
>
>       vs = qemu_mallocz(sizeof(*vs));
>       vs->pdu = pdu;
> @@ -3236,8 +3263,12 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu)
>           err = -ENOENT;
>           goto out;
>       }
> -
> -    err = v9fs_do_fstat(s, vs->fidp->fs.fd,&vs->stbuf);
> +    if ((fd = v9fs_get_fd(vs->fidp))>= 0) {
> +        err = v9fs_do_fstat(s, fd,&vs->stbuf);
> +    } else {
> +        err = -EINVAL;
> +        goto out;
> +    }
>       if (err<  0) {
>           err = -errno;
>           goto out;


[-- Attachment #2: Type: text/html, Size: 5588 bytes --]

[Qemu-devel] [PATCH] Fix bug with virtio-9p fsync

[Sassan Panahinejad]

JV: I hope I have correctly understood your description of how lock/getlock should behave.

v9fs_fsync and possibly others break when asked to operate on a directory.
It does not check fid_type to see if it is operating on a directory and therefore accesses the wrong element of the fs union.
This error can result in guest applications failing (in my case it was dpkg).
This patch fixes the issue and implements correct behaviour for fsync, wstat, lock and getlock.

Signed-off-by: Sassan Panahinejad <sassan@sassan.me.uk>
---
 hw/virtio-9p.c |   36 ++++++++++++++++++++++++++++++++----
 1 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7e29535..2530f6d 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -1875,7 +1875,13 @@ static void v9fs_fsync(V9fsState *s, V9fsPDU *pdu)
         v9fs_post_do_fsync(s, pdu, err);
         return;
     }
-    err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
+    if (fidp->fid_type == P9_FID_FILE) {
+        err = v9fs_do_fsync(s, fidp->fs.fd, datasync);
+    } else if (fidp->fid_type == P9_FID_DIR) {
+        err = v9fs_do_fsync(s, dirfd(fidp->fs.dir), datasync);
+    } else {
+        err = -EINVAL;
+    }
     v9fs_post_do_fsync(s, pdu, err);
 }
 
@@ -2999,7 +3005,13 @@ static void v9fs_wstat(V9fsState *s, V9fsPDU *pdu)
 
     /* do we need to sync the file? */
     if (donttouch_stat(&vs->v9stat)) {
-        err = v9fs_do_fsync(s, vs->fidp->fs.fd, 0);
+        if (vs->fidp->fid_type == P9_FID_FILE) {
+            err = v9fs_do_fsync(s, vs->fidp->fs.fd, 0);
+        } else if (vs->fidp->fid_type == P9_FID_DIR) {
+            err = v9fs_do_fsync(s, dirfd(vs->fidp->fs.dir), 0);
+        } else {
+            err = -EINVAL;
+        }
         v9fs_wstat_post_fsync(s, vs, err);
         return;
     }
@@ -3199,7 +3211,15 @@ static void v9fs_lock(V9fsState *s, V9fsPDU *pdu)
         goto out;
     }
 
-    err = v9fs_do_fstat(s, vs->fidp->fs.fd, &vs->stbuf);
+    if (vs->fidp->fid_type == P9_FID_FILE) {
+        err = v9fs_do_fstat(s, vs->fidp->fs.fd, &vs->stbuf);
+    } else if (vs->fidp->fid_type == P9_FID_DIR) {
+        err = -EISDIR;
+        goto out;
+    } else {
+        err = -EINVAL;
+        goto out;
+    }
     if (err < 0) {
         err = -errno;
         goto out;
@@ -3237,7 +3257,15 @@ static void v9fs_getlock(V9fsState *s, V9fsPDU *pdu)
         goto out;
     }
 
-    err = v9fs_do_fstat(s, vs->fidp->fs.fd, &vs->stbuf);
+    if (vs->fidp->fid_type == P9_FID_FILE) {
+        err = v9fs_do_fstat(s, vs->fidp->fs.fd, &vs->stbuf);
+    } else if (vs->fidp->fid_type == P9_FID_DIR) {
+        err = -EISDIR;
+        goto out;
+    } else {
+        err = -EINVAL;
+        goto out;
+    }
     if (err < 0) {
         err = -errno;
         goto out;
-- 
1.7.0.4