netdev - Re: [iproute PATCH 51/51] lib/bpf: Check return value of write()

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, 14 Aug 2017 19:25:26 +0200
From:   Phil Sutter <phil@....cc>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Stephen Hemminger <stephen@...workplumber.org>,
        netdev@...r.kernel.org
Subject: Re: [iproute PATCH 51/51] lib/bpf: Check return value of write()

On Mon, Aug 14, 2017 at 11:17:39AM +0200, Daniel Borkmann wrote:
> On 08/12/2017 02:05 PM, Phil Sutter wrote:
> > This is merely to silence the compiler warning. If write to stderr
> > failed, assume that printing an error message will fail as well so don't
> > even try.
> >
> > Signed-off-by: Phil Sutter <phil@....cc>
> > ---
> >   lib/bpf.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/bpf.c b/lib/bpf.c
> > index 1dcb261dc915f..825e071cea572 100644
> > --- a/lib/bpf.c
> > +++ b/lib/bpf.c
> > @@ -591,7 +591,8 @@ int bpf_trace_pipe(void)
> >
> >   		ret = read(fd, buff, sizeof(buff) - 1);
> >   		if (ret > 0) {
> > -			write(2, buff, ret);
> > +			if (write(STDERR_FILENO, buff, ret) != ret)
> > +				return -1;
> 
> Quite unlikely to fail, but we should probably bark loudly
> here instead of just returning -1. Perhaps assert() would
> suit better.

Well, according to assert(3), it will print to stderr before aborting
the program. So if writing STDERR_FILENO failed, I guess it won't
provide much more detail to the user, either. If bpf_trace_pipe()
returns non-zero, parse_bpf() prints an error message to stderr and
returns -1. Ultimately tc will return non-zero. With stderr unfit for
writing into, I doubt there's anything left we could do besides that.

But I really think we shouldn't make such a fuss about it - writing to
stderr either always works or we're in trouble everywhere. This patch
was merely to shut gcc up, so no need to waste much energy on a scenario
which won't happen anyway.

Thanks, Phil