linux-kernel - Re: [PATCH] kprobes: Print an error if probe is rejected

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: <20210610191643.d24e7d56d102567070fe8386@kernel.org>
Date:   Thu, 10 Jun 2021 19:16:43 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
Cc:     <linux-kernel@...r.kernel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>
Subject: Re: [PATCH] kprobes: Print an error if probe is rejected

Hi Naveen,

On Thu, 10 Jun 2021 14:26:17 +0530
"Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> wrote:

> When probing at different locations in the kernel, it is not always
> evident if the location can be probed or not. As an example:
> 
>     $ perf probe __radix__flush_tlb_range:35
>     Failed to write event: Invalid argument
>       Error: Failed to add events.
> 
> The probed line above is:
>      35         if (!mmu_has_feature(MMU_FTR_GTSE) && type == FLUSH_TYPE_GLOBAL) {
> 
> This ends up trying to probe on BUILD_BUG_ON(), which is rejected.
> However, the user receives no indication at all as to why the probe
> failed. Print an error in such cases so that it is clear that the probe
> was rejected.

Hmm, Nack for this way, but I understand that is a problem.
If you got the error in perf probe, which uses ftrace dynamic-event interface.
In that case, the errors should not be output in the dmesg, but are reported
via error_log in tracefs.
And kprobes itself should return different error code to the caller, instead
of printing error in dmesg. See below.

[...]
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1542,8 +1542,10 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  	int ret;
>  
>  	ret = arch_check_ftrace_location(p);
> -	if (ret)
> +	if (ret) {
> +		pr_err("kprobes: can't probe at the provided ftrace location\n");
>  		return ret;

This must be -EBUSY. (or arch depend return value)

> +	}
>  	jump_label_lock();
>  	preempt_disable();
>  
> @@ -1552,6 +1554,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
>  	    within_kprobe_blacklist((unsigned long) p->addr) ||
>  	    jump_label_text_reserved(p->addr, p->addr) ||
>  	    find_bug((unsigned long)p->addr)) {
> +		pr_err("kprobes: can't probe at address in reject list\n");
>  		ret = -EINVAL;

This maybe -EACCESS.

>  		goto out;
>  	}
> @@ -1976,8 +1979,10 @@ int register_kretprobe(struct kretprobe *rp)
>  	void *addr;
>  
>  	ret = kprobe_on_func_entry(rp->kp.addr, rp->kp.symbol_name, rp->kp.offset);
> -	if (ret)
> +	if (ret) {
> +		pr_err("kretprobes: can't probe at address outside function entry\n");
>  		return ret;

return -ERANGE.

> +	}
>  
>  	/* If only rp->kp.addr is specified, check reregistering kprobes */
>  	if (rp->kp.addr && warn_kprobe_rereg(&rp->kp))
> @@ -1989,8 +1994,10 @@ int register_kretprobe(struct kretprobe *rp)
>  			return PTR_ERR(addr);
>  
>  		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
> -			if (kretprobe_blacklist[i].addr == addr)
> +			if (kretprobe_blacklist[i].addr == addr) {
> +				pr_err("kretprobes: can't probe at address in reject list\n");
>  				return -EINVAL;

return -EACCESS too.

Thank you,

> +			}
>  		}
>  	}
>  
> 
> base-commit: 2e38eb04c95e5546b71bb86ee699a891c7d212b5
> -- 
> 2.31.1
> 


-- 
Masami Hiramatsu <mhiramat@...nel.org>