linux-kernel - Re: [BUGFIX][PATCH -tip] x86: fix text

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20090408123133.GE18581@elte.hu>
Date:	Wed, 8 Apr 2009 14:31:33 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Masami Hiramatsu <mhiramat@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	LKML <linux-kernel@...r.kernel.org>,
	systemtap-ml <systemtap@...rces.redhat.com>
Subject: Re: [BUGFIX][PATCH -tip] x86: fix text_poke to handle highmem pages


* Masami Hiramatsu <mhiramat@...hat.com> wrote:

> @@ -514,27 +515,39 @@ void *__kprobes text_poke(void *addr, const void *opcode, size_t len)
>  {
>  	unsigned long flags;
>  	char *vaddr;
> -	struct page *pages[2];
> +	struct page *page;
>  	int i;
> +	unsigned long endp = ((unsigned long)addr + len) & PAGE_MASK;
> 
> -	if (!core_kernel_text((unsigned long)addr)) {
> -		pages[0] = vmalloc_to_page(addr);
> -		pages[1] = vmalloc_to_page(addr + PAGE_SIZE);
> -	} else {
> -		pages[0] = virt_to_page(addr);
> -		WARN_ON(!PageReserved(pages[0]));
> -		pages[1] = virt_to_page(addr + PAGE_SIZE);
> +	/*
> +	 * If the written range covers 2 pages, we'll split it, because
> +	 * vmalloc pages are not always continuous -- e.g. 1st page is
> +	 * lowmem and 2nd page is highmem.
> +	 */
> +	if (((unsigned long)addr & PAGE_MASK) != endp) {
> +		text_poke(addr, opcode, endp - (unsigned long)addr);
> +		addr =  (void *)endp;
> +		opcode = (char *)opcode + (endp - (unsigned long)addr);
> +		len -= endp - (unsigned long)addr;
>  	}
> -	BUG_ON(!pages[0]);
> +
> +	if (!core_kernel_text((unsigned long)addr))
> +		page = vmalloc_to_page(addr);
> +	else
> +		page = virt_to_page(addr);

hm, the bug is upstream now. And your fix turns a 
supposed-to-be-simpler kmap based patching thing back into something 
fragile looking again. We might be better off with a revert - or we 
do a real clean patch.

Firstly, that core_kernel_text() distinction above looks 
artificially open-coded - dont we have a proper, generic 
"look-up-the-page" variant in the MM somewhere?

> +	BUG_ON(!page);
>  	local_irq_save(flags);
> -	set_fixmap(FIX_TEXT_POKE0, page_to_phys(pages[0]));
> -	if (pages[1])
> -		set_fixmap(FIX_TEXT_POKE1, page_to_phys(pages[1]));
> -	vaddr = (char *)fix_to_virt(FIX_TEXT_POKE0);
> +	if (PageHighMem(page))
> +		vaddr = kmap_atomic(page, KM_TEXT_POKE);
> +	else {
> +		set_fixmap(FIX_TEXT_POKE, page_to_phys(page));
> +		vaddr = (char *)fix_to_virt(FIX_TEXT_POKE);
> +	}

that too looks artificially complex. Why cannot we kmap lowmem pages 
too? If the API isnt available on !HIGHMEM kernels .. then the 
solution is to make it available, not to branch our way around it.

>  	memcpy(&vaddr[(unsigned long)addr & ~PAGE_MASK], opcode, len);
> -	clear_fixmap(FIX_TEXT_POKE0);
> -	if (pages[1])
> -		clear_fixmap(FIX_TEXT_POKE1);
> +	if (PageHighMem(page))
> +		kunmap_atomic(vaddr, KM_TEXT_POKE);
> +	else
> +		clear_fixmap(FIX_TEXT_POKE);

ditto.

Thanks,

	Ingo
--
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/