netdev - Re: [PATCH v8 bpf-next 09/13] bpf: Add d

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:   Wed, 29 Jul 2020 21:11:17 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Jiri Olsa <jolsa@...nel.org>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, Song Liu <songliubraving@...com>,
        Yonghong Song <yhs@...com>, Martin KaFai Lau <kafai@...com>,
        David Miller <davem@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Wenbo Zhang <ethercflow@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Brendan Gregg <bgregg@...flix.com>,
        Florent Revest <revest@...omium.org>
Subject: Re: [PATCH v8 bpf-next 09/13] bpf: Add d_path helper

On Wed, Jul 22, 2020 at 11:12:19PM +0200, Jiri Olsa wrote:

> +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> +{
> +	char *p = d_path(path, buf, sz - 1);
> +	int len;
> +
> +	if (IS_ERR(p)) {
> +		len = PTR_ERR(p);
> +	} else {
> +		len = strlen(p);
> +		if (len && p != buf)
> +			memmove(buf, p, len);

*blink*
What the hell do you need that strlen() for?  d_path() copies into
the end of buffer (well, starts there and prepends to it); all you
really need is memmove(buf, p, buf + sz - p)

> +		buf[len] = 0;

Wait a minute...  Why are you NUL-terminating it separately?
You do rely upon having NUL in the damn thing (and d_path() does
guarantee it there).  Without that strlen() would've gone into
the nasal demon country; you can't call it on non-NUL-terminated
array.  So you are guaranteed that p[len] will be '\0'; why bother
copying the first len bytes and then separately deal with that
NUL?  Just memmove() the fucker and be done with that...

If you are worried about stray NUL in the middle of the returned
data... can't happen.  Note the rename_lock use in fs/d_path.c;
the names of everything involved are guaranteed to have been
stable throughout the copying them into the buffer - if anything
were to be renamed while we are doing that, we'd repeat the whole
thing (with rename_lock taken exclusive the second time around).

So make it simply
	if (IS_ERR(p))
		return PTR_ERR(p);
	len = buf + sz - p;
	memmove(buf, p, len);
	return len;
and be done with that.  BTW, the odds of p == buf are pretty much
nil - it would happen only if sz - 1 happened to be the exact length
of pathname.