Re: [-next PATCH] security: use octal not symbolic permissions
On Wed, Jun 13, 2018 at 3:30 PM, Joe Perches wrote: > On Wed, 2018-06-13 at 12:19 -0400, Paul Moore wrote: >> On Wed, Jun 13, 2018 at 12:04 PM, Joe Perches wrote: >> > On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote: >> > > On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches wrote: >> > > > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote: >> > > > > Joe, in general I really appreciate the fixes you send, but these >> > > > > patches that cross a lot of subsystem boundaries (this isn't the >> > > > > first >> > > > > one that does this) causes unnecessary conflicts in -next and during >> > > > > the merge window. Could you split your patches up from now on >> > > > > please? >> > > > >> > > > Sorry. No. Merge conflicts are inherent in this system. >> > > >> > > Yes, merge conflicts are inherent in this system when one makes a >> > > single change which impacts multiple subsystems, e.g. changing a core >> > > kernel function which is called by multiple subsystems. However, that >> > > isn't what this patch does, it makes a number of self-contained >> > > changes across multiple subsystems; there are no cross-subsystem >> > > dependencies in this patch. You are increasing the likelihood of >> > > conflicts for no good reason; that is why I'm asking you to split this >> > > patch and others like it. >> > >> > No. History shows with high certainty that splitting >> > patches like this across multiple subsystems of a primary >> > subsystem means that the entire patchset is not completely >> > applied. >> >> I think that is due more to a lack of effort on the part of the patch >> author to keep pushing the individual patches. > > Nope. Try again. > > Resistance to change and desire for status quo > occurs in many subsystems. Which gets back to the need for persistence on the part of the patch author. If your solution to a stubborn susbsystem is to go around them by convincing another, potentially unrelated subsystem, to merge the patch then I firmly believe you are doing it wrong. >> > It's _much_ simpler and provides a generic mechanism to >> > get the entire patch applied to send a single patch to the >> > top level subsystem maintainer. >> >> I understand it is simpler for you, but it is more difficult for everyone >> else. > > Not true. I think we are at the agree to disagree stage. The way you have structured this patch it makes it easier for you to submit, but makes it potentially more difficult for me (likely other LSM maintainers too), the -next folks, and Linus. > It's simply a matter of merge resolution being pushed down > where and when necessary. > > See changes like the additions of the SPDX license tags. Please don't even try to suggest that this trivial patch you are proposing is even remotely as significant as the SPDX change. There are always going to be exceptions to every rule, and with each exception there needs to be a solid reason behind the change. The SPDX change had a legitimate reason (legal concern) for doing it the way it was done; this patch isn't close to that level of concern. >> Further, where the LSMs are concerned, there is no "top level >> subsystem maintainer" anymore. SELinux and AppArmor send pull >> requests directly to Linus. > > MAINTAINERS-SECURITY SUBSYSTEM > MAINTAINERS-M: James Morris > MAINTAINERS-M: "Serge E. Hallyn" > MAINTAINERS-L: linux-security-mod...@vger.kernel.org (suggested Cc:) > MAINTAINERS-T: git > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git > MAINTAINERS-W: http://kernsec.org/ > MAINTAINERS-S: Supported > MAINTAINERS:F: security/ > MAINTAINERS- > > If James is not approving or merging security/selinux or > security/tomoyo then perhaps the F: entries could be > augmented with appropriate X: entries or made specific > by using specific entries like: > > F: security/* > F: security/integrity/ > F: security/keys/ That is a good point. I'll put together a patch for selinux/next as soon as the merge window closes. I'll let the other LSMs do as they see fit. As I said previously, I believe the only other LSM that sends directly to Linux is AppArmor. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
On Wed, 2018-06-13 at 12:19 -0400, Paul Moore wrote: > On Wed, Jun 13, 2018 at 12:04 PM, Joe Perches wrote: > > On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote: > > > On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches wrote: > > > > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote: > > > > > Joe, in general I really appreciate the fixes you send, but these > > > > > patches that cross a lot of subsystem boundaries (this isn't the first > > > > > one that does this) causes unnecessary conflicts in -next and during > > > > > the merge window. Could you split your patches up from now on please? > > > > > > > > Sorry. No. Merge conflicts are inherent in this system. > > > > > > Yes, merge conflicts are inherent in this system when one makes a > > > single change which impacts multiple subsystems, e.g. changing a core > > > kernel function which is called by multiple subsystems. However, that > > > isn't what this patch does, it makes a number of self-contained > > > changes across multiple subsystems; there are no cross-subsystem > > > dependencies in this patch. You are increasing the likelihood of > > > conflicts for no good reason; that is why I'm asking you to split this > > > patch and others like it. > > > > No. History shows with high certainty that splitting > > patches like this across multiple subsystems of a primary > > subsystem means that the entire patchset is not completely > > applied. > > I think that is due more to a lack of effort on the part of the patch > author to keep pushing the individual patches. Nope. Try again. Resistance to change and desire for status quo occurs in many subsystems. > > It's _much_ simpler and provides a generic mechanism to > > get the entire patch applied to send a single patch to the > > top level subsystem maintainer. > > I understand it is simpler for you, but it is more difficult for everyone > else. Not true. It's simply a matter of merge resolution being pushed down where and when necessary. See changes like the additions of the SPDX license tags. > Further, where the LSMs are concerned, there is no "top level > subsystem maintainer" anymore. SELinux and AppArmor send pull > requests directly to Linus. MAINTAINERS-SECURITY SUBSYSTEM MAINTAINERS-M: James Morris MAINTAINERS-M: "Serge E. Hallyn" MAINTAINERS-L: linux-security-mod...@vger.kernel.org (suggested Cc:) MAINTAINERS-T: git git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git MAINTAINERS-W: http://kernsec.org/ MAINTAINERS-S: Supported MAINTAINERS:F: security/ MAINTAINERS- If James is not approving or merging security/selinux or security/tomoyo then perhaps the F: entries could be augmented with appropriate X: entries or made specific by using specific entries like: F: security/* F: security/integrity/ F: security/keys/ ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux-testsuite: Enhance inet_socket tests
On Wed, Jun 13, 2018 at 12:46 PM, Richard Haines
wrote:
> On Tue, 2018-06-12 at 18:02 -0400, Paul Moore wrote:
>> On Fri, Apr 13, 2018 at 6:13 AM, Richard Haines via Selinux
>> wrote:
>> > Enhance the tests as follows:
>> > 1) Determine number of tests to run with current config.
>> > 2) Add CALIPSO STREAM tests (DGRAM not supported in kernel. See
>> > [1]).
>> > 3) Add support for CIPSO TAGS 1 & 2. Closes [2].
>> > 4) Run scripts using /bin/sh.
>> > 5) Shorten sleep time as more tests.
>> >
>> > [1] https://github.com/SELinuxProject/selinux-kernel/issues/24
>> > [2] https://github.com/SELinuxProject/selinux-testsuite/issues/1
>> >
>> > Signed-off-by: Richard Haines
>> > ---
>> > tests/inet_socket/calipso-flush | 5 +
>> > tests/inet_socket/calipso-load | 7 +
>> > tests/inet_socket/cipso-fl-flush| 0
>> > tests/inet_socket/cipso-fl-load | 0
>> > tests/inet_socket/cipso-flush | 0
>> > tests/inet_socket/cipso-load-t1 | 11 +
>> > tests/inet_socket/cipso-load-t2 | 11 +
>> > tests/inet_socket/{cipso-load => cipso-load-t5} | 0
>> > tests/inet_socket/ipsec-flush | 0
>> > tests/inet_socket/ipsec-load| 0
>> > tests/inet_socket/iptables-flush| 0
>> > tests/inet_socket/iptables-load | 0
>> > tests/inet_socket/server.c | 16 +-
>> > tests/inet_socket/test | 348
>> > ++--
>> > 14 files changed, 310 insertions(+), 88 deletions(-)
>> > create mode 100644 tests/inet_socket/calipso-flush
>> > create mode 100644 tests/inet_socket/calipso-load
>> > mode change 100755 => 100644 tests/inet_socket/cipso-fl-flush
>> > mode change 100755 => 100644 tests/inet_socket/cipso-fl-load
>> > mode change 100755 => 100644 tests/inet_socket/cipso-flush
>> > create mode 100644 tests/inet_socket/cipso-load-t1
>> > create mode 100644 tests/inet_socket/cipso-load-t2
>> > rename tests/inet_socket/{cipso-load => cipso-load-t5} (100%)
>> > mode change 100755 => 100644
>> > mode change 100755 => 100644 tests/inet_socket/ipsec-flush
>> > mode change 100755 => 100644 tests/inet_socket/ipsec-load
>> > mode change 100755 => 100644 tests/inet_socket/iptables-flush
>> > mode change 100755 => 100644 tests/inet_socket/iptables-load
>> > mode change 100755 => 100644 tests/inet_socket/test
>>
>> I had to fixup the file mode bits on tests/inet_socket/test, but
>> other
>> than that this looks fine to me, merged. Thanks.
>
> The reason I have not been setting +x on the tests/*/test scripts is
> that the tests/Makefile does it for you. However as all the others are
> set, I'll set +x in future (as you flagged this on the sctp and binder
> patches I sent).
Please do. The issue is that whenever you run the tests it changes
the mode bits from how they are in the git repository. While not
really a problem for people who just take a snapshot of the tests, it
does cause problems for those of us who push/pull from the repo as it
registers as a change (check "git status").
>> I remain a little wary about the reduced sleep times (1s to 0.25s),
>> but I'm never comfortable with arbitrary sleep-and-hope-it-works
>> tricks anyway.
>
> I've been using this value in the SCTP tests for some time and not had
> any problems, that's why I used it for the inet tests (probably better
> to have the client try connecting x times and do away with the wait)
It's working on my test VMs, so from a selfish point of view I'm fine
with it for right now :) My concern isn't from an observed failure
with the change, but rather bad experiences with similar approaches on
other projects. In other words, I'm just being cranky.
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 01/13] selinux: Cleanup printk logging in conditional
On 6/12/18 11:23 PM, peter enderborg wrote:
On 06/12/2018 04:38 PM, Joe Perches wrote:
On Tue, 2018-06-12 at 10:09 +0200, Peter Enderborg wrote:
Replace printk with pr_* to avoid checkpatch warnings.
I believe it would be nicer to remove the
"SELinux: " prefix embbeded in each format
and use a specific
#define pr_fmt(fmt) "SELinux: " fmt
to automatically prefix these formats.
I cant argument about that, however some of the warnings and debug prints in
this set does not have this
so it will then change the actual output. (And I also think that they should
have a the prefix, but I don't
know why they don't) So I am not sure if it appropriate for a cleanup patch, it
supposed to have no functional change.
I would suggest that could be a follow-up patch.
I do like the cleanup, and it's better than the status quo.
Acked-by: Jay Freyensee
diff --git a/security/selinux/ss/conditional.c
b/security/selinux/ss/conditional.c
[]
@@ -96,7 +96,7 @@ int evaluate_cond_node(struct policydb *p, struct cond_node
*node)
if (new_state != node->cur_state) {
node->cur_state = new_state;
if (new_state == -1)
- printk(KERN_ERR "SELinux: expression result was undefined -
disabling all rules.\n");
+ pr_err("SELinux: expression result was undefined - disabling
all rules.\n");
/* turn the rules on or off */
for (cur = node->true_list; cur; cur = cur->next) {
if (new_state <= 0)
So, for instance, this patch could become:
(etc and so forth for each patch in this series)
---
security/selinux/ss/conditional.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/security/selinux/ss/conditional.c
b/security/selinux/ss/conditional.c
index c91543a617ac..e96820d92b61 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -7,6 +7,8 @@
*the Free Software Foundation, version 2.
*/
+#define pr_fmt(fmt) "SELinux: " fmt
+
#include
#include
#include
@@ -96,7 +98,7 @@ int evaluate_cond_node(struct policydb *p, struct cond_node
*node)
if (new_state != node->cur_state) {
node->cur_state = new_state;
if (new_state == -1)
- printk(KERN_ERR "SELinux: expression result was undefined -
disabling all rules.\n");
+ pr_err("expression result was undefined - disabling all
rules\n");
/* turn the rules on or off */
for (cur = node->true_list; cur; cur = cur->next) {
if (new_state <= 0)
@@ -287,7 +289,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key
*k, struct avtab_datum
*/
if (k->specified & AVTAB_TYPE) {
if (avtab_search(>te_avtab, k)) {
- printk(KERN_ERR "SELinux: type rule already exists outside
of a conditional.\n");
+ pr_err("type rule already exists outside of a
conditional\n");
goto err;
}
/*
@@ -302,7 +304,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key
*k, struct avtab_datum
node_ptr = avtab_search_node(>te_cond_avtab, k);
if (node_ptr) {
if (avtab_search_node_next(node_ptr,
k->specified)) {
- printk(KERN_ERR "SELinux: too many
conflicting type rules.\n");
+ pr_err("too many conflicting type
rules\n");
goto err;
}
found = 0;
@@ -313,13 +315,13 @@ static int cond_insertf(struct avtab *a, struct avtab_key
*k, struct avtab_datum
}
}
if (!found) {
- printk(KERN_ERR "SELinux: conflicting type
rules.\n");
+ pr_err("conflicting type rules\n");
goto err;
}
}
} else {
if (avtab_search(>te_cond_avtab, k)) {
- printk(KERN_ERR "SELinux: conflicting type rules
when adding type rule for true.\n");
+ pr_err("conflicting type rules when adding type rule
for true\n");
goto err;
}
}
@@ -327,7 +329,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key
*k, struct avtab_datum
node_ptr = avtab_insert_nonunique(>te_cond_avtab, k, d);
if (!node_ptr) {
- printk(KERN_ERR "SELinux: could not insert rule.\n");
+ pr_err("could not insert rule\n");
Re: [PATCH] selinux-testsuite: Enhance inet_socket tests
On Tue, 2018-06-12 at 18:02 -0400, Paul Moore wrote:
> On Fri, Apr 13, 2018 at 6:13 AM, Richard Haines via Selinux
> wrote:
> > Enhance the tests as follows:
> > 1) Determine number of tests to run with current config.
> > 2) Add CALIPSO STREAM tests (DGRAM not supported in kernel. See
> > [1]).
> > 3) Add support for CIPSO TAGS 1 & 2. Closes [2].
> > 4) Run scripts using /bin/sh.
> > 5) Shorten sleep time as more tests.
> >
> > [1] https://github.com/SELinuxProject/selinux-kernel/issues/24
> > [2] https://github.com/SELinuxProject/selinux-testsuite/issues/1
> >
> > Signed-off-by: Richard Haines
> > ---
> > tests/inet_socket/calipso-flush | 5 +
> > tests/inet_socket/calipso-load | 7 +
> > tests/inet_socket/cipso-fl-flush| 0
> > tests/inet_socket/cipso-fl-load | 0
> > tests/inet_socket/cipso-flush | 0
> > tests/inet_socket/cipso-load-t1 | 11 +
> > tests/inet_socket/cipso-load-t2 | 11 +
> > tests/inet_socket/{cipso-load => cipso-load-t5} | 0
> > tests/inet_socket/ipsec-flush | 0
> > tests/inet_socket/ipsec-load| 0
> > tests/inet_socket/iptables-flush| 0
> > tests/inet_socket/iptables-load | 0
> > tests/inet_socket/server.c | 16 +-
> > tests/inet_socket/test | 348
> > ++--
> > 14 files changed, 310 insertions(+), 88 deletions(-)
> > create mode 100644 tests/inet_socket/calipso-flush
> > create mode 100644 tests/inet_socket/calipso-load
> > mode change 100755 => 100644 tests/inet_socket/cipso-fl-flush
> > mode change 100755 => 100644 tests/inet_socket/cipso-fl-load
> > mode change 100755 => 100644 tests/inet_socket/cipso-flush
> > create mode 100644 tests/inet_socket/cipso-load-t1
> > create mode 100644 tests/inet_socket/cipso-load-t2
> > rename tests/inet_socket/{cipso-load => cipso-load-t5} (100%)
> > mode change 100755 => 100644
> > mode change 100755 => 100644 tests/inet_socket/ipsec-flush
> > mode change 100755 => 100644 tests/inet_socket/ipsec-load
> > mode change 100755 => 100644 tests/inet_socket/iptables-flush
> > mode change 100755 => 100644 tests/inet_socket/iptables-load
> > mode change 100755 => 100644 tests/inet_socket/test
>
> I had to fixup the file mode bits on tests/inet_socket/test, but
> other
> than that this looks fine to me, merged. Thanks.
The reason I have not been setting +x on the tests/*/test scripts is
that the tests/Makefile does it for you. However as all the others are
set, I'll set +x in future (as you flagged this on the sctp and binder
patches I sent).
>
> I remain a little wary about the reduced sleep times (1s to 0.25s),
> but I'm never comfortable with arbitrary sleep-and-hope-it-works
> tricks anyway.
I've been using this value in the SCTP tests for some time and not had
any problems, that's why I used it for the inet tests (probably better
to have the client try connecting x times and do away with the wait)
>
> > diff --git a/tests/inet_socket/calipso-flush
> > b/tests/inet_socket/calipso-flush
> > new file mode 100644
> > index 000..5143962
> > --- /dev/null
> > +++ b/tests/inet_socket/calipso-flush
> > @@ -0,0 +1,5 @@
> > +#!/bin/sh
> > +# Reset NetLabel configuration to unlabeled after CALIPSO/IPv6
> > tests.
> > +netlabelctl map del default
> > +netlabelctl calipso del doi:16
> > +netlabelctl map add default protocol:unlbl
> > diff --git a/tests/inet_socket/calipso-load
> > b/tests/inet_socket/calipso-load
> > new file mode 100644
> > index 000..4bb9c7f
> > --- /dev/null
> > +++ b/tests/inet_socket/calipso-load
> > @@ -0,0 +1,7 @@
> > +#!/bin/sh
> > +# Define a doi for testing loopback for CALIPSO/IPv6.
> > +netlabelctl calipso add pass doi:16
> > +netlabelctl map del default
> > +netlabelctl map add default address:0.0.0.0/0 protocol:unlbl
> > +netlabelctl map add default address:::/0 protocol:unlbl
> > +netlabelctl map add default address:::1 protocol:calipso,16
> > diff --git a/tests/inet_socket/cipso-fl-flush
> > b/tests/inet_socket/cipso-fl-flush
> > old mode 100755
> > new mode 100644
> > diff --git a/tests/inet_socket/cipso-fl-load
> > b/tests/inet_socket/cipso-fl-load
> > old mode 100755
> > new mode 100644
> > diff --git a/tests/inet_socket/cipso-flush
> > b/tests/inet_socket/cipso-flush
> > old mode 100755
> > new mode 100644
> > diff --git a/tests/inet_socket/cipso-load-t1
> > b/tests/inet_socket/cipso-load-t1
> > new file mode 100644
> > index 000..974e746
> > --- /dev/null
> > +++ b/tests/inet_socket/cipso-load-t1
> > @@ -0,0 +1,11 @@
> > +#!/bin/sh
> > +# Based on http://paulmoore.livejournal.com/7234.html.
> > +#
> > +# Modifications:
> > +# - Defined a doi for testing loopback for CIPSOv4.
> > +
> > +netlabelctl cipsov4 add pass doi:16 tags:1
> > +netlabelctl map del default
> > +netlabelctl map add default address:0.0.0.0/0
Re: [-next PATCH] security: use octal not symbolic permissions
On Wed, Jun 13, 2018 at 12:04 PM, Joe Perches wrote: > On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote: >> On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches wrote: >> > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote: >> > > Joe, in general I really appreciate the fixes you send, but these >> > > patches that cross a lot of subsystem boundaries (this isn't the first >> > > one that does this) causes unnecessary conflicts in -next and during >> > > the merge window. Could you split your patches up from now on please? >> > >> > Sorry. No. Merge conflicts are inherent in this system. >> >> Yes, merge conflicts are inherent in this system when one makes a >> single change which impacts multiple subsystems, e.g. changing a core >> kernel function which is called by multiple subsystems. However, that >> isn't what this patch does, it makes a number of self-contained >> changes across multiple subsystems; there are no cross-subsystem >> dependencies in this patch. You are increasing the likelihood of >> conflicts for no good reason; that is why I'm asking you to split this >> patch and others like it. > > No. History shows with high certainty that splitting > patches like this across multiple subsystems of a primary > subsystem means that the entire patchset is not completely > applied. I think that is due more to a lack of effort on the part of the patch author to keep pushing the individual patches. > It's _much_ simpler and provides a generic mechanism to > get the entire patch applied to send a single patch to the > top level subsystem maintainer. I understand it is simpler for you, but it is more difficult for everyone else. Further, where the LSMs are concerned, there is no "top level subsystem maintainer" anymore. SELinux and AppArmor send pull requests directly to Linus. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote: > On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches wrote: > > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote: > > > Joe, in general I really appreciate the fixes you send, but these > > > patches that cross a lot of subsystem boundaries (this isn't the first > > > one that does this) causes unnecessary conflicts in -next and during > > > the merge window. Could you split your patches up from now on please? > > > > Sorry. No. Merge conflicts are inherent in this system. > > Yes, merge conflicts are inherent in this system when one makes a > single change which impacts multiple subsystems, e.g. changing a core > kernel function which is called by multiple subsystems. However, that > isn't what this patch does, it makes a number of self-contained > changes across multiple subsystems; there are no cross-subsystem > dependencies in this patch. You are increasing the likelihood of > conflicts for no good reason; that is why I'm asking you to split this > patch and others like it. No. History shows with high certainty that splitting patches like this across multiple subsystems of a primary subsystem means that the entire patchset is not completely applied. It's _much_ simpler and provides a generic mechanism to get the entire patch applied to send a single patch to the top level subsystem maintainer. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches wrote: > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote: >> Joe, in general I really appreciate the fixes you send, but these >> patches that cross a lot of subsystem boundaries (this isn't the first >> one that does this) causes unnecessary conflicts in -next and during >> the merge window. Could you split your patches up from now on please? > > Sorry. No. Merge conflicts are inherent in this system. Yes, merge conflicts are inherent in this system when one makes a single change which impacts multiple subsystems, e.g. changing a core kernel function which is called by multiple subsystems. However, that isn't what this patch does, it makes a number of self-contained changes across multiple subsystems; there are no cross-subsystem dependencies in this patch. You are increasing the likelihood of conflicts for no good reason; that is why I'm asking you to split this patch and others like it. > There is just no good way to do this as sending a set > of per subsystem patches guarantees partial application > of the entire set. Please explain why all of these changes need to be made at the same time? Looking quickly at the patch it would appear that each chunk of the patch could be applied independently and the kernel would still build and operate correctly. I'm not suggesting that you decompose this patch with that level of granularity, that would be silly, but splitting this patch (and many of the others you tend to submit) at subsystem boundaries should be possible. Further, as one could see from the responses of the other LSM maintainers, splitting this patch is not just possible, it is desirable. > If you prefer, each sub-subsystem maintainer, at > whatever granularity desired, could apply the patch > to the appropriate subsystem by using > "git am --include=". Or you could split the patch up by subsystem before posting, like so many other developers do already. -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
Quoting Joe Perches (j...@perches.com):
> Currently security files use a mixture of octal and symbolic styles
> for permissions.
>
> Using octal and not symbolic permissions is preferred by many as more
> readable.
>
> see: https://lkml.org/lkml/2016/8/2/1945
Heh, well see also https://lkml.org/lkml/2016/8/2/2062 . Your patch
definately improves readability, but will doubtless make backpointing
future important patches more painful.
> Prefer the direct use of octal for permissions.
>
> Done using:
>
> $ git ls-files security | \
> xargs ./scripts/checkpatch.pl -f --fix-inplace --types=symbolic_perms
> --strict
>
> and some typing.
>
> Before:$ git grep -P -w "0[0-7]{3,3}" security | wc -l
> 53
> After: $ git grep -P -w "0[0-7]{3,3}" security | wc -l
> 136
>
> Miscellanea:
>
> o Whitespace neatening and line wrapping around these conversions.
> o Remove now superfluous parentheses around direct use of 0600
>
> Signed-off-by: Joe Perches
> ---
> security/apparmor/apparmorfs.c | 5 ++--
> security/apparmor/lsm.c | 23 -
> security/integrity/ima/ima.h| 4 +--
> security/integrity/ima/ima_fs.c | 13 +-
> security/selinux/hooks.c| 4 +--
> security/selinux/selinuxfs.c| 57
> -
> security/smack/smack_lsm.c | 6 ++---
> security/smack/smackfs.c| 46 -
> security/tomoyo/condition.c | 18 ++---
> 9 files changed, 85 insertions(+), 91 deletions(-)
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 949dd8a48164..c09dc0f3c3fe 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -2426,10 +2426,9 @@ static int aa_mk_null_file(struct dentry *parent)
> }
>
> inode->i_ino = get_next_ino();
> - inode->i_mode = S_IFCHR | S_IRUGO | S_IWUGO;
> + inode->i_mode = S_IFCHR | 0666;
> inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
> - init_special_inode(inode, S_IFCHR | S_IRUGO | S_IWUGO,
> -MKDEV(MEM_MAJOR, 3));
> + init_special_inode(inode, S_IFCHR | 0666, MKDEV(MEM_MAJOR, 3));
> d_instantiate(dentry, inode);
> aa_null.dentry = dget(dentry);
> aa_null.mnt = mntget(mount);
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index fbb08bc78bee..6759a70918de 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1255,45 +1255,42 @@ static int param_get_mode(char *buffer, const struct
> kernel_param *kp);
> /* AppArmor global enforcement switch - complain, enforce, kill */
> enum profile_mode aa_g_profile_mode = APPARMOR_ENFORCE;
> module_param_call(mode, param_set_mode, param_get_mode,
> - _g_profile_mode, S_IRUSR | S_IWUSR);
> + _g_profile_mode, 0600);
>
> /* whether policy verification hashing is enabled */
> bool aa_g_hash_policy = IS_ENABLED(CONFIG_SECURITY_APPARMOR_HASH_DEFAULT);
> #ifdef CONFIG_SECURITY_APPARMOR_HASH
> -module_param_named(hash_policy, aa_g_hash_policy, aabool, S_IRUSR | S_IWUSR);
> +module_param_named(hash_policy, aa_g_hash_policy, aabool, 0600);
> #endif
>
> /* Debug mode */
> bool aa_g_debug = IS_ENABLED(CONFIG_SECURITY_APPARMOR_DEBUG_MESSAGES);
> -module_param_named(debug, aa_g_debug, aabool, S_IRUSR | S_IWUSR);
> +module_param_named(debug, aa_g_debug, aabool, 0600);
>
> /* Audit mode */
> enum audit_mode aa_g_audit;
> -module_param_call(audit, param_set_audit, param_get_audit,
> - _g_audit, S_IRUSR | S_IWUSR);
> +module_param_call(audit, param_set_audit, param_get_audit, _g_audit,
> 0600);
>
> /* Determines if audit header is included in audited messages. This
> * provides more context if the audit daemon is not running
> */
> bool aa_g_audit_header = true;
> -module_param_named(audit_header, aa_g_audit_header, aabool,
> -S_IRUSR | S_IWUSR);
> +module_param_named(audit_header, aa_g_audit_header, aabool, 0600);
>
> /* lock out loading/removal of policy
> * TODO: add in at boot loading of policy, which is the only way to
> * load policy, if lock_policy is set
> */
> bool aa_g_lock_policy;
> -module_param_named(lock_policy, aa_g_lock_policy, aalockpolicy,
> -S_IRUSR | S_IWUSR);
> +module_param_named(lock_policy, aa_g_lock_policy, aalockpolicy, 0600);
>
> /* Syscall logging mode */
> bool aa_g_logsyscall;
> -module_param_named(logsyscall, aa_g_logsyscall, aabool, S_IRUSR | S_IWUSR);
> +module_param_named(logsyscall, aa_g_logsyscall, aabool, 0600);
>
> /* Maximum pathname length before accesses will start getting rejected */
> unsigned int aa_g_path_max = 2 * PATH_MAX;
> -module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
> +module_param_named(path_max, aa_g_path_max, aauint, 0400);
>
> /* Determines how paranoid loading of policy is and how much verification
> * on the loaded
Re: [-next PATCH] security: use octal not symbolic permissions
On Tue, 2018-06-12 at 14:29 -0700, John Johansen wrote: > On 06/12/2018 02:12 PM, Paul Moore wrote: > > On Tue, Jun 12, 2018 at 4:32 PM, James Morris wrote: > >> On Mon, 11 Jun 2018, Casey Schaufler wrote: > >> > >>> If you want to break this up by security module I would take > >>> the Smack part as soon as James does the tree update. If James > >>> wants to take the whole thing at once you can add my: > >>> > >>> Acked-by: Casey Schaufler > >>> > >>> for the Smack changes. > >> > >> It's probably simplest for me to take them as one patch. > > > > I would prefer if the SELinux changes were split into a separate > > patch. I'm guessing John would probably want the same for the > > AppArmor patches, but take his work for it, not mine. > > yes that would be preferred Agreed > > > > > Joe, in general I really appreciate the fixes you send, but these > > patches that cross a lot of subsystem boundaries (this isn't the first > > one that does this) causes unnecessary conflicts in -next and during > > the merge window. Could you split your patches up from now on please? > > > > yeah splitting patches at subsystem boundaries is highly recommended. Agreed ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH 01/13] selinux: Cleanup printk logging in conditional
On 06/12/2018 04:38 PM, Joe Perches wrote:
> On Tue, 2018-06-12 at 10:09 +0200, Peter Enderborg wrote:
>> Replace printk with pr_* to avoid checkpatch warnings.
> I believe it would be nicer to remove the
> "SELinux: " prefix embbeded in each format
> and use a specific
>
> #define pr_fmt(fmt) "SELinux: " fmt
>
> to automatically prefix these formats.
I cant argument about that, however some of the warnings and debug prints in
this set does not have this
so it will then change the actual output. (And I also think that they should
have a the prefix, but I don't
know why they don't) So I am not sure if it appropriate for a cleanup patch, it
supposed to have no functional change.
>> diff --git a/security/selinux/ss/conditional.c
>> b/security/selinux/ss/conditional.c
> []
>> @@ -96,7 +96,7 @@ int evaluate_cond_node(struct policydb *p, struct
>> cond_node *node)
>> if (new_state != node->cur_state) {
>> node->cur_state = new_state;
>> if (new_state == -1)
>> -printk(KERN_ERR "SELinux: expression result was
>> undefined - disabling all rules.\n");
>> +pr_err("SELinux: expression result was undefined -
>> disabling all rules.\n");
>> /* turn the rules on or off */
>> for (cur = node->true_list; cur; cur = cur->next) {
>> if (new_state <= 0)
> So, for instance, this patch could become:
> (etc and so forth for each patch in this series)
>
> ---
> security/selinux/ss/conditional.c | 18 ++
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/security/selinux/ss/conditional.c
> b/security/selinux/ss/conditional.c
> index c91543a617ac..e96820d92b61 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -7,6 +7,8 @@
> * the Free Software Foundation, version 2.
> */
>
> +#define pr_fmt(fmt) "SELinux: " fmt
> +
> #include
> #include
> #include
> @@ -96,7 +98,7 @@ int evaluate_cond_node(struct policydb *p, struct cond_node
> *node)
> if (new_state != node->cur_state) {
> node->cur_state = new_state;
> if (new_state == -1)
> - printk(KERN_ERR "SELinux: expression result was
> undefined - disabling all rules.\n");
> + pr_err("expression result was undefined - disabling all
> rules\n");
> /* turn the rules on or off */
> for (cur = node->true_list; cur; cur = cur->next) {
> if (new_state <= 0)
> @@ -287,7 +289,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key
> *k, struct avtab_datum
>*/
> if (k->specified & AVTAB_TYPE) {
> if (avtab_search(>te_avtab, k)) {
> - printk(KERN_ERR "SELinux: type rule already exists
> outside of a conditional.\n");
> + pr_err("type rule already exists outside of a
> conditional\n");
> goto err;
> }
> /*
> @@ -302,7 +304,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key
> *k, struct avtab_datum
> node_ptr = avtab_search_node(>te_cond_avtab, k);
> if (node_ptr) {
> if (avtab_search_node_next(node_ptr,
> k->specified)) {
> - printk(KERN_ERR "SELinux: too many
> conflicting type rules.\n");
> + pr_err("too many conflicting type
> rules\n");
> goto err;
> }
> found = 0;
> @@ -313,13 +315,13 @@ static int cond_insertf(struct avtab *a, struct
> avtab_key *k, struct avtab_datum
> }
> }
> if (!found) {
> - printk(KERN_ERR "SELinux: conflicting
> type rules.\n");
> + pr_err("conflicting type rules\n");
> goto err;
> }
> }
> } else {
> if (avtab_search(>te_cond_avtab, k)) {
> - printk(KERN_ERR "SELinux: conflicting type
> rules when adding type rule for true.\n");
> + pr_err("conflicting type rules when adding type
> rule for true\n");
> goto err;
> }
> }
> @@ -327,7 +329,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key
> *k, struct avtab_datum
>
> node_ptr = avtab_insert_nonunique(>te_cond_avtab, k, d);
> if (!node_ptr) {
> - printk(KERN_ERR "SELinux: could not insert rule.\n");
> + pr_err("could not insert rule\n");
> rc = -ENOMEM;
> goto err;
> }
> @@ -387,12 +389,12 @@ static int
Re: [-next PATCH] security: use octal not symbolic permissions
On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote: > On Tue, Jun 12, 2018 at 4:32 PM, James Morris wrote: > > On Mon, 11 Jun 2018, Casey Schaufler wrote: > > > > > If you want to break this up by security module I would take > > > the Smack part as soon as James does the tree update. If James > > > wants to take the whole thing at once you can add my: > > > > > > Acked-by: Casey Schaufler > > > > > > for the Smack changes. > > > > It's probably simplest for me to take them as one patch. > > I would prefer if the SELinux changes were split into a separate > patch. I'm guessing John would probably want the same for the > AppArmor patches, but take his work for it, not mine. > > Joe, in general I really appreciate the fixes you send, but these > patches that cross a lot of subsystem boundaries (this isn't the first > one that does this) causes unnecessary conflicts in -next and during > the merge window. Could you split your patches up from now on please? Sorry. No. Merge conflicts are inherent in this system. There is just no good way to do this as sending a set of per subsystem patches guarantees partial application of the entire set. The nominal best way is for a script to be run and applied at the top level rather than sending a patch at all. If you prefer, each sub-subsystem maintainer, at whatever granularity desired, could apply the patch to the appropriate subsystem by using "git am --include=". cheers, Joe ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
On 06/12/2018 02:12 PM, Paul Moore wrote: > On Tue, Jun 12, 2018 at 4:32 PM, James Morris wrote: >> On Mon, 11 Jun 2018, Casey Schaufler wrote: >> >>> If you want to break this up by security module I would take >>> the Smack part as soon as James does the tree update. If James >>> wants to take the whole thing at once you can add my: >>> >>> Acked-by: Casey Schaufler >>> >>> for the Smack changes. >> >> It's probably simplest for me to take them as one patch. > > I would prefer if the SELinux changes were split into a separate > patch. I'm guessing John would probably want the same for the > AppArmor patches, but take his work for it, not mine. yes that would be preferred > > Joe, in general I really appreciate the fixes you send, but these > patches that cross a lot of subsystem boundaries (this isn't the first > one that does this) causes unnecessary conflicts in -next and during > the merge window. Could you split your patches up from now on please? > yeah splitting patches at subsystem boundaries is highly recommended. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
On Tue, Jun 12, 2018 at 4:32 PM, James Morris wrote: > On Mon, 11 Jun 2018, Casey Schaufler wrote: > >> If you want to break this up by security module I would take >> the Smack part as soon as James does the tree update. If James >> wants to take the whole thing at once you can add my: >> >> Acked-by: Casey Schaufler >> >> for the Smack changes. > > It's probably simplest for me to take them as one patch. I would prefer if the SELinux changes were split into a separate patch. I'm guessing John would probably want the same for the AppArmor patches, but take his work for it, not mine. Joe, in general I really appreciate the fixes you send, but these patches that cross a lot of subsystem boundaries (this isn't the first one that does this) causes unnecessary conflicts in -next and during the merge window. Could you split your patches up from now on please? -- paul moore www.paul-moore.com ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [-next PATCH] security: use octal not symbolic permissions
On Mon, 11 Jun 2018, Casey Schaufler wrote: > If you want to break this up by security module I would take > the Smack part as soon as James does the tree update. If James > wants to take the whole thing at once you can add my: > > Acked-by: Casey Schaufler > > for the Smack changes. It's probably simplest for me to take them as one patch. -- James Morris ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
