linux-kernel - Re: [PATCH -tip v8 3/6] tracing/probe: Add ustring type for user-space string

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: <20190514191055.311470dbf67d3cc5ac64cdfb@kernel.org>
Date:   Tue, 14 May 2019 19:10:55 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Shuah Khan <shuah@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org,
        Andy Lutomirski <luto@...capital.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Changbin Du <changbin.du@...il.com>,
        Jann Horn <jannh@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Andy Lutomirski <luto@...nel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Nadav Amit <namit@...are.com>,
        Joel Fernandes <joel@...lfernandes.org>, yhs@...com
Subject: Re: [PATCH -tip v8 3/6] tracing/probe: Add ustring type for
 user-space string

On Tue, 14 May 2019 09:24:26 +0200
Ingo Molnar <mingo@...nel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@...nel.org> wrote:
> 
> > +/* Return the length of string -- including null terminal byte */
> > +static nokprobe_inline int
> > +fetch_store_strlen_user(unsigned long addr)
> > +{
> > +	return strnlen_unsafe_user((__force const void __user *)addr,
> > +				   MAX_STRING_SIZE);
> 
> Pointless line break that doesn't improve readability.

OK.

> 
> > +/*
> > + * Fetch a null-terminated string from user. Caller MUST set *(u32 *)buf
> > + * with max length and relative data location.
> > + */
> > +static nokprobe_inline int
> > +fetch_store_string_user(unsigned long addr, void *dest, void *base)
> > +{
> > +	const void __user *uaddr =  (__force const void __user *)addr;
> > +	int maxlen = get_loc_len(*(u32 *)dest);
> > +	u8 *dst = get_loc_data(dest, base);
> > +	long ret;
> > +
> > +	if (unlikely(!maxlen))
> > +		return -ENOMEM;
> > +	ret = strncpy_from_unsafe_user(dst, uaddr, maxlen);
> > +
> > +	if (ret >= 0)
> > +		*(u32 *)dest = make_data_loc(ret, (void *)dst - base);
> > +
> >  	return ret;
> 
> Firstly, why is there a 'dest' and a 'dst' variable name as well - the 
> two are very similar and the difference not explained at all.

Agreed. My bad habit, maybe '__dest' would better.

> Secondly, a style nit: if you group statements then please group 
> statements based on the usual logic - which is the group them by the flow 
> of logic. In the above case you grouped the 'maxlen' check with the 
> strncpy_from_unsafe_user() call, while the grouping should be the other 
> way around:
> 
> 	if (unlikely(!maxlen))
> 		return -ENOMEM;
> 
> 	ret = strncpy_from_unsafe_user(dst, uaddr, maxlen);
> 	if (ret >= 0)
> 		*(u32 *)dest = make_data_loc(ret, (void *)dst - base);
> 
> 	return ret;

OK.

> 
> Third, hiding the get_loc_data() call within variable initialization is 
> bad style - we usually only put 'trivial' (constant) initializations 
> there.

Hmm, it just decodes the location address from offset and start address.
Shouldn't it a trivial?
 
> Fourth, 'dst' is independent of 'maxlen', so it should probably 
> calculated *after* maxlen.

Ah, OK. I see what you pointed.

> 
> I.e. the whole sequence should be:
> 
> 
> 	maxlen = get_loc_len(*(u32 *)dest);
> 	if (unlikely(!maxlen))
> 		return -ENOMEM;
> 
> 	dst = get_loc_data(dest, base);

OK, in this case we can skip this conversion if maxlen == 0.

> 
> 	ret = strncpy_from_unsafe_user(dst, uaddr, maxlen);
> 	if (ret >= 0)
> 		*(u32 *)dest = make_data_loc(ret, (void *)dst - base);
> 
> 	return ret;
> 
> Fifth, we don't actually dereference 'dst', do we? So the whole type 
> casting to 'void *' could be avoided by declaring 'dst' (or whatever its 
> new, clearer name is) not as u8 *, but as void *.

OK, I'll use void* for that.

> 
> I.e. these are five problems in a short sequence of code, which it sad to 
> see in a v8 submission. :-/
> 
> Please review the other patches and the whole code base for similar 
> mishaps and small details as well.

OK, I'll update others too.

Thank you,

> 
> Thanks,
> 
> 	Ingo


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