[Qemu-devel] [PATCH] vhost: Fix use-after-free in vhost_log_put()

[Qemu-devel] [PATCH] vhost: Fix use-after-free in vhost_log_put()

[Jia-Shiun Li]

In commit 9e0bc24f dev->log_size was reset to zero too early before
syncing vhost log. It causes syncing to be skipped.

Move it to clear dev->log* after use.

Signed-off-by: Jia-Shiun Li <jsli@synology.com>
---
 hw/virtio/vhost.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6eddb09..c9ddf11 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -375,8 +375,6 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
     if (!log) {
         return;
     }
-    dev->log = NULL;
-    dev->log_size = 0;
 
     --log->refcnt;
     if (log->refcnt == 0) {
@@ -396,6 +394,8 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
 
         g_free(log);
     }
+    dev->log = NULL;
+    dev->log_size = 0;
 }
 
 static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
-- 
2.7.4

Re: [Qemu-devel] [PATCH] vhost: Fix use-after-free in vhost_log_put()

[Marc-André Lureau]

Hi

On Fri, Jun 23, 2017 at 6:28 AM Jia-Shiun Li <jsli@synology.com> wrote:

> In commit 9e0bc24f dev->log_size was reset to zero too early before
> syncing vhost log. It causes syncing to be skipped.
>
>
ooch, I guess I didn't realize it was also accessing dev->log_size when
taking dev->log in local variable.

I wonder why the code is written this way, it looks like the function may
be reentered. For consistency, and perhaps for the reentering case, I would
use a local log_size variable too.

Btw, how did you find this regression?


> Move it to clear dev->log* after use.
>
> Signed-off-by: Jia-Shiun Li <jsli@synology.com>
>
---
>  hw/virtio/vhost.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 6eddb09..c9ddf11 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -375,8 +375,6 @@ static void vhost_log_put(struct vhost_dev *dev, bool
> sync)
>      if (!log) {
>          return;
>      }
> -    dev->log = NULL;
> -    dev->log_size = 0;
>
>      --log->refcnt;
>      if (log->refcnt == 0) {
> @@ -396,6 +394,8 @@ static void vhost_log_put(struct vhost_dev *dev, bool
> sync)
>
>          g_free(log);
>      }
> +    dev->log = NULL;
> +    dev->log_size = 0;
>  }
>
>  static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
> --
> 2.7.4
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH] vhost: Fix use-after-free in vhost_log_put()

[jsli]

On 2017-06-29 05:12, Marc-André Lureau<marcandre.lureau@gmail.com>wrote:
> Hi
>   
> On Fri, Jun 23, 2017 at 6:28 AM Jia-Shiun Li<jsli@synology.com(mailto:jsli@synology.com)>wrote:
> > In commit 9e0bc24f dev->log_size was reset to zero too early before
> > syncing vhost log. It causes syncing to be skipped.
>   
> ooch, I guess I didn't realize it was also accessing dev->log_size when taking dev->log in local variable.
> I wonder why the code is written this way, it looks like the function may be reentered. For consistency, and perhaps for the reentering case, I would use a local log_size variable too.
> Btw, how did you find this regression?
>   
>   
>   

Ok, it makes sense to prevent reentering. Willregenerate patch.
We are trying to do vhost-scsi migration, and found it to cause datainconsistency migrating an i/o stressed guest.

-Jia-Shiun