Re: [PATCH parted 2/2] linux: Fix use after free in devicemapper code

[Hans de Goede]

Hi,

On 12/18/2009 01:26 PM, Jim Meyering wrote:

Hans de Goede wrote:

* libparted/arch/linux.c (_dm_add_partition): Fix use of dm_task
information after freeing it.
---
  libparted/arch/linux.c |    6 ++++--
  1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
index 9d15bf2..36a698d 100644
--- a/libparted/arch/linux.c
+++ b/libparted/arch/linux.c
@@ -2447,12 +2447,14 @@ _dm_add_partition (PedDisk* disk, PedPartition* part)
                  goto err;

          dev_name = dm_task_get_name (task);
-        dm_task_destroy (task);
-        task = NULL;

          if (asprintf (&vol_name, "%sp%d", dev_name, part->num) == -1)
                  goto err;

+        dm_task_destroy(task);
+        dev_name = NULL;
+        task = NULL;
+

Hi Hans,
Thanks for that patch!

Is this adjustment ok with you, since I'm leaving your name on it?
I don't want to leave the assignments to NULL, since they're
effectively dead code and I don't want to provoke warnings
from the likes of clang and coverity.

I understand the motivation in setting them to NULL,
but since there is obviously no further use of dev_name,
and the next use of "task" is an assignment, it seems safe.

You cannot remove the "task = NULL" statement, otherwise the task will
get destroyed a second time in the error path of the asprintf directly
below the moved "dm_task_destroy (task)"

As for the removal of the dev_name = NULL, that is ok, I put it there
to capture future uses of it after it being freed (as a NULL deref is
a guaranteed crash, where as using freed memory leads to random
behavior).

Regards,

Hans

 From 71fa59e78799589bed5b85baa7ba2c9efabc661e Mon Sep 17 00:00:00 2001
From: Hans de Goede<hdego...@redhat.com>
Date: Fri, 18 Dec 2009 10:33:18 +0100
Subject: [PATCH] linux: fix use-after-free in devicemapper code

* libparted/arch/linux.c (_dm_add_partition): Fix use of dm_task
information after freeing it.
---
  libparted/arch/linux.c |    5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
index 9d15bf2..d996f32 100644
--- a/libparted/arch/linux.c
+++ b/libparted/arch/linux.c
@@ -2447,12 +2447,13 @@ _dm_add_partition (PedDisk* disk, PedPartition* part)
                  goto err;

          dev_name = dm_task_get_name (task);
-        dm_task_destroy (task);
-        task = NULL;

          if (asprintf (&vol_name, "%sp%d", dev_name, part->num) == -1)
                  goto err;

+        /* Caution: dm_task_destroy frees dev_name.  */
+        dm_task_destroy (task);
+
          if (asprintf (&params, "%d:%d %lld", arch_specific->major,
                        arch_specific->minor, part->geom.start) == -1)
                  goto err;
--
1.6.6.rc3.271.g3d40f

_______________________________________________
bug-parted mailing list
bug-parted@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-parted