| 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
| ||
|
Message-ID: <20190809153210.GD2820@mini-arch>
Date: Fri, 9 Aug 2019 08:32:10 -0700
From: Stanislav Fomichev <sdf@...ichev.me>
To: Peter Wu <peter@...ensteyn.nl>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
Stanislav Fomichev <sdf@...gle.com>,
Jakub Kicinski <jakub.kicinski@...ronome.com>,
Quentin Monnet <quentin.monnet@...ronome.com>
Subject: Re: [PATCH v3] tools: bpftool: fix reading from /proc/config.gz
On 08/09, Peter Wu wrote:
> /proc/config has never existed as far as I can see, but /proc/config.gz
> is present on Arch Linux. Add support for decompressing config.gz using
> zlib which is a mandatory dependency of libelf. Replace existing stdio
> functions with gzFile operations since the latter transparently handles
> uncompressed and gzip-compressed files.
>
> Cc: Quentin Monnet <quentin.monnet@...ronome.com>
> Signed-off-by: Peter Wu <peter@...ensteyn.nl>
> ---
> v3: replace popen(gunzip) by linking directly to zlib. Reword commit
> message, remove "Fixes" line. (this patch)
> v2: fix style (reorder vars as reverse xmas tree, rename function,
> braces), fallback to /proc/config.gz if uname() fails.
> https://lkml.kernel.org/r/20190806010702.3303-1-peter@lekensteyn.nl
> v1: https://lkml.kernel.org/r/20190805001541.8096-1-peter@lekensteyn.nl
>
> Hi,
>
> Thanks to Jakub for observing that zlib is already used by libelf, this
> simplifies the patch tremendously as the same API can be used for both
> compressed and uncompressed files. No special case exists anymore for
> fclose/pclose.
>
> According to configure.ac in elfutils, zlib is mandatory, so I just
> assume it to be available. For simplicity I also silently assume lines
> to be less than 4096 characters. If that is not the case, then lines
> will appear truncated, but that should not be an issue for the
> CONFIG_xyz lines that we are scanning for.
>
> Jakub requested the handle leak fix to be posted separately against the
> bpf tree, but since the whole code is rewritten I am not sure if it is
> worth it. It is an unusual edge case: /boot/config-$(uname -r) could be
> opened, but starts with unexpected data.
>
> Kind regards,
> Peter
> ---
> tools/bpf/bpftool/Makefile | 2 +-
> tools/bpf/bpftool/feature.c | 105 ++++++++++++++++++------------------
> 2 files changed, 54 insertions(+), 53 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index a7afea4dec47..078bd0dcfba5 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -52,7 +52,7 @@ ifneq ($(EXTRA_LDFLAGS),)
> LDFLAGS += $(EXTRA_LDFLAGS)
> endif
>
> -LIBS = -lelf $(LIBBPF)
> +LIBS = -lelf -lz $(LIBBPF)
You're saying in the commit description that bpftool already links
against -lz (via -lelf), but then explicitly add -lz here, why?
> INSTALL ?= install
> RM ?= rm -f
> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> index d672d9086fff..03bdc5b3ac49 100644
> --- a/tools/bpf/bpftool/feature.c
> +++ b/tools/bpf/bpftool/feature.c
> @@ -14,6 +14,7 @@
>
> #include <bpf.h>
> #include <libbpf.h>
> +#include <zlib.h>
>
> #include "main.h"
>
> @@ -284,34 +285,32 @@ static void probe_jit_limit(void)
> }
> }
>
> -static char *get_kernel_config_option(FILE *fd, const char *option)
> +static bool read_next_kernel_config_option(gzFile file, char *buf, size_t n,
> + char **value)
> {
> - size_t line_n = 0, optlen = strlen(option);
> - char *res, *strval, *line = NULL;
> - ssize_t n;
> + char *sep;
>
> - rewind(fd);
> - while ((n = getline(&line, &line_n, fd)) > 0) {
> - if (strncmp(line, option, optlen))
> + while (gzgets(file, buf, n)) {
> + if (strncmp(buf, "CONFIG_", 7))
> continue;
> - /* Check we have at least '=', value, and '\n' */
> - if (strlen(line) < optlen + 3)
> - continue;
> - if (*(line + optlen) != '=')
> +
> + sep = strchr(buf, '=');
> + if (!sep)
> continue;
>
> /* Trim ending '\n' */
> - line[strlen(line) - 1] = '\0';
> + buf[strlen(buf) - 1] = '\0';
> +
> + /* Split on '=' and ensure that a value is present. */
> + *sep = '\0';
> + if (!sep[1])
> + continue;
>
> - /* Copy and return config option value */
> - strval = line + optlen + 1;
> - res = strdup(strval);
> - free(line);
> - return res;
> + *value = sep + 1;
> + return true;
> }
> - free(line);
>
> - return NULL;
> + return false;
> }
>
> static void probe_kernel_image_config(void)
> @@ -386,59 +385,61 @@ static void probe_kernel_image_config(void)
> /* test_bpf module for BPF tests */
> "CONFIG_TEST_BPF",
> };
> - char *value, *buf = NULL;
> + char *values[ARRAY_SIZE(options)] = { };
> struct utsname utsn;
> char path[PATH_MAX];
> - size_t i, n;
> - ssize_t ret;
> - FILE *fd;
> + gzFile file = NULL;
> + char buf[4096];
> + char *value;
> + size_t i;
>
> - if (uname(&utsn))
> - goto no_config;
> + if (!uname(&utsn)) {
> + snprintf(path, sizeof(path), "/boot/config-%s", utsn.release);
>
> - snprintf(path, sizeof(path), "/boot/config-%s", utsn.release);
> + /* gzopen also accepts uncompressed files. */
> + file = gzopen(path, "r");
> + }
>
> - fd = fopen(path, "r");
> - if (!fd && errno == ENOENT) {
> - /* Some distributions put the config file at /proc/config, give
> - * it a try.
> - * Sometimes it is also at /proc/config.gz but we do not try
> - * this one for now, it would require linking against libz.
> + if (!file) {
> + /* Some distributions build with CONFIG_IKCONFIG=y and put the
> + * config file at /proc/config.gz.
> */
> - fd = fopen("/proc/config", "r");
> + file = gzopen("/proc/config.gz", "r");
> }
> - if (!fd) {
> + if (!file) {
> p_info("skipping kernel config, can't open file: %s",
> strerror(errno));
> - goto no_config;
> + goto end_parse;
> }
> /* Sanity checks */
> - ret = getline(&buf, &n, fd);
> - ret = getline(&buf, &n, fd);
> - if (!buf || !ret) {
> + if (!gzgets(file, buf, sizeof(buf)) ||
> + !gzgets(file, buf, sizeof(buf))) {
> p_info("skipping kernel config, can't read from file: %s",
> strerror(errno));
> - free(buf);
> - goto no_config;
> + goto end_parse;
> }
> if (strcmp(buf, "# Automatically generated file; DO NOT EDIT.\n")) {
> p_info("skipping kernel config, can't find correct file");
> - free(buf);
> - goto no_config;
> + goto end_parse;
> }
> - free(buf);
>
> - for (i = 0; i < ARRAY_SIZE(options); i++) {
> - value = get_kernel_config_option(fd, options[i]);
> - print_kernel_option(options[i], value);
> - free(value);
> + while (read_next_kernel_config_option(file, buf, sizeof(buf), &value)) {
> + for (i = 0; i < ARRAY_SIZE(options); i++) {
> + if (values[i] || strcmp(buf, options[i]))
> + continue;
> +
> + values[i] = strdup(value);
> + }
> }
> - fclose(fd);
> - return;
>
> -no_config:
> - for (i = 0; i < ARRAY_SIZE(options); i++)
> - print_kernel_option(options[i], NULL);
> +end_parse:
> + if (file)
> + gzclose(file);
> +
> + for (i = 0; i < ARRAY_SIZE(options); i++) {
> + print_kernel_option(options[i], values[i]);
> + free(values[i]);
> + }
> }
>
> static bool probe_bpf_syscall(const char *define_prefix)
> --
> 2.22.0
>
Powered by blists - more mailing lists