linux-kernel - Re: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field to klp

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:	Thu, 12 Nov 2015 11:22:31 +0100 (CET)
From:	Miroslav Benes <mbenes@...e.cz>
To:	Chris J Arges <chris.j.arges@...onical.com>
cc:	live-patching@...r.kernel.org, jeyu@...hat.com,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Seth Jennings <sjenning@...hat.com>,
	Jiri Kosina <jikos@...nel.org>,
	Vojtech Pavlik <vojtech@...e.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3 v4] livepatch: add old_sympos as disambiguator field
 to klp_func


Next to Josh's remarks I have some more (mainly nitpicks, so it is often 
up to you).

On Wed, 11 Nov 2015, Chris J Arges wrote:

> In cases of duplicate symbols, old_sympos will be used to disambiguate
> instead of old_addr. By default old_sympos will be 0, and patching will
> only succeed if the symbol is unique. Specifying a positive value will
> ensure that occurrence of the symbol will be used for patching if it is

"...occurrence of the symbol in kallsyms for the patched object will 
be used..."

Just to have it even in the changelog for clarity.

> valid. Finally, old_addr is now an internal structure element and not to
> be specified by the user.


> @@ -159,36 +160,45 @@ static int klp_find_callback(void *data, const char *name,
>  		return 0;
>  
>  	/*
> -	 * args->addr might be overwritten if another match is found
> -	 * but klp_find_object_symbol() handles this and only returns the
> -	 * addr if count == 1.
> +	 * increment and assign address, return only if checking pos and
> +	 * it matches count.
>  	 */
> -	args->addr = addr;
>  	args->count++;
> +	args->addr = addr;

I guess that this row swap is remnant of a rebase. Anyway it is 
superfluous.

> +	if ((args->pos > 0) && (args->count == args->pos))
> +		return 1;

We could add an optimization here. If args->pos == 0 and args->count > 1 
we can return 1, because the symbol is not unique. The case is then 
correctly handled in klp_find_object_symbol. There is no need to walk 
through the rest of kallsyms.

Thanks,
Miroslav
--
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/