[Qemu-devel] [PATCH] block/pflash_cfi02: Fix memory leak and potential use-after-free

[Qemu-devel] [PATCH] block/pflash_cfi02: Fix memory leak and potential use-after-free

[Stephen Checkoway]

Don't dynamically allocate the pflash's timer. But do use timer_del in
an unrealize function to make sure that the timer can't fire after the
pflash_t has been freed.

Signed-off-by: Stephen Checkoway <stephen.checkoway@oberlin.edu>
---
 hw/block/pflash_cfi02.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 0f8b7b8c7b..1588aeff5a 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -84,7 +84,7 @@ struct pflash_t {
     uint16_t unlock_addr0;
     uint16_t unlock_addr1;
     uint8_t cfi_table[0x52];
-    QEMUTimer *timer;
+    QEMUTimer timer;
     /* The device replicates the flash memory across its memory space.  Emulate
      * that by having a container (.mem) filled with an array of aliases
      * (.mem_mappings) pointing to the flash memory (.orig_mem).
@@ -429,7 +429,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
             }
             pfl->status = 0x00;
             /* Let's wait 5 seconds before chip erase is done */
-            timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+            timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
                       (NANOSECONDS_PER_SECOND * 5));
             break;
         case 0x30:
@@ -444,7 +444,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
             }
             pfl->status = 0x00;
             /* Let's wait 1/2 second before sector erase is done */
-            timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+            timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
                       (NANOSECONDS_PER_SECOND / 2));
             break;
         default:
@@ -596,7 +596,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     pfl->rom_mode = 1;
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
 
-    pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
+    timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
     pfl->wcycle = 0;
     pfl->cmd = 0;
     pfl->status = 0;
@@ -695,11 +695,18 @@ static Property pflash_cfi02_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void pflash_cfi02_unrealize(DeviceState *dev, Error **errp)
+{
+    pflash_t *pfl = CFI_PFLASH02(dev);
+    timer_del(&pfl->timer);
+}
+
 static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = pflash_cfi02_realize;
+    dc->unrealize = pflash_cfi02_unrealize;
     dc->props = pflash_cfi02_properties;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
-- 
2.17.2 (Apple Git-113)

Re: [Qemu-devel] [PATCH] block/pflash_cfi02: Fix memory leak and potential use-after-free

[Philippe Mathieu-Daudé]

On 2/19/19 4:37 PM, Stephen Checkoway wrote:
> Don't dynamically allocate the pflash's timer. But do use timer_del in
> an unrealize function to make sure that the timer can't fire after the
> pflash_t has been freed.
> 
> Signed-off-by: Stephen Checkoway <stephen.checkoway@oberlin.edu>
> ---
>  hw/block/pflash_cfi02.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 0f8b7b8c7b..1588aeff5a 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -84,7 +84,7 @@ struct pflash_t {
>      uint16_t unlock_addr0;
>      uint16_t unlock_addr1;
>      uint8_t cfi_table[0x52];
> -    QEMUTimer *timer;
> +    QEMUTimer timer;
>      /* The device replicates the flash memory across its memory space.  Emulate
>       * that by having a container (.mem) filled with an array of aliases
>       * (.mem_mappings) pointing to the flash memory (.orig_mem).
> @@ -429,7 +429,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>              }
>              pfl->status = 0x00;
>              /* Let's wait 5 seconds before chip erase is done */
> -            timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +            timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                        (NANOSECONDS_PER_SECOND * 5));
>              break;
>          case 0x30:
> @@ -444,7 +444,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>              }
>              pfl->status = 0x00;
>              /* Let's wait 1/2 second before sector erase is done */
> -            timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +            timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                        (NANOSECONDS_PER_SECOND / 2));
>              break;
>          default:
> @@ -596,7 +596,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      pfl->rom_mode = 1;
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
>  
> -    pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
> +    timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>      pfl->wcycle = 0;
>      pfl->cmd = 0;
>      pfl->status = 0;
> @@ -695,11 +695,18 @@ static Property pflash_cfi02_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static void pflash_cfi02_unrealize(DeviceState *dev, Error **errp)
> +{
> +    pflash_t *pfl = CFI_PFLASH02(dev);
> +    timer_del(&pfl->timer);
> +}
> +
>  static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = pflash_cfi02_realize;
> +    dc->unrealize = pflash_cfi02_unrealize;
>      dc->props = pflash_cfi02_properties;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  }
> 

Re: [Qemu-devel] [PATCH] block/pflash_cfi02: Fix memory leak and potential use-after-free

[Wei Yang]

On Tue, Feb 19, 2019 at 10:37:27AM -0500, Stephen Checkoway wrote:
>Don't dynamically allocate the pflash's timer. But do use timer_del in
>an unrealize function to make sure that the timer can't fire after the
>pflash_t has been freed.
>
>Signed-off-by: Stephen Checkoway <stephen.checkoway@oberlin.edu>

Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>

>---
> hw/block/pflash_cfi02.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
>diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>index 0f8b7b8c7b..1588aeff5a 100644
>--- a/hw/block/pflash_cfi02.c
>+++ b/hw/block/pflash_cfi02.c
>@@ -84,7 +84,7 @@ struct pflash_t {
>     uint16_t unlock_addr0;
>     uint16_t unlock_addr1;
>     uint8_t cfi_table[0x52];
>-    QEMUTimer *timer;
>+    QEMUTimer timer;
>     /* The device replicates the flash memory across its memory space.  Emulate
>      * that by having a container (.mem) filled with an array of aliases
>      * (.mem_mappings) pointing to the flash memory (.orig_mem).
>@@ -429,7 +429,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>             }
>             pfl->status = 0x00;
>             /* Let's wait 5 seconds before chip erase is done */
>-            timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>+            timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                       (NANOSECONDS_PER_SECOND * 5));
>             break;
>         case 0x30:
>@@ -444,7 +444,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>             }
>             pfl->status = 0x00;
>             /* Let's wait 1/2 second before sector erase is done */
>-            timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>+            timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                       (NANOSECONDS_PER_SECOND / 2));
>             break;
>         default:
>@@ -596,7 +596,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>     pfl->rom_mode = 1;
>     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
> 
>-    pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
>+    timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
>     pfl->wcycle = 0;
>     pfl->cmd = 0;
>     pfl->status = 0;
>@@ -695,11 +695,18 @@ static Property pflash_cfi02_properties[] = {
>     DEFINE_PROP_END_OF_LIST(),
> };
> 
>+static void pflash_cfi02_unrealize(DeviceState *dev, Error **errp)
>+{
>+    pflash_t *pfl = CFI_PFLASH02(dev);
>+    timer_del(&pfl->timer);
>+}
>+
> static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> 
>     dc->realize = pflash_cfi02_realize;
>+    dc->unrealize = pflash_cfi02_unrealize;
>     dc->props = pflash_cfi02_properties;
>     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> }
>-- 
>2.17.2 (Apple Git-113)
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [Qemu-trivial] [PATCH] block/pflash_cfi02: Fix memory leak and potential use-after-free

[Laurent Vivier]

On 19/02/2019 16:37, Stephen Checkoway wrote:
> Don't dynamically allocate the pflash's timer. But do use timer_del in
> an unrealize function to make sure that the timer can't fire after the
> pflash_t has been freed.
> 
> Signed-off-by: Stephen Checkoway <stephen.checkoway@oberlin.edu>
> ---
>  hw/block/pflash_cfi02.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 0f8b7b8c7b..1588aeff5a 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -84,7 +84,7 @@ struct pflash_t {
>      uint16_t unlock_addr0;
>      uint16_t unlock_addr1;
>      uint8_t cfi_table[0x52];
> -    QEMUTimer *timer;
> +    QEMUTimer timer;
>      /* The device replicates the flash memory across its memory space.  Emulate
>       * that by having a container (.mem) filled with an array of aliases
>       * (.mem_mappings) pointing to the flash memory (.orig_mem).
> @@ -429,7 +429,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>              }
>              pfl->status = 0x00;
>              /* Let's wait 5 seconds before chip erase is done */
> -            timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +            timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                        (NANOSECONDS_PER_SECOND * 5));
>              break;
>          case 0x30:
> @@ -444,7 +444,7 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>              }
>              pfl->status = 0x00;
>              /* Let's wait 1/2 second before sector erase is done */
> -            timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +            timer_mod(&pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                        (NANOSECONDS_PER_SECOND / 2));
>              break;
>          default:
> @@ -596,7 +596,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>      pfl->rom_mode = 1;
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
>  
> -    pfl->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
> +    timer_init_ns(&pfl->timer, QEMU_CLOCK_VIRTUAL, pflash_timer, pfl);
>      pfl->wcycle = 0;
>      pfl->cmd = 0;
>      pfl->status = 0;
> @@ -695,11 +695,18 @@ static Property pflash_cfi02_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static void pflash_cfi02_unrealize(DeviceState *dev, Error **errp)
> +{
> +    pflash_t *pfl = CFI_PFLASH02(dev);
> +    timer_del(&pfl->timer);
> +}
> +
>  static void pflash_cfi02_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = pflash_cfi02_realize;
> +    dc->unrealize = pflash_cfi02_unrealize;
>      dc->props = pflash_cfi02_properties;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>  }
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [Qemu-devel] [Qemu-trivial] [PATCH] block/pflash_cfi02: Fix memory leak and potential use-after-free

[Stephen Checkoway]

> On Mar 6, 2019, at 04:38, Laurent Vivier <laurent@vivier.eu> wrote:
> 
> Applied to my trivial-patches branch.

Great, thanks!

Cheers,

Steve