selinux: Fix an uninitialized variable bug
Subsystems (3)
PATCH 1/1 selinux: Fix an uninitialized variable bug
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.
security/selinux/ss/policydb.c +1 -1
@@ -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 ];
Quoted 11 lines · 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 variableinitialisation in
range_read()")Signed-off-by Dan Carpenter <[email protected]>
Nice catch, thanks!
Quoted 16 lines
security/selinux/ss/policydb.c +0
-0@@ -1832,7 +1832,7 @@ u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name)
static intrange_read(struct policydb *p, void *fp){security/selinux/ss/policydb.c +1
-1-struct range_trans *rt; +struct range_trans *rt =NULL ;struct
mls_range*r = NULL;
int i, rc;
__le32buf[2];
Paul Moore
maintainerQuoted 9 lines · Stephen Smalley
Quoted 8 lines · 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.
Quoted 7 lines
Quoted 3 lines
Fixes 9c312e79d6af ("selinux: Delete an unnecessary variableinitialisation in
range_read()")Signed-off-by Dan Carpenter <[email protected]>Nice catch, thanks!
Acked-by Stephen Smalley <[email protected]>
Yes, indeed. Thanks Dan, I should have caught this when merging Markus' patch.
Merged.
Quoted 15 lines
Quoted 15 lines
security/selinux/ss/policydb.c +1
-1@@ -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];
Signature · 2 lines
paul moore www.paul-moore.com
Re: selinux: Fix an uninitialized variable bug in range_read()
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
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 variableinitialisation in
range_read()")Signed-off-by Dan Carpenter <[email protected]>
Quoted 5 lines
Quoted 3 lines
Nice catch, thanks!
Acked-by Stephen Smalley <[email protected]>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
Paul Moore
maintainerRe: selinux: Fix an uninitialized variable bug in range_read()
Quoted 14 lines · SF Markus Elfring
Quoted 14 lines
Quoted 14 lines
Quoted 11 lines
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 variableinitialisation in
range_read()")Signed-off-by Dan Carpenter <[email protected]>Nice catch, thanks!
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Acked-by Stephen Smalley <[email protected]>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.
Signature · 2 lines
paul moore www.paul-moore.com
James Morris
maintainerQuoted 14 lines · Paul Moore
Quoted 13 lines · Stephen Smalley
Quoted 11 lines · 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 variableinitialisation in
range_read()")Signed-off-by Dan Carpenter <[email protected]>
Quoted 6 lines
Quoted 3 lines
Nice catch, thanks!
Acked-by Stephen Smalley <[email protected]>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.
Signature · 2 lines
James Morris <[email protected]>
Paul Moore
maintainerQuoted 30 lines · James Morris
Quoted 14 lines · Paul Moore
Quoted 13 lines · Stephen Smalley
Quoted 11 lines · 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 variableinitialisation in
range_read()")Signed-off-by Dan Carpenter <[email protected]>Quoted 6 lines
Quoted 3 lines
Nice catch, thanks!
Acked-by Stephen Smalley <[email protected]>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.
Quoted 7 lines
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.
Signature · 2 lines
paul moore www.paul-moore.com
James Morris
maintainerQuoted 7 lines · Paul Moore
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
Signature · 2 lines
James Morris <[email protected]>