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] [day] [month] [year] [list]

Date:   Thu, 30 Jul 2020 12:22:52 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Jiri Olsa <jolsa@...nel.org>, 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 29, 2020 at 09:11:17PM +0100, Al Viro wrote:
> 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)

I used the code from some of the other users like
  backing_dev_show
  fsg_show_file

nice, looks like we could omit strlen call in perf mmap event call as well

> 
> 
> > +		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;

ok, will use this

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

ok, great

thanks,
jirka