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....@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, ¤t_insn, mm))
> + return 0;
> + /* register: was it changed by
> self-modifying code? */
> + if (memcmp(¤t_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
>
