linux-kernel - Re: [PATCH 5/6] x86: patch all traced function calls using the int3-based framework

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: <20131018123955.2e301b97@gandalf.local.home>
Date:	Fri, 18 Oct 2013 12:39:55 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Petr Mladek <pmladek@...e.cz>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Jiri Kosina <jkosina@...e.cz>, linux-kernel@...r.kernel.org,
	x86@...nel.org
Subject: Re: [PATCH 5/6] x86: patch all traced function calls using the
 int3-based framework

On Fri, 18 Oct 2013 16:27:24 +0200
Petr Mladek <pmladek@...e.cz> wrote:


>  void ftrace_replace_code(int enable)
>  {
>  	struct ftrace_rec_iter *iter;
>  	struct dyn_ftrace *rec;
> -	const char *report = "adding breakpoints";
> -	int count = 0;
> -	int ret;
> +	unsigned long *addr = NULL;
> +	void *opcode = NULL, *op = NULL;
> +	const unsigned char *new;
> +	size_t count = 0;
>  
>  	for_ftrace_rec_iter(iter) {
>  		rec = ftrace_rec_iter_record(iter);
> -
> -		ret = add_breakpoints(rec, enable);
> -		if (ret)
> -			goto remove_breakpoints;
> -		count++;
> -	}
> -
> -	run_sync();
> -
> -	report = "updating code";
> -
> -	for_ftrace_rec_iter(iter) {
> -		rec = ftrace_rec_iter_record(iter);
> -
> -		ret = add_update(rec, enable);
> -		if (ret)
> -			goto remove_breakpoints;
> +		new = ftrace_new_code(rec, enable);
> +
> +		/* check next record if there is no new code for this one */
> +		if (!new)
> +			continue;
> +
> +		/* allocate buffers only when there will be a change */
> +		if (unlikely(!addr)) {
> +			if (ftrace_allocate_replace_buffers(&addr, &opcode))
> +				goto out;
> +			op = opcode;
> +			count = 0;
> +		}
> +
> +		/* fill buffers */
> +		addr[count++] = rec->ip;
> +		memcpy(op, new, MCOUNT_INSN_SIZE);
> +		op += MCOUNT_INSN_SIZE;
> +
> +		/* write buffers if they are full */
> +		if (count == FTRACE_MAX_RECS_PATCH) {
> +			text_poke_bp((void **)addr, opcode,
> +				     MCOUNT_INSN_SIZE, count, NULL);
> +			op = opcode;
> +			count = 0;
> +		}
>  	}
>  
> -	run_sync();
> -
> -	report = "removing breakpoints";
> -
> -	for_ftrace_rec_iter(iter) {
> -		rec = ftrace_rec_iter_record(iter);
> +	if (count)
> +		text_poke_bp((void **)addr, opcode,
> +			     MCOUNT_INSN_SIZE, count, NULL);
>  
> -		ret = finish_update(rec, enable);
> -		if (ret)
> -			goto remove_breakpoints;
> -	}
> -
> -	run_sync();
> -
> -	return;
> -
> - remove_breakpoints:
> -	ftrace_bug(ret, rec ? rec->ip : 0);
> -	printk(KERN_WARNING "Failed on %s (%d):\n", report, count);
> -	for_ftrace_rec_iter(iter) {
> -		rec = ftrace_rec_iter_record(iter);
> -		remove_breakpoint(rec);

Also you must keep the ftrace_bug() functionality. I notice there's no
error check for text_poke_bp(). Not only do we need to check the error
status, but if an error does happen, we must know the following:

 1) What record it failed on
 2) Did in fail on the read, compare, or write?

If it failed on the read:     -EFAULT is returned
If it failed on the compare:  -EINVAL is returned
If it failed on the write:    -EPERM is returned

Look at "ftrace_bug()", and then you can also do a google search on
"ftrace faulted" or "ftrace failed to modify", and see that this is an
extremely useful debugging tool, and not something I will allow to be
removed.

-- Steve


> -	}
> +out:
> +	kfree(addr);
> +	kfree(opcode);
>  }
>  

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