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]

Date:	Mon, 4 Nov 2013 12:17:06 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Namhyung Kim <namhyung@...nel.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 Mon, 4 Nov 2013 17:44:31 +0100
Oleg Nesterov <oleg@...hat.com> wrote:

> On 11/04, Namhyung Kim wrote:
> >
> > On Thu, 31 Oct 2013 19:22:18 +0100, Oleg Nesterov wrote:
> > > On 10/29, Namhyung Kim wrote:
> > >>
> > >> +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);
> > >
> > > Hmm. But unless I missed something this "addr" passed as an argument can
> > > be wrong? And if nothing else this or another thread can unmap the vma?
> >
> > You mean WARN_ON_ONCE here is superfluous?  I admit that it should
> > protect concurrent vma [un]mappings.  Please see my reply in other
> > thread for a new approach.
> 
> Whatever we do this address can be unmapped. For example, just because of
> @invalid_address passed to trace_uprobe.c.
> 
> We do not really care, copy_from_user() should fail. But we should not
> WARN() in this case.
> 

I agree, the WARN_ON_ONCE() above looks like it's uncalled for.
WARN()ings should only be used when an anomaly in the kernel logic is
detected. Can this trigger on bad input from user space, or something
else that userspace does? (a race with unmapping memory?). If so, error
out to the user process, but do not call any of the WARN() functions.

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