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  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(-)
>>>
>>
> 

Powered by blists - more mailing lists