| Message ID | 20220314182400.121510-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dfc652f01ea2 |
| Headers | show |
| Series | libsepol: Use calloc when initializing bool_val_to_struct array | expand |
On Mon, 14 Mar 2022 at 19:24, James Carter <jwcart2@gmail.com> 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@gmail.com> > --- > 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 > Can this be merged? I think it might hurt the fuzzer, e.g. cause the flakiness in issue #45327. On a technical note: In src/policydb.c::policydb_index_others() the return value of cond_init_bool_indexes() is not checked. diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c index fc71463e..e29cbd51 100644 --- a/libsepol/src/policydb.c +++ b/libsepol/src/policydb.c @@ -1252,7 +1252,8 @@ int policydb_index_others(sepol_handle_t * handle, if (!p->type_val_to_struct) return -1; - cond_init_bool_indexes(p); + if (cond_init_bool_indexes(p) == -1) + return -1; for (i = SYM_ROLES; i < SYM_NUM; i++) { free(p->sym_val_to_name[i]);
On Mon, Mar 14, 2022 at 2:24 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > --- > 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.
On Wed, Mar 30, 2022 at 9:55 AM Joshua Brindle <joshua.brindle@crunchydata.com> wrote: > > On Mon, Mar 14, 2022 at 2:24 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > > --- > > 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. Jim Jim
On Wed, Mar 30, 2022 at 10:51 AM James Carter <jwcart2@gmail.com> wrote: > > On Wed, Mar 30, 2022 at 9:55 AM Joshua Brindle > <joshua.brindle@crunchydata.com> wrote: > > > > On Mon, Mar 14, 2022 at 2:24 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > > > --- > > > 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.
On Wed, Mar 30, 2022 at 11:09 AM Joshua Brindle <joshua.brindle@crunchydata.com> wrote: > > On Wed, Mar 30, 2022 at 10:51 AM James Carter <jwcart2@gmail.com> wrote: > > > > On Wed, Mar 30, 2022 at 9:55 AM Joshua Brindle > > <joshua.brindle@crunchydata.com> wrote: > > > > > > On Mon, Mar 14, 2022 at 2:24 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > > > > --- > > > > 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. Jim
On Wed, Mar 30, 2022 at 11:10 AM James Carter <jwcart2@gmail.com> wrote: > > On Wed, Mar 30, 2022 at 11:09 AM Joshua Brindle > <joshua.brindle@crunchydata.com> wrote: > > > > On Wed, Mar 30, 2022 at 10:51 AM James Carter <jwcart2@gmail.com> wrote: > > > > > > On Wed, Mar 30, 2022 at 9:55 AM Joshua Brindle > > > <joshua.brindle@crunchydata.com> wrote: > > > > > > > > On Mon, Mar 14, 2022 at 2:24 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > > > > > --- > > > > > 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
On Wed, Mar 30, 2022 at 10:46 AM Joshua Brindle <joshua.brindle@crunchydata.com> wrote: > > On Mon, Mar 14, 2022 at 2:24 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > > --- > > 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. +1, it also gets rid of initialization code like this (note the loop setting to NULL): int sepol_sidtab_init(sidtab_t * s) { int i; s->htable = mallocarray(SIDTAB_SIZE, sizeof(sidtab_ptr_t)); if (!s->htable) return -ENOMEM; for (i = 0; i < SIDTAB_SIZE; i++) s->htable[i] = (sidtab_ptr_t) NULL; s->nel = 0; s->next_sid = 1; s->shutdown = 0; INIT_SIDTAB_LOCK(s); return 0; }
On Tue, Mar 29, 2022 at 3:43 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > On Mon, 14 Mar 2022 at 19:24, James Carter <jwcart2@gmail.com> 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@gmail.com> > > --- > > 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 > > > > Can this be merged? I think it might hurt the fuzzer, e.g. cause the > flakiness in issue #45327. > This has been merged. Jim > On a technical note: > In src/policydb.c::policydb_index_others() the return value of > cond_init_bool_indexes() is not checked. > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > index fc71463e..e29cbd51 100644 > --- a/libsepol/src/policydb.c > +++ b/libsepol/src/policydb.c > @@ -1252,7 +1252,8 @@ int policydb_index_others(sepol_handle_t * handle, > if (!p->type_val_to_struct) > return -1; > > - cond_init_bool_indexes(p); > + if (cond_init_bool_indexes(p) == -1) > + return -1; > > for (i = SYM_ROLES; i < SYM_NUM; i++) { > free(p->sym_val_to_name[i]);
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;
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@gmail.com> --- libsepol/src/conditional.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)