[PATCH 0/2] Misc fixes

[PATCH 0/2] Misc fixes

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

Two fixes:

The first is to avoid a use-after-free case. I looked at this one for
a while, and I believe this is the intention of the code. Ie. once we
find the first entry, bail out to the end rather than going back
re-iterating over the list that we just freed.

The second to avoid the case where a GPT partition table entry is
larger than 128 bytes, in which case we would read it and corrupt the
stack.

Jes Sorensen (2):
  Avoid use after free
  Avoid stack overflow if GPT partition entries on disk are > 128 bytes

 util.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

-- 
1.7.6.4

[PATCH 1/2] Avoid use after free

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

If picking just one spare disk from the container, jump out of the
loop once freeing the list. Otherwise we end up accessing the list
that we just freed.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 util.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/util.c b/util.c
index 2cf617d..1bbd87f 100644
--- a/util.c
+++ b/util.c
@@ -1766,6 +1766,7 @@ struct mdinfo *container_choose_spares(struct supertype *st,
 			if (get_one) {
 				sysfs_free(*dp);
 				d->next = NULL;
+				goto out;
 			}
 		} else {
 			*dp = d->next;
@@ -1773,5 +1774,6 @@ struct mdinfo *container_choose_spares(struct supertype *st,
 			sysfs_free(d);
 		}
 	}
+out:
 	return disks;
 }
-- 
1.7.6.4

[PATCH 2/2] Avoid stack overflow if GPT partition entries on disk are > 128 bytes

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Per [1] GPT partition table entries are not guaranteed to be 128
bytes, in which case read() straight into a struct GPT_part_entry
would result in a buffer overflow corrupting the stack.

[1] http://en.wikipedia.org/wiki/GUID_Partition_Table

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 util.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/util.c b/util.c
index 1bbd87f..f65bf79 100644
--- a/util.c
+++ b/util.c
@@ -1127,7 +1127,8 @@ static int get_gpt_last_partition_end(int fd, unsigned long long *endofpart)
 {
 	struct GPT gpt;
 	unsigned char empty_gpt_entry[16]= {0};
-	struct GPT_part_entry part;
+	struct GPT_part_entry *part;
+	char buf[512];
 	unsigned long long curr_part_end;
 	unsigned all_partitions, entry_size;
 	unsigned part_nr;
@@ -1151,18 +1152,20 @@ static int get_gpt_last_partition_end(int fd, unsigned long long *endofpart)
 
 	/* sanity checks */
 	if (all_partitions > 1024 ||
-	    entry_size > 512)
+	    entry_size > sizeof(buf))
 		return -1;
 
+	part = (struct GPT_part_entry *)buf;
+
 	for (part_nr=0; part_nr < all_partitions; part_nr++) {
 		/* read partition entry */
-		if (read(fd, &part, entry_size) != (ssize_t)entry_size)
+		if (read(fd, buf, entry_size) != (ssize_t)entry_size)
 			return 0;
 
 		/* is this valid partition? */
-		if (memcmp(part.type_guid, empty_gpt_entry, 16) != 0) {
+		if (memcmp(part->type_guid, empty_gpt_entry, 16) != 0) {
 			/* check the last lba for the current partition */
-			curr_part_end = __le64_to_cpu(part.ending_lba);
+			curr_part_end = __le64_to_cpu(part->ending_lba);
 			if (curr_part_end > *endofpart)
 				*endofpart = curr_part_end;
 		}
-- 
1.7.6.4

Re: [PATCH 1/2] Avoid use after free

[NeilBrown]

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

On Fri, 28 Oct 2011 21:50:50 +0200 Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> If picking just one spare disk from the container, jump out of the
> loop once freeing the list. Otherwise we end up accessing the list
> that we just freed.
> 
> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> ---
>  util.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/util.c b/util.c
> index 2cf617d..1bbd87f 100644
> --- a/util.c
> +++ b/util.c
> @@ -1766,6 +1766,7 @@ struct mdinfo *container_choose_spares(struct supertype *st,
>  			if (get_one) {
>  				sysfs_free(*dp);
>  				d->next = NULL;
> +				goto out;
>  			}
>  		} else {
>  			*dp = d->next;
> @@ -1773,5 +1774,6 @@ struct mdinfo *container_choose_spares(struct supertype *st,
>  			sysfs_free(d);
>  		}
>  	}
> +out:
>  	return disks;
>  }


Hi Jes,
  I dont' think patch is needed.
  The while loop that it jumps out of is
    while (*dp)

  at the place you put the goto,
     dp == &d->next
  As d->next was just set to NULL, *dp will be NULL, so the loop will
  exit with the need for a goto.

 I have applied the second patch - the GPT stack overflow one, thanks.

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] Avoid use after free

[Jes Sorensen]

On 10/31/11 00:43, NeilBrown wrote:
> On Fri, 28 Oct 2011 21:50:50 +0200 Jes.Sorensen@redhat.com wrote:
> 
>> > From: Jes Sorensen <Jes.Sorensen@redhat.com>
>> > 
>> > If picking just one spare disk from the container, jump out of the
>> > loop once freeing the list. Otherwise we end up accessing the list
>> > that we just freed.
>> > 
>> > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
>> > ---
>> >  util.c |    2 ++
>> >  1 files changed, 2 insertions(+), 0 deletions(-)
>> > 
>> > diff --git a/util.c b/util.c
>> > index 2cf617d..1bbd87f 100644
>> > --- a/util.c
>> > +++ b/util.c
>> > @@ -1766,6 +1766,7 @@ struct mdinfo *container_choose_spares(struct supertype *st,
>> >  			if (get_one) {
>> >  				sysfs_free(*dp);
>> >  				d->next = NULL;
>> > +				goto out;
>> >  			}
>> >  		} else {
>> >  			*dp = d->next;
>> > @@ -1773,5 +1774,6 @@ struct mdinfo *container_choose_spares(struct supertype *st,
>> >  			sysfs_free(d);
>> >  		}
>> >  	}
>> > +out:
>> >  	return disks;
>> >  }
> 
> Hi Jes,
>   I dont' think patch is needed.
>   The while loop that it jumps out of is
>     while (*dp)
> 
>   at the place you put the goto,
>      dp == &d->next
>   As d->next was just set to NULL, *dp will be NULL, so the loop will
>   exit with the need for a goto.

Hi Neil,

It has to be said, I didn't actually come up with this one on my own, it
was found by the Coverity security checker. I stared at this one for
quite a while. It is well obfuscated. So far I managed to convince
myself several times that you were right and also that Coverity is
correct. In the end I think that you are right indeed and this is a
false positive.

However, in order to avoid the confusion, how about changing the d->next
= NULL assignment to *dp = NULL instead? It should make it a lot more
clear for anyone reading the code what is going on.

Cheers,
Jes