linux-kernel - Re: [LKP] [x86/mm/ASLR] f47233c2d34: WARNING: CPU: 0 PID: 1 at arch/x86/mm/ioremap.c:63 __ioremap_check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 28 Feb 2015 20:52:24 +0100
From:	Borislav Petkov <bp@...e.de>
To:	Matt Fleming <matt@...eblueprint.co.uk>
Cc:	"H. Peter Anvin" <hpa@...or.com>,
	Kees Cook <keescook@...omium.org>,
	Jiri Kosina <jkosina@...e.cz>, Ingo Molnar <mingo@...nel.org>,
	Huang Ying <ying.huang@...el.com>,
	LKML <linux-kernel@...r.kernel.org>, LKP ML <lkp@...org>,
	x86-ml <x86@...nel.org>, Dave Young <dyoung@...hat.com>,
	Andy Lutomirski <luto@...capital.net>
Subject: Re: [LKP] [x86/mm/ASLR] f47233c2d34: WARNING: CPU: 0 PID: 1 at
 arch/x86/mm/ioremap.c:63 __ioremap_check_ram+0x445/0x4a0()

On Sat, Feb 28, 2015 at 07:20:09PM +0000, Matt Fleming wrote:
> Doing a static allocation is fine, and the memory is even reserved from
> being overwritten via memblock_x86_reserve_range_setup_data(), but it

First of all, the naming of that function has failed really badly
somewhere along the way.

But more importantly, parse_setup_data() runs before it and the memory
is already overwritten even at parse_setup_data() time according to my
observations. So I think the more robust way is to stick setup_data
stuff below the minimum address decompress_kernel() works on... Where is
hpa when you need him? :-)

And this, dear children, is the bigger problem...

> looks like that reservation gets dropped before
> get_setup_data_total_num() runs, which is what is causing ioremap() to
> complain - it really is usable RAM we're trying to ioremap().

... while this is the easier problem which we can take care of by doing
monkey business like zapping the entry from the setup_data linked list
so that future examiners don't even get to see it:

---
@@ -460,9 +468,34 @@ static void __init parse_setup_data(void)
                case SETUP_KASLR:
                        parse_kaslr_setup(pa_data, data_len);
                        break;
+
+
+                       /*
+                        * Zap this element from the list so that nothing sees
+                        * it later on.
+                        */
+                       if (!pa_prev) {
+                               boot_params.hdr.setup_data = pa_next;
+                       } else if (pa_next)
+                               struct setup_data *p;
+
+                               p = early_memremap(pa_prev, sizeof(*p));
+                               p->next = pa_next;
+                               early_iounmap(p, sizeof(*p));
+                       }
+
+                       pa_data = pa_next;
+                       continue;
+                       break;
                default:
+                       WARN_ON(1);
                        break;
                }
+               pa_prev = pa_data;
                pa_data = pa_next;
        }
 }

> Dropping the reservation looks to happen in memblock_x86_fill(), because
> you'll note that we explicitly reserve the boot services regions
> immediately after memblock_x86_fill() in setup_arch().

Well, this is not actually needed since parse_setup_data() runs much
earlier and we're perfectly fine with keeping that region until it gets
parsed there and freeing it afterwards because we don't need it.

Which reminds me that if we do that, we'll have to edit the setup_data
list which would actually make the zapping mandatory...

Yep, exactly: so I probably should add the above hunk to the final patch
too - we only need that entry until parse_setup_data() so that we can
fish out kaslr enabled state and after that we can free it.

> What isn't clear right now is why the ioremap() warning isn't triggering
> for a bunch of other callsites that get this wrong, i.e.
> pcibios_add_device().

That's a good question.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/