linux-kernel - Re: [PATCH v3 1/8] x86: allow to handle errors in 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <5284AF89.20505@hitachi.com>
Date:	Thu, 14 Nov 2013 20:10:01 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Petr Mladek <pmladek@...e.cz>
Cc:	Steven Rostedt <rostedt@...dmis.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Jiri Kosina <jkosina@...e.cz>, linux-kernel@...r.kernel.org,
	x86@...nel.org
Subject: Re: [PATCH v3 1/8] x86: allow to handle errors in text_poke function
 family

(2013/11/14 19:41), Petr Mladek wrote:
> The text_poke functions called BUG() in case of error. This was too strict.
> There are situations when the system is still usable even when the patching
> has failed, for example when enabling the dynamic ftrace.
> 
> This commit modifies text_poke, text_poke_early, and text_poke_bp functions
> to return an error code instead calling BUG(). The code is returned instead
> of the patched address. The address was just copied from the first parameter,
> so it was no extra information. It has not been used anywhere yet.
> 
> The commit also modifies the few locations where text_poke functions were used
> and the error code has to be handled now. It just passes the error code if
> there already is an existing error handling, for example in
> kgdb_arch_set_breakpoint. It calls BUG() in the other locations.
> 
> Note that BUG() still need to be called in text_poke_bp when the code already is
> partially modified but the operation can not be finished.

Mostly this looks good for me, see below.

> @@ -566,7 +576,8 @@ void *__kprobes text_poke(void *addr, const void *opcode, size_t len)
>  		WARN_ON(!PageReserved(pages[0]));
>  		pages[1] = virt_to_page(addr + PAGE_SIZE);
>  	}
> -	BUG_ON(!pages[0]);
> +	if (unlikely(!pages[0]))
> +		return -EFAULT;
>  	local_irq_save(flags);
>  	set_fixmap(FIX_TEXT_POKE0, page_to_phys(pages[0]));
>  	if (pages[1])
> @@ -580,10 +591,11 @@ void *__kprobes text_poke(void *addr, const void *opcode, size_t len)
>  	sync_core();
>  	/* Could also do a CLFLUSH here to speed up CPU recovery; but
>  	   that causes hangs on some VIA CPUs. */
> -	for (i = 0; i < len; i++)
> -		BUG_ON(((char *)addr)[i] != ((char *)opcode)[i]);
>  	local_irq_restore(flags);
> -	return addr;
> +	for (i = 0; i < len; i++)
> +		if (((char *)addr)[i] != ((char *)opcode)[i])
> +			return -EPERM;
> +	return 0;

Hmm, IMHO, it would be better to keep the old one, because it actually failed to modify
as expected for some reason (not only permission error). And we are not sure the
code is same as old one. (only compared with what we expected)

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com


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