On Sat, 16 Oct 2021 at 21:30, Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
>
> 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?
>
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@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?
>
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@xxxxxxxxxxxxxx/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
> >
>
