linux-kernel - Re: [PATCH 12/13] tracing/uprobes: Add more fetch functions

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: <20131101175334.GA27088@redhat.com>
Date:	Fri, 1 Nov 2013 18:53:34 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Namhyung Kim <namhyung@...nel.org>
Cc:	Steven Rostedt <rostedt@...dmis.org>,
	Namhyung Kim <namhyung.kim@....com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Hyeoncheol Lee <cheol.lee@....com>,
	Hemant Kumar <hkshaw@...ux.vnet.ibm.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	"zhangwei(Jovi)" <jovi.zhangwei@...wei.com>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>
Subject: Re: [PATCH 12/13] tracing/uprobes: Add more fetch functions

On 10/29, Namhyung Kim wrote:
>
> +static unsigned long get_user_stack_nth(struct pt_regs *regs, unsigned int n)
> +{
> +	struct vm_area_struct *vma;
> +	unsigned long addr = user_stack_pointer(regs);
> +	bool valid = false;
> +	unsigned long ret = 0;
> +
> +	down_read(&current->mm->mmap_sem);
> +	vma = find_vma(current->mm, addr);
> +	if (vma && vma->vm_start <= addr) {
> +		if (within_user_stack(vma, addr, n))
> +			valid = true;
> +	}
> +	up_read(&current->mm->mmap_sem);
> +
> +	addr = adjust_stack_addr(addr, n);
> +
> +	if (valid && copy_from_user(&ret, (void __force __user *)addr,
> +				    sizeof(ret)) == 0)
> +		return ret;
> +	return 0;
> +}

Namhyung, I am just curious, why do we need find_vma/within_user_stack?
copy_from_user() should fail or expand the stack. Yes, we can actually
look into the wrong vma, but do we really care?

> +static void __user *get_user_vaddr(unsigned long addr, struct trace_uprobe *tu)
> +{
> +	unsigned long pgoff = addr >> PAGE_SHIFT;
> +	struct vm_area_struct *vma;
> +	struct address_space *mapping;
> +	unsigned long vaddr = 0;
> +
> +	if (tu == NULL) {
> +		/* A NULL tu means that we already got the vaddr */
> +		return (void __force __user *) addr;
> +	}
> +
> +	mapping = tu->inode->i_mapping;
> +
> +	mutex_lock(&mapping->i_mmap_mutex);
> +	vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) {
> +		if (vma->vm_mm != current->mm)
> +			continue;
> +		if (!(vma->vm_flags & VM_READ))
> +			continue;
> +
> +		vaddr = offset_to_vaddr(vma, addr);
> +		break;
> +	}
> +	mutex_unlock(&mapping->i_mmap_mutex);
> +
> +	WARN_ON_ONCE(vaddr == 0);
> +	return (void __force __user *) vaddr;

So. If I understand correctly, @addr cat only read the memory mmaped
to the probed binary, and we need to translate the address... And in
general we can't read the data from bss.

Right?

I'll probably ask another question about this later...

> +static __kprobes void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs,
> +					void *addr, void *dest, void *priv)
> +{
> +	long ret;
> +	u32 rloc = *(u32 *)dest;
> +	int maxlen = get_rloc_len(rloc);
> +	u8 *dst = get_rloc_data(dest);
> +	void __user *vaddr = get_user_vaddr((unsigned long)addr, priv);
> +	void __user *src = vaddr;
> +
> +	if (!maxlen)
> +		return;
> +
> +	do {
> +		ret = copy_from_user(dst, src, sizeof(*dst));
> +		dst++;
> +		src++;
> +	} while (dst[-1] && ret == 0 && (src - vaddr) < maxlen);

Can't we use strncpy_from_user() ?

> +static __kprobes void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs,
> +					   void *addr, void *dest, void *priv)
> +{
> +	int ret, len = 0;
> +	u8 c;
> +	void __user *vaddr = get_user_vaddr((unsigned long)addr, priv);
> +
> +	do {
> +		ret = __copy_from_user_inatomic(&c, vaddr + len, 1);

Hmm. I guess I need to actually apply this series ;)

Why inatomic? it seems that this is for uprobes, no? And perhaps
strnlen_user() should work just fine?

Oleg.

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