* [PATCH] hfsplus: Prevent overflows in comparisons
@ 2010-12-30 7:28 Dave Vasilevsky
2010-12-30 7:46 ` Dave Vasilevsky
0 siblings, 1 reply; 3+ messages in thread
From: Dave Vasilevsky @ 2010-12-30 7:28 UTC (permalink / raw)
To: grub-devel
When very high Catalog Node IDs are in use, comparing CNIDs via
subtraction may overflow. This causes files and folders to appear to be
missing in the btree. It's safer to just use comparison operations
rather than subtraction.
Also fixes the parent field in grub_hfsplus_catkey_internal, which is
currently using an unqualified int to hold an unsigned value.
=== modified file 'grub-core/fs/hfsplus.c'
--- grub-core/fs/hfsplus.c 2010-01-20 08:12:47 +0000
+++ grub-core/fs/hfsplus.c 2010-12-30 07:10:03 +0000
@@ -178,7 +178,7 @@
/* Internal representation of a catalog key. */
struct grub_hfsplus_catkey_internal
{
- int parent;
+ grub_uint32_t parent;
char *name;
};
@@ -520,9 +520,12 @@
int i;
int diff;
- diff = grub_be_to_cpu32 (catkey_a->parent) - catkey_b->parent;
- if (diff)
- return diff;
+ /* Safe unsigned comparison */
+ grub_uint32_t aparent = grub_be_to_cpu32 (catkey_a->parent);
+ if (aparent > catkey_b->parent)
+ return 1;
+ if (aparent < catkey_b->parent)
+ return -1;
/* Change the filename in keya so the endianness is correct. */
for (i = 0; i < grub_be_to_cpu16 (catkey_a->namelen); i++)
@@ -555,15 +558,21 @@
{
struct grub_hfsplus_extkey *extkey_a = &keya->extkey;
struct grub_hfsplus_extkey_internal *extkey_b = &keyb->extkey;
- int diff;
-
- diff = grub_be_to_cpu32 (extkey_a->fileid) - extkey_b->fileid;
-
- if (diff)
- return diff;
-
- diff = grub_be_to_cpu32 (extkey_a->start) - extkey_b->start;
- return diff;
+ grub_uint32_t akey;
+
+ /* Safe unsigned comparison */
+ akey = grub_be_to_cpu32 (extkey_a->fileid);
+ if (akey > extkey_b->fileid)
+ return 1;
+ if (akey < extkey_b->fileid)
+ return -1;
+
+ akey = grub_be_to_cpu32 (extkey_a->start);
+ if (akey > extkey_b->start)
+ return 1;
+ if (akey < extkey_b->start)
+ return -1;
+ return 0;
}
static char *
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] hfsplus: Prevent overflows in comparisons
2010-12-30 7:28 [PATCH] hfsplus: Prevent overflows in comparisons Dave Vasilevsky
@ 2010-12-30 7:46 ` Dave Vasilevsky
2011-01-03 14:35 ` Vladimir 'φ-coder/phcoder' Serbinenko
0 siblings, 1 reply; 3+ messages in thread
From: Dave Vasilevsky @ 2010-12-30 7:46 UTC (permalink / raw)
To: grub-devel
Now with a ChangeLog entry. -V
=== modified file 'ChangeLog'
--- ChangeLog 2010-12-27 06:19:51 +0000
+++ ChangeLog 2010-12-30 07:39:45 +0000
@@ -1,3 +1,11 @@
+2010-12-30 Dave Vasilevsky <dave@vasilevsky.ca>
+
+ * grub-core/fs/hfsplus.c: Parent field of
+ grub_hfsplus_catkey_internal should be unsigned.
+ (grub_hfsplus_cmp_catkey): Don't compare using subtraction, it
+ overflows.
+ (grub_hfsplus_cmp_extkey): Likewise
+
2010-12-27 Vladimir Serbinenko <phcoder@gmail.com>
* grub-core/loader/xnu.c (grub_cmd_xnu_kernel) [! GRUB_MACHINE_EFI]:
=== modified file 'grub-core/fs/hfsplus.c'
--- grub-core/fs/hfsplus.c 2010-01-20 08:12:47 +0000
+++ grub-core/fs/hfsplus.c 2010-12-30 06:10:23 +0000
@@ -178,7 +178,7 @@
/* Internal representation of a catalog key. */
struct grub_hfsplus_catkey_internal
{
- int parent;
+ grub_uint32_t parent;
char *name;
};
@@ -520,9 +520,12 @@
int i;
int diff;
- diff = grub_be_to_cpu32 (catkey_a->parent) - catkey_b->parent;
- if (diff)
- return diff;
+ /* Safe unsigned comparison */
+ grub_uint32_t aparent = grub_be_to_cpu32 (catkey_a->parent);
+ if (aparent > catkey_b->parent)
+ return 1;
+ if (aparent < catkey_b->parent)
+ return -1;
/* Change the filename in keya so the endianness is correct. */
for (i = 0; i < grub_be_to_cpu16 (catkey_a->namelen); i++)
@@ -555,15 +558,21 @@
{
struct grub_hfsplus_extkey *extkey_a = &keya->extkey;
struct grub_hfsplus_extkey_internal *extkey_b = &keyb->extkey;
- int diff;
-
- diff = grub_be_to_cpu32 (extkey_a->fileid) - extkey_b->fileid;
-
- if (diff)
- return diff;
-
- diff = grub_be_to_cpu32 (extkey_a->start) - extkey_b->start;
- return diff;
+ grub_uint32_t akey;
+
+ /* Safe unsigned comparison */
+ akey = grub_be_to_cpu32 (extkey_a->fileid);
+ if (akey > extkey_b->fileid)
+ return 1;
+ if (akey < extkey_b->fileid)
+ return -1;
+
+ akey = grub_be_to_cpu32 (extkey_a->start);
+ if (akey > extkey_b->start)
+ return 1;
+ if (akey < extkey_b->start)
+ return -1;
+ return 0;
}
static char *
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] hfsplus: Prevent overflows in comparisons
2010-12-30 7:46 ` Dave Vasilevsky
@ 2011-01-03 14:35 ` Vladimir 'φ-coder/phcoder' Serbinenko
0 siblings, 0 replies; 3+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2011-01-03 14:35 UTC (permalink / raw)
To: grub-devel
[-- Attachment #1: Type: text/plain, Size: 2402 bytes --]
Applied, thanks.
On 12/30/2010 08:46 AM, Dave Vasilevsky wrote:
> === modified file 'ChangeLog'
> --- ChangeLog 2010-12-27 06:19:51 +0000
> +++ ChangeLog 2010-12-30 07:39:45 +0000
> @@ -1,3 +1,11 @@
> +2010-12-30 Dave Vasilevsky <dave@vasilevsky.ca>
> +
> + * grub-core/fs/hfsplus.c: Parent field of
> + grub_hfsplus_catkey_internal should be unsigned.
> + (grub_hfsplus_cmp_catkey): Don't compare using subtraction, it
> + overflows.
> + (grub_hfsplus_cmp_extkey): Likewise
> +
> 2010-12-27 Vladimir Serbinenko <phcoder@gmail.com>
>
> * grub-core/loader/xnu.c (grub_cmd_xnu_kernel) [! GRUB_MACHINE_EFI]:
>
> === modified file 'grub-core/fs/hfsplus.c'
> --- grub-core/fs/hfsplus.c 2010-01-20 08:12:47 +0000
> +++ grub-core/fs/hfsplus.c 2010-12-30 06:10:23 +0000
> @@ -178,7 +178,7 @@
> /* Internal representation of a catalog key. */
> struct grub_hfsplus_catkey_internal
> {
> - int parent;
> + grub_uint32_t parent;
> char *name;
> };
>
> @@ -520,9 +520,12 @@
> int i;
> int diff;
>
> - diff = grub_be_to_cpu32 (catkey_a->parent) - catkey_b->parent;
> - if (diff)
> - return diff;
> + /* Safe unsigned comparison */
> + grub_uint32_t aparent = grub_be_to_cpu32 (catkey_a->parent);
> + if (aparent > catkey_b->parent)
> + return 1;
> + if (aparent < catkey_b->parent)
> + return -1;
>
> /* Change the filename in keya so the endianness is correct. */
> for (i = 0; i < grub_be_to_cpu16 (catkey_a->namelen); i++)
> @@ -555,15 +558,21 @@
> {
> struct grub_hfsplus_extkey *extkey_a = &keya->extkey;
> struct grub_hfsplus_extkey_internal *extkey_b = &keyb->extkey;
> - int diff;
> -
> - diff = grub_be_to_cpu32 (extkey_a->fileid) - extkey_b->fileid;
> -
> - if (diff)
> - return diff;
> -
> - diff = grub_be_to_cpu32 (extkey_a->start) - extkey_b->start;
> - return diff;
> + grub_uint32_t akey;
> +
> + /* Safe unsigned comparison */
> + akey = grub_be_to_cpu32 (extkey_a->fileid);
> + if (akey > extkey_b->fileid)
> + return 1;
> + if (akey < extkey_b->fileid)
> + return -1;
> +
> + akey = grub_be_to_cpu32 (extkey_a->start);
> + if (akey > extkey_b->start)
> + return 1;
> + if (akey < extkey_b->start)
> + return -1;
> + return 0;
> }
>
> static char *
>
>
>
>
--
Regards
Vladimir 'φ-coder/phcoder' Serbinenko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-01-03 14:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-30 7:28 [PATCH] hfsplus: Prevent overflows in comparisons Dave Vasilevsky
2010-12-30 7:46 ` Dave Vasilevsky
2011-01-03 14:35 ` Vladimir 'φ-coder/phcoder' Serbinenko
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).