netdev - Re: [PATCH iproute2 v3 0/2] bpf: memory access fixes

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:   Sat, 23 May 2020 03:33:47 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jamal Hadi Salim <jhs@...atatu.com>, stephen@...workplumber.org
Cc:     netdev@...r.kernel.org, dsahern@...il.com, aclaudi@...hat.com,
        asmadeus@...ewreck.org
Subject: Re: [PATCH iproute2 v3 0/2] bpf: memory access fixes

On 5/18/20 3:00 PM, Jamal Hadi Salim wrote:
> ping?
> 
> Note: these are trivial bug fixes.

Looking at c0325b06382c ("bpf: replace snprintf with asprintf when dealing with long buffers"),
I wonder whether it's best to just revert and redo cleanly from scratch.. How much testing has
been performed on the original patch? We know it is causing regressions, and looking Jamal's
2nd patch we do have patterns all over the place wrt error path that go like:

   +	char *file = NULL;
   +	char buff[4096];
  	FILE *fp;
   +	int ret;

   -	snprintf(file, sizeof(file), "/proc/%d/fdinfo/%d", getpid(), fd);
   +	ret = asprintf(&file, "/proc/%d/fdinfo/%d", getpid(), fd);
   +	if (ret < 0) {
   +		fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
   +		free(file);
   +		return ret;
   +	}

The man page on asprintf(char **strp, ...) says: "When successful, these functions return
the number of bytes printed, just like sprintf(3). If memory allocation wasn't possible,
or some other error occurs, these functions will return -1, and the contents of strp are
undefined." What is the rationale that are we passing it to free() /everywhere/ in error
path when the API spec does say it's undefined? It may happen to work but file's value
could just as well be, say, 42 ...

Thanks,
Daniel

> cheers,
> jamal
> 
> On 2020-04-28 12:15 p.m., Jamal Hadi Salim wrote:
>> Stephen,
>> What happened to this?
>>
>> cheers,
>> jamal
>>
>> On 2020-04-23 1:58 p.m., Jamal Hadi Salim wrote:
>>> From: Jamal Hadi Salim <jhs@...atatu.com>
>>>
>>> Changes from V2:
>>>   1) Dont initialize tmp on stack (Stephen)
>>>   2) Dont look at the return code of snprintf (Dominique)
>>>   3) Set errno to EINVAL instead of returning -EINVAL for consistency (Dominique)
>>>
>>> Changes from V1:
>>>   1) use snprintf instead of sprintf and fix corresponding error message.
>>>   Caught-by: Dominique Martinet <asmadeus@...ewreck.org>
>>>   2) Fix memory leak and extraneous free() in error path
>>>
>>> Jamal Hadi Salim (2):
>>>    bpf: Fix segfault when custom pinning is used
>>>    bpf: Fix mem leak and extraneous free() in error path
>>>
>>>   lib/bpf.c | 17 +++++++----------
>>>   1 file changed, 7 insertions(+), 10 deletions(-)
>>>
>>
>