On Wed, Mar 30, 2022 at 11:10 AM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> On Wed, Mar 30, 2022 at 11:09 AM Joshua Brindle
> <joshua.brindle@xxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Mar 30, 2022 at 10:51 AM James Carter <jwcart2@xxxxxxxxx> wrote:
> > >
> > > On Wed, Mar 30, 2022 at 9:55 AM Joshua Brindle
> > > <joshua.brindle@xxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Mar 14, 2022 at 2:24 PM James Carter <jwcart2@xxxxxxxxx> wrote:
> > > > >
> > > > > Use calloc() instead of mallocarray() so that everything is
> > > > > initialized to zero to prevent the use of unitialized memory when
> > > > > validating malformed binary policies.
> > > > >
> > > > > Found by oss-fuzz (#45493)
> > > > >
> > > > > Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> > > > > ---
> > > > > libsepol/src/conditional.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/libsepol/src/conditional.c b/libsepol/src/conditional.c
> > > > > index f78b38a2..a620451d 100644
> > > > > --- a/libsepol/src/conditional.c
> > > > > +++ b/libsepol/src/conditional.c
> > > > > @@ -522,7 +522,7 @@ int cond_init_bool_indexes(policydb_t * p)
> > > > > if (p->bool_val_to_struct)
> > > > > free(p->bool_val_to_struct);
> > > > > p->bool_val_to_struct = (cond_bool_datum_t **)
> > > > > - mallocarray(p->p_bools.nprim, sizeof(cond_bool_datum_t *));
> > > > > + calloc(p->p_bools.nprim, sizeof(cond_bool_datum_t *));
> > > > > if (!p->bool_val_to_struct)
> > > > > return -1;
> > > > > return 0;
> > > > > --
> > > > > 2.34.1
> > > >
> > > > Why not change the mallocarray macro to use calloc? I see a number of
> > > > mallocarray calls that should be audited if this approach is taken.
> > >
> > > Many of the calls to mallocarray() should be replaced by calloc()
> > > because the array is initialized to zero right after the mallocarray()
> > > call. I guess all of the calls can be replaced if you don't mind
> > > having the memory set to zero and then immediately setting the array
> > > to different values.
> > >
> > > I will merge this patch and send another patch replacing mallocarray()
> > > where appropriate.
> > >
> >
> > Hrm, I meant something like:
> >
> > diff --git a/libsepol/src/private.h b/libsepol/src/private.h
> > index a8cc1472..9a51fb34 100644
> > --- a/libsepol/src/private.h
> > +++ b/libsepol/src/private.h
> > @@ -90,7 +90,7 @@ static inline void* mallocarray(size_t nmemb, size_t size) {
> > return NULL;
> > }
> >
> > - return malloc(nmemb * size);
> > + return calloc(nmemb, size);
> > }
> >
> > #ifndef HAVE_REALLOCARRAY
> >
> >
> > Since mallocarray has additional length checking logic that you lose
> > if you just switch from mallocarray to calloc.
>
> Does it? The man page for calloc says that it will return an error if
> nmemb * size will overflow.
Alright, +1
