| Message ID | 20170331152118.GA8141@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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];
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];
>>> 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
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.
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.
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.
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
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];
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>