| Message ID | 20200429073053.83660-1-weiyongjun1@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [-next] selinux: fix error return code in policydb_read() | expand |
On Wed, Apr 29, 2020 at 07:30:53AM +0000, Wei Yongjun wrote: > Fix to return negative error code -ENOMEM from the kvcalloc() error > handling case instead of 0, as done elsewhere in this function. > Please add a Fixes tag and Cc Kent. Fixes: acdf52d97f82 ("selinux: convert to kvmalloc") > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > --- > security/selinux/ss/policydb.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index a0b3dc152468..a51e051df2cc 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -2638,6 +2638,7 @@ int policydb_read(struct policydb *p, void *fp) > if (rc) > goto bad; > > + rc = -ENOMEM; > p->type_attr_map_array = kvcalloc(p->p_types.nprim, > sizeof(*p->type_attr_map_array), > GFP_KERNEL); There is another bug earlier in the function as well: security/selinux/ss/policydb.c 2537 2538 rc = next_entry(buf, fp, sizeof(u32)); 2539 if (rc) 2540 goto bad; 2541 nel = le32_to_cpu(buf[0]); 2542 2543 p->role_tr = hashtab_create(role_trans_hash, role_trans_cmp, nel); 2544 if (!p->role_tr) 2545 goto bad; ^^^^^^^^ 2546 for (i = 0; i < nel; i++) { 2547 rc = -ENOMEM; This style of setting the error code seems very bug prone. The Fixes tag for this one is: Fixes: e67b2ec9f617 ("selinux: store role transitions in a hash table") Just put both tags in the commit message. regards, dan carpenter
On Wed, Apr 29, 2020 at 3:15 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Wed, Apr 29, 2020 at 07:30:53AM +0000, Wei Yongjun wrote: > > Fix to return negative error code -ENOMEM from the kvcalloc() error > > handling case instead of 0, as done elsewhere in this function. > > > > Please add a Fixes tag and Cc Kent. > > Fixes: acdf52d97f82 ("selinux: convert to kvmalloc") > > > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > --- > > security/selinux/ss/policydb.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > > index a0b3dc152468..a51e051df2cc 100644 > > --- a/security/selinux/ss/policydb.c > > +++ b/security/selinux/ss/policydb.c > > @@ -2638,6 +2638,7 @@ int policydb_read(struct policydb *p, void *fp) > > if (rc) > > goto bad; > > > > + rc = -ENOMEM; > > p->type_attr_map_array = kvcalloc(p->p_types.nprim, > > sizeof(*p->type_attr_map_array), > > GFP_KERNEL); > > There is another bug earlier in the function as well: > > security/selinux/ss/policydb.c > 2537 > 2538 rc = next_entry(buf, fp, sizeof(u32)); > 2539 if (rc) > 2540 goto bad; > 2541 nel = le32_to_cpu(buf[0]); > 2542 > 2543 p->role_tr = hashtab_create(role_trans_hash, role_trans_cmp, nel); > 2544 if (!p->role_tr) > 2545 goto bad; > ^^^^^^^^ > > 2546 for (i = 0; i < nel; i++) { > 2547 rc = -ENOMEM; > > This style of setting the error code seems very bug prone. > > The Fixes tag for this one is: > Fixes: e67b2ec9f617 ("selinux: store role transitions in a hash table") FYI, this one is also indirectly fixed by this patch, which is currently pending review: https://patchwork.kernel.org/patch/11514495/ > > Just put both tags in the commit message.
On Wed, Apr 29, 2020 at 03:32:13PM +0200, Ondrej Mosnacek wrote: > On Wed, Apr 29, 2020 at 3:15 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Wed, Apr 29, 2020 at 07:30:53AM +0000, Wei Yongjun wrote: > > > Fix to return negative error code -ENOMEM from the kvcalloc() error > > > handling case instead of 0, as done elsewhere in this function. > > > > > > > Please add a Fixes tag and Cc Kent. > > > > Fixes: acdf52d97f82 ("selinux: convert to kvmalloc") > > > > > > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > > --- > > > security/selinux/ss/policydb.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > > > index a0b3dc152468..a51e051df2cc 100644 > > > --- a/security/selinux/ss/policydb.c > > > +++ b/security/selinux/ss/policydb.c > > > @@ -2638,6 +2638,7 @@ int policydb_read(struct policydb *p, void *fp) > > > if (rc) > > > goto bad; > > > > > > + rc = -ENOMEM; > > > p->type_attr_map_array = kvcalloc(p->p_types.nprim, > > > sizeof(*p->type_attr_map_array), > > > GFP_KERNEL); > > > > There is another bug earlier in the function as well: > > > > security/selinux/ss/policydb.c > > 2537 > > 2538 rc = next_entry(buf, fp, sizeof(u32)); > > 2539 if (rc) > > 2540 goto bad; > > 2541 nel = le32_to_cpu(buf[0]); > > 2542 > > 2543 p->role_tr = hashtab_create(role_trans_hash, role_trans_cmp, nel); > > 2544 if (!p->role_tr) > > 2545 goto bad; > > ^^^^^^^^ > > > > 2546 for (i = 0; i < nel; i++) { > > 2547 rc = -ENOMEM; > > > > This style of setting the error code seems very bug prone. > > > > The Fixes tag for this one is: > > Fixes: e67b2ec9f617 ("selinux: store role transitions in a hash table") > > FYI, this one is also indirectly fixed by this patch, which is > currently pending review: > https://patchwork.kernel.org/patch/11514495/ > Ah. Fantastic. Wei, could you just resend your original commit with the first Fixes tag and Kent CC'd? regards, dan carpenter
On Wed, Apr 29, 2020 at 9:15 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Wed, Apr 29, 2020 at 07:30:53AM +0000, Wei Yongjun wrote: > > Fix to return negative error code -ENOMEM from the kvcalloc() error > > handling case instead of 0, as done elsewhere in this function. > > Please add a Fixes tag and Cc Kent. Kent? Who is Kent?
On Wed, Apr 29, 2020 at 3:29 AM Wei Yongjun <weiyongjun1@huawei.com> wrote: > > Fix to return negative error code -ENOMEM from the kvcalloc() error > handling case instead of 0, as done elsewhere in this function. > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > --- > security/selinux/ss/policydb.c | 1 + > 1 file changed, 1 insertion(+) Regardless of the other error, this patch fixes a legitimate problem so I've merged it into selinux/next with the appropriate fixes tag. Thanks.
On Wed, Apr 29, 2020 at 9:32 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Wed, Apr 29, 2020 at 3:15 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Wed, Apr 29, 2020 at 07:30:53AM +0000, Wei Yongjun wrote: > > > Fix to return negative error code -ENOMEM from the kvcalloc() error > > > handling case instead of 0, as done elsewhere in this function. > > > > > > > Please add a Fixes tag and Cc Kent. > > > > Fixes: acdf52d97f82 ("selinux: convert to kvmalloc") > > > > > > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > > --- > > > security/selinux/ss/policydb.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > > > index a0b3dc152468..a51e051df2cc 100644 > > > --- a/security/selinux/ss/policydb.c > > > +++ b/security/selinux/ss/policydb.c > > > @@ -2638,6 +2638,7 @@ int policydb_read(struct policydb *p, void *fp) > > > if (rc) > > > goto bad; > > > > > > + rc = -ENOMEM; > > > p->type_attr_map_array = kvcalloc(p->p_types.nprim, > > > sizeof(*p->type_attr_map_array), > > > GFP_KERNEL); > > > > There is another bug earlier in the function as well: > > > > security/selinux/ss/policydb.c > > 2537 > > 2538 rc = next_entry(buf, fp, sizeof(u32)); > > 2539 if (rc) > > 2540 goto bad; > > 2541 nel = le32_to_cpu(buf[0]); > > 2542 > > 2543 p->role_tr = hashtab_create(role_trans_hash, role_trans_cmp, nel); > > 2544 if (!p->role_tr) > > 2545 goto bad; > > ^^^^^^^^ > > > > 2546 for (i = 0; i < nel; i++) { > > 2547 rc = -ENOMEM; > > > > This style of setting the error code seems very bug prone. > > > > The Fixes tag for this one is: > > Fixes: e67b2ec9f617 ("selinux: store role transitions in a hash table") > > FYI, this one is also indirectly fixed by this patch, which is > currently pending review: > https://patchwork.kernel.org/patch/11514495/ If we know the code is broken, let's fix it independently of a feature patch. Ondrej, I believe the original patch is yours so please send me a new patch with just the fix, thank you.
On Fri, May 1, 2020 at 9:08 PM Paul Moore <paul@paul-moore.com> wrote: > On Wed, Apr 29, 2020 at 9:32 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Wed, Apr 29, 2020 at 3:15 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > On Wed, Apr 29, 2020 at 07:30:53AM +0000, Wei Yongjun wrote: > > > > Fix to return negative error code -ENOMEM from the kvcalloc() error > > > > handling case instead of 0, as done elsewhere in this function. > > > > > > > > > > Please add a Fixes tag and Cc Kent. > > > > > > Fixes: acdf52d97f82 ("selinux: convert to kvmalloc") > > > > > > > > > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > > > --- > > > > security/selinux/ss/policydb.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > > > > index a0b3dc152468..a51e051df2cc 100644 > > > > --- a/security/selinux/ss/policydb.c > > > > +++ b/security/selinux/ss/policydb.c > > > > @@ -2638,6 +2638,7 @@ int policydb_read(struct policydb *p, void *fp) > > > > if (rc) > > > > goto bad; > > > > > > > > + rc = -ENOMEM; > > > > p->type_attr_map_array = kvcalloc(p->p_types.nprim, > > > > sizeof(*p->type_attr_map_array), > > > > GFP_KERNEL); > > > > > > There is another bug earlier in the function as well: > > > > > > security/selinux/ss/policydb.c > > > 2537 > > > 2538 rc = next_entry(buf, fp, sizeof(u32)); > > > 2539 if (rc) > > > 2540 goto bad; > > > 2541 nel = le32_to_cpu(buf[0]); > > > 2542 > > > 2543 p->role_tr = hashtab_create(role_trans_hash, role_trans_cmp, nel); > > > 2544 if (!p->role_tr) > > > 2545 goto bad; > > > ^^^^^^^^ > > > > > > 2546 for (i = 0; i < nel; i++) { > > > 2547 rc = -ENOMEM; > > > > > > This style of setting the error code seems very bug prone. > > > > > > The Fixes tag for this one is: > > > Fixes: e67b2ec9f617 ("selinux: store role transitions in a hash table") > > > > FYI, this one is also indirectly fixed by this patch, which is > > currently pending review: > > https://patchwork.kernel.org/patch/11514495/ > > If we know the code is broken, let's fix it independently of a feature > patch. Ondrej, I believe the original patch is yours so please send > me a new patch with just the fix, thank you. Sure, here you go: https://lore.kernel.org/selinux/20200501195111.3335258-1-omosnace@redhat.com/T/
On Fri, May 01, 2020 at 12:46:47PM -0400, Paul Moore wrote: > On Wed, Apr 29, 2020 at 9:15 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Wed, Apr 29, 2020 at 07:30:53AM +0000, Wei Yongjun wrote: > > > Fix to return negative error code -ENOMEM from the kvcalloc() error > > > handling case instead of 0, as done elsewhere in this function. > > > > Please add a Fixes tag and Cc Kent. > > Kent? Who is Kent? commit acdf52d97f824019888422842757013b37441dd1 Author: Kent Overstreet <kent.overstreet@gmail.com> Date: Mon Mar 11 23:31:10 2019 -0700 selinux: convert to kvmalloc regards, dan carepnter
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index a0b3dc152468..a51e051df2cc 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -2638,6 +2638,7 @@ int policydb_read(struct policydb *p, void *fp) if (rc) goto bad; + rc = -ENOMEM; p->type_attr_map_array = kvcalloc(p->p_types.nprim, sizeof(*p->type_attr_map_array), GFP_KERNEL);
Fix to return negative error code -ENOMEM from the kvcalloc() error handling case instead of 0, as done elsewhere in this function. Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> --- security/selinux/ss/policydb.c | 1 + 1 file changed, 1 insertion(+)