[PATCH] selinux: Fix an uninitialized variable bug

[PATCH] selinux: Fix an uninitialized variable bug

[Dan Carpenter]

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];

Re: [PATCH] selinux: Fix an uninitialized variable bug

[Stephen Smalley]

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];

Re: [PATCH] selinux: Fix an uninitialized variable bug

[Paul Moore]

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

Re: selinux: Fix an uninitialized variable bug in range_read()

[SF Markus Elfring]

>>> 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

Re: selinux: Fix an uninitialized variable bug in range_read()

[Paul Moore]

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

Re: [PATCH] selinux: Fix an uninitialized variable bug

[James Morris]

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>

Re: [PATCH] selinux: Fix an uninitialized variable bug

[Paul Moore]

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

Re: [PATCH] selinux: Fix an uninitialized variable bug

[James Morris]

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>