On Wed, Oct 13, 2021 at 2:54 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxx/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
>
