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<address@hidden>
> 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