Skip to contentSkip to conversation

ARM: Ignore memory tags with invalid data

PATCH In review Corey Minyard 4 participants 5 messages Jan 22, 2008 – Feb 10, 2008 Arm (32-bit)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ARM: Ignore memory tags with invalid data

From: Corey Minyard <[email protected]>

The DNS-323 system has several bogus memory entries in the tag table, and it caused the system to crash at startup. Ignore tag entries that are obviously bogus.

---
 arch/arm/kernel/setup.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
arch/arm/kernel/setup.c +6 -1
index bf56eb3..dfdb469 100644
@@ -630,7 +630,12 @@ __tagtable(ATAG_CORE, parse_tag_core);
 
 static int __init parse_tag_mem32(const struct tag *tag)
 {
-	if (meminfo.nr_banks >= NR_BANKS) {
+	/*
+	 * Make sure that the memory size is non-zero, page aligned,
+	 * and that it doesn't overflow the meminfo table.
+	 */
+	if (meminfo.nr_banks >= NR_BANKS || tag->u.mem.size & ~PAGE_MASK ||
+	    tag->u.mem.size == 0 || tag->u.mem.start & ~PAGE_MASK) {
 		printk(KERN_WARNING
 		       "Ignoring memory bank 0x%08x size %dKB\n",
 			tag->u.mem.start, tag->u.mem.size / 1024);
#1 lore raw
Quoted 5 lines · Corey Minyard

From: Corey Minyard <[email protected]>

The DNS-323 system has several bogus memory entries in the tag table, and it caused the system to crash at startup. Ignore tag entries that are obviously bogus.

This should have gone to the linux-arm-kernel mailing list as well, so that all the people interested in ARM can see it as soon as possible. See [email protected]

The only suggestion I could think of would to be change to having an check_tag_mem32() function, or put each check on its own line to make it easier to read what each check is doing.

ie:

	if (meminfo.nr_banks >= NR_BANKS ||
	    tag->u.mem.size & ~PAGE_MASK || /* size is unaligned */
	    tag->u.mem.size == 0 ||	    /* no memory here */
	    tag->u.mem.start & ~PAGE_MASK)  /* start is unaligned */ 
	{

(even without the comments it makes it easier to see what each test is.

Quoted 28 lines
---
 arch/arm/kernel/setup.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
arch/arm/kernel/setup.c +6 -2
index bf56eb3..dfdb469 100644
@@ -630,7 +630,12 @@ __tagtable(ATAG_CORE, parse_tag_core);
 
 static int __init parse_tag_mem32(const struct tag *tag)
 {
-	if (meminfo.nr_banks >= NR_BANKS) {
+	/*
+	 * Make sure that the memory size is non-zero, page aligned,
+	 * and that it doesn't overflow the meminfo table.
+	 */
+	if (meminfo.nr_banks >= NR_BANKS || tag->u.mem.size & ~PAGE_MASK ||
+	    tag->u.mem.size == 0 || tag->u.mem.start & ~PAGE_MASK) {
 		printk(KERN_WARNING
 		       "Ignoring memory bank 0x%08x size %dKB\n",
 			tag->u.mem.start, tag->u.mem.size / 1024);
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'
#2 lore raw
Quoted 4 lines · Ben Dooks
Quoted 2 lines · Corey Minyard

From: Corey Minyard <[email protected]>

Quoted 9 lines
Quoted 4 lines

The DNS-323 system has several bogus memory entries in the tag table, and it caused the system to crash at startup. Ignore tag entries that are obviously bogus.

This should have gone to the linux-arm-kernel mailing list as well, so that all the people interested in ARM can see it as soon as possible. See [email protected]

You have to be a member to post, and I'm not a member. I guess I'll join, though it seems somewhat pointless for a single patch.

Quoted 5 lines

The only suggestion I could think of would to be change to having an check_tag_mem32() function, or put each check on its own line to make it easier to read what each check is doing.

ie:

Quoted 9 lines
	if (meminfo.nr_banks >= NR_BANKS ||
	    tag->u.mem.size & ~PAGE_MASK || /* size is unaligned */
	    tag->u.mem.size == 0 ||	    /* no memory here */
	    tag->u.mem.start & ~PAGE_MASK)  /* start is unaligned */ 
	{

(even without the comments it makes it easier to see what each test is.

That's fine, it is easier to read.

Thanks,

-corey

#3 lore raw
Quoted 7 lines · Corey Minyard
Quoted 3 lines · Ben Dooks

This should have gone to the linux-arm-kernel mailing list as well, so that all the people interested in ARM can see it as soon as possible. See [email protected]

You have to be a member to post, and I'm not a member. I guess I'll join, though it seems somewhat pointless for a single patch.

Incorrect - we have quite a number of people who are allowed to post but who are not members, and it's a growing number. The list may default to member only posting but there are a couple of sane human beings behind it who don't randomly throw stuff away for no reason.

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
#4 lore raw
In reply to Corey Minyard
Quoted 20 lines · Corey Minyard

From: Corey Minyard <[email protected]>

The DNS-323 system has several bogus memory entries in the tag table, and it caused the system to crash at startup. Ignore tag entries that are obviously bogus.

---
 arch/arm/kernel/setup.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
arch/arm/kernel/setup.c +0 -0
index bf56eb3..dfdb469 100644
@@ -630,7 +630,12 @@ __tagtable(ATAG_CORE, parse_tag_core);
 
 static int __init parse_tag_mem32(const struct tag *tag)
 {

arch/arm/kernel/setup.c __tagtable(ATAG_CORE, parse_tag_core); · line 641

Quoted 10 lines
arch/arm/kernel/setup.c +6 -1
-       if (meminfo.nr_banks >= NR_BANKS) {
+       /*
+        * Make sure that the memory size is non-zero, page aligned,
+        * and that it doesn't overflow the meminfo table.
+        */
+       if (meminfo.nr_banks >= NR_BANKS || tag->u.mem.size & ~PAGE_MASK ||
+           tag->u.mem.size == 0 || tag->u.mem.start & ~PAGE_MASK) {
                printk(KERN_WARNING
                       "Ignoring memory bank 0x%08x size %dKB\n",
                        tag->u.mem.start, tag->u.mem.size / 1024);

[Cc: linux-arm-kernel]

What's the status of this patch? It would be good to see it go in because the problem it fixes can be seen on a large number of NAS devices.

Cheers,

Signature · 1 line
Byron Bradley