| Message ID | 20221214010046.668024-1-toke@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] libbpf: Fix signedness confusion when using libbpf_is_mem_zeroed() | expand |
On Tue, Dec 13, 2022 at 5:01 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > The commit in the Fixes tag refactored the check for zeroed memory in > libbpf_validate_opts() into a separate libbpf_is_mem_zeroed() function. > This function has a 'len' argument of the signed 'ssize_t' type, which in > both callers is computed by subtracting two unsigned size_t values from > each other. In both subtractions, one of the values being subtracted is > converted to 'ssize_t', while the other stays 'size_t'. > > The problem with this is that, because both sizes are the same > rank ('ssize_t' is defined as 'long' and 'size_t' is 'unsigned long'), the > type of the mixed-sign arithmetic operation ends up being converted back to > unsigned. This means it can underflow if the user-specified size in > opts->sz is smaller than the size of the type as defined by libbpf. If that > happens, it will cause out-of-bounds reads in libbpf_is_mem_zeroed(). hmm... but libbpf_is_mem_zeroed expects signed ssize_t, so that "underflow" will turn into a proper negative ssize_t value. What am I missing? Seems to be working fine: $ cat test.c #include <stdio.h> void testit(ssize_t sz) { printf("%zd\n", sz); } int main() { ssize_t slarge = 100; size_t ularge = 100; ssize_t ssmall = 50; size_t usmall = 50; testit(ssmall - slarge); testit(ssmall - ularge); testit(usmall - slarge); testit(usmall - ularge); } $ cc test.c && ./a.out -50 -50 -50 -50 > > To fix this, change libbpf_is_mem_zeroed() to take unsigned start and end > offsets instead of a signed length. This avoids all casts between signed > and unsigned types and should hopefully prevent a similar error from > reappearing in the future. > > Fixes: 3ec84f4b1638 ("libbpf: Add bpf_cookie support to bpf_link_create() API") > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > tools/lib/bpf/libbpf_internal.h | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h > index 377642ff51fc..92375a86b15c 100644 > --- a/tools/lib/bpf/libbpf_internal.h > +++ b/tools/lib/bpf/libbpf_internal.h > @@ -267,13 +267,14 @@ void *libbpf_add_mem(void **data, size_t *cap_cnt, size_t elem_sz, > size_t cur_cnt, size_t max_cnt, size_t add_cnt); > int libbpf_ensure_mem(void **data, size_t *cap_cnt, size_t elem_sz, size_t need_cnt); > > -static inline bool libbpf_is_mem_zeroed(const char *p, ssize_t len) > +static inline bool libbpf_is_mem_zeroed(const char *obj, > + size_t off_start, size_t off_end) > { > - while (len > 0) { > + const char *p; > + > + for (p = obj + off_start; p < obj + off_end; p++) { > if (*p) > return false; > - p++; > - len--; > } > return true; > } > @@ -286,7 +287,7 @@ static inline bool libbpf_validate_opts(const char *opts, > pr_warn("%s size (%zu) is too small\n", type_name, user_sz); > return false; > } > - if (!libbpf_is_mem_zeroed(opts + opts_sz, (ssize_t)user_sz - opts_sz)) { > + if (!libbpf_is_mem_zeroed(opts, opts_sz, user_sz)) { > pr_warn("%s has non-zero extra bytes\n", type_name); > return false; > } > @@ -309,11 +310,10 @@ static inline bool libbpf_validate_opts(const char *opts, > } while (0) > > #define OPTS_ZEROED(opts, last_nonzero_field) \ > -({ \ > - ssize_t __off = offsetofend(typeof(*(opts)), last_nonzero_field); \ > - !(opts) || libbpf_is_mem_zeroed((const void *)opts + __off, \ > - (opts)->sz - __off); \ > -}) > + (!(opts) || libbpf_is_mem_zeroed((const void *)opts, \ > + offsetofend(typeof(*(opts)), \ > + last_nonzero_field), \ > + (opts)->sz)) > > enum kern_feature_id { > /* v4.14: kernel support for program & map names. */ > -- > 2.38.1 >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Tue, Dec 13, 2022 at 5:01 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> The commit in the Fixes tag refactored the check for zeroed memory in >> libbpf_validate_opts() into a separate libbpf_is_mem_zeroed() function. >> This function has a 'len' argument of the signed 'ssize_t' type, which in >> both callers is computed by subtracting two unsigned size_t values from >> each other. In both subtractions, one of the values being subtracted is >> converted to 'ssize_t', while the other stays 'size_t'. >> >> The problem with this is that, because both sizes are the same >> rank ('ssize_t' is defined as 'long' and 'size_t' is 'unsigned long'), the >> type of the mixed-sign arithmetic operation ends up being converted back to >> unsigned. This means it can underflow if the user-specified size in >> opts->sz is smaller than the size of the type as defined by libbpf. If that >> happens, it will cause out-of-bounds reads in libbpf_is_mem_zeroed(). > > hmm... but libbpf_is_mem_zeroed expects signed ssize_t, so that > "underflow" will turn into a proper negative ssize_t value. What am I > missing? Seems to be working fine: > > $ cat test.c > #include <stdio.h> > > void testit(ssize_t sz) > { > printf("%zd\n", sz); > } > > int main() > { > ssize_t slarge = 100; > size_t ularge = 100; > ssize_t ssmall = 50; > size_t usmall = 50; > > testit(ssmall - slarge); > testit(ssmall - ularge); > testit(usmall - slarge); > testit(usmall - ularge); > } > > $ cc test.c && ./a.out > -50 > -50 > -50 > -50 Hmnm, yeah, you're right. Not sure how I managed to convince myself there was an actual bug there :( Sorry for the noise! -Toke
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h index 377642ff51fc..92375a86b15c 100644 --- a/tools/lib/bpf/libbpf_internal.h +++ b/tools/lib/bpf/libbpf_internal.h @@ -267,13 +267,14 @@ void *libbpf_add_mem(void **data, size_t *cap_cnt, size_t elem_sz, size_t cur_cnt, size_t max_cnt, size_t add_cnt); int libbpf_ensure_mem(void **data, size_t *cap_cnt, size_t elem_sz, size_t need_cnt); -static inline bool libbpf_is_mem_zeroed(const char *p, ssize_t len) +static inline bool libbpf_is_mem_zeroed(const char *obj, + size_t off_start, size_t off_end) { - while (len > 0) { + const char *p; + + for (p = obj + off_start; p < obj + off_end; p++) { if (*p) return false; - p++; - len--; } return true; } @@ -286,7 +287,7 @@ static inline bool libbpf_validate_opts(const char *opts, pr_warn("%s size (%zu) is too small\n", type_name, user_sz); return false; } - if (!libbpf_is_mem_zeroed(opts + opts_sz, (ssize_t)user_sz - opts_sz)) { + if (!libbpf_is_mem_zeroed(opts, opts_sz, user_sz)) { pr_warn("%s has non-zero extra bytes\n", type_name); return false; } @@ -309,11 +310,10 @@ static inline bool libbpf_validate_opts(const char *opts, } while (0) #define OPTS_ZEROED(opts, last_nonzero_field) \ -({ \ - ssize_t __off = offsetofend(typeof(*(opts)), last_nonzero_field); \ - !(opts) || libbpf_is_mem_zeroed((const void *)opts + __off, \ - (opts)->sz - __off); \ -}) + (!(opts) || libbpf_is_mem_zeroed((const void *)opts, \ + offsetofend(typeof(*(opts)), \ + last_nonzero_field), \ + (opts)->sz)) enum kern_feature_id { /* v4.14: kernel support for program & map names. */
The commit in the Fixes tag refactored the check for zeroed memory in libbpf_validate_opts() into a separate libbpf_is_mem_zeroed() function. This function has a 'len' argument of the signed 'ssize_t' type, which in both callers is computed by subtracting two unsigned size_t values from each other. In both subtractions, one of the values being subtracted is converted to 'ssize_t', while the other stays 'size_t'. The problem with this is that, because both sizes are the same rank ('ssize_t' is defined as 'long' and 'size_t' is 'unsigned long'), the type of the mixed-sign arithmetic operation ends up being converted back to unsigned. This means it can underflow if the user-specified size in opts->sz is smaller than the size of the type as defined by libbpf. If that happens, it will cause out-of-bounds reads in libbpf_is_mem_zeroed(). To fix this, change libbpf_is_mem_zeroed() to take unsigned start and end offsets instead of a signed length. This avoids all casts between signed and unsigned types and should hopefully prevent a similar error from reappearing in the future. Fixes: 3ec84f4b1638 ("libbpf: Add bpf_cookie support to bpf_link_create() API") Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- tools/lib/bpf/libbpf_internal.h | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)