* [PATCH] selinux: Fix an uninitialized variable bug
@ 2017-03-31 15:21 ` Dan Carpenter
0 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2017-03-31 15:21 UTC (permalink / raw)
To: Paul Moore, Markus Elfring
Cc: Stephen Smalley, Eric Paris, James Morris, Serge E. Hallyn,
William Roberts, selinux, linux-security-module, kernel-janitors
We removed this initialization as a cleanup but it is probably required.
The concern is that "nel" can be zero. I'm not an expert on SELinux
code but I think it looks possible to write an SELinux policy which
triggers this bug. GCC doesn't catch this, but my static checker does.
Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable initialisation in range_read()")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 658247f98dc1..0080122760ad 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1832,7 +1832,7 @@ u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name)
static int range_read(struct policydb *p, void *fp)
{
- struct range_trans *rt;
+ struct range_trans *rt = NULL;
struct mls_range *r = NULL;
int i, rc;
__le32 buf[2];
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH] selinux: Fix an uninitialized variable bug @ 2017-03-31 15:21 ` Dan Carpenter 0 siblings, 0 replies; 24+ messages in thread From: Dan Carpenter @ 2017-03-31 15:21 UTC (permalink / raw) To: linux-security-module We removed this initialization as a cleanup but it is probably required. The concern is that "nel" can be zero. I'm not an expert on SELinux code but I think it looks possible to write an SELinux policy which triggers this bug. GCC doesn't catch this, but my static checker does. Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable initialisation in range_read()") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 658247f98dc1..0080122760ad 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -1832,7 +1832,7 @@ u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name) static int range_read(struct policydb *p, void *fp) { - struct range_trans *rt; + struct range_trans *rt = NULL; struct mls_range *r = NULL; int i, rc; __le32 buf[2]; -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH] selinux: Fix an uninitialized variable bug @ 2017-03-31 15:21 ` Dan Carpenter 0 siblings, 0 replies; 24+ messages in thread From: Dan Carpenter @ 2017-03-31 15:21 UTC (permalink / raw) To: linux-security-module We removed this initialization as a cleanup but it is probably required. The concern is that "nel" can be zero. I'm not an expert on SELinux code but I think it looks possible to write an SELinux policy which triggers this bug. GCC doesn't catch this, but my static checker does. Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable initialisation in range_read()") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 658247f98dc1..0080122760ad 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -1832,7 +1832,7 @@ u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name) static int range_read(struct policydb *p, void *fp) { - struct range_trans *rt; + struct range_trans *rt = NULL; struct mls_range *r = NULL; int i, rc; __le32 buf[2]; ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] selinux: Fix an uninitialized variable bug 2017-03-31 15:21 ` Dan Carpenter (?) @ 2017-03-31 15:52 ` Stephen Smalley -1 siblings, 0 replies; 24+ messages in thread From: Stephen Smalley @ 2017-03-31 15:52 UTC (permalink / raw) To: Dan Carpenter, Paul Moore, Markus Elfring Cc: Eric Paris, James Morris, Serge E. Hallyn, William Roberts, selinux, linux-security-module, kernel-janitors On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: > We removed this initialization as a cleanup but it is probably > required. > > The concern is that "nel" can be zero. I'm not an expert on SELinux > code but I think it looks possible to write an SELinux policy which > triggers this bug. GCC doesn't catch this, but my static checker > does. > > Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable > initialisation in range_read()") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Nice catch, thanks! Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > > diff --git a/security/selinux/ss/policydb.c > b/security/selinux/ss/policydb.c > index 658247f98dc1..0080122760ad 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -1832,7 +1832,7 @@ u32 string_to_av_perm(struct policydb *p, u16 > tclass, const char *name) > > static int range_read(struct policydb *p, void *fp) > { > - struct range_trans *rt; > + struct range_trans *rt = NULL; > struct mls_range *r = NULL; > int i, rc; > __le32 buf[2]; ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] selinux: Fix an uninitialized variable bug @ 2017-03-31 15:52 ` Stephen Smalley 0 siblings, 0 replies; 24+ messages in thread From: Stephen Smalley @ 2017-03-31 15:52 UTC (permalink / raw) To: linux-security-module On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: > We removed this initialization as a cleanup but it is probably > required. > > The concern is that "nel" can be zero.??I'm not an expert on SELinux > code but I think it looks possible to write an SELinux policy which > triggers this bug.??GCC doesn't catch this, but my static checker > does. > > Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable > initialisation in range_read()") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Nice catch, thanks! Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > > diff --git a/security/selinux/ss/policydb.c > b/security/selinux/ss/policydb.c > index 658247f98dc1..0080122760ad 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -1832,7 +1832,7 @@ u32 string_to_av_perm(struct policydb *p, u16 > tclass, const char *name) > ? > ?static int range_read(struct policydb *p, void *fp) > ?{ > - struct range_trans *rt; > + struct range_trans *rt = NULL; > ? struct mls_range *r = NULL; > ? int i, rc; > ? __le32 buf[2]; -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] selinux: Fix an uninitialized variable bug @ 2017-03-31 15:52 ` Stephen Smalley 0 siblings, 0 replies; 24+ messages in thread From: Stephen Smalley @ 2017-03-31 15:52 UTC (permalink / raw) To: linux-security-module On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: > We removed this initialization as a cleanup but it is probably > required. > > The concern is that "nel" can be zero. I'm not an expert on SELinux > code but I think it looks possible to write an SELinux policy which > triggers this bug. GCC doesn't catch this, but my static checker > does. > > Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable > initialisation in range_read()") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Nice catch, thanks! Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > > diff --git a/security/selinux/ss/policydb.c > b/security/selinux/ss/policydb.c > index 658247f98dc1..0080122760ad 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -1832,7 +1832,7 @@ u32 string_to_av_perm(struct policydb *p, u16 > tclass, const char *name) > > static int range_read(struct policydb *p, void *fp) > { > - struct range_trans *rt; > + struct range_trans *rt = NULL; > struct mls_range *r = NULL; > int i, rc; > __le32 buf[2]; ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] selinux: Fix an uninitialized variable bug 2017-03-31 15:52 ` Stephen Smalley (?) @ 2017-03-31 19:18 ` Paul Moore -1 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2017-03-31 19:18 UTC (permalink / raw) To: Stephen Smalley Cc: Dan Carpenter, Markus Elfring, Eric Paris, James Morris, Serge E. Hallyn, William Roberts, selinux, linux-security-module, kernel-janitors On Fri, Mar 31, 2017 at 11:52 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: >> We removed this initialization as a cleanup but it is probably >> required. >> >> The concern is that "nel" can be zero. I'm not an expert on SELinux >> code but I think it looks possible to write an SELinux policy which >> triggers this bug. GCC doesn't catch this, but my static checker >> does. >> >> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >> initialisation in range_read()") >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Nice catch, thanks! > > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. Merged. >> diff --git a/security/selinux/ss/policydb.c >> b/security/selinux/ss/policydb.c >> index 658247f98dc1..0080122760ad 100644 >> --- a/security/selinux/ss/policydb.c >> +++ b/security/selinux/ss/policydb.c >> @@ -1832,7 +1832,7 @@ u32 string_to_av_perm(struct policydb *p, u16 >> tclass, const char *name) >> >> static int range_read(struct policydb *p, void *fp) >> { >> - struct range_trans *rt; >> + struct range_trans *rt = NULL; >> struct mls_range *r = NULL; >> int i, rc; >> __le32 buf[2]; -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] selinux: Fix an uninitialized variable bug @ 2017-03-31 19:18 ` Paul Moore 0 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2017-03-31 19:18 UTC (permalink / raw) To: linux-security-module On Fri, Mar 31, 2017 at 11:52 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: >> We removed this initialization as a cleanup but it is probably >> required. >> >> The concern is that "nel" can be zero. I'm not an expert on SELinux >> code but I think it looks possible to write an SELinux policy which >> triggers this bug. GCC doesn't catch this, but my static checker >> does. >> >> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >> initialisation in range_read()") >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Nice catch, thanks! > > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. Merged. >> diff --git a/security/selinux/ss/policydb.c >> b/security/selinux/ss/policydb.c >> index 658247f98dc1..0080122760ad 100644 >> --- a/security/selinux/ss/policydb.c >> +++ b/security/selinux/ss/policydb.c >> @@ -1832,7 +1832,7 @@ u32 string_to_av_perm(struct policydb *p, u16 >> tclass, const char *name) >> >> static int range_read(struct policydb *p, void *fp) >> { >> - struct range_trans *rt; >> + struct range_trans *rt = NULL; >> struct mls_range *r = NULL; >> int i, rc; >> __le32 buf[2]; -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] selinux: Fix an uninitialized variable bug @ 2017-03-31 19:18 ` Paul Moore 0 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2017-03-31 19:18 UTC (permalink / raw) To: linux-security-module On Fri, Mar 31, 2017 at 11:52 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: >> We removed this initialization as a cleanup but it is probably >> required. >> >> The concern is that "nel" can be zero. I'm not an expert on SELinux >> code but I think it looks possible to write an SELinux policy which >> triggers this bug. GCC doesn't catch this, but my static checker >> does. >> >> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >> initialisation in range_read()") >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Nice catch, thanks! > > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. Merged. >> diff --git a/security/selinux/ss/policydb.c >> b/security/selinux/ss/policydb.c >> index 658247f98dc1..0080122760ad 100644 >> --- a/security/selinux/ss/policydb.c >> +++ b/security/selinux/ss/policydb.c >> @@ -1832,7 +1832,7 @@ u32 string_to_av_perm(struct policydb *p, u16 >> tclass, const char *name) >> >> static int range_read(struct policydb *p, void *fp) >> { >> - struct range_trans *rt; >> + struct range_trans *rt = NULL; >> struct mls_range *r = NULL; >> int i, rc; >> __le32 buf[2]; -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: selinux: Fix an uninitialized variable bug in range_read() 2017-03-31 19:18 ` Paul Moore (?) @ 2017-04-01 6:40 ` SF Markus Elfring -1 siblings, 0 replies; 24+ messages in thread From: SF Markus Elfring @ 2017-04-01 6:40 UTC (permalink / raw) To: Paul Moore, Stephen Smalley Cc: Dan Carpenter, Eric Paris, James Morris, Serge E. Hallyn, William Roberts, selinux, linux-security-module, kernel-janitors >>> We removed this initialization as a cleanup but it is probably >>> required. >>> >>> The concern is that "nel" can be zero. I'm not an expert on SELinux >>> code but I think it looks possible to write an SELinux policy which >>> triggers this bug. GCC doesn't catch this, but my static checker >>> does. >>> >>> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >>> initialisation in range_read()") >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> >> Nice catch, thanks! >> >> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > > Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. How do you think about the possibility to call the function “range_read” only with a system configuration where the interface “le32_to_cpu” will be resolved to a positive value so that statements in the corresponding for loop will be executed at least once? Regards, Markus ^ permalink raw reply [flat|nested] 24+ messages in thread
* selinux: Fix an uninitialized variable bug in range_read() @ 2017-04-01 6:40 ` SF Markus Elfring 0 siblings, 0 replies; 24+ messages in thread From: SF Markus Elfring @ 2017-04-01 6:40 UTC (permalink / raw) To: linux-security-module >>> We removed this initialization as a cleanup but it is probably >>> required. >>> >>> The concern is that "nel" can be zero. I'm not an expert on SELinux >>> code but I think it looks possible to write an SELinux policy which >>> triggers this bug. GCC doesn't catch this, but my static checker >>> does. >>> >>> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >>> initialisation in range_read()") >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> >> Nice catch, thanks! >> >> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > > Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. How do you think about the possibility to call the function ?range_read? only with a system configuration where the interface ?le32_to_cpu? will be resolved to a positive value so that statements in the corresponding for loop will be executed at least once? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: selinux: Fix an uninitialized variable bug in range_read() @ 2017-04-01 6:40 ` SF Markus Elfring 0 siblings, 0 replies; 24+ messages in thread From: SF Markus Elfring @ 2017-04-01 6:40 UTC (permalink / raw) To: linux-security-module >>> We removed this initialization as a cleanup but it is probably >>> required. >>> >>> The concern is that "nel" can be zero. I'm not an expert on SELinux >>> code but I think it looks possible to write an SELinux policy which >>> triggers this bug. GCC doesn't catch this, but my static checker >>> does. >>> >>> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >>> initialisation in range_read()") >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> >> Nice catch, thanks! >> >> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > > Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. How do you think about the possibility to call the function “range_read” only with a system configuration where the interface “le32_to_cpu” will be resolved to a positive value so that statements in the corresponding for loop will be executed at least once? Regards, Markus ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: selinux: Fix an uninitialized variable bug in range_read() 2017-04-01 6:40 ` SF Markus Elfring (?) @ 2017-04-01 14:52 ` Paul Moore -1 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2017-04-01 14:52 UTC (permalink / raw) To: SF Markus Elfring Cc: Stephen Smalley, Dan Carpenter, Eric Paris, James Morris, Serge E. Hallyn, William Roberts, selinux, linux-security-module, kernel-janitors On Sat, Apr 1, 2017 at 2:40 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>>> We removed this initialization as a cleanup but it is probably >>>> required. >>>> >>>> The concern is that "nel" can be zero. I'm not an expert on SELinux >>>> code but I think it looks possible to write an SELinux policy which >>>> triggers this bug. GCC doesn't catch this, but my static checker >>>> does. >>>> >>>> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >>>> initialisation in range_read()") >>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>> >>> Nice catch, thanks! >>> >>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> >> >> Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. > > How do you think about the possibility to call the function “range_read” only with > a system configuration where the interface “le32_to_cpu” will be resolved to > a positive value so that statements in the corresponding for loop will be executed > at least once? I suggest we leave this as-is for the moment. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* selinux: Fix an uninitialized variable bug in range_read() @ 2017-04-01 14:52 ` Paul Moore 0 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2017-04-01 14:52 UTC (permalink / raw) To: linux-security-module On Sat, Apr 1, 2017 at 2:40 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>>> We removed this initialization as a cleanup but it is probably >>>> required. >>>> >>>> The concern is that "nel" can be zero. I'm not an expert on SELinux >>>> code but I think it looks possible to write an SELinux policy which >>>> triggers this bug. GCC doesn't catch this, but my static checker >>>> does. >>>> >>>> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >>>> initialisation in range_read()") >>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>> >>> Nice catch, thanks! >>> >>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> >> >> Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. > > How do you think about the possibility to call the function ?range_read? only with > a system configuration where the interface ?le32_to_cpu? will be resolved to > a positive value so that statements in the corresponding for loop will be executed > at least once? I suggest we leave this as-is for the moment. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: selinux: Fix an uninitialized variable bug in range_read() @ 2017-04-01 14:52 ` Paul Moore 0 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2017-04-01 14:52 UTC (permalink / raw) To: linux-security-module On Sat, Apr 1, 2017 at 2:40 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>>> We removed this initialization as a cleanup but it is probably >>>> required. >>>> >>>> The concern is that "nel" can be zero. I'm not an expert on SELinux >>>> code but I think it looks possible to write an SELinux policy which >>>> triggers this bug. GCC doesn't catch this, but my static checker >>>> does. >>>> >>>> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >>>> initialisation in range_read()") >>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>> >>> Nice catch, thanks! >>> >>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> >> >> Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. > > How do you think about the possibility to call the function “range_read” only with > a system configuration where the interface “le32_to_cpu” will be resolved to > a positive value so that statements in the corresponding for loop will be executed > at least once? I suggest we leave this as-is for the moment. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] selinux: Fix an uninitialized variable bug 2017-03-31 19:18 ` Paul Moore (?) @ 2017-04-03 1:10 ` James Morris -1 siblings, 0 replies; 24+ messages in thread From: James Morris @ 2017-04-03 1:10 UTC (permalink / raw) To: Paul Moore Cc: Stephen Smalley, Dan Carpenter, Markus Elfring, Eric Paris, James Morris, Serge E. Hallyn, William Roberts, selinux, linux-security-module, kernel-janitors On Fri, 31 Mar 2017, Paul Moore wrote: > On Fri, Mar 31, 2017 at 11:52 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: > >> We removed this initialization as a cleanup but it is probably > >> required. > >> > >> The concern is that "nel" can be zero. I'm not an expert on SELinux > >> code but I think it looks possible to write an SELinux policy which > >> triggers this bug. GCC doesn't catch this, but my static checker > >> does. > >> > >> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable > >> initialisation in range_read()") > >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Nice catch, thanks! > > > > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > > Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. > I'd like to reiterate that I generally don't want to accept cleanup patches into the security tree from Markus (or indeed from others who only do cleanup/whitespace work). See https://lkml.org/lkml/2017/1/29/172, and please click through and read Dan's comments. All patches carry risks of introducing new bugs, and kernel "cleanup: patches generally offer a pretty high cost/benefit ratio. If such patches come from core developers of that code, or from kernel developers with experience in *analyzing and fixing* bugs, that's very different. Paul, please review all of these patches very carefully before sending your pull request. -- James Morris <jmorris@namei.org> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] selinux: Fix an uninitialized variable bug @ 2017-04-03 1:10 ` James Morris 0 siblings, 0 replies; 24+ messages in thread From: James Morris @ 2017-04-03 1:10 UTC (permalink / raw) To: linux-security-module On Fri, 31 Mar 2017, Paul Moore wrote: > On Fri, Mar 31, 2017 at 11:52 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: > >> We removed this initialization as a cleanup but it is probably > >> required. > >> > >> The concern is that "nel" can be zero. I'm not an expert on SELinux > >> code but I think it looks possible to write an SELinux policy which > >> triggers this bug. GCC doesn't catch this, but my static checker > >> does. > >> > >> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable > >> initialisation in range_read()") > >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Nice catch, thanks! > > > > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > > Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. > I'd like to reiterate that I generally don't want to accept cleanup patches into the security tree from Markus (or indeed from others who only do cleanup/whitespace work). See https://lkml.org/lkml/2017/1/29/172, and please click through and read Dan's comments. All patches carry risks of introducing new bugs, and kernel "cleanup: patches generally offer a pretty high cost/benefit ratio. If such patches come from core developers of that code, or from kernel developers with experience in *analyzing and fixing* bugs, that's very different. Paul, please review all of these patches very carefully before sending your pull request. -- James Morris <jmorris@namei.org> -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] selinux: Fix an uninitialized variable bug @ 2017-04-03 1:10 ` James Morris 0 siblings, 0 replies; 24+ messages in thread From: James Morris @ 2017-04-03 1:10 UTC (permalink / raw) To: linux-security-module On Fri, 31 Mar 2017, Paul Moore wrote: > On Fri, Mar 31, 2017 at 11:52 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: > >> We removed this initialization as a cleanup but it is probably > >> required. > >> > >> The concern is that "nel" can be zero. I'm not an expert on SELinux > >> code but I think it looks possible to write an SELinux policy which > >> triggers this bug. GCC doesn't catch this, but my static checker > >> does. > >> > >> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable > >> initialisation in range_read()") > >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Nice catch, thanks! > > > > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> > > Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. > I'd like to reiterate that I generally don't want to accept cleanup patches into the security tree from Markus (or indeed from others who only do cleanup/whitespace work). See https://lkml.org/lkml/2017/1/29/172, and please click through and read Dan's comments. All patches carry risks of introducing new bugs, and kernel "cleanup: patches generally offer a pretty high cost/benefit ratio. If such patches come from core developers of that code, or from kernel developers with experience in *analyzing and fixing* bugs, that's very different. Paul, please review all of these patches very carefully before sending your pull request. -- James Morris <jmorris@namei.org> ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] selinux: Fix an uninitialized variable bug 2017-04-03 1:10 ` James Morris (?) @ 2017-04-03 21:45 ` Paul Moore -1 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2017-04-03 21:45 UTC (permalink / raw) To: James Morris Cc: Stephen Smalley, Dan Carpenter, Markus Elfring, Eric Paris, James Morris, Serge E. Hallyn, William Roberts, selinux, linux-security-module, kernel-janitors On Sun, Apr 2, 2017 at 9:10 PM, James Morris <jmorris@namei.org> wrote: > On Fri, 31 Mar 2017, Paul Moore wrote: > >> On Fri, Mar 31, 2017 at 11:52 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> > On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: >> >> We removed this initialization as a cleanup but it is probably >> >> required. >> >> >> >> The concern is that "nel" can be zero. I'm not an expert on SELinux >> >> code but I think it looks possible to write an SELinux policy which >> >> triggers this bug. GCC doesn't catch this, but my static checker >> >> does. >> >> >> >> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >> >> initialisation in range_read()") >> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> > >> > Nice catch, thanks! >> > >> > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> >> >> Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. >> > > I'd like to reiterate that I generally don't want to accept cleanup > patches into the security tree from Markus (or indeed from others who > only do cleanup/whitespace work). > > See https://lkml.org/lkml/2017/1/29/172, and please click through and read > Dan's comments. > > All patches carry risks of introducing new bugs, and kernel "cleanup: > patches generally offer a pretty high cost/benefit ratio. If such patches > come from core developers of that code, or from kernel developers with > experience in *analyzing and fixing* bugs, that's very different. > > Paul, please review all of these patches very carefully before sending > your pull request. James, I know you don't track SELinux development very closely so I'm going to give you the benefit of the doubt and assume you haven't already seen my responses to Markus in his 46 patch thread. If you did follow the thread you would have noticed that I rejected several of his patches as only adding noise/churn, and several more due to personal preference; those that I did merge, I did review. I review every patch that I merge, even the patches from Stephen. Most of the patches I merged either fixed up some sizeof(...) calculations, made better use of the various kmalloc(...) variants, or fixed some checkpatch.pl reported problem. The first two types of patches, while technically noise, do bring value to the kernel by making the code a bit more robust, and the third, while much less valuable, does bring some consistency. I did tell Markus I was merging the checkpatch.pl fixes begrudgingly and I didn't particularly appreciate those patches; like Julia in the comments you linked, I tried to suggest Markus find a more productive way of contributing. That said, am I disappointed with myself that I didn't catch the bug Dan found? Of course I am. Am I annoyed that the regression was associated with a stupid, low-value patch? Of course I am. I'm very grateful to Dan that he caught the mistake (thanks again Dan!). While I understand Dan's move to ban Markus, from a maintainer's perspective it is easier that way, but I think the better approach is to encourage Markus to contribute more meaningful patches. I'm not going to reject patches simply because of their author, but I do reject patches that provide little value. We all started somewhere, and I bet some of us started with small patches, some of which were rejected; let's try to remember that when dealing with people like Markus. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] selinux: Fix an uninitialized variable bug @ 2017-04-03 21:45 ` Paul Moore 0 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2017-04-03 21:45 UTC (permalink / raw) To: linux-security-module On Sun, Apr 2, 2017 at 9:10 PM, James Morris <jmorris@namei.org> wrote: > On Fri, 31 Mar 2017, Paul Moore wrote: > >> On Fri, Mar 31, 2017 at 11:52 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> > On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: >> >> We removed this initialization as a cleanup but it is probably >> >> required. >> >> >> >> The concern is that "nel" can be zero. I'm not an expert on SELinux >> >> code but I think it looks possible to write an SELinux policy which >> >> triggers this bug. GCC doesn't catch this, but my static checker >> >> does. >> >> >> >> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >> >> initialisation in range_read()") >> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> > >> > Nice catch, thanks! >> > >> > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> >> >> Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. >> > > I'd like to reiterate that I generally don't want to accept cleanup > patches into the security tree from Markus (or indeed from others who > only do cleanup/whitespace work). > > See https://lkml.org/lkml/2017/1/29/172, and please click through and read > Dan's comments. > > All patches carry risks of introducing new bugs, and kernel "cleanup: > patches generally offer a pretty high cost/benefit ratio. If such patches > come from core developers of that code, or from kernel developers with > experience in *analyzing and fixing* bugs, that's very different. > > Paul, please review all of these patches very carefully before sending > your pull request. James, I know you don't track SELinux development very closely so I'm going to give you the benefit of the doubt and assume you haven't already seen my responses to Markus in his 46 patch thread. If you did follow the thread you would have noticed that I rejected several of his patches as only adding noise/churn, and several more due to personal preference; those that I did merge, I did review. I review every patch that I merge, even the patches from Stephen. Most of the patches I merged either fixed up some sizeof(...) calculations, made better use of the various kmalloc(...) variants, or fixed some checkpatch.pl reported problem. The first two types of patches, while technically noise, do bring value to the kernel by making the code a bit more robust, and the third, while much less valuable, does bring some consistency. I did tell Markus I was merging the checkpatch.pl fixes begrudgingly and I didn't particularly appreciate those patches; like Julia in the comments you linked, I tried to suggest Markus find a more productive way of contributing. That said, am I disappointed with myself that I didn't catch the bug Dan found? Of course I am. Am I annoyed that the regression was associated with a stupid, low-value patch? Of course I am. I'm very grateful to Dan that he caught the mistake (thanks again Dan!). While I understand Dan's move to ban Markus, from a maintainer's perspective it is easier that way, but I think the better approach is to encourage Markus to contribute more meaningful patches. I'm not going to reject patches simply because of their author, but I do reject patches that provide little value. We all started somewhere, and I bet some of us started with small patches, some of which were rejected; let's try to remember that when dealing with people like Markus. -- paul moore www.paul-moore.com -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] selinux: Fix an uninitialized variable bug @ 2017-04-03 21:45 ` Paul Moore 0 siblings, 0 replies; 24+ messages in thread From: Paul Moore @ 2017-04-03 21:45 UTC (permalink / raw) To: linux-security-module On Sun, Apr 2, 2017 at 9:10 PM, James Morris <jmorris@namei.org> wrote: > On Fri, 31 Mar 2017, Paul Moore wrote: > >> On Fri, Mar 31, 2017 at 11:52 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> > On Fri, 2017-03-31 at 18:21 +0300, Dan Carpenter wrote: >> >> We removed this initialization as a cleanup but it is probably >> >> required. >> >> >> >> The concern is that "nel" can be zero. I'm not an expert on SELinux >> >> code but I think it looks possible to write an SELinux policy which >> >> triggers this bug. GCC doesn't catch this, but my static checker >> >> does. >> >> >> >> Fixes: 9c312e79d6af ("selinux: Delete an unnecessary variable >> >> initialisation in range_read()") >> >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> > >> > Nice catch, thanks! >> > >> > Acked-by: Stephen Smalley <sds@tycho.nsa.gov> >> >> Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch. >> > > I'd like to reiterate that I generally don't want to accept cleanup > patches into the security tree from Markus (or indeed from others who > only do cleanup/whitespace work). > > See https://lkml.org/lkml/2017/1/29/172, and please click through and read > Dan's comments. > > All patches carry risks of introducing new bugs, and kernel "cleanup: > patches generally offer a pretty high cost/benefit ratio. If such patches > come from core developers of that code, or from kernel developers with > experience in *analyzing and fixing* bugs, that's very different. > > Paul, please review all of these patches very carefully before sending > your pull request. James, I know you don't track SELinux development very closely so I'm going to give you the benefit of the doubt and assume you haven't already seen my responses to Markus in his 46 patch thread. If you did follow the thread you would have noticed that I rejected several of his patches as only adding noise/churn, and several more due to personal preference; those that I did merge, I did review. I review every patch that I merge, even the patches from Stephen. Most of the patches I merged either fixed up some sizeof(...) calculations, made better use of the various kmalloc(...) variants, or fixed some checkpatch.pl reported problem. The first two types of patches, while technically noise, do bring value to the kernel by making the code a bit more robust, and the third, while much less valuable, does bring some consistency. I did tell Markus I was merging the checkpatch.pl fixes begrudgingly and I didn't particularly appreciate those patches; like Julia in the comments you linked, I tried to suggest Markus find a more productive way of contributing. That said, am I disappointed with myself that I didn't catch the bug Dan found? Of course I am. Am I annoyed that the regression was associated with a stupid, low-value patch? Of course I am. I'm very grateful to Dan that he caught the mistake (thanks again Dan!). While I understand Dan's move to ban Markus, from a maintainer's perspective it is easier that way, but I think the better approach is to encourage Markus to contribute more meaningful patches. I'm not going to reject patches simply because of their author, but I do reject patches that provide little value. We all started somewhere, and I bet some of us started with small patches, some of which were rejected; let's try to remember that when dealing with people like Markus. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] selinux: Fix an uninitialized variable bug 2017-04-03 21:45 ` Paul Moore (?) @ 2017-04-04 0:03 ` James Morris -1 siblings, 0 replies; 24+ messages in thread From: James Morris @ 2017-04-04 0:03 UTC (permalink / raw) To: Paul Moore Cc: Stephen Smalley, Dan Carpenter, Markus Elfring, Eric Paris, James Morris, Serge E. Hallyn, William Roberts, selinux, linux-security-module, kernel-janitors On Mon, 3 Apr 2017, Paul Moore wrote: > James, I know you don't track SELinux development very closely so I'm > going to give you the benefit of the doubt and assume you haven't > already seen my responses to Markus in his 46 patch thread. If you > did follow the thread you would have noticed that I rejected several > of his patches as only adding noise/churn, and several more due to > personal preference; those that I did merge, I did review. I review > every patch that I merge, even the patches from Stephen. I did see the thread, and my email came across as critical of you, which was not my intention. I do certainly trust your skill and judgement. - James -- James Morris <jmorris@namei.org> ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH] selinux: Fix an uninitialized variable bug @ 2017-04-04 0:03 ` James Morris 0 siblings, 0 replies; 24+ messages in thread From: James Morris @ 2017-04-04 0:03 UTC (permalink / raw) To: linux-security-module On Mon, 3 Apr 2017, Paul Moore wrote: > James, I know you don't track SELinux development very closely so I'm > going to give you the benefit of the doubt and assume you haven't > already seen my responses to Markus in his 46 patch thread. If you > did follow the thread you would have noticed that I rejected several > of his patches as only adding noise/churn, and several more due to > personal preference; those that I did merge, I did review. I review > every patch that I merge, even the patches from Stephen. I did see the thread, and my email came across as critical of you, which was not my intention. I do certainly trust your skill and judgement. - James -- James Morris <jmorris@namei.org> -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] selinux: Fix an uninitialized variable bug @ 2017-04-04 0:03 ` James Morris 0 siblings, 0 replies; 24+ messages in thread From: James Morris @ 2017-04-04 0:03 UTC (permalink / raw) To: linux-security-module On Mon, 3 Apr 2017, Paul Moore wrote: > James, I know you don't track SELinux development very closely so I'm > going to give you the benefit of the doubt and assume you haven't > already seen my responses to Markus in his 46 patch thread. If you > did follow the thread you would have noticed that I rejected several > of his patches as only adding noise/churn, and several more due to > personal preference; those that I did merge, I did review. I review > every patch that I merge, even the patches from Stephen. I did see the thread, and my email came across as critical of you, which was not my intention. I do certainly trust your skill and judgement. - James -- James Morris <jmorris@namei.org> ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2017-04-04 0:03 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-31 15:21 [PATCH] selinux: Fix an uninitialized variable bug Dan Carpenter 2017-03-31 15:21 ` Dan Carpenter 2017-03-31 15:21 ` Dan Carpenter 2017-03-31 15:52 ` Stephen Smalley 2017-03-31 15:52 ` Stephen Smalley 2017-03-31 15:52 ` Stephen Smalley 2017-03-31 19:18 ` Paul Moore 2017-03-31 19:18 ` Paul Moore 2017-03-31 19:18 ` Paul Moore 2017-04-01 6:40 ` selinux: Fix an uninitialized variable bug in range_read() SF Markus Elfring 2017-04-01 6:40 ` SF Markus Elfring 2017-04-01 6:40 ` SF Markus Elfring 2017-04-01 14:52 ` Paul Moore 2017-04-01 14:52 ` Paul Moore 2017-04-01 14:52 ` Paul Moore 2017-04-03 1:10 ` [PATCH] selinux: Fix an uninitialized variable bug James Morris 2017-04-03 1:10 ` James Morris 2017-04-03 1:10 ` James Morris 2017-04-03 21:45 ` Paul Moore 2017-04-03 21:45 ` Paul Moore 2017-04-03 21:45 ` Paul Moore 2017-04-04 0:03 ` James Morris 2017-04-04 0:03 ` James Morris 2017-04-04 0:03 ` James Morris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.