* [PATCH -next] selinux: Fix error return code in policydb_read()
@ 2019-01-18 14:23 Wei Yongjun
2019-01-18 21:28 ` Paul Moore
0 siblings, 1 reply; 17+ messages in thread
From: Wei Yongjun @ 2019-01-18 14:23 UTC (permalink / raw)
To: Paul Moore, Stephen Smalley, Eric Paris, peter enderborg,
Kent Overstreet, Tetsuo Handa, Ondrej Mosnacek, Andrew Morton,
Stephen Rothwell
Cc: Wei Yongjun, selinux, kernel-janitors
Fix to return a negative error code -ENOMEM from the error handling
case instead of 0, as done elsewhere in this function.
Fixes: 31696241e96e ("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 6b576e5..ef616dd 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -2490,6 +2490,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);
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: Fix error return code in policydb_read()
2019-01-18 14:23 [PATCH -next] selinux: Fix error return code in policydb_read() Wei Yongjun
@ 2019-01-18 21:28 ` Paul Moore
2019-01-19 0:21 ` Stephen Rothwell
0 siblings, 1 reply; 17+ messages in thread
From: Paul Moore @ 2019-01-18 21:28 UTC (permalink / raw)
To: Wei Yongjun
Cc: Stephen Smalley, Eric Paris, peter enderborg, Kent Overstreet,
Tetsuo Handa, Ondrej Mosnacek, Andrew Morton, Stephen Rothwell,
selinux, kernel-janitors
On Fri, Jan 18, 2019 at 9:18 AM Wei Yongjun <weiyongjun1@huawei.com> wrote:
>
> Fix to return a negative error code -ENOMEM from the error handling
> case instead of 0, as done elsewhere in this function.
>
> Fixes: 31696241e96e ("selinux: convert to kvmalloc")
Unfortunately this commit isn't to be found in any of the SELinux
branches, or Linus' master branch; based on the subject line I'm
guessing the original patch is in a -next branch somewhere. Please
find whoever pushed this patch to linux-next and have them apply the
fix.
Thank you.
> 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 6b576e5..ef616dd 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2490,6 +2490,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);
>
>
>
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: Fix error return code in policydb_read()
2019-01-18 21:28 ` Paul Moore
@ 2019-01-19 0:21 ` Stephen Rothwell
2019-01-20 20:04 ` Stephen Rothwell
0 siblings, 1 reply; 17+ messages in thread
From: Stephen Rothwell @ 2019-01-19 0:21 UTC (permalink / raw)
To: Paul Moore
Cc: Wei Yongjun, Stephen Smalley, Eric Paris, peter enderborg,
Kent Overstreet, Tetsuo Handa, Ondrej Mosnacek, Andrew Morton,
selinux, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 790 bytes --]
Hi Paul,
On Fri, 18 Jan 2019 16:28:07 -0500 Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Jan 18, 2019 at 9:18 AM Wei Yongjun <weiyongjun1@huawei.com> wrote:
> >
> > Fix to return a negative error code -ENOMEM from the error handling
> > case instead of 0, as done elsewhere in this function.
> >
> > Fixes: 31696241e96e ("selinux: convert to kvmalloc")
>
> Unfortunately this commit isn't to be found in any of the SELinux
> branches, or Linus' master branch; based on the subject line I'm
> guessing the original patch is in a -next branch somewhere. Please
> find whoever pushed this patch to linux-next and have them apply the
> fix.
Yeah, Kent has a series doing conversions that is in Andrew Morton's
mmotm patch series.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: Fix error return code in policydb_read()
2019-01-19 0:21 ` Stephen Rothwell
@ 2019-01-20 20:04 ` Stephen Rothwell
2019-01-22 17:39 ` Paul Moore
0 siblings, 1 reply; 17+ messages in thread
From: Stephen Rothwell @ 2019-01-20 20:04 UTC (permalink / raw)
To: Paul Moore
Cc: Wei Yongjun, Stephen Smalley, Eric Paris, peter enderborg,
Kent Overstreet, Tetsuo Handa, Ondrej Mosnacek, Andrew Morton,
selinux, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 995 bytes --]
Hi all,
On Sat, 19 Jan 2019 11:21:48 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> On Fri, 18 Jan 2019 16:28:07 -0500 Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Fri, Jan 18, 2019 at 9:18 AM Wei Yongjun <weiyongjun1@huawei.com> wrote:
> > >
> > > Fix to return a negative error code -ENOMEM from the error handling
> > > case instead of 0, as done elsewhere in this function.
> > >
> > > Fixes: 31696241e96e ("selinux: convert to kvmalloc")
> >
> > Unfortunately this commit isn't to be found in any of the SELinux
> > branches, or Linus' master branch; based on the subject line I'm
> > guessing the original patch is in a -next branch somewhere. Please
> > find whoever pushed this patch to linux-next and have them apply the
> > fix.
>
> Yeah, Kent has a series doing conversions that is in Andrew Morton's
> mmotm patch series.
I have added that patch to the akpm-current tree in linux-next from today.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: Fix error return code in policydb_read()
2019-01-20 20:04 ` Stephen Rothwell
@ 2019-01-22 17:39 ` Paul Moore
2019-01-25 21:59 ` Paul Moore
0 siblings, 1 reply; 17+ messages in thread
From: Paul Moore @ 2019-01-22 17:39 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Wei Yongjun, Stephen Smalley, Eric Paris, peter enderborg,
Kent Overstreet, Tetsuo Handa, Ondrej Mosnacek, Andrew Morton,
selinux, kernel-janitors
On Sun, Jan 20, 2019 at 3:04 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi all,
>
> On Sat, 19 Jan 2019 11:21:48 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > On Fri, 18 Jan 2019 16:28:07 -0500 Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > On Fri, Jan 18, 2019 at 9:18 AM Wei Yongjun <weiyongjun1@huawei.com> wrote:
> > > >
> > > > Fix to return a negative error code -ENOMEM from the error handling
> > > > case instead of 0, as done elsewhere in this function.
> > > >
> > > > Fixes: 31696241e96e ("selinux: convert to kvmalloc")
> > >
> > > Unfortunately this commit isn't to be found in any of the SELinux
> > > branches, or Linus' master branch; based on the subject line I'm
> > > guessing the original patch is in a -next branch somewhere. Please
> > > find whoever pushed this patch to linux-next and have them apply the
> > > fix.
> >
> > Yeah, Kent has a series doing conversions that is in Andrew Morton's
> > mmotm patch series.
>
> I have added that patch to the akpm-current tree in linux-next from today.
Thanks Stephen.
Wei Yongjun, have you talked with Kent and/or Andrew about getting
your fix added to their tree? It's good that Stephen picked it up,
but it needs to get added to the original set of patches so the fix
makes it's way into Linus' tree at the same time as the original
patches.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: Fix error return code in policydb_read()
2019-01-22 17:39 ` Paul Moore
@ 2019-01-25 21:59 ` Paul Moore
0 siblings, 0 replies; 17+ messages in thread
From: Paul Moore @ 2019-01-25 21:59 UTC (permalink / raw)
To: Wei Yongjun
Cc: Stephen Smalley, Eric Paris, peter enderborg, Kent Overstreet,
Stephen Rothwell, Tetsuo Handa, Ondrej Mosnacek, Andrew Morton,
selinux, kernel-janitors
On Tue, Jan 22, 2019 at 12:39 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Sun, Jan 20, 2019 at 3:04 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi all,
> >
> > On Sat, 19 Jan 2019 11:21:48 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > >
> > > On Fri, 18 Jan 2019 16:28:07 -0500 Paul Moore <paul@paul-moore.com> wrote:
> > > >
> > > > On Fri, Jan 18, 2019 at 9:18 AM Wei Yongjun <weiyongjun1@huawei.com> wrote:
> > > > >
> > > > > Fix to return a negative error code -ENOMEM from the error handling
> > > > > case instead of 0, as done elsewhere in this function.
> > > > >
> > > > > Fixes: 31696241e96e ("selinux: convert to kvmalloc")
> > > >
> > > > Unfortunately this commit isn't to be found in any of the SELinux
> > > > branches, or Linus' master branch; based on the subject line I'm
> > > > guessing the original patch is in a -next branch somewhere. Please
> > > > find whoever pushed this patch to linux-next and have them apply the
> > > > fix.
> > >
> > > Yeah, Kent has a series doing conversions that is in Andrew Morton's
> > > mmotm patch series.
> >
> > I have added that patch to the akpm-current tree in linux-next from today.
>
> Thanks Stephen.
>
> Wei Yongjun, have you talked with Kent and/or Andrew about getting
> your fix added to their tree? It's good that Stephen picked it up,
> but it needs to get added to the original set of patches so the fix
> makes it's way into Linus' tree at the same time as the original
> patches.
Wei Yongjun, any progress on this?
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: fix error return code in policydb_read()
2020-05-01 16:46 ` Paul Moore
@ 2020-05-04 19:17 ` Dan Carpenter
0 siblings, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2020-05-04 19:17 UTC (permalink / raw)
To: Paul Moore
Cc: Wei Yongjun, Stephen Smalley, Eric Paris, Ondrej Mosnacek,
Jeff Vander Stoep, selinux, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: fix error return code in policydb_read()
2020-05-01 19:08 ` Paul Moore
@ 2020-05-01 19:52 ` Ondrej Mosnacek
0 siblings, 0 replies; 17+ messages in thread
From: Ondrej Mosnacek @ 2020-05-01 19:52 UTC (permalink / raw)
To: Paul Moore
Cc: Dan Carpenter, Wei Yongjun, Stephen Smalley, Eric Paris,
Jeff Vander Stoep, SElinux list, kernel-janitors
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/
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: fix error return code in policydb_read()
2020-04-29 13:32 ` Ondrej Mosnacek
2020-04-29 13:47 ` Dan Carpenter
@ 2020-05-01 19:08 ` Paul Moore
2020-05-01 19:52 ` Ondrej Mosnacek
1 sibling, 1 reply; 17+ messages in thread
From: Paul Moore @ 2020-05-01 19:08 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Dan Carpenter, Wei Yongjun, Stephen Smalley, Eric Paris,
Jeff Vander Stoep, SElinux list, kernel-janitors
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.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: fix error return code in policydb_read()
2020-04-29 7:30 [PATCH -next] selinux: fix " Wei Yongjun
2020-04-29 13:07 ` Dan Carpenter
@ 2020-05-01 19:04 ` Paul Moore
1 sibling, 0 replies; 17+ messages in thread
From: Paul Moore @ 2020-05-01 19:04 UTC (permalink / raw)
To: Wei Yongjun
Cc: Stephen Smalley, Eric Paris, Ondrej Mosnacek, Jeff Vander Stoep,
selinux, kernel-janitors
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.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: fix error return code in policydb_read()
2020-04-29 13:07 ` Dan Carpenter
2020-04-29 13:32 ` Ondrej Mosnacek
@ 2020-05-01 16:46 ` Paul Moore
2020-05-04 19:17 ` Dan Carpenter
1 sibling, 1 reply; 17+ messages in thread
From: Paul Moore @ 2020-05-01 16:46 UTC (permalink / raw)
To: Dan Carpenter
Cc: Wei Yongjun, Stephen Smalley, Eric Paris, Ondrej Mosnacek,
Jeff Vander Stoep, selinux, kernel-janitors
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?
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: fix error return code in policydb_read()
2020-04-29 13:32 ` Ondrej Mosnacek
@ 2020-04-29 13:47 ` Dan Carpenter
2020-05-01 19:08 ` Paul Moore
1 sibling, 0 replies; 17+ messages in thread
From: Dan Carpenter @ 2020-04-29 13:47 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Wei Yongjun, Paul Moore, Stephen Smalley, Eric Paris,
Jeff Vander Stoep, SElinux list, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: fix error return code in policydb_read()
2020-04-29 13:07 ` Dan Carpenter
@ 2020-04-29 13:32 ` Ondrej Mosnacek
2020-04-29 13:47 ` Dan Carpenter
2020-05-01 19:08 ` Paul Moore
2020-05-01 16:46 ` Paul Moore
1 sibling, 2 replies; 17+ messages in thread
From: Ondrej Mosnacek @ 2020-04-29 13:32 UTC (permalink / raw)
To: Dan Carpenter
Cc: Wei Yongjun, Paul Moore, Stephen Smalley, Eric Paris,
Jeff Vander Stoep, SElinux list, kernel-janitors
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.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -next] selinux: fix error return code in policydb_read()
2020-04-29 7:30 [PATCH -next] selinux: fix " Wei Yongjun
@ 2020-04-29 13:07 ` Dan Carpenter
2020-04-29 13:32 ` Ondrej Mosnacek
2020-05-01 16:46 ` Paul Moore
2020-05-01 19:04 ` Paul Moore
1 sibling, 2 replies; 17+ messages in thread
From: Dan Carpenter @ 2020-04-29 13:07 UTC (permalink / raw)
To: Wei Yongjun
Cc: Paul Moore, Stephen Smalley, Eric Paris, Ondrej Mosnacek,
Jeff Vander Stoep, selinux, kernel-janitors
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH -next] selinux: fix error return code in policydb_read()
@ 2020-04-29 7:30 Wei Yongjun
2020-04-29 13:07 ` Dan Carpenter
2020-05-01 19:04 ` Paul Moore
0 siblings, 2 replies; 17+ messages in thread
From: Wei Yongjun @ 2020-04-29 7:30 UTC (permalink / raw)
To: Paul Moore, Stephen Smalley, Eric Paris, Ondrej Mosnacek,
Jeff Vander Stoep
Cc: Wei Yongjun, selinux, kernel-janitors
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(+)
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);
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH -next] SELinux: fix error return code in policydb_read()
2016-09-10 7:43 [PATCH -next] SELinux: " Wei Yongjun
@ 2016-09-13 21:19 ` Paul Moore
0 siblings, 0 replies; 17+ messages in thread
From: Paul Moore @ 2016-09-13 21:19 UTC (permalink / raw)
To: Wei Yongjun
Cc: Stephen Smalley, Eric Paris, James Morris, Serge E. Hallyn,
William Roberts, Wei Yongjun, selinux, linux-security-module
On Sat, Sep 10, 2016 at 3:43 AM, Wei Yongjun <weiyj.lk@gmail.com> wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> Fix to return error code -EINVAL from the error handling case instead
> of 0(rc is overwrite to 0 when policyvers >= POLICYDB_VERSION_ROLETRANS),
> 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(+)
Nice catch, thanks for the patch; it has been merged into the
selinux#next branch.
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 8c661f0..ace6838 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2417,6 +2417,7 @@ int policydb_read(struct policydb *p, void *fp)
> } else
> tr->tclass = p->process_class;
>
> + rc = -EINVAL;
> if (!policydb_role_isvalid(p, tr->role) ||
> !policydb_type_isvalid(p, tr->type) ||
> !policydb_class_isvalid(p, tr->tclass) ||
>
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH -next] SELinux: fix error return code in policydb_read()
@ 2016-09-10 7:43 Wei Yongjun
2016-09-13 21:19 ` Paul Moore
0 siblings, 1 reply; 17+ messages in thread
From: Wei Yongjun @ 2016-09-10 7:43 UTC (permalink / raw)
To: Paul Moore, Stephen Smalley, Eric Paris, James Morris,
Serge E. Hallyn, William Roberts
Cc: Wei Yongjun, selinux, linux-security-module
From: Wei Yongjun <weiyongjun1@huawei.com>
Fix to return error code -EINVAL from the error handling case instead
of 0(rc is overwrite to 0 when policyvers >= POLICYDB_VERSION_ROLETRANS),
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(+)
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 8c661f0..ace6838 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -2417,6 +2417,7 @@ int policydb_read(struct policydb *p, void *fp)
} else
tr->tclass = p->process_class;
+ rc = -EINVAL;
if (!policydb_role_isvalid(p, tr->role) ||
!policydb_type_isvalid(p, tr->type) ||
!policydb_class_isvalid(p, tr->tclass) ||
^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-05-04 19:19 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-18 14:23 [PATCH -next] selinux: Fix error return code in policydb_read() Wei Yongjun
2019-01-18 21:28 ` Paul Moore
2019-01-19 0:21 ` Stephen Rothwell
2019-01-20 20:04 ` Stephen Rothwell
2019-01-22 17:39 ` Paul Moore
2019-01-25 21:59 ` Paul Moore
-- strict thread matches above, loose matches on Subject: below --
2020-04-29 7:30 [PATCH -next] selinux: fix " Wei Yongjun
2020-04-29 13:07 ` Dan Carpenter
2020-04-29 13:32 ` Ondrej Mosnacek
2020-04-29 13:47 ` Dan Carpenter
2020-05-01 19:08 ` Paul Moore
2020-05-01 19:52 ` Ondrej Mosnacek
2020-05-01 16:46 ` Paul Moore
2020-05-04 19:17 ` Dan Carpenter
2020-05-01 19:04 ` Paul Moore
2016-09-10 7:43 [PATCH -next] SELinux: " Wei Yongjun
2016-09-13 21:19 ` Paul Moore
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).