| Message ID | 20190125100651.21753-4-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Report raw context in AVCs + refactoring | expand |
On 1/25/19 5:06 AM, Ondrej Mosnacek wrote: > These BUG_ONs do not really protect from any catastrophic situation so > there is no need to have them there. They are to catch bugs in callers that pass requested==0. That is always indicative of a bug in the caller (e.g. failed to correctly compute the permissions). Otherwise, we will silently allow such calls and not notice them. At the least, they should be WARN_ONs. > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > security/selinux/avc.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c > index 5ebad47391c9..478fa4213c25 100644 > --- a/security/selinux/avc.c > +++ b/security/selinux/avc.c > @@ -1044,7 +1044,6 @@ int avc_has_extended_perms(struct selinux_state *state, > int rc = 0, rc2; > > xp_node = &local_xp_node; > - BUG_ON(!requested); > > rcu_read_lock(); > > @@ -1134,8 +1133,6 @@ inline int avc_has_perm_noaudit(struct selinux_state *state, > int rc = 0; > u32 denied; > > - BUG_ON(!requested); > - > rcu_read_lock(); > > node = avc_lookup(state->avc, ssid, tsid, tclass); >
On Fri, Jan 25, 2019 at 2:49 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 1/25/19 5:06 AM, Ondrej Mosnacek wrote: > > These BUG_ONs do not really protect from any catastrophic situation so > > there is no need to have them there. > > They are to catch bugs in callers that pass requested==0. That is > always indicative of a bug in the caller (e.g. failed to correctly > compute the permissions). Otherwise, we will silently allow such calls > and not notice them. > > At the least, they should be WARN_ONs. OK, seems that switching to WARN_ON() will be a better choice. Paul, you can apply the series without this patch and I will post a corrected patch separately (if that's OK with you). > > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > security/selinux/avc.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c > > index 5ebad47391c9..478fa4213c25 100644 > > --- a/security/selinux/avc.c > > +++ b/security/selinux/avc.c > > @@ -1044,7 +1044,6 @@ int avc_has_extended_perms(struct selinux_state *state, > > int rc = 0, rc2; > > > > xp_node = &local_xp_node; > > - BUG_ON(!requested); > > > > rcu_read_lock(); > > > > @@ -1134,8 +1133,6 @@ inline int avc_has_perm_noaudit(struct selinux_state *state, > > int rc = 0; > > u32 denied; > > > > - BUG_ON(!requested); > > - > > rcu_read_lock(); > > > > node = avc_lookup(state->avc, ssid, tsid, tclass); > > >
On Fri, Jan 25, 2019 at 11:15 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Fri, Jan 25, 2019 at 2:49 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On 1/25/19 5:06 AM, Ondrej Mosnacek wrote: > > > These BUG_ONs do not really protect from any catastrophic situation so > > > there is no need to have them there. > > > > They are to catch bugs in callers that pass requested==0. That is > > always indicative of a bug in the caller (e.g. failed to correctly > > compute the permissions). Otherwise, we will silently allow such calls > > and not notice them. > > > > At the least, they should be WARN_ONs. > > OK, seems that switching to WARN_ON() will be a better choice. > > Paul, you can apply the series without this patch and I will post a > corrected patch separately (if that's OK with you). Yep. Patches 1, 2, and 4 should now be in selinux/next.
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 5ebad47391c9..478fa4213c25 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -1044,7 +1044,6 @@ int avc_has_extended_perms(struct selinux_state *state, int rc = 0, rc2; xp_node = &local_xp_node; - BUG_ON(!requested); rcu_read_lock(); @@ -1134,8 +1133,6 @@ inline int avc_has_perm_noaudit(struct selinux_state *state, int rc = 0; u32 denied; - BUG_ON(!requested); - rcu_read_lock(); node = avc_lookup(state->avc, ssid, tsid, tclass);
These BUG_ONs do not really protect from any catastrophic situation so there is no need to have them there. Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/avc.c | 3 --- 1 file changed, 3 deletions(-)