* [PATCH 0/2] Misc fixes @ 2011-10-28 19:50 Jes.Sorensen 2011-10-28 19:50 ` [PATCH 1/2] Avoid use after free Jes.Sorensen 2011-10-28 19:50 ` [PATCH 2/2] Avoid stack overflow if GPT partition entries on disk are > 128 bytes Jes.Sorensen 0 siblings, 2 replies; 5+ messages in thread From: Jes.Sorensen @ 2011-10-28 19:50 UTC (permalink / raw) To: neilb; +Cc: linux-raid, dledford 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] Avoid use after free 2011-10-28 19:50 [PATCH 0/2] Misc fixes Jes.Sorensen @ 2011-10-28 19:50 ` Jes.Sorensen 2011-10-30 23:43 ` NeilBrown 2011-10-28 19:50 ` [PATCH 2/2] Avoid stack overflow if GPT partition entries on disk are > 128 bytes Jes.Sorensen 1 sibling, 1 reply; 5+ messages in thread From: Jes.Sorensen @ 2011-10-28 19:50 UTC (permalink / raw) To: neilb; +Cc: linux-raid, dledford 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] Avoid use after free 2011-10-28 19:50 ` [PATCH 1/2] Avoid use after free Jes.Sorensen @ 2011-10-30 23:43 ` NeilBrown 2011-10-31 7:41 ` Jes Sorensen 0 siblings, 1 reply; 5+ messages in thread From: NeilBrown @ 2011-10-30 23:43 UTC (permalink / raw) To: Jes.Sorensen; +Cc: linux-raid, dledford [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] Avoid use after free 2011-10-30 23:43 ` NeilBrown @ 2011-10-31 7:41 ` Jes Sorensen 0 siblings, 0 replies; 5+ messages in thread From: Jes Sorensen @ 2011-10-31 7:41 UTC (permalink / raw) To: NeilBrown; +Cc: linux-raid, dledford 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] Avoid stack overflow if GPT partition entries on disk are > 128 bytes 2011-10-28 19:50 [PATCH 0/2] Misc fixes Jes.Sorensen 2011-10-28 19:50 ` [PATCH 1/2] Avoid use after free Jes.Sorensen @ 2011-10-28 19:50 ` Jes.Sorensen 1 sibling, 0 replies; 5+ messages in thread From: Jes.Sorensen @ 2011-10-28 19:50 UTC (permalink / raw) To: neilb; +Cc: linux-raid, dledford 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-10-31 7:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-28 19:50 [PATCH 0/2] Misc fixes Jes.Sorensen 2011-10-28 19:50 ` [PATCH 1/2] Avoid use after free Jes.Sorensen 2011-10-30 23:43 ` NeilBrown 2011-10-31 7:41 ` Jes Sorensen 2011-10-28 19:50 ` [PATCH 2/2] Avoid stack overflow if GPT partition entries on disk are > 128 bytes Jes.Sorensen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).