Out-of-bound read suspected in libbpf.c

Commit Message

Nandakumar Edamana Feb. 19, 2025, 3:13 p.m. UTC

I think I've come across an out-of-bound read in libbpf.c that could 
result in
potential information leak (or crash). I wanted to test it myself, but
unfortunately it will take a while for me to set up things. So, let me just
describe it and submit a candidate patch.

Codebase at: commit 6537cfb395f352782918d8ee7b7f10ba2cc3cbf2 (HEAD -> 
master, origin/master, origin/HEAD)
File: ./tools/lib/bpf/libbpf.c
Function: set_kcfg_value_str

The first subscripted read of `value` (which is untrusted, IIUC) at line 
2108
(`value[len - 1]`) is safe because `set_kcfg_value_str` is called only after
making sure `value` is at least one character long (although not a good 
practice
in long-term). The problem is at the strip-quotes part. Here, `len -= 2` is
performed, possibly thinking `value` is at least two characters long, 
since the
caller makes sure `value` has an opening quote and this function
(`set_kcfg_value_str) has a check `if (value[len - 1] != '"')` for the 
closing
quote. But the problem is, both the opening and closing quotes could be the
same. More specifically, `value` could be a string that consists of a single
quote alone, and it could pass all these tests. Then `len` underflows, 
resulting
in a wrap-around since it is of type `size_t`. The next branch
`if (len >= ext->kcfg.sz)` will possibly reduce the value of `len`, but will
still be greater than `strlen(value)`, making `memcpy()` at 2122 read
out-of-bound.

Suggested change:

                 return -EINVAL;

Comments

Eduard Zingerman Feb. 20, 2025, 9:20 p.m. UTC | #1

On Wed, 2025-02-19 at 20:43 +0530, Nandakumar Edamana wrote:

[...]

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 194809da5172..1cc87dbd015d 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2106,7 +2106,7 @@ static int set_kcfg_value_str(struct extern_desc 
> *ext, char *ext_val,
>          }
> 
>          len = strlen(value);
> -       if (value[len - 1] != '"') {
> +       if (len < 2 || value[len - 1] != '"') {

Makes sense to me, could you please send a formal patch?

>                  pr_warn("extern (kcfg) '%s': invalid string config '%s'\n",
>                          ext->name, value);
>                  return -EINVAL;
>

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 194809da5172..1cc87dbd015d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2106,7 +2106,7 @@  static int set_kcfg_value_str(struct extern_desc 
*ext, char *ext_val,
         }

         len = strlen(value);
-       if (value[len - 1] != '"') {
+       if (len < 2 || value[len - 1] != '"') {
                 pr_warn("extern (kcfg) '%s': invalid string config '%s'\n",
                         ext->name, value);