Re: [PATCH] uprop: Add new verification condition to verify_opcode

[Oleg Nesterov <oleg@redhat.com>]

Hi Penglei,

On 03/21, Penglei Jiang wrote:
>
> If, during probe registration, the instruction at the probe point
> differs between the file and memory (due to self-modifying code),
> and the probe registration succeeds, the following errors occur:

Thanks for the patch, but I'd say we do not care.

uprobes.c assumes that user-space should not modify the probed memory.

And I don't think your patch can really help. What if user-space
modifies the probed insn right after verify_opcode() succeeds?

Oleg.

>   1. When unregistering the probe, it restores the wrong original
>      instruction.
>      - If the instruction crosses a page boundary: it restores only
>        part of the instruction fetched from the file (the part saved
>        in the first page) to the probe point, and does not restore
>        the remaining part that crosses the page boundary, which can
>        lead to unknown instruction.
>      - If the instruction does not cross a page boundary: it restores
>        the instruction fetched from the file to the probe point.
> 
>   2. The code logic changes because the saved original instruction
>      is fetched from the file.
> 
> Fix:
> 
>   Add an extra check in the registration branch of verify_opcode.
>   If the instruction at the probe point differs between the file
>   and memory, the verification fails.
> 
> Signed-off-by: Penglei Jiang <superman.xpt@gmail.com>
> ---
>  kernel/events/uprobes.c | 47 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index b4ca8898fe17..580dd9fe4015 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -264,10 +264,46 @@ static void copy_to_page(struct page *page, unsigned long vaddr, const void *src
>  	kunmap_atomic(kaddr);
>  }
>  
> -static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode)
> +static int copy_insn_from_mem(struct page *p, unsigned long vaddr,
> +		struct arch_uprobe *dst, struct mm_struct *mm)
> +{
> +	pgoff_t offset = offset_in_page(vaddr);
> +	void *dst_insn = &dst->insn;
> +	size_t size = sizeof(dst->insn);
> +	size_t first_size;
> +	int err;
> +
> +	first_size = min_t(size_t, size, PAGE_SIZE - offset);
> +	copy_from_page(p, offset, dst_insn, first_size);
> +	vaddr += first_size;
> +	dst_insn += first_size;
> +	size -= first_size;
> +
> +	/*
> +	 * If the instruction spans the page boundary, continue copying
> +	 * the remaining instruction from the second page.
> +	 */
> +	if (size) {
> +		err = get_user_pages_remote(mm, vaddr, 1, FOLL_FORCE, &p, NULL);
> +		if (err < 0)
> +			return err;
> +		if (err == 0)
> +			return -EBUSY;
> +
> +		copy_from_page(p, 0, dst_insn, size);
> +		put_page(p);
> +	}
> +
> +	return 0;
> +}
> +
> +static int verify_opcode(struct page *page, unsigned long vaddr,
> +		uprobe_opcode_t *new_opcode, const struct arch_uprobe *auprobe,
> +		struct mm_struct *mm)
>  {
>  	uprobe_opcode_t old_opcode;
>  	bool is_swbp;
> +	struct arch_uprobe current_insn;
>  
>  	/*
>  	 * Note: We only check if the old_opcode is UPROBE_SWBP_INSN here.
> @@ -284,6 +320,13 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t
>  	if (is_swbp_insn(new_opcode)) {
>  		if (is_swbp)		/* register: already installed? */
>  			return 0;
> +
> +		if (copy_insn_from_mem(page, vaddr, &current_insn, mm))
> +			return 0;
> +					/* register: was it changed by self-modifying code? */
> +		if (memcmp(&current_insn.insn, &auprobe->insn,
> +			sizeof(current_insn.insn)))
> +			return 0;
>  	} else {
>  		if (!is_swbp)		/* unregister: was it changed by us? */
>  			return 0;
> @@ -491,7 +534,7 @@ int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  	if (IS_ERR(old_page))
>  		return PTR_ERR(old_page);
>  
> -	ret = verify_opcode(old_page, vaddr, &opcode);
> +	ret = verify_opcode(old_page, vaddr, &opcode, auprobe, mm);
>  	if (ret <= 0)
>  		goto put_old;
>  
> -- 
> 2.17.1
>