| Message ID | 20211013125358.15534-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/3] libsepol: do not pass NULL to memcpy | expand |
On Wed, Oct 13, 2021 at 2:54 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > For the first iteration `mod->perm_map[sclassi]` is NULL, thus do not > use it as source of a memcpy(3), even with a size of 0. memcpy(3) might > be annotated with the function attribute nonnull and UBSan then > complains: > > link.c:193:3: runtime error: null pointer passed as argument 2, which is declared to never be null > > Use a realloc + memset instead of a calloc and free to increase the size > of `mod->perm_map[sclassi]`. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > libsepol/src/link.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/libsepol/src/link.c b/libsepol/src/link.c > index 7512a4d9..75ce2b20 100644 > --- a/libsepol/src/link.c > +++ b/libsepol/src/link.c > @@ -185,14 +185,12 @@ static int permission_copy_callback(hashtab_key_t key, hashtab_datum_t datum, > * may have originated from the class -or- it could be from > * the class's common parent.*/ > if (perm->s.value > mod->perm_map_len[sclassi]) { > - uint32_t *newmap = calloc(perm->s.value, sizeof(*newmap)); > + uint32_t *newmap = realloc(mod->perm_map[sclassi], perm->s.value * sizeof(*newmap)); Hello, It seems sad to transform a calloc call to an explicit multiplication which can overflow. Nowadays glibc provides reallocarray, for a "realloc with multiplication overflow checking". Could you use it here instead? I saw in another patch ([RFC PATCH 09/35] libsepol: use reallocarray wrapper to avoid overflows ; https://lore.kernel.org/selinux/20211011162533.53404-10-cgzones@googlemail.com/T/#u) that you introduced reallocarray calls in other places, with a fallback implementation for systems which lack it. When I saw it, I was wondering: which C library does not provide this function? - glibc has it since version 2.29 (according to https://man7.org/linux/man-pages/man3/realloc.3.html ; this version was released on 2019-02-01) - musl has it since version 1.2.2 (thanks to https://git.musl-libc.org/cgit/musl/commit/?id=821083ac7b54eaa040d5a8ddc67c6206a175e0ca ; released on 2021-01-15) - bionic has it since 2018 (https://android.googlesource.com/platform/bionic/+/b177085ce7219562eecf77f2e8de49f8f2605005%5E%21/) - newlib has it since 2017 (https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=eb14d0cc649978c10928bf38c1847f295bf0de69) However uclic-ng 1.0.39 (released ten days ago) does not provide reallocarray. It is the only maintained and open-source C library compatible with Linux that I could find which would not provide reallocarray. Did I miss a library which is likely to be used by libsepol users? In my humble opinion, this means that for future releases we can consider that systems are expected to provide reallocarray, and that we can introduce a Makefile variable which would introduce a custom internal reallocarray implementation in libsepol. This means that I suggest using something like this in libsepol/src/Makefile: USE_INTERNAL_REALLOCARRAY ?= n ifeq (USE_INTERNAL_REALLOCARRAY, y) CFLAGS += -DUSE_INTERNAL_REALLOCARRAY endif and I suggest putting the internal reallocarray implementation from your patch in libsepol/src/private.h (like you did), around "#ifdef USE_INTERNAL_REALLOCARRAY" statements. This way, libsepol would work out-of-the-box on most systems, would be compatible on systems without reallocarray by using "make USE_INTERNAL_REALLOCARRAY=y", and would not try to auto-detect it at build-time by some compiler magic in Makefile (which would avoid issues similar as the one libapparmor had in http://lists.busybox.net/pipermail/buildroot/2020-October/294691.html). What do you think of these suggestions? Thanks, Nicolas > if (newmap == NULL) { > ERR(state->handle, "Out of memory!"); > return -1; > } > - memcpy(newmap, mod->perm_map[sclassi], > - mod->perm_map_len[sclassi] * sizeof(*newmap)); > - free(mod->perm_map[sclassi]); > + memset(newmap + mod->perm_map_len[sclassi], '\0', perm->s.value - mod->perm_map_len[sclassi]); > mod->perm_map[sclassi] = newmap; > mod->perm_map_len[sclassi] = perm->s.value; > } > -- > 2.33.0 >
On Sat, 16 Oct 2021 at 21:30, Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > On Wed, Oct 13, 2021 at 2:54 PM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > For the first iteration `mod->perm_map[sclassi]` is NULL, thus do not > > use it as source of a memcpy(3), even with a size of 0. memcpy(3) might > > be annotated with the function attribute nonnull and UBSan then > > complains: > > > > link.c:193:3: runtime error: null pointer passed as argument 2, which is declared to never be null > > > > Use a realloc + memset instead of a calloc and free to increase the size > > of `mod->perm_map[sclassi]`. > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > libsepol/src/link.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/libsepol/src/link.c b/libsepol/src/link.c > > index 7512a4d9..75ce2b20 100644 > > --- a/libsepol/src/link.c > > +++ b/libsepol/src/link.c > > @@ -185,14 +185,12 @@ static int permission_copy_callback(hashtab_key_t key, hashtab_datum_t datum, > > * may have originated from the class -or- it could be from > > * the class's common parent.*/ > > if (perm->s.value > mod->perm_map_len[sclassi]) { > > - uint32_t *newmap = calloc(perm->s.value, sizeof(*newmap)); > > + uint32_t *newmap = realloc(mod->perm_map[sclassi], perm->s.value * sizeof(*newmap)); > > Hello, > It seems sad to transform a calloc call to an explicit multiplication > which can overflow. Nowadays glibc provides reallocarray, for a > "realloc with multiplication overflow checking". Could you use it here > instead? > Yes, I thought about that, but I wanted this diff to be as small as possible. One option would be to wait until reallocarray is available (via fallback or requirement (see later)) or keep the calloc+memcpy, but not invoke the memset on a size of 0. > I saw in another patch ([RFC PATCH 09/35] libsepol: use reallocarray > wrapper to avoid overflows ; > https://lore.kernel.org/selinux/20211011162533.53404-10-cgzones@googlemail.com/T/#u) > that you introduced reallocarray calls in other places, with a > fallback implementation for systems which lack it. When I saw it, I > was wondering: which C library does not provide this function? > > - glibc has it since version 2.29 (according to > https://man7.org/linux/man-pages/man3/realloc.3.html ; this version > was released on 2019-02-01) > - musl has it since version 1.2.2 (thanks to > https://git.musl-libc.org/cgit/musl/commit/?id=821083ac7b54eaa040d5a8ddc67c6206a175e0ca > ; released on 2021-01-15) > - bionic has it since 2018 > (https://android.googlesource.com/platform/bionic/+/b177085ce7219562eecf77f2e8de49f8f2605005%5E%21/) > - newlib has it since 2017 > (https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=eb14d0cc649978c10928bf38c1847f295bf0de69) > > However uclic-ng 1.0.39 (released ten days ago) does not provide > reallocarray. It is the only maintained and open-source C library > compatible with Linux that I could find which would not provide > reallocarray. Did I miss a library which is likely to be used by > libsepol users? > > In my humble opinion, this means that for future releases we can > consider that systems are expected to provide reallocarray, and that > we can introduce a Makefile variable which would introduce a custom > internal reallocarray implementation in libsepol. This means that I > suggest using something like this in libsepol/src/Makefile: > > USE_INTERNAL_REALLOCARRAY ?= n > ifeq (USE_INTERNAL_REALLOCARRAY, y) > CFLAGS += -DUSE_INTERNAL_REALLOCARRAY > endif > > and I suggest putting the internal reallocarray implementation from > your patch in libsepol/src/private.h (like you did), around "#ifdef > USE_INTERNAL_REALLOCARRAY" statements. This way, libsepol would work > out-of-the-box on most systems, would be compatible on systems without > reallocarray by using "make USE_INTERNAL_REALLOCARRAY=y", and would > not try to auto-detect it at build-time by some compiler magic in > Makefile (which would avoid issues similar as the one libapparmor had > in http://lists.busybox.net/pipermail/buildroot/2020-October/294691.html). > What do you think of these suggestions? > Looking for example at https://github.com/SELinuxProject/selinux/commit/4859b738136ef8889fbb71a166cfec8d313ba4e0 (supporting gcc 4.8) I am not sure what compilers and standard C libraries should be supported for building the SELinux userland libraries and tools. Until any statement of the maintainer team, I prefer the Makefile hack from https://lore.kernel.org/selinux/20211011162533.53404-10-cgzones@googlemail.com/T/#u to support standard C libraries without reallocarray(3). > Thanks, > Nicolas > > > if (newmap == NULL) { > > ERR(state->handle, "Out of memory!"); > > return -1; > > } > > - memcpy(newmap, mod->perm_map[sclassi], > > - mod->perm_map_len[sclassi] * sizeof(*newmap)); > > - free(mod->perm_map[sclassi]); > > + memset(newmap + mod->perm_map_len[sclassi], '\0', perm->s.value - mod->perm_map_len[sclassi]); > > mod->perm_map[sclassi] = newmap; > > mod->perm_map_len[sclassi] = perm->s.value; > > } > > -- > > 2.33.0 > > >
diff --git a/libsepol/src/link.c b/libsepol/src/link.c index 7512a4d9..75ce2b20 100644 --- a/libsepol/src/link.c +++ b/libsepol/src/link.c @@ -185,14 +185,12 @@ static int permission_copy_callback(hashtab_key_t key, hashtab_datum_t datum, * may have originated from the class -or- it could be from * the class's common parent.*/ if (perm->s.value > mod->perm_map_len[sclassi]) { - uint32_t *newmap = calloc(perm->s.value, sizeof(*newmap)); + uint32_t *newmap = realloc(mod->perm_map[sclassi], perm->s.value * sizeof(*newmap)); if (newmap == NULL) { ERR(state->handle, "Out of memory!"); return -1; } - memcpy(newmap, mod->perm_map[sclassi], - mod->perm_map_len[sclassi] * sizeof(*newmap)); - free(mod->perm_map[sclassi]); + memset(newmap + mod->perm_map_len[sclassi], '\0', perm->s.value - mod->perm_map_len[sclassi]); mod->perm_map[sclassi] = newmap; mod->perm_map_len[sclassi] = perm->s.value; }
For the first iteration `mod->perm_map[sclassi]` is NULL, thus do not use it as source of a memcpy(3), even with a size of 0. memcpy(3) might be annotated with the function attribute nonnull and UBSan then complains: link.c:193:3: runtime error: null pointer passed as argument 2, which is declared to never be null Use a realloc + memset instead of a calloc and free to increase the size of `mod->perm_map[sclassi]`. Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- libsepol/src/link.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)