linux-kernel - Re: [PATCH 1/4 v5] 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]

Message-ID: <20151113162705.GW2599@pathway.suse.cz>
Date:	Fri, 13 Nov 2015 17:27:05 +0100
From:	Petr Mladek <pmladek@...e.com>
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/4 v5] livepatch: add old_sympos as disambiguator field
 to klp_func

On Thu 2015-11-12 10:59:50, 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 in kallsyms for the patched object
> will be used for patching if it is valid.
> 
> In addition, make old_addr an internal structure field not to be specified
> by the user. Finally, remove klp_find_verify_func_addr as it can be
> replaced by klp_find_object_symbol directly.
> 
> Signed-off-by: Chris J Arges <chris.j.arges@...onical.com>
> ---
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..479d75e 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -159,36 +154,46 @@ 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 the address. Return 1 when the desired symbol
> +	 * position is found or the search for a unique symbol fails.
>  	 */
>  	args->addr = addr;
>  	args->count++;

I would avoid the obvious things and move the comment here:

	/*
	 * Finish the search when the symbol is found on the desired
	 * position or the position is not defined for non-unique
	 * symbol.
	 */

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

I wonder if this is better readable:

	if ((args->pos && (args->count == args->pos)) ||
	    (!args->pos && (args->count > 1)))
		return 1;

>  	return 0;
>  }
>  
[...]
> @@ -277,7 +261,7 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
>  	preempt_enable();
>  
>  	/* otherwise check if it's in another .o within the patch module */
> -	return klp_find_object_symbol(pmod->name, name, addr);
> +	return klp_find_object_symbol(pmod->name, name, addr, 0);

Please, add a comment here that external symbols always have to be unique.

>  }
>  
>  static int klp_write_object_relocations(struct module *pmod,
> @@ -307,7 +291,7 @@ static int klp_write_object_relocations(struct module *pmod,
>  			else
>  				ret = klp_find_object_symbol(obj->mod->name,
>  							     reloc->name,
> -							     &reloc->val);
> +							     &reloc->val, 0);

Please, mention in the commit message that the support for sympos will
be added into relocations in a separate patch. Or add a FIXME comment
here. It will make easier the review and git history archaeology.

Thank you,
Petr
--
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/