Re: [PATCH v2] hooks: fix a missing-check bug in selinux_sb_eat_lsm_opts()
On Thu, May 30, 2019 at 9:34 PM Gen Zhang wrote:
>
> In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> returns NULL when fails. So 'arg' should be checked.
>
> Signed-off-by: Gen Zhang
> Reviewed-by: Ondrej Mosnacek
> Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
One quick note about the subject line, instead of using "hooks:" you
should use "selinux:" since this is specific to SELinux. If the patch
did apply to the LSM framework as a whole, I would suggest using
"lsm:" instead of "hooks:" as "hooks" is too ambiguous of a prefix.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702c..5a9e959 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2635,6 +2635,8 @@ static int selinux_sb_eat_lsm_opts(char *options, void
> **mnt_opts)
> *q++ = c;
> }
> arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
> + if (!arg)
> + return -ENOMEM;
It seems like we should also check for, and potentially free *mnt_opts
as the selinux_add_opt() error handling does just below this change,
yes? If that is the case we might want to move that error handling
code to the bottom of the function and jump there on error.
> }
> rc = selinux_add_opt(token, arg, mnt_opts);
> if (unlikely(rc)) {
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V6 02/10] audit: add container id
On Mon, Jul 8, 2019 at 2:06 PM Richard Guy Briggs wrote: > On 2019-05-30 15:29, Paul Moore wrote: ... > > [REMINDER: It is an "*audit* container ID" and not a general > > "container ID" ;) Smiley aside, I'm not kidding about that part.] > > > > I'm not interested in supporting/merging something that isn't useful; > > if this doesn't work for your use case then we need to figure out what > > would work. It sounds like nested containers are much more common in > > the lxc world, can you elaborate a bit more on this? > > > > As far as the possible solutions you mention above, I'm not sure I > > like the per-userns audit container IDs, I'd much rather just emit the > > necessary tracking information via the audit record stream and let the > > log analysis tools figure it out. However, the bigger question is how > > to limit (re)setting the audit container ID when you are in a non-init > > userns. For reasons already mentioned, using capable() is a non > > starter for everything but the initial userns, and using ns_capable() > > is equally poor as it essentially allows any userns the ability to > > munge it's audit container ID (obviously not good). It appears we > > need a different method for controlling access to the audit container > > ID. > > We're not quite ready yet for multiple audit daemons and possibly not > yet for audit namespaces, but this is starting to look a lot like the > latter. A few quick comments on audit namespaces: the audit container ID is not envisioned as a new namespace (even in nested form) and neither do I consider running multiple audit daemons to be a new namespace. >From my perspective we create namespaces to allow us to redefine a global resource for some subset of the system, e.g. providing a unique /tmp for some number of processes on the system. While it may be tempting to think of the audit container ID as something we could "namespace", especially when multiple audit daemons are concerned, in some ways this would be counter productive; the audit container ID is intended to be a global ID that can be used to associate audit event records with a "container" where the "container" is defined by an orchestrator outside the audit subsystem. The global nature of the audit container ID allows us to maintain a sane(ish) view of the system in the audit log, if we were to "namespace" the audit container ID such that the value was no longer guaranteed to be unique throughout the system, we would need to additionally track the audit namespace along with the audit container ID which starts to border on insanity IMHO. > If we can't trust ns_capable() then why are we passing on > CAP_AUDIT_CONTROL? It is being passed down and not stripped purposely > by the orchestrator/engine. If ns_capable() isn't inherited how is it > gained otherwise? Can it be inserted by cotainer image? I think the > answer is "no". Either we trust ns_capable() or we have audit > namespaces (recommend based on user namespace) (or both). My thinking is that since ns_capable() checks the credentials with respect to the current user namespace we can't rely on it to control access since it would be possible for a privileged process running inside an unprivileged container to manipulate the audit container ID (containerized process has CAP_AUDIT_CONTROL, e.g. running as root in the container, while the container itself does not). > At this point I would say we are at an impasse unless we trust > ns_capable() or we implement audit namespaces. I'm not sure how we can trust ns_capable(), but if you can think of a way I would love to hear it. I'm also not sure how namespacing audit is helpful (see my above comments), but if you think it is please explain. > I don't think another mechanism to trust nested orchestrators/engines > will buy us anything. > > Am I missing something? Based on your questions/comments above it looks like your understanding of ns_capable() does not match mine; if I'm thinking about ns_capable() incorrectly, please educate me. -- paul moore www.paul-moore.com
Re: [PATCH ghak90 v10 01/11] audit: collect audit task parameters
On Mon, Dec 21, 2020 at 11:57 AM Richard Guy Briggs wrote: > > The audit-related parameters in struct task_struct should ideally be > collected together and accessed through a standard audit API and the audit > structures made opaque to other kernel subsystems. > > Collect the existing loginuid, sessionid and audit_context together in a > new opaque struct audit_task_info called "audit" in struct task_struct. > > Use kmem_cache to manage this pool of memory. > Un-inline audit_free() to be able to always recover that memory. > > Please see the upstream github issues > https://github.com/linux-audit/audit-kernel/issues/81 > https://github.com/linux-audit/audit-kernel/issues/90 > > Signed-off-by: Richard Guy Briggs > Acked-by: Neil Horman > Reviewed-by: Ondrej Mosnacek Did Neil and Ondrej really ACK/Review the changes that you made here in v10 or are you just carrying over the ACK/Review? I'm hopeful it is the former, because I'm going to be a little upset if it is the latter. > --- > fs/io-wq.c| 8 +-- > fs/io_uring.c | 16 ++--- > include/linux/audit.h | 49 +- > include/linux/sched.h | 7 +- > init/init_task.c | 3 +- > init/main.c | 2 + > kernel/audit.c| 154 +- > kernel/audit.h| 7 ++ > kernel/auditsc.c | 24 ++++--- > kernel/fork.c | 1 - > 10 files changed, 205 insertions(+), 66 deletions(-) -- paul moore www.paul-moore.com
[PATCH] lsm,selinux: pass flowi_common instead of flowi to the LSM hooks
As pointed out by Herbert in a recent related patch, the LSM hooks do
not have the necessary address family information to use the flowi
struct safely. As none of the LSMs currently use any of the protocol
specific flowi information, replace the flowi pointers with pointers
to the address family independent flowi_common struct.
Reported-by: Herbert Xu
Signed-off-by: Paul Moore
---
.../chelsio/inline_crypto/chtls/chtls_cm.c |2 +-
drivers/net/wireguard/socket.c |4 ++-
include/linux/lsm_hook_defs.h |4 ++-
include/linux/lsm_hooks.h |2 +-
include/linux/security.h | 23
include/net/flow.h | 10 +
include/net/route.h|6 +++--
net/dccp/ipv4.c|2 +-
net/dccp/ipv6.c|6 +++--
net/ipv4/icmp.c|4 ++-
net/ipv4/inet_connection_sock.c|4 ++-
net/ipv4/ip_output.c |2 +-
net/ipv4/ping.c|2 +-
net/ipv4/raw.c |2 +-
net/ipv4/syncookies.c |2 +-
net/ipv4/udp.c |2 +-
net/ipv6/af_inet6.c|2 +-
net/ipv6/datagram.c|2 +-
net/ipv6/icmp.c|6 +++--
net/ipv6/inet6_connection_sock.c |4 ++-
net/ipv6/netfilter/nf_reject_ipv6.c|2 +-
net/ipv6/ping.c|2 +-
net/ipv6/raw.c |2 +-
net/ipv6/syncookies.c |2 +-
net/ipv6/tcp_ipv6.c|4 ++-
net/ipv6/udp.c |2 +-
net/l2tp/l2tp_ip6.c|2 +-
net/netfilter/nf_synproxy_core.c |2 +-
net/xfrm/xfrm_state.c |6 +++--
security/security.c| 17 ---
security/selinux/hooks.c |4 ++-
security/selinux/include/xfrm.h|2 +-
security/selinux/xfrm.c| 13 ++-
33 files changed, 85 insertions(+), 66 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
index ec4f79049a06..42e4e43cdd91 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c
@@ -1148,7 +1148,7 @@ static struct sock *chtls_recv_sock(struct sock *lsk,
fl6.daddr = ip6h->saddr;
fl6.fl6_dport = inet_rsk(oreq)->ir_rmt_port;
fl6.fl6_sport = htons(inet_rsk(oreq)->ir_num);
- security_req_classify_flow(oreq, flowi6_to_flowi(&fl6));
+ security_req_classify_flow(oreq, flowi6_to_flowi_common(&fl6));
dst = ip6_dst_lookup_flow(sock_net(lsk), lsk, &fl6, NULL);
if (IS_ERR(dst))
goto free_sk;
diff --git a/drivers/net/wireguard/socket.c b/drivers/net/wireguard/socket.c
index c33e2c81635f..410b318e57fb 100644
--- a/drivers/net/wireguard/socket.c
+++ b/drivers/net/wireguard/socket.c
@@ -49,7 +49,7 @@ static int send4(struct wg_device *wg, struct sk_buff *skb,
rt = dst_cache_get_ip4(cache, &fl.saddr);
if (!rt) {
- security_sk_classify_flow(sock, flowi4_to_flowi(&fl));
+ security_sk_classify_flow(sock, flowi4_to_flowi_common(&fl));
if (unlikely(!inet_confirm_addr(sock_net(sock), NULL, 0,
fl.saddr, RT_SCOPE_HOST))) {
endpoint->src4.s_addr = 0;
@@ -129,7 +129,7 @@ static int send6(struct wg_device *wg, struct sk_buff *skb,
dst = dst_cache_get_ip6(cache, &fl.saddr);
if (!dst) {
- security_sk_classify_flow(sock, flowi6_to_flowi(&fl));
+ security_sk_classify_flow(sock, flowi6_to_flowi_common(&fl));
if (unlikely(!ipv6_addr_any(&fl.saddr) &&
!ipv6_chk_addr(sock_net(sock), &fl.saddr, NULL,
0))) {
endpoint->src6 = fl.saddr = in6addr_any;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 32a940117e7a..f70984c8318b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -311,7 +311,7 @@ LSM_HOOK(int, 0, secmark_relabel_packet, u32 secid)
LSM_HOOK(void, LSM_RET_VOID, secmark_refcount_
Re: [PATCH] lsm,selinux: pass flowi_common instead of flowi to the LSM hooks
On Thu, Nov 19, 2020 at 10:02 PM James Morris wrote: > On Thu, 19 Nov 2020, Paul Moore wrote: > > As pointed out by Herbert in a recent related patch, the LSM hooks do > > not have the necessary address family information to use the flowi > > struct safely. As none of the LSMs currently use any of the protocol > > specific flowi information, replace the flowi pointers with pointers > > to the address family independent flowi_common struct. > > > > Reported-by: Herbert Xu > > Signed-off-by: Paul Moore > > Acked-by: James Morris Thanks. Seeing no further comments or objections, and given the discussion in the previous draft of the patch, I've gone again and merged this into selinux/next. -- paul moore www.paul-moore.com
[PATCH] netlabel: fix an uninitialized warning in netlbl_unlabel_staticlist()
Static checking revealed that a previous fix to
netlbl_unlabel_staticlist() leaves a stack variable uninitialized,
this patches fixes that.
Fixes: 866358ec331f ("netlabel: fix our progress tracking in
netlbl_unlabel_staticlist()")
Reported-by: Dan Carpenter
Signed-off-by: Paul Moore
---
net/netlabel/netlabel_unlabeled.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netlabel/netlabel_unlabeled.c
b/net/netlabel/netlabel_unlabeled.c
index fc55c9116da0..ccb491642811 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -1167,7 +1167,7 @@ static int netlbl_unlabel_staticlist(struct sk_buff *skb,
u32 skip_bkt = cb->args[0];
u32 skip_chain = cb->args[1];
u32 skip_addr4 = cb->args[2];
- u32 iter_bkt, iter_chain, iter_addr4 = 0, iter_addr6 = 0;
+ u32 iter_bkt, iter_chain = 0, iter_addr4 = 0, iter_addr6 = 0;
struct netlbl_unlhsh_iface *iface;
struct list_head *iter_list;
struct netlbl_af4list *addr4;
Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls
On Wed, Oct 21, 2020 at 12:39 PM Richard Guy Briggs wrote: > Here is an exmple I was able to generate after updating the testsuite > script to include a signalling example of a nested audit container > identifier: > > > type=PROCTITLE msg=audit(2020-10-21 10:31:16.655:6731) : > proctitle=/usr/bin/perl -w containerid/test > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > contid=7129731255799087104^941723245477888 > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115583 oauid=root > ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > ocomm=perl > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > contid=941723245477888 > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115580 oauid=root > ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > ocomm=perl > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > contid=8098399240850112512^941723245477888 > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115582 oauid=root > ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > ocomm=perl > type=SYSCALL msg=audit(2020-10-21 10:31:16.655:6731) : arch=x86_64 > syscall=kill success=yes exit=0 a0=0xfffe3c84 a1=SIGTERM a2=0x4d524554 a3=0x0 > items=0 ppid=115564 pid=115567 auid=root uid=root gid=root euid=root > suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl > exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > key=testsuite-1603290671-AcLtUulY > > > There are three CONTAINER_ID records which need some way of associating with > OBJ_PID records. An additional CONTAINER_ID record would be present if the > killing process itself had an audit container identifier. I think the most > obvious way to connect them is with a pid= field in the CONTAINER_ID record. Using a "pid=" field as a way to link CONTAINER_ID records to other records raises a few questions. What happens if/when we need to represent those PIDs in the context of a namespace? Are we ever going to need to link to records which don't have a "pid=" field? I haven't done the homework to know if either of these are a concern right now, but I worry that this might become a problem in the future. The idea of using something like "item=" is interesting. As you mention, the "item=" field does present some overlap problems with the PATH record, but perhaps we can do something similar. What if we added a "record=" (or similar, I'm not worried about names at this point) to each record, reset to 0/1 at the start of each event, and when we needed to link records somehow we could add a "related=1,..,N" field. This would potentially be useful beyond just the audit container ID work. -- paul moore www.paul-moore.com
Re: [PATCH net-next] net: netlabel: Fix kerneldoc warnings
On Tue, Oct 27, 2020 at 8:54 PM Andrew Lunn wrote: > > net/netlabel/netlabel_calipso.c:376: warning: Function parameter or member > 'ops' not described in 'netlbl_calipso_ops_register' > > Signed-off-by: Andrew Lunn > --- > net/netlabel/netlabel_calipso.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/netlabel/netlabel_calipso.c b/net/netlabel/netlabel_calipso.c > index 4e62f2ad3575..a4efa99fb1f8 100644 > --- a/net/netlabel/netlabel_calipso.c > +++ b/net/netlabel/netlabel_calipso.c > @@ -366,6 +366,7 @@ static const struct netlbl_calipso_ops *calipso_ops; > > /** > * netlbl_calipso_ops_register - Register the CALIPSO operations > + * @ops: Ops to register If we are being nitpicky, it might be better to drop the capitalization for the sake of consistency, e.g. "@ops: ops to register". Acked-by: Paul Moore > * > * Description: > * Register the CALIPSO packet engine operations. -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls
On Fri, Oct 23, 2020 at 4:40 PM Richard Guy Briggs wrote: > On 2020-10-22 21:21, Paul Moore wrote: > > On Wed, Oct 21, 2020 at 12:39 PM Richard Guy Briggs wrote: > > > Here is an exmple I was able to generate after updating the testsuite > > > script to include a signalling example of a nested audit container > > > identifier: > > > > > > > > > type=PROCTITLE msg=audit(2020-10-21 10:31:16.655:6731) : > > > proctitle=/usr/bin/perl -w containerid/test > > > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > > > contid=7129731255799087104^941723245477888 > > > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115583 > > > oauid=root ouid=root oses=1 > > > obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl > > > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > > > contid=941723245477888 > > > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115580 > > > oauid=root ouid=root oses=1 > > > obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl > > > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : > > > contid=8098399240850112512^941723245477888 > > > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115582 > > > oauid=root ouid=root oses=1 > > > obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl > > > type=SYSCALL msg=audit(2020-10-21 10:31:16.655:6731) : arch=x86_64 > > > syscall=kill success=yes exit=0 a0=0xfffe3c84 a1=SIGTERM a2=0x4d524554 > > > a3=0x0 items=0 ppid=115564 pid=115567 auid=root uid=root gid=root > > > euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 > > > ses=1 comm=perl exe=/usr/bin/perl > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 > > > key=testsuite-1603290671-AcLtUulY > > > > > > > > > There are three CONTAINER_ID records which need some way of associating > > > with OBJ_PID records. An additional CONTAINER_ID record would be present > > > if the killing process itself had an audit container identifier. I think > > > the most obvious way to connect them is with a pid= field in the > > > CONTAINER_ID record. > > > > Using a "pid=" field as a way to link CONTAINER_ID records to other > > records raises a few questions. What happens if/when we need to > > represent those PIDs in the context of a namespace? Are we ever going > > to need to link to records which don't have a "pid=" field? I haven't > > done the homework to know if either of these are a concern right now, > > but I worry that this might become a problem in the future. > > Good point about PID namespaces in the future but those accompanying > records will already have to be conditioned for the PID namespace > context that is requesting it, so I don't see this as a showstopper. Possibly, it just gets very messy. Doubly so when you start looking at potentially adjusting for multiple audit daemons. Thankfully it doesn't look like using the PID is a good idea for other reasons. > I've forgotten about an important one we already hit, which is a network > event that only has a NETFILTER_PKT record, but in that case, there is > no ambiguity since there are no other records associated with that > event. So the second is already an issue now. Using > task_tgid_nr(current), in the contid testsuite script network event it > attributed it to ping which caused the event, but we cannot use this > since it wasn't triggered by a syscall and doesn't accurately reflect > the kernel thread that received it. It could just be set to zero for > network events. Possibly. It just seems like too much hackery to start; that's the stuff you do once it has been in a kernel release for years and need to find a workaround that doesn't break everything. I think we should aim a bit higher right now. > > The idea of using something like "item=" is interesting. As you > > mention, the "item=" field does present some overlap problems with the > > PATH record, but perhaps we can do something similar. What if we > > added a "record=" (or similar, I'm not worried about names at this > > point) to each record, reset to 0/1 at the start of each event, and > > when we needed to link records somehow we could add a "related=1,..,N" > > field. This would potentially be useful beyond just the audit > > container ID work. > > Does it make any sense to use the same keyword in each type of record > such as record/records as in PATH/SYSCALL: item/items ? That was mentioned above, if you can assure yourself and the rest of us that it can be safely reused I think that might be okay, but I'm not convinced that is safe at the moment. Although I will admit those are fears are not based on an exhaustive search through the code (or a determined "think"). > (I prefer 0-indexed like item=...) I have no preference on where we start the index, but it makes sense to keep the same index starting point as the PATH records. -- paul moore www.paul-moore.com
Re: [RFC PATCH] lsm,selinux: pass the family information along with xfrm flow
On Wed, Sep 30, 2020 at 9:44 AM Paul Moore wrote: > On Tue, Sep 29, 2020 at 7:09 PM James Morris wrote: > > I'm not keen on adding a parameter which nobody is using. Perhaps a note > > in the header instead? > > On Wed, Sep 30, 2020 at 6:14 AM Herbert Xu > wrote: > > Please at least change to the struct flowi to flowi_common if we're > > not adding a family field. > > It did feel a bit weird adding a (currently) unused parameter, so I > can understand the concern, I just worry that a comment in the code > will be easily overlooked. I also thought about passing a pointer to > the nested flowi_common struct, but it doesn't appear that this is > done anywhere else in the stack so it felt wrong to do it here. With the merge window behind us, where do stand on this? I see the ACK from Casey and some grumbling about adding an unused parameter (which is a valid argument, I just feel the alternative is worse), but I haven't seen any serious NACKs. Any objections or other strong feelings to me merging this via the selinux/next branch? -- paul moore www.paul-moore.com
[PATCH] netlabel: fix our progress tracking in netlbl_unlabel_staticlist()
The current NetLabel code doesn't correctly keep track of the netlink
dump state in some cases, in particular when multiple interfaces with
large configurations are loaded. The problem manifests itself by not
reporting the full configuration to userspace, even though it is
loaded and active in the kernel. This patch fixes this by ensuring
that the dump state is properly reset when necessary inside the
netlbl_unlabel_staticlist() function.
Fixes: 8cc44579d1bd ("NetLabel: Introduce static network labels for unlabeled
connections")
Signed-off-by: Paul Moore
---
net/netlabel/netlabel_unlabeled.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/net/netlabel/netlabel_unlabeled.c
b/net/netlabel/netlabel_unlabeled.c
index 2e8e3f7b2111..fc55c9116da0 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c
@@ -1166,12 +1166,13 @@ static int netlbl_unlabel_staticlist(struct sk_buff
*skb,
struct netlbl_unlhsh_walk_arg cb_arg;
u32 skip_bkt = cb->args[0];
u32 skip_chain = cb->args[1];
- u32 iter_bkt;
- u32 iter_chain = 0, iter_addr4 = 0, iter_addr6 = 0;
+ u32 skip_addr4 = cb->args[2];
+ u32 iter_bkt, iter_chain, iter_addr4 = 0, iter_addr6 = 0;
struct netlbl_unlhsh_iface *iface;
struct list_head *iter_list;
struct netlbl_af4list *addr4;
#if IS_ENABLED(CONFIG_IPV6)
+ u32 skip_addr6 = cb->args[3];
struct netlbl_af6list *addr6;
#endif
@@ -1182,7 +1183,7 @@ static int netlbl_unlabel_staticlist(struct sk_buff *skb,
rcu_read_lock();
for (iter_bkt = skip_bkt;
iter_bkt < rcu_dereference(netlbl_unlhsh)->size;
-iter_bkt++, iter_chain = 0, iter_addr4 = 0, iter_addr6 = 0) {
+iter_bkt++) {
iter_list = &rcu_dereference(netlbl_unlhsh)->tbl[iter_bkt];
list_for_each_entry_rcu(iface, iter_list, list) {
if (!iface->valid ||
@@ -1190,7 +1191,7 @@ static int netlbl_unlabel_staticlist(struct sk_buff *skb,
continue;
netlbl_af4list_foreach_rcu(addr4,
&iface->addr4_list) {
- if (iter_addr4++ < cb->args[2])
+ if (iter_addr4++ < skip_addr4)
continue;
if (netlbl_unlabel_staticlist_gen(
NLBL_UNLABEL_C_STATICLIST,
@@ -1203,10 +1204,12 @@ static int netlbl_unlabel_staticlist(struct sk_buff
*skb,
goto unlabel_staticlist_return;
}
}
+ iter_addr4 = 0;
+ skip_addr4 = 0;
#if IS_ENABLED(CONFIG_IPV6)
netlbl_af6list_foreach_rcu(addr6,
&iface->addr6_list) {
- if (iter_addr6++ < cb->args[3])
+ if (iter_addr6++ < skip_addr6)
continue;
if (netlbl_unlabel_staticlist_gen(
NLBL_UNLABEL_C_STATICLIST,
@@ -1219,8 +1222,12 @@ static int netlbl_unlabel_staticlist(struct sk_buff *skb,
goto unlabel_staticlist_return;
}
}
+ iter_addr6 = 0;
+ skip_addr6 = 0;
#endif /* IPv6 */
}
+ iter_chain = 0;
+ skip_chain = 0;
}
unlabel_staticlist_return:
Re: KASAN: use-after-free Read in cipso_v4_genopt
On Tue, Mar 2, 2021 at 6:03 AM Dmitry Vyukov wrote: > ... > Besides these 2 crashes, we've also seen one on a 4.19 based kernel, see > below. > Based on the reports with mismatching stacks, it looks like > cipso_v4_genopt is doing some kind of wild pointer access (uninit > pointer?). Hmm, interesting. Looking quickly at the stack dump, it appears that the problem occurs (at least in the recent kernel) when accessing the cipso_v4_doi.tags[] array which is embedded in the cipso_v4_doi struct. Based on the code in cipso_v4_genopt() it doesn't appear that we are shooting past the end of the array/struct and the cipso_v4_doi struct appears to be refcounted correctly in cipso_v4_doi_getdef() and cipso_v4_doi_putdef(). I'll look at it some more today to see if something jumps out at me, but obviously a reproducer would be very helpful if you are able to find one. It's also worth adding that this code really hasn't changed much in a *long* time, not that this means it isn't broken, just that it might also be worth looking at other odd memory bugs to see if there is chance they are wandering around and stomping on memory ... -- paul moore www.paul-moore.com
Re: KASAN: use-after-free Read in cipso_v4_genopt
On Tue, Mar 2, 2021 at 2:15 PM Dmitry Vyukov wrote: ... > Not sure if it's the root cause or not, but I am looking at this > reference drop in cipso_v4_doi_remove: > https://elixir.bootlin.com/linux/v5.12-rc1/source/net/ipv4/cipso_ipv4.c#L522 > The thing is that it does not remove from the list if reference is not > 0, right? So what if I send 1000 of netlink remove messages? Will it > drain refcount to 0? > I did not read all involved code, but the typical pattern is to drop > refcount and always remove from the list. Then the last use will > delete the object. > Does it make any sense? Looking at it quickly, the logic above seems sane. I wrote this code a *long* time ago, so let me get my head back into it and make sure that still holds. -- paul moore www.paul-moore.com
Re: KASAN: use-after-free Write in cipso_v4_doi_putdef
On Wed, Mar 3, 2021 at 10:53 AM syzbot wrote: > > Hello, > > syzbot found the following issue on: > > HEAD commit:7a7fd0de Merge branch 'kmap-conversion-for-5.12' of git://.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=164a74dad0 > kernel config: https://syzkaller.appspot.com/x/.config?x=779a2568b654c1c6 > dashboard link: https://syzkaller.appspot.com/bug?extid=521772a90166b3fca21f > compiler: Debian clang version 11.0.1-2 > > Unfortunately, I don't have any reproducer for this issue yet. > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+521772a90166b3fca...@syzkaller.appspotmail.com > > == > BUG: KASAN: use-after-free in instrument_atomic_read_write > include/linux/instrumented.h:101 [inline] > BUG: KASAN: use-after-free in atomic_fetch_sub_release > include/asm-generic/atomic-instrumented.h:220 [inline] > BUG: KASAN: use-after-free in __refcount_sub_and_test > include/linux/refcount.h:272 [inline] > BUG: KASAN: use-after-free in __refcount_dec_and_test > include/linux/refcount.h:315 [inline] > BUG: KASAN: use-after-free in refcount_dec_and_test > include/linux/refcount.h:333 [inline] > BUG: KASAN: use-after-free in cipso_v4_doi_putdef+0x2d/0x190 > net/ipv4/cipso_ipv4.c:586 > Write of size 4 at addr 8880179ecb18 by task syz-executor.5/20110 Almost surely the same problem as the others, I'm currently chasing down a few remaining spots to make sure the fix I'm working on is correct. -- paul moore www.paul-moore.com
Re: KASAN: use-after-free Write in cipso_v4_doi_putdef
On Wed, Mar 3, 2021 at 11:20 AM Paul Moore wrote: > On Wed, Mar 3, 2021 at 10:53 AM syzbot > wrote: > > > > Hello, > > > > syzbot found the following issue on: > > > > HEAD commit:7a7fd0de Merge branch 'kmap-conversion-for-5.12' of git://.. > > git tree: upstream > > console output: https://syzkaller.appspot.com/x/log.txt?x=164a74dad0 > > kernel config: https://syzkaller.appspot.com/x/.config?x=779a2568b654c1c6 > > dashboard link: https://syzkaller.appspot.com/bug?extid=521772a90166b3fca21f > > compiler: Debian clang version 11.0.1-2 > > > > Unfortunately, I don't have any reproducer for this issue yet. > > > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > > Reported-by: syzbot+521772a90166b3fca...@syzkaller.appspotmail.com > > > > == > > BUG: KASAN: use-after-free in instrument_atomic_read_write > > include/linux/instrumented.h:101 [inline] > > BUG: KASAN: use-after-free in atomic_fetch_sub_release > > include/asm-generic/atomic-instrumented.h:220 [inline] > > BUG: KASAN: use-after-free in __refcount_sub_and_test > > include/linux/refcount.h:272 [inline] > > BUG: KASAN: use-after-free in __refcount_dec_and_test > > include/linux/refcount.h:315 [inline] > > BUG: KASAN: use-after-free in refcount_dec_and_test > > include/linux/refcount.h:333 [inline] > > BUG: KASAN: use-after-free in cipso_v4_doi_putdef+0x2d/0x190 > > net/ipv4/cipso_ipv4.c:586 > > Write of size 4 at addr 8880179ecb18 by task syz-executor.5/20110 > > Almost surely the same problem as the others, I'm currently chasing > down a few remaining spots to make sure the fix I'm working on is > correct. I think I've now managed to convince myself that the patch I've got here is reasonable. I'm looping over a series of tests right now and plan to let it continue overnight; assuming everything still looks good in the morning I'll post it. Thanks for your help. -- paul moore www.paul-moore.com
[PATCH] cipso,calipso: resolve a number of problems with the DOI refcounts
The current CIPSO and CALIPSO refcounting scheme for the DOI
definitions is a bit flawed in that we:
1. Don't correctly match gets/puts in netlbl_cipsov4_list().
2. Decrement the refcount on each attempt to remove the DOI from the
DOI list, only removing it from the list once the refcount drops
to zero.
This patch fixes these problems by adding the missing "puts" to
netlbl_cipsov4_list() and introduces a more conventional, i.e.
not-buggy, refcounting mechanism to the DOI definitions. Upon the
addition of a DOI to the DOI list, it is initialized with a refcount
of one, removing a DOI from the list removes it from the list and
drops the refcount by one; "gets" and "puts" behave as expected with
respect to refcounts, increasing and decreasing the DOI's refcount by
one.
Fixes: b1edeb102397 ("netlabel: Replace protocol/NetLabel linking with
refrerence counts")
Fixes: d7cce01504a0 ("netlabel: Add support for removing a CALIPSO DOI.")
Reported-by: syzbot+9ec037722d2603a9f...@syzkaller.appspotmail.com
Signed-off-by: Paul Moore
---
net/ipv4/cipso_ipv4.c| 11 +--
net/ipv6/calipso.c | 14 +-
net/netlabel/netlabel_cipso_v4.c |3 +++
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 471d33a0d095..be09c7669a79 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -519,16 +519,10 @@ int cipso_v4_doi_remove(u32 doi, struct netlbl_audit
*audit_info)
ret_val = -ENOENT;
goto doi_remove_return;
}
- if (!refcount_dec_and_test(&doi_def->refcount)) {
- spin_unlock(&cipso_v4_doi_list_lock);
- ret_val = -EBUSY;
- goto doi_remove_return;
- }
list_del_rcu(&doi_def->list);
spin_unlock(&cipso_v4_doi_list_lock);
- cipso_v4_cache_invalidate();
- call_rcu(&doi_def->rcu, cipso_v4_doi_free_rcu);
+ cipso_v4_doi_putdef(doi_def);
ret_val = 0;
doi_remove_return:
@@ -585,9 +579,6 @@ void cipso_v4_doi_putdef(struct cipso_v4_doi *doi_def)
if (!refcount_dec_and_test(&doi_def->refcount))
return;
- spin_lock(&cipso_v4_doi_list_lock);
- list_del_rcu(&doi_def->list);
- spin_unlock(&cipso_v4_doi_list_lock);
cipso_v4_cache_invalidate();
call_rcu(&doi_def->rcu, cipso_v4_doi_free_rcu);
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index 51184a70ac7e..1578ed9e97d8 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -83,6 +83,9 @@ struct calipso_map_cache_entry {
static struct calipso_map_cache_bkt *calipso_cache;
+static void calipso_cache_invalidate(void);
+static void calipso_doi_putdef(struct calipso_doi *doi_def);
+
/* Label Mapping Cache Functions
*/
@@ -444,15 +447,10 @@ static int calipso_doi_remove(u32 doi, struct
netlbl_audit *audit_info)
ret_val = -ENOENT;
goto doi_remove_return;
}
- if (!refcount_dec_and_test(&doi_def->refcount)) {
- spin_unlock(&calipso_doi_list_lock);
- ret_val = -EBUSY;
- goto doi_remove_return;
- }
list_del_rcu(&doi_def->list);
spin_unlock(&calipso_doi_list_lock);
- call_rcu(&doi_def->rcu, calipso_doi_free_rcu);
+ calipso_doi_putdef(doi_def);
ret_val = 0;
doi_remove_return:
@@ -508,10 +506,8 @@ static void calipso_doi_putdef(struct calipso_doi *doi_def)
if (!refcount_dec_and_test(&doi_def->refcount))
return;
- spin_lock(&calipso_doi_list_lock);
- list_del_rcu(&doi_def->list);
- spin_unlock(&calipso_doi_list_lock);
+ calipso_cache_invalidate();
call_rcu(&doi_def->rcu, calipso_doi_free_rcu);
}
diff --git a/net/netlabel/netlabel_cipso_v4.c b/net/netlabel/netlabel_cipso_v4.c
index 726dda95934c..4f50a64315cf 100644
--- a/net/netlabel/netlabel_cipso_v4.c
+++ b/net/netlabel/netlabel_cipso_v4.c
@@ -575,6 +575,7 @@ static int netlbl_cipsov4_list(struct sk_buff *skb, struct
genl_info *info)
break;
}
+ cipso_v4_doi_putdef(doi_def);
rcu_read_unlock();
genlmsg_end(ans_skb, data);
@@ -583,12 +584,14 @@ static int netlbl_cipsov4_list(struct sk_buff *skb,
struct genl_info *info)
list_retry:
/* XXX - this limit is a guesstimate */
if (nlsze_mult < 4) {
+ cipso_v4_doi_putdef(doi_def);
rcu_read_unlock();
kfree_skb(ans_skb);
nlsze_mult *= 2;
goto list_start;
}
list_failure_lock:
+ cipso_v4_doi_putdef(doi_def);
rcu_read_unlock();
list_failure:
kfree_skb(ans_skb);
Re: [PATCH] cipso,calipso: resolve a number of problems with the DOI refcounts
On Thu, Mar 4, 2021 at 4:29 PM Paul Moore wrote:
>
> The current CIPSO and CALIPSO refcounting scheme for the DOI
> definitions is a bit flawed in that we:
>
> 1. Don't correctly match gets/puts in netlbl_cipsov4_list().
> 2. Decrement the refcount on each attempt to remove the DOI from the
>DOI list, only removing it from the list once the refcount drops
>to zero.
>
> This patch fixes these problems by adding the missing "puts" to
> netlbl_cipsov4_list() and introduces a more conventional, i.e.
> not-buggy, refcounting mechanism to the DOI definitions. Upon the
> addition of a DOI to the DOI list, it is initialized with a refcount
> of one, removing a DOI from the list removes it from the list and
> drops the refcount by one; "gets" and "puts" behave as expected with
> respect to refcounts, increasing and decreasing the DOI's refcount by
> one.
>
> Fixes: b1edeb102397 ("netlabel: Replace protocol/NetLabel linking with
> refrerence counts")
> Fixes: d7cce01504a0 ("netlabel: Add support for removing a CALIPSO DOI.")
> Reported-by: syzbot+9ec037722d2603a9f...@syzkaller.appspotmail.com
> Signed-off-by: Paul Moore
> ---
> net/ipv4/cipso_ipv4.c| 11 +--
> net/ipv6/calipso.c | 14 +-
> net/netlabel/netlabel_cipso_v4.c |3 +++
> 3 files changed, 9 insertions(+), 19 deletions(-)
As a FYI, this patch has been tested by looping through a number of
NetLabel/CALIPSO/CIPSO tests overnight, a reproducer from one of the
syzbot reports (multiple times), and the selinux-testsuite tests;
everything looked good at the end of the testing.
Thanks to syzbot and Dmitry for finding and reporting the bug.
--
paul moore
www.paul-moore.com
Re: [PATCH] cipso,calipso: resolve a number of problems with the DOI refcounts
On Thu, Mar 4, 2021 at 5:33 PM David Miller wrote: > From: Paul Moore > Date: Thu, 04 Mar 2021 16:29:51 -0500 > > > +static void calipso_doi_putdef(struct calipso_doi *doi_def); > > + > > This is a global symbol, so why the static decl here? To resolve this: CC net/ipv6/calipso.o net/ipv6/calipso.c: In function ‘calipso_doi_remove’: net/ipv6/calipso.c:453:2: error: implicit declaration of function ‘calipso_doi_p utdef’ I think there are some odd things with how the CALIPSO prototypes are handled, some of that I'm guessing is due to handling IPv6 as-a-module, but regardless of the reason it seemed like the smallest fix was to add the forward declaration at the top of the file. Considering that I believe this should be sent to -stable I figured a smaller patch, with less chance for merge conflicts, would be more desirable. Or are you simply concerned about the static tag? I simply kept the static from the function definition; removing it causes a mismatch which makes the compiler unhappy. Either way, if you want this done another way, let me know what you want and I'll respin it. -- paul moore www.paul-moore.com
Re: [PATCH v2] CIPSO: Fix unaligned memory access in cipso_v4_gentag_hdr
On Fri, Mar 5, 2021 at 3:05 AM Sergey Nazarov wrote:
>
> We need to use put_unaligned when writing 32-bit DOI value
> in cipso_v4_gentag_hdr to avoid unaligned memory access.
>
> v2: unneeded type cast removed as Ondrej Mosnacek suggested.
>
> Signed-off-by: Sergey Nazarov
> ---
> net/ipv4/cipso_ipv4.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Paul Moore
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 471d33a..6e59902 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1162,7 +1162,7 @@ static void cipso_v4_gentag_hdr(const struct
> cipso_v4_doi *doi_def,
> {
> buf[0] = IPOPT_CIPSO;
> buf[1] = CIPSO_V4_HDR_LEN + len;
> - *(__be32 *)&buf[2] = htonl(doi_def->doi);
> + put_unaligned_be32(doi_def->doi, &buf[2]);
> }
>
> /**
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
Re: [PATCH] selinux: vsock: Set SID for socket returned by accept()
On Wed, Mar 17, 2021 at 11:44 AM David Brazdil wrote:
>
> For AF_VSOCK, accept() currently returns sockets that are unlabelled.
> Other socket families derive the child's SID from the SID of the parent
> and the SID of the incoming packet. This is typically done as the
> connected socket is placed in the queue that accept() removes from.
>
> Implement an LSM hook 'vsock_sk_clone' that takes the parent (server)
> and child (connection) struct socks, and assigns the parent SID to the
> child. There is no packet SID in this case.
>
> Signed-off-by: David Brazdil
> ---
> This is my first patch in this part of the kernel so please comment if I
> missed anything, specifically whether there is a packet SID that should
> be mixed into the child SID.
>
> Tested on Android.
>
> include/linux/lsm_hook_defs.h | 1 +
> include/linux/lsm_hooks.h | 7 +++
> include/linux/security.h | 5 +
> net/vmw_vsock/af_vsock.c | 1 +
> security/security.c | 5 +
> security/selinux/hooks.c | 10 ++
> 6 files changed, 29 insertions(+)
Additional comments below, but I think it would be a good idea for you
to test your patches on a more traditional Linux distribution as well
as Android.
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 5546710d8ac1..a9bf3b90cb2f 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -755,6 +755,7 @@ static struct sock *__vsock_create(struct net *net,
> vsk->buffer_size = psk->buffer_size;
> vsk->buffer_min_size = psk->buffer_min_size;
> vsk->buffer_max_size = psk->buffer_max_size;
> + security_vsock_sk_clone(parent, sk);
Did you try calling the existing security_sk_clone() hook here? I
would be curious to hear why it doesn't work in this case.
Feel free to educate me on AF_VSOCK, it's entirely possible I'm
misunderstanding something here :)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ddd097790d47..7b92d6f2e0fd 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5616,6 +5616,15 @@ static int selinux_tun_dev_open(void *security)
> return 0;
> }
>
> +static void selinux_socket_vsock_sk_clone(struct sock *sock, struct sock
> *newsk)
> +{
> + struct sk_security_struct *sksec_sock = sock->sk_security;
> + struct sk_security_struct *sksec_new = newsk->sk_security;
> +
> + /* Always returns 0 when packet SID is SECSID_NULL. */
> + WARN_ON_ONCE(selinux_conn_sid(sksec_sock->sid, SECSID_NULL,
> &sksec_new->sid));
> +}
If you are using selinux_conn_sid() with the second argument always
SECSID_NULL it probably isn't the best choice; it ends up doing a
simple "sksec_new->sid = sksec_sock->sid" ... which gets us back to
this function looking like a reimplementation of
selinux_sk_clone_security(), minus the peer_sid and sclass
initializations (which should be important things to have).
I strongly suggest you try making use of the existing
security_sk_clone() hook in the vsock code, it seems like a better way
to solve this problem.
--
paul moore
www.paul-moore.com
[net-next PATCH] netlabel: fix problems with mapping removal
This patch fixes two main problems seen when removing NetLabel
mappings: memory leaks and potentially extra audit noise.
The memory leaks are caused by not properly free'ing the mapping's
address selector struct when free'ing the entire entry as well as
not properly cleaning up a temporary mapping entry when adding new
address selectors to an existing entry. This patch fixes both these
problems such that kmemleak reports no NetLabel associated leaks
after running the SELinux test suite.
The potentially extra audit noise was caused by the auditing code in
netlbl_domhsh_remove_entry() being called regardless of the entry's
validity. If another thread had already marked the entry as invalid,
but not removed/free'd it from the list of mappings, then it was
possible that an additional mapping removal audit record would be
generated. This patch fixes this by returning early from the removal
function when the entry was previously marked invalid. This change
also had the side benefit of improving the code by decreasing the
indentation level of large chunk of code by one (accounting for most
of the diffstat).
Reported-by: Stephen Smalley
Signed-off-by: Paul Moore
---
net/netlabel/netlabel_domainhash.c | 59 ++--
1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/net/netlabel/netlabel_domainhash.c
b/net/netlabel/netlabel_domainhash.c
index d07de2c0fbc7..f73a8382c275 100644
--- a/net/netlabel/netlabel_domainhash.c
+++ b/net/netlabel/netlabel_domainhash.c
@@ -85,6 +85,7 @@ static void netlbl_domhsh_free_entry(struct rcu_head *entry)
kfree(netlbl_domhsh_addr6_entry(iter6));
}
#endif /* IPv6 */
+ kfree(ptr->def.addrsel);
}
kfree(ptr->domain);
kfree(ptr);
@@ -537,6 +538,8 @@ int netlbl_domhsh_add(struct netlbl_dom_map *entry,
goto add_return;
}
#endif /* IPv6 */
+ /* cleanup the new entry since we've moved everything over */
+ netlbl_domhsh_free_entry(&entry->rcu);
} else
ret_val = -EINVAL;
@@ -580,6 +583,12 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map
*entry,
{
int ret_val = 0;
struct audit_buffer *audit_buf;
+ struct netlbl_af4list *iter4;
+ struct netlbl_domaddr4_map *map4;
+#if IS_ENABLED(CONFIG_IPV6)
+ struct netlbl_af6list *iter6;
+ struct netlbl_domaddr6_map *map6;
+#endif /* IPv6 */
if (entry == NULL)
return -ENOENT;
@@ -597,6 +606,9 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map *entry,
ret_val = -ENOENT;
spin_unlock(&netlbl_domhsh_lock);
+ if (ret_val)
+ return ret_val;
+
audit_buf = netlbl_audit_start_common(AUDIT_MAC_MAP_DEL, audit_info);
if (audit_buf != NULL) {
audit_log_format(audit_buf,
@@ -606,40 +618,29 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map
*entry,
audit_log_end(audit_buf);
}
- if (ret_val == 0) {
- struct netlbl_af4list *iter4;
- struct netlbl_domaddr4_map *map4;
-#if IS_ENABLED(CONFIG_IPV6)
- struct netlbl_af6list *iter6;
- struct netlbl_domaddr6_map *map6;
-#endif /* IPv6 */
-
- switch (entry->def.type) {
- case NETLBL_NLTYPE_ADDRSELECT:
- netlbl_af4list_foreach_rcu(iter4,
-&entry->def.addrsel->list4) {
- map4 = netlbl_domhsh_addr4_entry(iter4);
- cipso_v4_doi_putdef(map4->def.cipso);
- }
+ switch (entry->def.type) {
+ case NETLBL_NLTYPE_ADDRSELECT:
+ netlbl_af4list_foreach_rcu(iter4, &entry->def.addrsel->list4) {
+ map4 = netlbl_domhsh_addr4_entry(iter4);
+ cipso_v4_doi_putdef(map4->def.cipso);
+ }
#if IS_ENABLED(CONFIG_IPV6)
- netlbl_af6list_foreach_rcu(iter6,
-&entry->def.addrsel->list6) {
- map6 = netlbl_domhsh_addr6_entry(iter6);
- calipso_doi_putdef(map6->def.calipso);
- }
+ netlbl_af6list_foreach_rcu(iter6, &entry->def.addrsel->list6) {
+ map6 = netlbl_domhsh_addr6_entry(iter6);
+ calipso_doi_putdef(map6->def.calipso);
+ }
#endif /* IPv6 */
- break;
- case NETLBL_NLTYPE_CIPSOV4:
- cipso_v4_doi_putdef(entry->def.cipso);
- break;
+ break;
+ case NETLBL_NLTYPE_CIPSOV4:
+ cipso_v4_doi_putdef(entry
Re: [PATCH ghak90 V9 08/13] audit: add containerid support for user records
On Fri, Jul 17, 2020 at 8:44 PM Richard Guy Briggs wrote: > On 2020-07-05 11:11, Paul Moore wrote: > > On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs wrote: > > > > > > Add audit container identifier auxiliary record to user event standalone > > > records. > > > > > > Signed-off-by: Richard Guy Briggs > > > Acked-by: Neil Horman > > > Reviewed-by: Ondrej Mosnacek > > > --- > > > kernel/audit.c | 19 --- > > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > index 54dd2cb69402..997c34178ee8 100644 > > > --- a/kernel/audit.c > > > +++ b/kernel/audit.c > > > @@ -1507,6 +1504,14 @@ static int audit_receive_msg(struct sk_buff *skb, > > > struct nlmsghdr *nlh) > > > audit_log_n_untrustedstring(ab, str, > > > data_len); > > > } > > > audit_log_end(ab); > > > + rcu_read_lock(); > > > + cont = _audit_contobj_get(current); > > > + rcu_read_unlock(); > > > + audit_log_container_id(context, cont); > > > + rcu_read_lock(); > > > + _audit_contobj_put(cont); > > > + rcu_read_unlock(); > > > + audit_free_context(context); > > > > I haven't searched the entire patchset, but it seems like the pattern > > above happens a couple of times in this patchset, yes? If so would it > > make sense to wrap the above get/log/put in a helper function? > > I've redone the locking with an rcu lock around the get and a spinlock > around the put. It occurs to me that putting an rcu lock around the > whole thing and doing a get without the refcount increment would save > us the spinlock and put and be fine since we'd be fine with stale but > consistent information traversing the contobj list from this point to > report it. Problem with that is needing to use GFP_ATOMIC due to the > rcu lock. If I stick with the spinlock around the put then I can use > GFP_KERNEL and just grab the spinlock while traversing the contobj list. > > > Not a big deal either way, I'm pretty neutral on it at this point in > > the patchset but thought it might be worth mentioning in case you > > noticed the same and were on the fence. > > There is only one other place this is used, in audit_log_exit in > auditsc.c. I had noted the pattern but wasn't sure it was worth it. > Inline or not? Should we just let the compiler decide? I'm generally not a fan of explicit inlines unless it has been shown to be a real problem. -- paul moore www.paul-moore.com
Re: [PATCH ghak90 V9 06/13] audit: add contid support for signalling the audit daemon
On Wed, Jul 29, 2020 at 3:00 PM Richard Guy Briggs wrote:
> On 2020-07-05 11:10, Paul Moore wrote:
> > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote:
> > >
> > > Add audit container identifier support to the action of signalling the
> > > audit daemon.
> > >
> > > Since this would need to add an element to the audit_sig_info struct,
> > > a new record type AUDIT_SIGNAL_INFO2 was created with a new
> > > audit_sig_info2 struct. Corresponding support is required in the
> > > userspace code to reflect the new record request and reply type.
> > > An older userspace won't break since it won't know to request this
> > > record type.
> > >
> > > Signed-off-by: Richard Guy Briggs
> > > ---
> > > include/linux/audit.h | 8
> > > include/uapi/linux/audit.h | 1 +
> > > kernel/audit.c | 95
> > > -
> > > security/selinux/nlmsgtab.c | 1 +
> > > 4 files changed, 104 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index 5eeba0efffc2..89cf7c66abe6 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -22,6 +22,13 @@ struct audit_sig_info {
> > > charctx[];
> > > };
> > >
> > > +struct audit_sig_info2 {
> > > + uid_t uid;
> > > + pid_t pid;
> > > + u32 cid_len;
> > > + chardata[];
> > > +};
> > > +
> > > struct audit_buffer;
> > > struct audit_context;
> > > struct inode;
> > > @@ -105,6 +112,7 @@ struct audit_contobj {
> > > u64 id;
> > > struct task_struct *owner;
> > > refcount_t refcount;
> > > + refcount_t sigflag;
> > > struct rcu_head rcu;
> > > };
> >
> > It seems like we need some protection in audit_set_contid() so that we
> > don't allow reuse of an audit container ID when "refcount == 0 &&
> > sigflag != 0", yes?
>
> We have it, see -ESHUTDOWN below.
That check in audit_set_contid() is checking ->refcount and not
->sigflag; ->sigflag is more important in this context, yes?
> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index fd98460c983f..a56ad77069b9 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -72,6 +72,7 @@
> > > #define AUDIT_SET_FEATURE 1018/* Turn an audit feature on or
> > > off */
> > > #define AUDIT_GET_FEATURE 1019/* Get which features are enabled
> > > */
> > > #define AUDIT_CONTAINER_OP 1020/* Define the container id and
> > > info */
> > > +#define AUDIT_SIGNAL_INFO2 1021/* Get info auditd signal sender
> > > */
> > >
> > > #define AUDIT_FIRST_USER_MSG 1100/* Userspace messages mostly
> > > uninteresting to kernel */
> > > #define AUDIT_USER_AVC 1107/* We filter this differently */
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index a09f8f661234..54dd2cb69402 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -126,6 +126,8 @@ struct auditd_connection {
> > > kuid_t audit_sig_uid = INVALID_UID;
> > > pid_t audit_sig_pid = -1;
> > > u32audit_sig_sid = 0;
> > > +static struct audit_contobj *audit_sig_cid;
> > > +static struct task_struct *audit_sig_atsk;
> >
> > This looks like a typo, or did you mean "atsk" for some reason?
>
> No, I meant atsk to refer specifically to the audit daemon task and not
> any other random one that is doing the signalling. I can change it is
> there is a strong objection.
Esh, yeah, "atsk" looks too much like a typo ;) At the very leask add
a 'd' in there, e.g. "adtsk", but something better than that would be
welcome.
> > > @@ -2532,6 +2620,11 @@ int audit_set_contid(struct task_struct *task, u64
> > > contid)
> > > if (cont->id == contid) {
> > > /* task injection to existing container */
> > > if (current == cont->owner) {
> > > + if
&
Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls
On Wed, Jul 29, 2020 at 3:41 PM Richard Guy Briggs wrote:
> On 2020-07-05 11:10, Paul Moore wrote:
> > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote:
...
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index f03d3eb0752c..9e79645e5c0e 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -1458,6 +1466,7 @@ static void audit_log_exit(void)
> > > struct audit_buffer *ab;
> > > struct audit_aux_data *aux;
> > > struct audit_names *n;
> > > + struct audit_contobj *cont;
> > >
> > > context->personality = current->personality;
> > >
> > > @@ -1541,7 +1550,7 @@ static void audit_log_exit(void)
> > > for (aux = context->aux_pids; aux; aux = aux->next) {
> > > struct audit_aux_data_pids *axs = (void *)aux;
> > >
> > > - for (i = 0; i < axs->pid_count; i++)
> > > + for (i = 0; i < axs->pid_count; i++) {
> > > if (audit_log_pid_context(context,
> > > axs->target_pid[i],
> > > axs->target_auid[i],
> > > axs->target_uid[i],
> > > @@ -1549,14 +1558,20 @@ static void audit_log_exit(void)
> > > axs->target_sid[i],
> > > axs->target_comm[i]))
> > > call_panic = 1;
> > > + audit_log_container_id(context,
> > > axs->target_cid[i]);
> > > + }
> >
> > It might be nice to see an audit event example including the
> > ptrace/signal information. I'm concerned there may be some confusion
> > about associating the different audit container IDs with the correct
> > information in the event.
>
> This is the subject of ghat81, which is a test for ptrace and signal
> records.
>
> This was the reason I had advocated for an op= field since there is a
> possibility of multiple contid records per event.
I think an "op=" field is the wrong way to link audit container ID to
a particular record. It may be convenient, but I fear that it would
be overloading the field too much.
Like I said above, I think it would be good to see an audit event
example including the ptrace/signal information. This way we can talk
about it on-list and hash out the various solutions if it proves to be
a problem.
> > > @@ -1575,6 +1590,14 @@ static void audit_log_exit(void)
> > >
> > > audit_log_proctitle();
> > >
> > > + rcu_read_lock();
> > > + cont = _audit_contobj_get(current);
> > > + rcu_read_unlock();
> > > + audit_log_container_id(context, cont);
> > > + rcu_read_lock();
> > > + _audit_contobj_put(cont);
> > > + rcu_read_unlock();
> >
> > Do we need to grab an additional reference for the audit container
> > object here? We don't create any additional references here that
> > persist beyond the lifetime of this function, right?
>
> Why do we need another reference? There's one for each pointer pointing
> to it and so far we have just one from this task. Or are you thinking
> of the contid hash list, which is only added to when a task points to it
> and gets removed from that list when the last task stops pointing to it.
> Later that gets more complicated with network namespaces and nested
> container objects. For now we just needed it while generating the
> record, then it gets freed.
I don't think we need to grab an additional reference here, that is
why I asked the question. The code above grabs a reference for the
audit container ID object associated with the current task and then
drops it before returning; if the current task, and it's associated
audit container ID object, disappears in the middle of the function
we've got much bigger worries :)
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 02/13] audit: add container id
On Wed, Jul 29, 2020 at 4:06 PM Richard Guy Briggs wrote:
> On 2020-07-05 11:09, Paul Moore wrote:
> > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs wrote:
...
> > > @@ -212,6 +219,33 @@ void __init audit_task_init(void)
> > > 0, SLAB_PANIC, NULL);
> > > }
> > >
> > > +/* rcu_read_lock must be held by caller unless new */
> > > +static struct audit_contobj *_audit_contobj_hold(struct audit_contobj
> > > *cont)
> > > +{
> > > + if (cont)
> > > + refcount_inc(&cont->refcount);
> > > + return cont;
> > > +}
> > > +
> > > +static struct audit_contobj *_audit_contobj_get(struct task_struct *tsk)
> > > +{
> > > + if (!tsk->audit)
> > > + return NULL;
> > > + return _audit_contobj_hold(tsk->audit->cont);
> > > +}
> > > +
> > > +/* rcu_read_lock must be held by caller */
> > > +static void _audit_contobj_put(struct audit_contobj *cont)
> > > +{
> > > + if (!cont)
> > > + return;
> > > + if (refcount_dec_and_test(&cont->refcount)) {
> > > + put_task_struct(cont->owner);
> > > + list_del_rcu(&cont->list);
> >
> > You should check your locking; I'm used to seeing exclusive locks
> > (e.g. the spinlock) around list adds/removes, it just reads/traversals
> > that can be done with just the RCU lock held.
>
> Ok, I've redone the locking yet again. I knew this on one level but
> that didn't translate consistently to code...
>
> > > + kfree_rcu(cont, rcu);
> > > + }
> > > +}
> >
> > Another nitpick, but it might be nice to have similar arguments to the
> > _get() and _put() functions, e.g. struct audit_contobj, but that is
> > some serious bikeshedding (basically rename _hold() to _get() and
> > rename _hold to audit_task_contid_hold() or similar).
>
> I have some idea what you are trying to say, but I think you misspoke.
> Did you mean rename _hold to _get, rename _get to
> audit_task_contobj_hold()?
It reads okay to me, but I know what I'm intending here :) I agree it
could be a bit confusing. Let me try to put my suggestion into some
quick pseudo-code function prototypes to make things a bit more
concrete.
The _audit_contobj_hold() function would become:
struct audit_contobj *_audit_contobj_hold(struct task_struct *tsk);
The _audit_contobj_get() function would become:
struct audit_contobj *_audit_contobj_get(struct audit_contobj *cont);
The _audit_contobj_put() function would become:
void _audit_contobj_put(struct audit_contobj *cont);
Basically swap the _get() and _hold() function names so that the
arguments are the same for both the _get() and _set() functions. Does
this make more sense?
> > > /**
> > > * audit_alloc - allocate an audit info block for a task
> > > * @tsk: task
> > > @@ -232,6 +266,9 @@ int audit_alloc(struct task_struct *tsk)
> > > }
> > > info->loginuid = audit_get_loginuid(current);
> > > info->sessionid = audit_get_sessionid(current);
> > > + rcu_read_lock();
> > > + info->cont = _audit_contobj_get(current);
> > > + rcu_read_unlock();
> >
> > The RCU locks aren't strictly necessary here, are they? In fact I
> > suppose we could probably just replace the _get() call with a
> > refcount_set(1) just as we do in audit_set_contid(), yes?
>
> I don't understand what you are getting at here. It needs a *contobj,
> along with bumping up the refcount of the existing contobj.
Sorry, you can disregard. My mental definition for audit_alloc() is
permanently messed up; I usually double check myself before commenting
on related code, but I must have forgotten here.
--
paul moore
www.paul-moore.com
Re: [PATCH ghak90 V9 11/13] audit: contid check descendancy and nesting
On Fri, Aug 7, 2020 at 1:10 PM Richard Guy Briggs wrote: > On 2020-07-05 11:11, Paul Moore wrote: > > On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs wrote: > > > Require the target task to be a descendant of the container > > > orchestrator/engine. If you want to get formal about this, you need to define "target" in the sentence above. Target of what? FWIW, I read the above to basically mean that a task can only set the audit container ID of processes which are beneath it in the "process tree" where the "process tree" is defined as the relationship between a parent and children processes such that the children processes are branches below the parent process. I have no problem with that, with the understanding that nesting complicates it somewhat. For example, this isn't true when one of the children is a nested orchestrator, is it? > > > You would only change the audit container ID from one set or inherited > > > value to another if you were nesting containers. I thought we decided we were going to allow an orchestrator to move a process between audit container IDs, yes? no? > > > If changing the contid, the container orchestrator/engine must be a > > > descendant and not same orchestrator as the one that set it so it is not > > > possible to change the contid of another orchestrator's container. Try rephrasing the above please, it isn't clear to me what you are trying to say. > Are we able to agree on the premises above? Is anything asserted that > should not be and is there anything missing? See above. If you want to go back to the definitions/assumptions stage, it probably isn't worth worrying about the other comments until we get the above sorted. -- paul moore www.paul-moore.com
[net PATCH] netlabel: fix problems with mapping removal
This patch fixes two main problems seen when removing NetLabel
mappings: memory leaks and potentially extra audit noise.
The memory leaks are caused by not properly free'ing the mapping's
address selector struct when free'ing the entire entry as well as
not properly cleaning up a temporary mapping entry when adding new
address selectors to an existing entry. This patch fixes both these
problems such that kmemleak reports no NetLabel associated leaks
after running the SELinux test suite.
The potentially extra audit noise was caused by the auditing code in
netlbl_domhsh_remove_entry() being called regardless of the entry's
validity. If another thread had already marked the entry as invalid,
but not removed/free'd it from the list of mappings, then it was
possible that an additional mapping removal audit record would be
generated. This patch fixes this by returning early from the removal
function when the entry was previously marked invalid. This change
also had the side benefit of improving the code by decreasing the
indentation level of large chunk of code by one (accounting for most
of the diffstat).
Fixes: 63c416887437 ("netlabel: Add network address selectors to the
NetLabel/LSM domain mapping")
Reported-by: Stephen Smalley
Signed-off-by: Paul Moore
---
net/netlabel/netlabel_domainhash.c | 59 ++--
1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/net/netlabel/netlabel_domainhash.c
b/net/netlabel/netlabel_domainhash.c
index d07de2c0fbc7..f73a8382c275 100644
--- a/net/netlabel/netlabel_domainhash.c
+++ b/net/netlabel/netlabel_domainhash.c
@@ -85,6 +85,7 @@ static void netlbl_domhsh_free_entry(struct rcu_head *entry)
kfree(netlbl_domhsh_addr6_entry(iter6));
}
#endif /* IPv6 */
+ kfree(ptr->def.addrsel);
}
kfree(ptr->domain);
kfree(ptr);
@@ -537,6 +538,8 @@ int netlbl_domhsh_add(struct netlbl_dom_map *entry,
goto add_return;
}
#endif /* IPv6 */
+ /* cleanup the new entry since we've moved everything over */
+ netlbl_domhsh_free_entry(&entry->rcu);
} else
ret_val = -EINVAL;
@@ -580,6 +583,12 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map
*entry,
{
int ret_val = 0;
struct audit_buffer *audit_buf;
+ struct netlbl_af4list *iter4;
+ struct netlbl_domaddr4_map *map4;
+#if IS_ENABLED(CONFIG_IPV6)
+ struct netlbl_af6list *iter6;
+ struct netlbl_domaddr6_map *map6;
+#endif /* IPv6 */
if (entry == NULL)
return -ENOENT;
@@ -597,6 +606,9 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map *entry,
ret_val = -ENOENT;
spin_unlock(&netlbl_domhsh_lock);
+ if (ret_val)
+ return ret_val;
+
audit_buf = netlbl_audit_start_common(AUDIT_MAC_MAP_DEL, audit_info);
if (audit_buf != NULL) {
audit_log_format(audit_buf,
@@ -606,40 +618,29 @@ int netlbl_domhsh_remove_entry(struct netlbl_dom_map
*entry,
audit_log_end(audit_buf);
}
- if (ret_val == 0) {
- struct netlbl_af4list *iter4;
- struct netlbl_domaddr4_map *map4;
-#if IS_ENABLED(CONFIG_IPV6)
- struct netlbl_af6list *iter6;
- struct netlbl_domaddr6_map *map6;
-#endif /* IPv6 */
-
- switch (entry->def.type) {
- case NETLBL_NLTYPE_ADDRSELECT:
- netlbl_af4list_foreach_rcu(iter4,
-&entry->def.addrsel->list4) {
- map4 = netlbl_domhsh_addr4_entry(iter4);
- cipso_v4_doi_putdef(map4->def.cipso);
- }
+ switch (entry->def.type) {
+ case NETLBL_NLTYPE_ADDRSELECT:
+ netlbl_af4list_foreach_rcu(iter4, &entry->def.addrsel->list4) {
+ map4 = netlbl_domhsh_addr4_entry(iter4);
+ cipso_v4_doi_putdef(map4->def.cipso);
+ }
#if IS_ENABLED(CONFIG_IPV6)
- netlbl_af6list_foreach_rcu(iter6,
-&entry->def.addrsel->list6) {
- map6 = netlbl_domhsh_addr6_entry(iter6);
- calipso_doi_putdef(map6->def.calipso);
- }
+ netlbl_af6list_foreach_rcu(iter6, &entry->def.addrsel->list6) {
+ map6 = netlbl_domhsh_addr6_entry(iter6);
+ calipso_doi_putdef(map6->def.calipso);
+ }
#endif /* IPv6 */
- break;
- case NETLBL_NLTYPE_CIPSOV4:
- cipso_v4_doi_putdef(entry->def.cipso);
-
Re: [net-next PATCH] netlabel: fix problems with mapping removal
On Fri, Aug 21, 2020 at 2:38 PM David Miller wrote: > From: Paul Moore > Date: Thu, 20 Aug 2020 21:46:14 -0400 > > > This patch fixes two main problems seen when removing NetLabel > > mappings: memory leaks and potentially extra audit noise. > > These are bug fixes therefore this needs to target the 'net' tree > and you must also provide appropriate "Fixes:" tags. > > Thank you. Reposted as requested: https://lore.kernel.org/selinux/159804209207.16190.14955035148979265114.stgit@sifl -- paul moore www.paul-moore.com
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
wrote:
> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
> with the old programs that can pass sockaddr_in with AF_UNSPEC and
> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
> It was found with LTP/asapi_01 test.
>
> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
> case to AF_INET and make sure that the address is INADDR_ANY.
>
> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>
> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
> Signed-off-by: Alexey Kodanev
> ---
> security/selinux/hooks.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
Thanks for finding and reporting this regression.
I think I would prefer to avoid having to duplicate the
AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
it is a small bit of code and unlikely to change. I'm wondering if it
would be better to check both the socket and sockaddr address family
in the main if conditional inside selinux_socket_bind(), what do you
think? Another option would be to go back to just checking the
soackaddr address family; we moved away from that for a reason which
escapes at the moment (code cleanliness?), but perhaps that was a
mistake.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4cafe6a19167..a3789b167667 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket *sock, struc>
{
struct sock *sk = sock->sk;
u16 family;
+ u16 family_sa;
int err;
err = sock_has_perm(sk, SOCKET__BIND);
@@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket *sock, struc>
/* If PF_INET or PF_INET6, check name_bind permission for the port. */
family = sk->sk_family;
- if (family == PF_INET || family == PF_INET6) {
+ family_sa = address->sa_family;
+ if ((family == PF_INET || family == PF_INET6) &&
+ (family_sa == PF_INET || family_sa == PF_INET6)) {
char *addrp;
struct sk_security_struct *sksec = sk->sk_security;
struct common_audit_data ad;
@@ -4601,7 +4604,7 @@ static int selinux_socket_bind(struct socket *sock, struc>
* need to check address->sa_family as it is possible to have
* sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
*/
- switch (address->sa_family) {
+ switch (family_sa) {
case AF_INET:
if (addrlen < sizeof(struct sockaddr_in))
return -EINVAL;
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 4cafe6a..649a3be 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4602,10 +4602,16 @@ static int selinux_socket_bind(struct socket *sock,
> struct sockaddr *address, in
> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
> */
> switch (address->sa_family) {
> + case AF_UNSPEC:
> case AF_INET:
> if (addrlen < sizeof(struct sockaddr_in))
> return -EINVAL;
> addr4 = (struct sockaddr_in *)address;
> +
> + if (address->sa_family == AF_UNSPEC &&
> + addr4->sin_addr.s_addr != htonl(INADDR_ANY))
> + return -EAFNOSUPPORT;
> +
> snum = ntohs(addr4->sin_port);
> addrp = (char *)&addr4->sin_addr.s_addr;
> break;
> @@ -4681,10 +4687,10 @@ static int selinux_socket_bind(struct socket *sock,
> struct sockaddr *address, in
> ad.u.net->sport = htons(snum);
> ad.u.net->family = family;
>
> - if (address->sa_family == AF_INET)
> - ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
> - else
> + if (address->sa_family == AF_INET6)
> ad.u.net->v6info.saddr = addr6->sin6_addr;
> + else
> + ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
>
> err = avc_has_perm(&selinux_state,
>sksec->sid, sid,
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley wrote:
> On 05/08/2018 01:05 PM, Paul Moore wrote:
>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>> wrote:
>>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>>> It was found with LTP/asapi_01 test.
>>>
>>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>>> case to AF_INET and make sure that the address is INADDR_ANY.
>>>
>>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>>
>>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>>> Signed-off-by: Alexey Kodanev
>>> ---
>>> security/selinux/hooks.c | 12 +---
>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> Thanks for finding and reporting this regression.
>>
>> I think I would prefer to avoid having to duplicate the
>> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
>> it is a small bit of code and unlikely to change. I'm wondering if it
>> would be better to check both the socket and sockaddr address family
>> in the main if conditional inside selinux_socket_bind(), what do you
>> think? Another option would be to go back to just checking the
>> soackaddr address family; we moved away from that for a reason which
>> escapes at the moment (code cleanliness?), but perhaps that was a
>> mistake.
>
> We've always used the sk->sk_family there; it was only the recent code from
> Richard that started
> using the socket address family.
Yes I know, I thought I was the one that suggested it at some point
(I'll take the blame) ... although now that I'm looking at the git
log, maybe I'm confusing it with something else.
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 4cafe6a19167..a3789b167667 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket *sock,
>> struc>
>> {
>>struct sock *sk = sock->sk;
>>u16 family;
>> + u16 family_sa;
>>int err;
>>
>>err = sock_has_perm(sk, SOCKET__BIND);
>> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket *sock,
>> struc>
>>
>>/* If PF_INET or PF_INET6, check name_bind permission for the port. */
>>family = sk->sk_family;
>> - if (family == PF_INET || family == PF_INET6) {
>> + family_sa = address->sa_family;
>> + if ((family == PF_INET || family == PF_INET6) &&
>> + (family_sa == PF_INET || family_sa == PF_INET6)) {
>
> Wouldn't this allow bypassing the name_bind permission check by passing in
> AF_UNSPEC?
I believe these name_bind permission checkis skipped for AF_UNSPEC
already, isn't it? The only way the name_bind check would be
triggered is if the source port, snum, was non-zero and I didn't think
that was really legal for AF_UNSPEC/INADDR_ANY, is it?
>>char *addrp;
>>struct sk_security_struct *sksec = sk->sk_security;
>>struct common_audit_data ad;
>> @@ -4601,7 +4604,7 @@ static int selinux_socket_bind(struct socket *sock,
>> struc>
>> * need to check address->sa_family as it is possible to have
>> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>> */
>> - switch (address->sa_family) {
>> + switch (family_sa) {
>>case AF_INET:
>>if (addrlen < sizeof(struct sockaddr_in))
>>return -EINVAL;
>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 4cafe6a..649a3be 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -4602,10 +4602,16 @@ static int selinux_socket_bind(struct socket *sock,
>>> struct sockaddr *address, in
>>> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>>> */
>>> switch (address->sa_family) {
>>> + case AF_UNSPEC:
>>
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley wrote:
> On 05/08/2018 08:25 PM, Paul Moore wrote:
>> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley wrote:
>>> On 05/08/2018 01:05 PM, Paul Moore wrote:
>>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>>>> wrote:
>>>>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>>>>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>>>>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>>>>> It was found with LTP/asapi_01 test.
>>>>>
>>>>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>>>>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>>>>> case to AF_INET and make sure that the address is INADDR_ANY.
>>>>>
>>>>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>>>>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>>>>
>>>>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>>>>> Signed-off-by: Alexey Kodanev
>>>>> ---
>>>>> security/selinux/hooks.c | 12 +---
>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> Thanks for finding and reporting this regression.
>>>>
>>>> I think I would prefer to avoid having to duplicate the
>>>> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
>>>> it is a small bit of code and unlikely to change. I'm wondering if it
>>>> would be better to check both the socket and sockaddr address family
>>>> in the main if conditional inside selinux_socket_bind(), what do you
>>>> think? Another option would be to go back to just checking the
>>>> soackaddr address family; we moved away from that for a reason which
>>>> escapes at the moment (code cleanliness?), but perhaps that was a
>>>> mistake.
>>>
>>> We've always used the sk->sk_family there; it was only the recent code from
>>> Richard that started
>>> using the socket address family.
>>
>> Yes I know, I thought I was the one that suggested it at some point
>> (I'll take the blame) ... although now that I'm looking at the git
>> log, maybe I'm confusing it with something else.
>>
>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>> index 4cafe6a19167..a3789b167667 100644
>>>> --- a/security/selinux/hooks.c
>>>> +++ b/security/selinux/hooks.c
>>>> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket *sock,
>>>> struc>
>>>> {
>>>>struct sock *sk = sock->sk;
>>>>u16 family;
>>>> + u16 family_sa;
>>>>int err;
>>>>
>>>>err = sock_has_perm(sk, SOCKET__BIND);
>>>> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket *sock,
>>>> struc>
>>>>
>>>>/* If PF_INET or PF_INET6, check name_bind permission for the port.
>>>> */
>>>>family = sk->sk_family;
>>>> - if (family == PF_INET || family == PF_INET6) {
>>>> + family_sa = address->sa_family;
>>>> + if ((family == PF_INET || family == PF_INET6) &&
>>>> + (family_sa == PF_INET || family_sa == PF_INET6)) {
>>>
>>> Wouldn't this allow bypassing the name_bind permission check by passing in
>>> AF_UNSPEC?
>>
>> I believe these name_bind permission checkis skipped for AF_UNSPEC
>> already, isn't it? The only way the name_bind check would be
>> triggered is if the source port, snum, was non-zero and I didn't think
>> that was really legal for AF_UNSPEC/INADDR_ANY, is it?
>
> 1) What in inet_bind() prevents that from occurring?
> 2) Regardless, what about the node_bind check?
Fair enough. As mentioned above, perhaps the right fix is to move the
address family checking back to how it was pre-SCTP.
Alexey, is this something you want to do, or should we take care of that?
>>>>char *addrp;
>>>>struct sk_security_struct *sksec = sk->sk_security;
>>>>struct common_audit_data ad;
>>>> @@ -4601,7 +4604,7 @@ static int selinux_socket_bind(struct socket *sock,
>>>> struc>
>>>
Re: [PATCH ghak81 RFC V1 1/5] audit: normalize loginuid read access
On Fri, May 4, 2018 at 4:54 PM, Richard Guy Briggs wrote:
> Recognizing that the loginuid is an internal audit value, use an access
> function to retrieve the audit loginuid value for the task rather than
> reaching directly into the task struct to get it.
>
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/auditsc.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 479c031..f3817d0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -374,7 +374,7 @@ static int audit_field_compare(struct task_struct *tsk,
> case AUDIT_COMPARE_EGID_TO_OBJ_GID:
> return audit_compare_gid(cred->egid, name, f, ctx);
> case AUDIT_COMPARE_AUID_TO_OBJ_UID:
> - return audit_compare_uid(tsk->loginuid, name, f, ctx);
> + return audit_compare_uid(audit_get_loginuid(tsk), name, f,
> ctx);
> case AUDIT_COMPARE_SUID_TO_OBJ_UID:
> return audit_compare_uid(cred->suid, name, f, ctx);
> case AUDIT_COMPARE_SGID_TO_OBJ_GID:
> @@ -385,7 +385,7 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_compare_gid(cred->fsgid, name, f, ctx);
> /* uid comparisons */
> case AUDIT_COMPARE_UID_TO_AUID:
> - return audit_uid_comparator(cred->uid, f->op, tsk->loginuid);
> + return audit_uid_comparator(cred->uid, f->op,
> audit_get_loginuid(tsk));
> case AUDIT_COMPARE_UID_TO_EUID:
> return audit_uid_comparator(cred->uid, f->op, cred->euid);
> case AUDIT_COMPARE_UID_TO_SUID:
> @@ -394,11 +394,11 @@ static int audit_field_compare(struct task_struct *tsk,
> return audit_uid_comparator(cred->uid, f->op, cred->fsuid);
> /* auid comparisons */
> case AUDIT_COMPARE_AUID_TO_EUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->euid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->euid);
> case AUDIT_COMPARE_AUID_TO_SUID:
> - return audit_uid_comparator(tsk->loginuid, f->op, cred->suid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->suid);
> case AUDIT_COMPARE_AUID_TO_FSUID:
> - return audit_uid_comparator(tsk->loginuid, f->op,
> cred->fsuid);
> + return audit_uid_comparator(audit_get_loginuid(tsk), f->op,
> cred->fsuid);
> /* euid comparisons */
> case AUDIT_COMPARE_EUID_TO_SUID:
> return audit_uid_comparator(cred->euid, f->op, cred->suid);
> @@ -611,7 +611,7 @@ static int audit_filter_rules(struct task_struct *tsk,
> result = match_tree_refs(ctx, rule->tree);
> break;
> case AUDIT_LOGINUID:
> - result = audit_uid_comparator(tsk->loginuid, f->op,
> f->uid);
> + result =
> audit_uid_comparator(audit_get_loginuid(tsk), f->op, f->uid);
> break;
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk),
> f->op, f->val);
> @@ -2287,8 +2287,8 @@ int audit_signal_info(int sig, struct task_struct *t)
> (sig == SIGTERM || sig == SIGHUP ||
> sig == SIGUSR1 || sig == SIGUSR2)) {
> audit_sig_pid = task_tgid_nr(tsk);
> - if (uid_valid(tsk->loginuid))
> - audit_sig_uid = tsk->loginuid;
> + if (uid_valid(audit_get_loginuid(tsk)))
> + audit_sig_uid = audit_get_loginuid(tsk);
I realize this comment is a little silly given the nature of loginuid,
but if we are going to abstract away loginuid accesses (which I think
is good), we should probably access it once, store it in a local
variable, perform the validity check on the local variable, then
commit the local variable to audit_sig_uid. I realize a TOCTOU
problem is unlikely here, but with this new layer of abstraction it
seems that some additional safety might be a good thing.
> else
> audit_sig_uid = uid;
> security_task_getsecid(tsk, &audit_sig_sid);
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
Re: [PATCH ghak81 RFC V1 2/5] audit: convert sessionid unset to a macro
On Tue, May 8, 2018 at 9:34 PM, Richard Guy Briggs wrote:
> On 2018-05-04 16:54, Richard Guy Briggs wrote:
>> Use a macro, "AUDIT_SID_UNSET", to replace each instance of
>> initialization and comparison to an audit session ID.
>>
>> Signed-off-by: Richard Guy Briggs
>
> There's a minor issue with this patch, adding a header include to
> init/init_task.c in this patch and removing it from patch 5. That'll be
> in the next revision.
Okay, thanks for the heads-up. FWIW, this patch looks reasonable in
principle; changing magic numbers to macros/constants is almost always
a step in the right direction.
> I have dynamic allocation working, so that has a good chance of
> appearing too.
I'll comment on that in your patch 0, I just want to get through the
rest of the patches first.
>> ---
>> include/linux/audit.h | 2 +-
>> include/net/xfrm.h | 2 +-
>> include/uapi/linux/audit.h | 1 +
>> init/init_task.c | 2 +-
>> kernel/auditsc.c | 4 ++--
>> 5 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 75d5b03..5f86f7c 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -513,7 +513,7 @@ static inline kuid_t audit_get_loginuid(struct
>> task_struct *tsk)
>> }
>> static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
>> {
>> - return -1;
>> + return AUDIT_SID_UNSET;
>> }
>> static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>> { }
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index a872379..fcce8ee 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -751,7 +751,7 @@ static inline void xfrm_audit_helper_usrinfo(bool
>> task_valid,
>> audit_get_loginuid(current) :
>> INVALID_UID);
>> const unsigned int ses = task_valid ? audit_get_sessionid(current) :
>> - (unsigned int) -1;
>> + AUDIT_SID_UNSET;
>>
>> audit_log_format(audit_buf, " auid=%u ses=%u", auid, ses);
>> audit_log_task_context(audit_buf);
>> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
>> index 4e61a9e..04f9bd2 100644
>> --- a/include/uapi/linux/audit.h
>> +++ b/include/uapi/linux/audit.h
>> @@ -465,6 +465,7 @@ struct audit_tty_status {
>> };
>>
>> #define AUDIT_UID_UNSET (unsigned int)-1
>> +#define AUDIT_SID_UNSET ((unsigned int)-1)
>>
>> /* audit_rule_data supports filter rules with both integer and string
>> * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> diff --git a/init/init_task.c b/init/init_task.c
>> index 3ac6e75..c788f91 100644
>> --- a/init/init_task.c
>> +++ b/init/init_task.c
>> @@ -119,7 +119,7 @@ struct task_struct init_task
>> .thread_node= LIST_HEAD_INIT(init_signals.thread_head),
>> #ifdef CONFIG_AUDITSYSCALL
>> .loginuid = INVALID_UID,
>> - .sessionid = (unsigned int)-1,
>> + .sessionid = AUDIT_SID_UNSET,
>> #endif
>> #ifdef CONFIG_PERF_EVENTS
>> .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index f3817d0..6e3ceb9 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -2050,7 +2050,7 @@ static void audit_log_set_loginuid(kuid_t
>> koldloginuid, kuid_t kloginuid,
>> int audit_set_loginuid(kuid_t loginuid)
>> {
>> struct task_struct *task = current;
>> - unsigned int oldsessionid, sessionid = (unsigned int)-1;
>> + unsigned int oldsessionid, sessionid = AUDIT_SID_UNSET;
>> kuid_t oldloginuid;
>> int rc;
>>
>> @@ -2064,7 +2064,7 @@ int audit_set_loginuid(kuid_t loginuid)
>> /* are we setting or clearing? */
>> if (uid_valid(loginuid)) {
>> sessionid = (unsigned int)atomic_inc_return(&session_id);
>> - if (unlikely(sessionid == (unsigned int)-1))
>> + if (unlikely(sessionid == AUDIT_SID_UNSET))
>> sessionid = (unsigned
>> int)atomic_inc_return(&session_id);
>> }
>>
>> --
>> 1.8.3.1
>>
>> --
>> Linux-audit mailing list
>> linux-au...@redhat.com
>> https://www.redhat.com/mailman/listinfo/linux-audit
>
> - RGB
>
> --
> Richard Guy Briggs
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>
> --
> Linux-audit mailing list
> linux-au...@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
Re: [PATCH ghak81 RFC V1 3/5] audit: use inline function to get audit context
On Fri, May 4, 2018 at 4:54 PM, Richard Guy Briggs wrote:
> Recognizing that the audit context is an internal audit value, use an
> access function to retrieve the audit context pointer for the task
> rather than reaching directly into the task struct to get it.
>
> Signed-off-by: Richard Guy Briggs
> ---
> include/linux/audit.h| 16 ---
> include/net/xfrm.h | 2 +-
> kernel/audit.c | 4 +--
> kernel/audit_watch.c | 2 +-
> kernel/auditsc.c | 52
> ++--
> net/bridge/netfilter/ebtables.c | 2 +-
> net/core/dev.c | 2 +-
> net/netfilter/x_tables.c | 2 +-
> net/netlabel/netlabel_user.c | 2 +-
> security/integrity/ima/ima_api.c | 2 +-
> security/integrity/integrity_audit.c | 2 +-
> security/lsm_audit.c | 2 +-
> security/selinux/hooks.c | 4 +--
> security/selinux/selinuxfs.c | 6 ++---
> security/selinux/ss/services.c | 12 -
> 15 files changed, 60 insertions(+), 52 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 5f86f7c..93e4c61 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -235,26 +235,30 @@ extern void __audit_inode_child(struct inode *parent,
> extern void __audit_seccomp(unsigned long syscall, long signr, int code);
> extern void __audit_ptrace(struct task_struct *t);
>
> +static inline struct audit_context *audit_context(struct task_struct *task)
> +{
> + return task->audit_context;
> +}
Another case where I think I agree with everything here on principle,
especially when one considers it in the larger context of the audit
container ID work. However, I think we might be able to somply this a
bit by eliminating the parameter to the new audit_context() helper and
making it always reference the current task_struct. Based on this
patch it would appear that this change would work for all callers
except for audit_take_context() and __audit_syscall_entry(), both of
which are contained within the core audit code and are enough of a
special case that I think it is acceptable for them to access the
context directly. I'm trying to think of reasons why a non-audit
kernel subsystem would ever need to access the audit context of a
process other than current and I can't think of any ... removing the
task_struct pointer might help prevent mistakes/abuse in the future.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6e3ceb9..a4bbdcc 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -836,7 +836,7 @@ static inline struct audit_context
> *audit_take_context(struct task_struct *tsk,
> int return_valid,
> long return_code)
> {
> - struct audit_context *context = tsk->audit_context;
> + struct audit_context *context = audit_context(tsk);
>
> if (!context)
> return NULL;
> @@ -1510,7 +1510,7 @@ void __audit_syscall_entry(int major, unsigned long a1,
> unsigned long a2,
>unsigned long a3, unsigned long a4)
> {
> struct task_struct *tsk = current;
> - struct audit_context *context = tsk->audit_context;
> + struct audit_context *context = audit_context(tsk);
> enum audit_state state;
>
> if (!audit_enabled || !context)
--
paul moore
www.paul-moore.com
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Wed, May 9, 2018 at 11:11 AM, Stephen Smalley wrote:
> On 05/09/2018 11:01 AM, Paul Moore wrote:
>> On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley wrote:
>>> On 05/08/2018 08:25 PM, Paul Moore wrote:
>>>> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley wrote:
>>>>> On 05/08/2018 01:05 PM, Paul Moore wrote:
>>>>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>>>>>> wrote:
>>>>>>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>>>>>>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>>>>>>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>>>>>>> It was found with LTP/asapi_01 test.
>>>>>>>
>>>>>>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>>>>>>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>>>>>>> case to AF_INET and make sure that the address is INADDR_ANY.
>>>>>>>
>>>>>>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>>>>>>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>>>>>>
>>>>>>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>>>>>>> Signed-off-by: Alexey Kodanev
>>>>>>> ---
>>>>>>> security/selinux/hooks.c | 12 +---
>>>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> Thanks for finding and reporting this regression.
>>>>>>
>>>>>> I think I would prefer to avoid having to duplicate the
>>>>>> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
>>>>>> it is a small bit of code and unlikely to change. I'm wondering if it
>>>>>> would be better to check both the socket and sockaddr address family
>>>>>> in the main if conditional inside selinux_socket_bind(), what do you
>>>>>> think? Another option would be to go back to just checking the
>>>>>> soackaddr address family; we moved away from that for a reason which
>>>>>> escapes at the moment (code cleanliness?), but perhaps that was a
>>>>>> mistake.
>>>>>
>>>>> We've always used the sk->sk_family there; it was only the recent code
>>>>> from Richard that started
>>>>> using the socket address family.
>>>>
>>>> Yes I know, I thought I was the one that suggested it at some point
>>>> (I'll take the blame) ... although now that I'm looking at the git
>>>> log, maybe I'm confusing it with something else.
>>>>
>>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>>> index 4cafe6a19167..a3789b167667 100644
>>>>>> --- a/security/selinux/hooks.c
>>>>>> +++ b/security/selinux/hooks.c
>>>>>> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket
>>>>>> *sock, struc>
>>>>>> {
>>>>>>struct sock *sk = sock->sk;
>>>>>>u16 family;
>>>>>> + u16 family_sa;
>>>>>>int err;
>>>>>>
>>>>>>err = sock_has_perm(sk, SOCKET__BIND);
>>>>>> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket
>>>>>> *sock, struc>
>>>>>>
>>>>>>/* If PF_INET or PF_INET6, check name_bind permission for the
>>>>>> port. */
>>>>>>family = sk->sk_family;
>>>>>> - if (family == PF_INET || family == PF_INET6) {
>>>>>> + family_sa = address->sa_family;
>>>>>> + if ((family == PF_INET || family == PF_INET6) &&
>>>>>> + (family_sa == PF_INET || family_sa == PF_INET6)) {
>>>>>
>>>>> Wouldn't this allow bypassing the name_bind permission check by passing
>>>>> in AF_UNSPEC?
>>>>
>>>> I believe these name_bind permission checkis skipped for AF_UNSPEC
>>>> already, isn't it? The only way the name_bind check would be
>>>> triggered is if the source port, snum, was non-zero and I didn't
Re: [PATCH ghak81 RFC V1 5/5] audit: collect audit task parameters
info struct, should the audit_task_info struct be placed
outside the CONFIG_AUDITSYSCALL protections? Or rather, shouldn't the
CONFIG_AUDITSYSCALL protections be moved inside audit_task_info or
removed entirely?
> diff --git a/init/init_task.c b/init/init_task.c
> index c788f91..d33260d 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -118,8 +119,11 @@ struct task_struct init_task
> .thread_group = LIST_HEAD_INIT(init_task.thread_group),
> .thread_node= LIST_HEAD_INIT(init_signals.thread_head),
> #ifdef CONFIG_AUDITSYSCALL
> - .loginuid = INVALID_UID,
> - .sessionid = AUDIT_SID_UNSET,
> + .audit = {
> + .loginuid = INVALID_UID,
> + .sessionid = AUDIT_SID_UNSET,
> + .ctx= NULL,
> + },
> #endif
> #ifdef CONFIG_PERF_EVENTS
> .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f294e4a..b5d8bff 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2068,8 +2068,8 @@ int audit_set_loginuid(kuid_t loginuid)
> sessionid = (unsigned
> int)atomic_inc_return(&session_id);
> }
>
> - task->sessionid = sessionid;
> - task->loginuid = loginuid;
> + task->audit.sessionid = sessionid;
> + task->audit.loginuid = loginuid;
> out:
> audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid,
> sessionid, rc);
> return rc;
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
Re: [PATCH ghak81 RFC V1 0/5] audit: group task params
On Fri, May 4, 2018 at 4:54 PM, Richard Guy Briggs wrote: > Group the audit parameters for each task into one structure. > In particular, remove the loginuid and sessionid values and the audit > context pointer from the task structure, replacing them with an audit > task information structure to contain them. Use access functions to > access audit values. > > Note: Use static allocation of the audit task information structure > initially. Dynamic allocation was considered and attempted, but isn't > ready yet. Static allocation has the limitation that future audit task > information structure changes would cause a visible change to the rest > of the kernel, whereas dynamic allocation would mostly hide any future > changes. > > The first four access normalization patches could stand alone. I agree that the first four patches have some standalone value, and since we are currently at -rc4, did you want to post another patchset of just those four patches with feedback incorporated? I imagine that should be quick work, and that way they aren't help up with any problems/discussion regarding the take_struct changes. > Passes audit-testsuite. > > Richard Guy Briggs (5): > audit: normalize loginuid read access > audit: convert sessionid unset to a macro > audit: use inline function to get audit context > audit: use inline function to set audit context > audit: collect audit task parameters > > MAINTAINERS | 2 +- > include/linux/audit.h| 30 ++--- > include/linux/audit_task.h | 31 ++ > include/linux/sched.h| 6 +-- > include/net/xfrm.h | 4 +- > include/uapi/linux/audit.h | 1 + > init/init_task.c | 8 +++- > kernel/audit.c | 4 +- > kernel/audit_watch.c | 2 +- > kernel/auditsc.c | 82 > ++-- > kernel/fork.c| 2 +- > net/bridge/netfilter/ebtables.c | 2 +- > net/core/dev.c | 2 +- > net/netfilter/x_tables.c | 2 +- > net/netlabel/netlabel_user.c | 2 +- > security/integrity/ima/ima_api.c | 2 +- > security/integrity/integrity_audit.c | 2 +- > security/lsm_audit.c | 2 +- > security/selinux/hooks.c | 4 +- > security/selinux/selinuxfs.c | 6 +-- > security/selinux/ss/services.c | 12 +++--- > 21 files changed, 129 insertions(+), 79 deletions(-) > create mode 100644 include/linux/audit_task.h > > -- > 1.8.3.1 > -- paul moore www.paul-moore.com
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Wed, May 9, 2018 at 11:34 AM, Paul Moore wrote:
> On Wed, May 9, 2018 at 11:11 AM, Stephen Smalley wrote:
>> On 05/09/2018 11:01 AM, Paul Moore wrote:
>>> On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley wrote:
>>>> On 05/08/2018 08:25 PM, Paul Moore wrote:
>>>>> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley
>>>>> wrote:
>>>>>> On 05/08/2018 01:05 PM, Paul Moore wrote:
>>>>>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>>>>>>> wrote:
>>>>>>>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>>>>>>>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>>>>>>>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>>>>>>>> It was found with LTP/asapi_01 test.
>>>>>>>>
>>>>>>>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>>>>>>>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>>>>>>>> case to AF_INET and make sure that the address is INADDR_ANY.
>>>>>>>>
>>>>>>>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>>>>>>>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>>>>>>>
>>>>>>>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>>>>>>>> Signed-off-by: Alexey Kodanev
>>>>>>>> ---
>>>>>>>> security/selinux/hooks.c | 12 +---
>>>>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> Thanks for finding and reporting this regression.
>>>>>>>
>>>>>>> I think I would prefer to avoid having to duplicate the
>>>>>>> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
>>>>>>> it is a small bit of code and unlikely to change. I'm wondering if it
>>>>>>> would be better to check both the socket and sockaddr address family
>>>>>>> in the main if conditional inside selinux_socket_bind(), what do you
>>>>>>> think? Another option would be to go back to just checking the
>>>>>>> soackaddr address family; we moved away from that for a reason which
>>>>>>> escapes at the moment (code cleanliness?), but perhaps that was a
>>>>>>> mistake.
>>>>>>
>>>>>> We've always used the sk->sk_family there; it was only the recent code
>>>>>> from Richard that started
>>>>>> using the socket address family.
>>>>>
>>>>> Yes I know, I thought I was the one that suggested it at some point
>>>>> (I'll take the blame) ... although now that I'm looking at the git
>>>>> log, maybe I'm confusing it with something else.
>>>>>
>>>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>>>> index 4cafe6a19167..a3789b167667 100644
>>>>>>> --- a/security/selinux/hooks.c
>>>>>>> +++ b/security/selinux/hooks.c
>>>>>>> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket
>>>>>>> *sock, struc>
>>>>>>> {
>>>>>>>struct sock *sk = sock->sk;
>>>>>>>u16 family;
>>>>>>> + u16 family_sa;
>>>>>>>int err;
>>>>>>>
>>>>>>>err = sock_has_perm(sk, SOCKET__BIND);
>>>>>>> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket
>>>>>>> *sock, struc>
>>>>>>>
>>>>>>>/* If PF_INET or PF_INET6, check name_bind permission for the
>>>>>>> port. */
>>>>>>>family = sk->sk_family;
>>>>>>> - if (family == PF_INET || family == PF_INET6) {
>>>>>>> + family_sa = address->sa_family;
>>>>>>> + if ((family == PF_INET || family == PF_INET6) &&
>>>>>>> + (family_sa == PF_INET || family_sa == PF_INET6)) {
>>>>>>
>>>>>> Wouldn't this allow bypassing th
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On Thu, May 10, 2018 at 5:28 AM, Alexey Kodanev
wrote:
> On 10.05.2018 01:02, Paul Moore wrote:
> ...
>> I just had a better look at this and I believe that Alexey and Stephen
>> are right: this is the best option. My apologies for the noise
>> earlier. However, while looking at the code I think there are some
>> additional necessary changes:
>>
>> * In the case of an SCTP socket, we should return -EINVAL, just as we
>> do with other address families.
>
> Right.
>
>> * While not strictly related to AF_UNSPEC, we really should be passing
>> the address family of the sockaddr, and not the socket, to functions
>> that need to interpret the bind address/port.
>
> That looks like a correct solution. I guess we need the same fix for
> sctp_connectx(), in selinux_socket_connect_helper().
Yes. I think we also need a small change to
selinux_sctp_bind_connect() to both not error out on AF_UNSPEC, and to
return EINVAL on a bad address family (some of the callers pass on the
return value, some don't).
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5f30045b2053..a8bac9b37ee7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5277,6 +5277,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int>
while (walk_size < addrlen) {
addr = addr_buf;
switch (addr->sa_family) {
+ case AF_UNSPEC:
case AF_INET:
len = sizeof(struct sockaddr_in);
break;
@@ -5284,7 +5285,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int>
len = sizeof(struct sockaddr_in6);
break;
default:
- return -EAFNOSUPPORT;
+ return -EINVAL;
}
err = -EINVAL;
>> I'm waiting for my kernel to compile so I haven't given this any
>> sanity testing, but the patch below is what I think we need ...
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 4cafe6a19167..5f30045b2053 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -4576,6 +4576,7 @@ static int selinux_socket_post_create(struct socket
>> *sock,
>> int family,
>> static int selinux_socket_bind(struct socket *sock, struct sockaddr
>> *address, i
>> nt addrlen)
>> {
>>struct sock *sk = sock->sk;
>> + struct sk_security_struct *sksec = sk->sk_security;
>>u16 family;
>>int err;
>>
>> @@ -4587,13 +4588,13 @@ static int selinux_socket_bind(struct socket *sock,
>> stru
>> ct sockaddr *address, in
>>family = sk->sk_family;
>>if (family == PF_INET || family == PF_INET6) {
>>char *addrp;
>> - struct sk_security_struct *sksec = sk->sk_security;
>>struct common_audit_data ad;
>>struct lsm_network_audit net = {0,};
>>struct sockaddr_in *addr4 = NULL;
>>struct sockaddr_in6 *addr6 = NULL;
>>unsigned short snum;
>>u32 sid, node_perm;
>> + u16 family_sa = address->sa_family;
>>
>>/*
>> * sctp_bindx(3) calls via selinux_sctp_bind_connect()
>> @@ -4601,11 +4602,19 @@ static int selinux_socket_bind(struct socket *sock,
>> stru
>> ct sockaddr *address, in
>> * need to check address->sa_family as it is possible to have
>> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>> */
>> - switch (address->sa_family) {
>> + switch (family_sa) {
>> + case AF_UNSPEC:
>>case AF_INET:
>>if (addrlen < sizeof(struct sockaddr_in))
>>return -EINVAL;
>>addr4 = (struct sockaddr_in *)address;
>> + if (family_sa == AF_UNSPEC) {
>> + /* see "__inet_bind()", we only want to allow
>> +* AF_UNSPEC if the address is INADDR_ANY */
>> + if (addr4->sin_addr.s_addr !=
>> htonl(INADDR_ANY))
>> + goto err_af;
>> + family_sa = AF_INET;
>> + }
>>snum = ntohs(addr4->sin_port);
>>addrp = (char *)&addr4->sin_addr.s_ad
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
+static int audit_set_containerid_perm(struct task_struct *task, u64
> containerid)
> +{
> + struct task_struct *parent;
> + u64 pcontainerid, ccontainerid;
> +
> + /* Don't allow to set our own containerid */
> + if (current == task)
> + return -EPERM;
Why not? Is there some obvious security concern that I missing?
I ask because I suppose it might be possible for some container
runtime to do a fork, setup some of the environment and them exec the
container (before you answer the obvious "namespaces!" please remember
we're not trying to define containers).
> + /* Don't allow the containerid to be unset */
> + if (!cid_valid(containerid))
> + return -EINVAL;
> + /* if we don't have caps, reject */
> + if (!capable(CAP_AUDIT_CONTROL))
> + return -EPERM;
> + /* if containerid is unset, allow */
> + if (!audit_containerid_set(task))
> + return 0;
> + /* it is already set, and not inherited from the parent, reject */
> + ccontainerid = audit_get_containerid(task);
> + rcu_read_lock();
> + parent = rcu_dereference(task->real_parent);
> + rcu_read_unlock();
> + task_lock(parent);
> + pcontainerid = audit_get_containerid(parent);
> + task_unlock(parent);
> + if (ccontainerid != pcontainerid)
> + return -EPERM;
> + return 0;
> +}
> +
> +static void audit_log_set_containerid(struct task_struct *task, u64
> oldcontainerid,
> + u64 containerid, int rc)
> +{
> + struct audit_buffer *ab;
> + uid_t uid;
> + struct tty_struct *tty;
> +
> + if (!audit_enabled)
> + return;
> +
> + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONTAINER);
> + if (!ab)
> + return;
> +
> + uid = from_kuid(&init_user_ns, task_uid(current));
> + tty = audit_get_tty(current);
> +
> + audit_log_format(ab, "op=set pid=%d uid=%u", task_tgid_nr(current),
> uid);
> + audit_log_task_context(ab);
> + audit_log_format(ab, " auid=%u tty=%s ses=%u opid=%d old-contid=%llu
> contid=%llu res=%d",
> +from_kuid(&init_user_ns,
> audit_get_loginuid(current)),
> +tty ? tty_name(tty) : "(none)",
> audit_get_sessionid(current),
> + task_tgid_nr(task), oldcontainerid, containerid,
> !rc);
> +
> + audit_put_tty(tty);
> + audit_log_end(ab);
> +}
> +
> +/**
> + * audit_set_containerid - set current task's audit_context containerid
> + * @containerid: containerid value
> + *
> + * Returns 0 on success, -EPERM on permission failure.
> + *
> + * Called (set) from fs/proc/base.c::proc_containerid_write().
> + */
> +int audit_set_containerid(struct task_struct *task, u64 containerid)
> +{
> + u64 oldcontainerid;
> + int rc;
> +
> + oldcontainerid = audit_get_containerid(task);
> +
> + rc = audit_set_containerid_perm(task, containerid);
> + if (!rc) {
> + task_lock(task);
> + task->containerid = containerid;
> + task_unlock(task);
> + }
> +
> + audit_log_set_containerid(task, oldcontainerid, containerid, rc);
> + return rc;
Why are audit_set_containerid_perm() and audit_log_containerid()
separate functions?
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 02/13] audit: check children and threading before allowing containerid
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote: > Check if a task has existing children or co-threads and refuse to set > the container ID if either are present. Failure to check this could > permit games where a child scratches its parent's back to work around > inheritance and double-setting policy. > > Signed-off-by: Richard Guy Briggs > --- > kernel/auditsc.c | 4 > 1 file changed, 4 insertions(+) I would just include this in patch 1/2 as I can't think of world where we wouldn't this check. > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 29c8482..a6b0a52 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2087,6 +2087,10 @@ static int audit_set_containerid_perm(struct > task_struct *task, u64 containerid) > /* if we don't have caps, reject */ > if (!capable(CAP_AUDIT_CONTROL)) > return -EPERM; > + /* if task has children or is not single-threaded, deny */ > + if (!list_empty(&task->children) || > + !(thread_group_leader(task) && thread_group_empty(task))) > + return -EPERM; > /* if containerid is unset, allow */ > if (!audit_containerid_set(task)) > return 0; > -- > 1.8.3.1 -- paul moore www.paul-moore.com
Re: [RFC PATCH ghak32 V2 04/13] audit: add containerid filtering
if (f->val != sizeof(u64))
> + goto exit_free;
> + str = audit_unpack_string(&bufp, &remain, f->val);
> + if (IS_ERR(str))
> + goto exit_free;
> + f->val64 = ((u64 *)str)[0];
> + break;
> }
> }
>
> @@ -666,6 +675,11 @@ static struct audit_rule_data
> *audit_krule_to_data(struct audit_krule *krule)
> data->buflen += data->values[i] =
> audit_pack_string(&bufp,
> audit_mark_path(krule->exe));
> break;
> + case AUDIT_CONTAINERID:
> + data->buflen += data->values[i] = sizeof(u64);
> + for (i = 0; i < sizeof(u64); i++)
> + ((char *)bufp)[i] = ((char *)&f->val64)[i];
> + break;
> case AUDIT_LOGINUID_SET:
> if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val)
> {
> data->fields[i] = AUDIT_LOGINUID;
> @@ -752,6 +766,10 @@ static int audit_compare_rule(struct audit_krule *a,
> struct audit_krule *b)
> if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> return 1;
> break;
> + case AUDIT_CONTAINERID:
> + if (a->fields[i].val64 != b->fields[i].val64)
> + return 1;
> + break;
> default:
> if (a->fields[i].val != b->fields[i].val)
> return 1;
> @@ -1210,6 +1228,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> }
> }
>
> +int audit_comparator64(u64 left, u32 op, u64 right)
> +{
> + switch (op) {
> + case Audit_equal:
> + return (left == right);
> + case Audit_not_equal:
> + return (left != right);
> + case Audit_lt:
> + return (left < right);
> + case Audit_le:
> + return (left <= right);
> + case Audit_gt:
> + return (left > right);
> + case Audit_ge:
> + return (left >= right);
> + case Audit_bitmask:
> + return (left & right);
> + case Audit_bittest:
> + return ((left & right) == right);
> + default:
> + BUG();
> + return 0;
> + }
> +}
> +
> int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> {
> switch (op) {
> @@ -1348,6 +1391,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> result =
> audit_comparator(audit_loginuid_set(current),
> f->op, f->val);
> break;
> + case AUDIT_CONTAINERID:
> + result =
> audit_comparator64(audit_get_containerid(current),
> + f->op,
> f->val64);
> + break;
> case AUDIT_MSGTYPE:
> result = audit_comparator(msgtype, f->op,
> f->val);
> break;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 65be110..2bba324 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -614,6 +614,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk),
> f->op, f->val);
> break;
> + case AUDIT_CONTAINERID:
> + result =
> audit_comparator64(audit_get_containerid(tsk), f->op, f->val64);
> + break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> linux-au...@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 05/13] audit: add containerid support for ptrace and signals
gt; audit_log_pid_context(context, context->target_pid,
> context->target_auid, context->target_uid,
> context->target_sessionid,
> - context->target_sid, context->target_comm))
> + context->target_sid, context->target_comm)
> + && audit_log_container_info(context, "target",
> context->target_cid))
Same question.
> call_panic = 1;
>
> if (context->pwd.dentry && context->pwd.mnt) {
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 06/13] audit: add support for non-syscall auxiliary records
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
> Standalone audit records have the timestamp and serial number generated
> on the fly and as such are unique, making them standalone. This new
> function audit_alloc_local() generates a local audit context that will
> be used only for a standalone record and its auxiliary record(s). The
> context is discarded immediately after the local associated records are
> produced.
>
> Signed-off-by: Richard Guy Briggs
> ---
> include/linux/audit.h | 8
> kernel/auditsc.c | 20 +++-
> 2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index ed16bb6..c0b83cb 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -227,7 +227,9 @@ static inline int audit_log_container_info(struct
> audit_context *context,
> /* These are defined in auditsc.c */
> /* Public API */
> extern int audit_alloc(struct task_struct *task);
> +extern struct audit_context *audit_alloc_local(void);
> extern void __audit_free(struct task_struct *task);
> +extern void audit_free_context(struct audit_context *context);
> extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long
> a1,
> unsigned long a2, unsigned long a3);
> extern void __audit_syscall_exit(int ret_success, long ret_value);
> @@ -472,6 +474,12 @@ static inline int audit_alloc(struct task_struct *task)
> {
> return 0;
> }
> +static inline struct audit_context *audit_alloc_local(void)
> +{
> + return NULL;
> +}
> +static inline void audit_free_context(struct audit_context *context)
> +{ }
> static inline void audit_free(struct task_struct *task)
> { }
> static inline void audit_syscall_entry(int major, unsigned long a0,
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 2932ef1..7103d23 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -959,8 +959,26 @@ int audit_alloc(struct task_struct *tsk)
> return 0;
> }
>
> -static inline void audit_free_context(struct audit_context *context)
> +struct audit_context *audit_alloc_local(void)
> {
> + struct audit_context *context;
> +
> + if (!audit_ever_enabled)
> + return NULL; /* Return if not auditing. */
> +
> + context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
> + if (!context)
> + return NULL;
> + context->serial = audit_serial();
> + context->ctime = current_kernel_time64();
> + context->in_syscall = 1;
> + return context;
> +}
> +
> +inline void audit_free_context(struct audit_context *context)
> +{
> + if (!context)
> + return;
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> free_tree_refs(context);
I'm reserving the option to comment on this idea further as I make my
way through the patchset, but audit_free_context() definitely
shouldn't be declared as an inline function.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 07/13] audit: add container aux record to watch/tree/mark
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
> Add container ID auxiliary record to mark, watch and tree rule
> configuration standalone records.
>
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/audit_fsnotify.c | 5 -
> kernel/audit_tree.c | 5 -
> kernel/audit_watch.c| 33 +++--
> 3 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index 52f368b..18c110d 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -124,10 +124,11 @@ static void audit_mark_log_rule_change(struct
> audit_fsnotify_mark *audit_mark, c
> {
> struct audit_buffer *ab;
> struct audit_krule *rule = audit_mark->rule;
> + struct audit_context *context = audit_alloc_local();
>
> if (!audit_enabled)
> return;
Move the audit_alloc_local() after the audit_enabled check.
> - ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> + ab = audit_log_start(context, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> audit_log_format(ab, "auid=%u ses=%u op=%s",
> @@ -138,6 +139,8 @@ static void audit_mark_log_rule_change(struct
> audit_fsnotify_mark *audit_mark, c
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=1", rule->listnr);
> audit_log_end(ab);
> + audit_log_container_info(context, "config",
> audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> void audit_remove_mark(struct audit_fsnotify_mark *audit_mark)
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 67e6956..7c085be 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -496,8 +496,9 @@ static int tag_chunk(struct inode *inode, struct
> audit_tree *tree)
> static void audit_tree_log_remove_rule(struct audit_krule *rule)
> {
> struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
Sort of independent of the audit container ID work, but shouldn't we
have an audit_enabled check here?
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> audit_log_format(ab, "op=remove_rule");
> @@ -506,6 +507,8 @@ static void audit_tree_log_remove_rule(struct audit_krule
> *rule)
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=1", rule->listnr);
> audit_log_end(ab);
> + audit_log_container_info(context, "config",
> audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> static void kill_rules(struct audit_tree *tree)
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 9eb8b35..60d75a2 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -238,20 +238,25 @@ static struct audit_watch *audit_dupe_watch(struct
> audit_watch *old)
>
> static void audit_watch_log_rule_change(struct audit_krule *r, struct
> audit_watch *w, char *op)
> {
> - if (audit_enabled) {
> - struct audit_buffer *ab;
> - ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> - if (unlikely(!ab))
> - return;
> - audit_log_format(ab, "auid=%u ses=%u op=%s",
> -from_kuid(&init_user_ns,
> audit_get_loginuid(current)),
> -audit_get_sessionid(current), op);
> - audit_log_format(ab, " path=");
> - audit_log_untrustedstring(ab, w->path);
> - audit_log_key(ab, r->filterkey);
> - audit_log_format(ab, " list=%d res=1", r->listnr);
> - audit_log_end(ab);
> - }
> + struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
> +
> + if (!audit_enabled)
> + return;
Same as above, do the allocation after the audit_enabled check.
> + ab = audit_log_start(context, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> + if (unlikely(!ab))
> + return;
> + audit_log_format(ab, "auid=%u ses=%u op=%s",
> +from_kuid(&init_user_ns,
> audit_get_loginuid(current)),
> +audit_get_sessionid(current), op);
> + audit_log_format(ab, " path=");
> + audit_log_untrustedstring(ab, w->path);
> + audit_log_key(ab, r->filterkey);
> + audit_log_format(ab, " list=%d res=1", r->listnr);
> + audit_log_end(ab);
> + audit_log_container_info(context, "config",
> audit_get_containerid(current));
> + audit_free_context(context);
> }
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Wed, Apr 18, 2018 at 8:41 PM, Casey Schaufler wrote:
> On 4/18/2018 4:47 PM, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
>>> Implement the proc fs write to set the audit container ID of a process,
>>> emitting an AUDIT_CONTAINER record to document the event.
>>> ...
>>>
>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>> index d258826..1b82191 100644
>>> --- a/include/linux/sched.h
>>> +++ b/include/linux/sched.h
>>> @@ -796,6 +796,7 @@ struct task_struct {
>>> #ifdef CONFIG_AUDITSYSCALL
>>> kuid_t loginuid;
>>> unsigned intsessionid;
>>> + u64 containerid;
>> This one line addition to the task_struct scares me the most of
>> anything in this patchset. Why? It's a field named "containerid" in
>> a perhaps one of the most widely used core kernel structures; the
>> possibilities for abuse are endless, and it's foolish to think we
>> would ever be able to adequately police this.
>
> If we can get the LSM infrastructure managed task blobs from
> module stacking in ahead of this we could create a trivial security
> module to manage this. It's not as if there aren't all sorts of
> interactions between security modules and the audit system already.
While yes, there are plenty of interactions between the two, it is
possible to use audit without the LSMs and I would like to preserve
that. Further, I don't want to entangle two very complicated code
changes or make the audit container ID effort dependent on LSM
stacking.
You're a good salesman Casey, but you're not that good ;)
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records
NABLE; > old.log_passwd = !!(t & AUDIT_TTY_LOG_PASSWD); > > - audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE); > + audit_log_common_recv_msg(context, &ab, AUDIT_CONFIG_CHANGE); > audit_log_format(ab, " op=tty_set old-enabled=%d > new-enabled=%d" > " old-log_passwd=%d new-log_passwd=%d > res=%d", > old.enabled, s.enabled, old.log_passwd, > s.log_passwd, !err); > audit_log_end(ab); > + audit_log_container_info(context, "config", > +audit_get_containerid(current)); > + audit_free_context(context); > break; > } > default: > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index c4c8746..5f7f4d6 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -1109,11 +1109,12 @@ static void audit_log_rule_change(char *action, > struct audit_krule *rule, int re > struct audit_buffer *ab; > uid_t loginuid = from_kuid(&init_user_ns, > audit_get_loginuid(current)); > unsigned int sessionid = audit_get_sessionid(current); > + struct audit_context *context = audit_alloc_local(); > > if (!audit_enabled) > return; Well, first I think we should be able to get rid of the local context, but if for some reason we can't use current->audit_context then do the allocation after the audit_enabled check. > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE); > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE); > if (!ab) > return; > audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid); > @@ -1122,6 +1123,8 @@ static void audit_log_rule_change(char *action, struct > audit_krule *rule, int re > audit_log_key(ab, rule->filterkey); > audit_log_format(ab, " list=%d res=%d", rule->listnr, res); > audit_log_end(ab); > + audit_log_container_info(context, "config", > audit_get_containerid(current)); > + audit_free_context(context); > } -- paul moore www.paul-moore.com
Re: [RFC PATCH ghak32 V2 10/13] audit: add containerid support for seccomp and anom_abend records
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
> Add container ID auxiliary records to secure computing and abnormal end
> standalone records.
>
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/auditsc.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 7103d23..2f02ed9 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2571,6 +2571,7 @@ static void audit_log_task(struct audit_buffer *ab)
> void audit_core_dumps(long signr)
> {
> struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
Looking quickly at do_coredump() I *believe* we can use current here.
> if (!audit_enabled)
> return;
> @@ -2578,19 +2579,22 @@ void audit_core_dumps(long signr)
> if (signr == SIGQUIT) /* don't care for those */
> return;
>
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_ANOM_ABEND);
> if (unlikely(!ab))
> return;
> audit_log_task(ab);
> audit_log_format(ab, " sig=%ld res=1", signr);
> audit_log_end(ab);
> + audit_log_container_info(context, "abend",
> audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> void __audit_seccomp(unsigned long syscall, long signr, int code)
> {
> struct audit_buffer *ab;
> + struct audit_context *context = audit_alloc_local();
We can definitely use current here.
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_SECCOMP);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_SECCOMP);
> if (unlikely(!ab))
> return;
> audit_log_task(ab);
> @@ -2598,6 +2602,8 @@ void __audit_seccomp(unsigned long syscall, long signr,
> int code)
> signr, syscall_get_arch(), syscall,
> in_compat_syscall(), KSTK_EIP(current), code);
> audit_log_end(ab);
> + audit_log_container_info(context, "seccomp",
> audit_get_containerid(current));
> + audit_free_context(context);
> }
>
> struct list_head *audit_killed_trees(void)
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces
aces(unsigned long flags, struct
> task_struct *tsk)
> return PTR_ERR(new_ns);
>
> tsk->nsproxy = new_ns;
> + net_add_audit_containerid(new_ns->net_ns, containerid);
> return 0;
> }
Hopefully we can handle this in audit_net_init(), we just need to
figure out where we can get the correct task_struct for the audit
container ID (some backpointer in the net struct?).
> @@ -217,6 +219,7 @@ int unshare_nsproxy_namespaces(unsigned long
> unshare_flags,
> void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> {
> struct nsproxy *ns;
> + u64 containerid = audit_get_containerid(p);
>
> might_sleep();
>
> @@ -224,6 +227,9 @@ void switch_task_namespaces(struct task_struct *p, struct
> nsproxy *new)
> ns = p->nsproxy;
> p->nsproxy = new;
> task_unlock(p);
> + net_del_audit_containerid(ns->net_ns, containerid);
> + if (new)
> + net_add_audit_containerid(new->net_ns, containerid);
Okay, we might need a hook here for switching namespaces, but I would
much rather it be a generic audit hook that calls directly into audit.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 12/13] audit: NETFILTER_PKT: record each container ID associated with a netNS
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
> Add container ID auxiliary record(s) to NETFILTER_PKT event standalone
> records. Iterate through all potential container IDs associated with a
> network namespace.
>
> Signed-off-by: Richard Guy Briggs
> ---
> kernel/audit.c | 1 +
> kernel/auditsc.c | 2 ++
> net/netfilter/xt_AUDIT.c | 15 ++-
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 08662b4..3c77e47 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2102,6 +2102,7 @@ int audit_log_container_info(struct audit_context
> *context,
> audit_log_end(ab);
> return 0;
> }
> +EXPORT_SYMBOL(audit_log_container_info);
>
> void audit_log_key(struct audit_buffer *ab, char *key)
> {
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 208da962..af68d01 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -975,6 +975,7 @@ struct audit_context *audit_alloc_local(void)
> context->in_syscall = 1;
> return context;
> }
> +EXPORT_SYMBOL(audit_alloc_local);
>
> inline void audit_free_context(struct audit_context *context)
> {
> @@ -989,6 +990,7 @@ inline void audit_free_context(struct audit_context
> *context)
> audit_proctitle_free(context);
> kfree(context);
> }
> +EXPORT_SYMBOL(audit_free_context);
>
> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> kuid_t auid, kuid_t uid, unsigned int
> sessionid,
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index c502419..edaa456 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -71,10 +71,14 @@ static bool audit_ip6(struct audit_buffer *ab, struct
> sk_buff *skb)
> {
> struct audit_buffer *ab;
> int fam = -1;
> + struct audit_context *context = audit_alloc_local();
> + struct audit_containerid *cont;
> + int i = 0;
> + struct net *net;
>
> if (audit_enabled == 0)
> goto errout;
Do I need to say it? I probably should ... the allocation should
happen after the audit_enabled check.
> - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (ab == NULL)
> goto errout;
>
> @@ -104,7 +108,16 @@ static bool audit_ip6(struct audit_buffer *ab, struct
> sk_buff *skb)
>
> audit_log_end(ab);
>
> + net = sock_net(NETLINK_CB(skb).sk);
> + list_for_each_entry(cont, &net->audit_containerid, list) {
> + char buf[14];
> +
> + sprintf(buf, "net%u", i++);
> + audit_log_container_info(context, buf, cont->id);
> + }
It seems like this could (should?) be hidden inside an audit function,
e.g. audit_log_net_containers() or something like that.
> errout:
> + audit_free_context(context);
> return XT_CONTINUE;
> }
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records
On Thu, Apr 19, 2018 at 8:31 AM, Richard Guy Briggs wrote:
> On 2018-04-18 21:27, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
>> > Add container ID auxiliary records to configuration change, feature set
>> > change
>> > and user generated standalone records.
>> >
>> > Signed-off-by: Richard Guy Briggs
>> > ---
>> > kernel/audit.c | 50
>> > --
>> > kernel/auditfilter.c | 5 -
>> > 2 files changed, 44 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/kernel/audit.c b/kernel/audit.c
>> > index b238be5..08662b4 100644
>> > --- a/kernel/audit.c
>> > +++ b/kernel/audit.c
>> > @@ -400,8 +400,9 @@ static int audit_log_config_change(char
>> > *function_name, u32 new, u32 old,
>> > {
>> > struct audit_buffer *ab;
>> > int rc = 0;
>> > + struct audit_context *context = audit_alloc_local();
>>
>> We should be able to use current->audit_context here right? If we
>> can't for every caller, perhaps we pass an audit_context as an
>> argument and only allocate a local context when the passed
>> audit_context is NULL.
>>
>> Also, if you're not comfortable always using current, just pass the
>> audit_context as you do with audit_log_common_recv_msg().
>
> As mentioned in the tree/watch/mark patch, this is all obsoleted by
> making the AUDIT_CONFIG_CHANGE record a SYSCALL auxiliary record.
You've known about my desire to connect records for quite some time.
> This review would have been more helpful a month and a half ago.
If you really want to sink to that level of discussion, better quality
patches from you would have been helpful too, that is the one of the
main reasons why it takes so long to review your code. Let's keep the
commentary focused on the code, discussions like this aren't likely to
be helpful to anyone.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 12/13] audit: NETFILTER_PKT: record each container ID associated with a netNS
On Thu, Apr 19, 2018 at 8:45 AM, Richard Guy Briggs wrote:
> On 2018-04-18 22:10, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
>> > Add container ID auxiliary record(s) to NETFILTER_PKT event standalone
>> > records. Iterate through all potential container IDs associated with a
>> > network namespace.
>> >
>> > Signed-off-by: Richard Guy Briggs
>> > ---
>> > kernel/audit.c | 1 +
>> > kernel/auditsc.c | 2 ++
>> > net/netfilter/xt_AUDIT.c | 15 ++-
>> > 3 files changed, 17 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/kernel/audit.c b/kernel/audit.c
>> > index 08662b4..3c77e47 100644
>> > --- a/kernel/audit.c
>> > +++ b/kernel/audit.c
>> > @@ -2102,6 +2102,7 @@ int audit_log_container_info(struct audit_context
>> > *context,
>> > audit_log_end(ab);
>> > return 0;
>> > }
>> > +EXPORT_SYMBOL(audit_log_container_info);
>> >
>> > void audit_log_key(struct audit_buffer *ab, char *key)
>> > {
>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 208da962..af68d01 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -975,6 +975,7 @@ struct audit_context *audit_alloc_local(void)
>> > context->in_syscall = 1;
>> > return context;
>> > }
>> > +EXPORT_SYMBOL(audit_alloc_local);
>> >
>> > inline void audit_free_context(struct audit_context *context)
>> > {
>> > @@ -989,6 +990,7 @@ inline void audit_free_context(struct audit_context
>> > *context)
>> > audit_proctitle_free(context);
>> > kfree(context);
>> > }
>> > +EXPORT_SYMBOL(audit_free_context);
>> >
>> > static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>> > kuid_t auid, kuid_t uid, unsigned int
>> > sessionid,
>> > diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
>> > index c502419..edaa456 100644
>> > --- a/net/netfilter/xt_AUDIT.c
>> > +++ b/net/netfilter/xt_AUDIT.c
>> > @@ -71,10 +71,14 @@ static bool audit_ip6(struct audit_buffer *ab, struct
>> > sk_buff *skb)
>> > {
>> > struct audit_buffer *ab;
>> > int fam = -1;
>> > + struct audit_context *context = audit_alloc_local();
>> > + struct audit_containerid *cont;
>> > + int i = 0;
>> > + struct net *net;
>> >
>> > if (audit_enabled == 0)
>> > goto errout;
>>
>> Do I need to say it? I probably should ... the allocation should
>> happen after the audit_enabled check.
>
> Already fixed in V3 in my tree a couple of weeks ago...
... which you never posted, at least not anywhere I've seen. Which
effectively means I wasted a good chunk of time reviewing this code
late last night. Awesome.
> More timely review please?
More patience on your part?
>> > - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
>> > + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
>> > if (ab == NULL)
>> > goto errout;
>> >
>> > @@ -104,7 +108,16 @@ static bool audit_ip6(struct audit_buffer *ab, struct
>> > sk_buff *skb)
>> >
>> > audit_log_end(ab);
>> >
>> > + net = sock_net(NETLINK_CB(skb).sk);
>> > + list_for_each_entry(cont, &net->audit_containerid, list) {
>> > + char buf[14];
>> > +
>> > + sprintf(buf, "net%u", i++);
>> > + audit_log_container_info(context, buf, cont->id);
>> > + }
>>
>> It seems like this could (should?) be hidden inside an audit function,
>> e.g. audit_log_net_containers() or something like that.
>
> Perhaps... It was open-coded since at this point there are no other
> users. That'll make this tidier though.
If the code was all contained within a single subsystem them I would
generally agree that open coding is preferable, but since we are
crossing a subsystem boundary I think it would be preferable to
abstract away the details into a separate function.
This will probably also be necessary once you change to using the
audit_net/net_generic mechanism.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 10/13] audit: add containerid support for seccomp and anom_abend records
On Thu, Apr 19, 2018 at 8:42 PM, Richard Guy Briggs wrote:
> On 2018-04-18 21:31, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
>> > Add container ID auxiliary records to secure computing and abnormal end
>> > standalone records.
>> >
>> > Signed-off-by: Richard Guy Briggs
>> > ---
>> > kernel/auditsc.c | 10 --
>> > 1 file changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 7103d23..2f02ed9 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -2571,6 +2571,7 @@ static void audit_log_task(struct audit_buffer *ab)
>> > void audit_core_dumps(long signr)
>> > {
>> > struct audit_buffer *ab;
>> > + struct audit_context *context = audit_alloc_local();
>>
>> Looking quickly at do_coredump() I *believe* we can use current here.
>>
>> > if (!audit_enabled)
>> > return;
>> > @@ -2578,19 +2579,22 @@ void audit_core_dumps(long signr)
>> > if (signr == SIGQUIT) /* don't care for those */
>> > return;
>> >
>> > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
>> > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_ANOM_ABEND);
>> > if (unlikely(!ab))
>> > return;
>> > audit_log_task(ab);
>> > audit_log_format(ab, " sig=%ld res=1", signr);
>> > audit_log_end(ab);
>> > + audit_log_container_info(context, "abend",
>> > audit_get_containerid(current));
>> > + audit_free_context(context);
>> > }
>> >
>> > void __audit_seccomp(unsigned long syscall, long signr, int code)
>> > {
>> > struct audit_buffer *ab;
>> > + struct audit_context *context = audit_alloc_local();
>>
>> We can definitely use current here.
>
> Ok, so both syscall aux records. That elimintes this patch from the
> set, can go in independently.
Yep. It should help shrink the audit container ID patchset and
perhaps more importantly it should put some distance between the
connected-record debate and the audit container ID debate.
I understand we are going to need a "local" context for some things,
the network packets are probably the best example, but whenever
possible I would like to connect these records back to a task's
context.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 05/13] audit: add containerid support for ptrace and signals
On Thu, Apr 19, 2018 at 9:03 PM, Richard Guy Briggs wrote:
> On 2018-04-18 20:32, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
...
>> > /*
>> > * audit_log_container_info - report container info
>> > - * @tsk: task to be recorded
>> > * @context: task or local context for record
>> > + * @op: containerid string description
>> > + * @containerid: container ID to report
>> > */
>> > -int audit_log_container_info(struct task_struct *tsk, struct
>> > audit_context *context)
>> > +int audit_log_container_info(struct audit_context *context,
>> > + char *op, u64 containerid)
>> > {
>> > struct audit_buffer *ab;
>> >
>> > - if (!audit_containerid_set(tsk))
>> > + if (!cid_valid(containerid))
>> > return 0;
>> > /* Generate AUDIT_CONTAINER_INFO with container ID */
>> > ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER_INFO);
>> > if (!ab)
>> > return -ENOMEM;
>> > - audit_log_format(ab, "contid=%llu", audit_get_containerid(tsk));
>> > + audit_log_format(ab, "op=%s contid=%llu", op, containerid);
>> > audit_log_end(ab);
>> > return 0;
>> > }
>>
>> Let's get these changes into the first patch where
>> audit_log_container_info() is defined. Why? This inserts a new field
>> into the record which is a no-no. Yes, it is one single patchset, but
>> they are still separate patches and who knows which patches a given
>> distribution and/or tree may decide to backport.
>
> Fair enough. That first thought went through my mind... Would it be
> sufficient to move that field addition to the first patch and leave the
> rest here to support trace and signals?
I should have been more clear ... yes, that's what I was thinking; the
record format is the important part as it's user visible.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 06/13] audit: add support for non-syscall auxiliary records
On Thu, Apr 19, 2018 at 9:23 PM, Richard Guy Briggs wrote:
> On 2018-04-18 20:39, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
>> > Standalone audit records have the timestamp and serial number generated
>> > on the fly and as such are unique, making them standalone. This new
>> > function audit_alloc_local() generates a local audit context that will
>> > be used only for a standalone record and its auxiliary record(s). The
>> > context is discarded immediately after the local associated records are
>> > produced.
>> >
>> > Signed-off-by: Richard Guy Briggs
>> > ---
>> > include/linux/audit.h | 8
>> > kernel/auditsc.c | 20 +++-
>> > 2 files changed, 27 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/linux/audit.h b/include/linux/audit.h
>> > index ed16bb6..c0b83cb 100644
>> > --- a/include/linux/audit.h
>> > +++ b/include/linux/audit.h
>> > @@ -227,7 +227,9 @@ static inline int audit_log_container_info(struct
>> > audit_context *context,
>> > /* These are defined in auditsc.c */
>> > /* Public API */
>> > extern int audit_alloc(struct task_struct *task);
>> > +extern struct audit_context *audit_alloc_local(void);
>> > extern void __audit_free(struct task_struct *task);
>> > +extern void audit_free_context(struct audit_context *context);
>> > extern void __audit_syscall_entry(int major, unsigned long a0, unsigned
>> > long a1,
>> > unsigned long a2, unsigned long a3);
>> > extern void __audit_syscall_exit(int ret_success, long ret_value);
>> > @@ -472,6 +474,12 @@ static inline int audit_alloc(struct task_struct
>> > *task)
>> > {
>> > return 0;
>> > }
>> > +static inline struct audit_context *audit_alloc_local(void)
>> > +{
>> > + return NULL;
>> > +}
>> > +static inline void audit_free_context(struct audit_context *context)
>> > +{ }
>> > static inline void audit_free(struct task_struct *task)
>> > { }
>> > static inline void audit_syscall_entry(int major, unsigned long a0,
>> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> > index 2932ef1..7103d23 100644
>> > --- a/kernel/auditsc.c
>> > +++ b/kernel/auditsc.c
>> > @@ -959,8 +959,26 @@ int audit_alloc(struct task_struct *tsk)
>> > return 0;
>> > }
>> >
>> > -static inline void audit_free_context(struct audit_context *context)
>> > +struct audit_context *audit_alloc_local(void)
>> > {
>> > + struct audit_context *context;
>> > +
>> > + if (!audit_ever_enabled)
>> > + return NULL; /* Return if not auditing. */
>> > +
>> > + context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
>> > + if (!context)
>> > + return NULL;
>> > + context->serial = audit_serial();
>> > + context->ctime = current_kernel_time64();
>> > + context->in_syscall = 1;
>> > + return context;
>> > +}
>> > +
>> > +inline void audit_free_context(struct audit_context *context)
>> > +{
>> > + if (!context)
>> > + return;
>> > audit_free_names(context);
>> > unroll_tree_refs(context, NULL, 0);
>> > free_tree_refs(context);
>>
>> I'm reserving the option to comment on this idea further as I make my
>> way through the patchset, but audit_free_context() definitely
>> shouldn't be declared as an inline function.
>
> Ok, I think I follow. When it wasn't exported, inline was fine, but now
> that it has been exported, it should no longer be inlined ...
Pretty much. Based on a few comments I've seen by compiler folks over
the years, my current thinking is that we shouldn't worry about
explicit inlining static functions in C files (header files are a
different story). The basic idea being that the compiler almost
always does a better job than us stupid developers.
> ... or should use
> an intermediate function name to export so that local uses of it can
> remain inline.
Possibly, but my guess is that the compiler could (will?) do that by
itself for code that lives in the same file.
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces
On Fri, Apr 20, 2018 at 4:02 PM, Richard Guy Briggs wrote: > On 2018-04-18 21:46, Paul Moore wrote: >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote: >> > Audit events could happen in a network namespace outside of a task >> > context due to packets received from the net that trigger an auditing >> > rule prior to being associated with a running task. The network >> > namespace could in use by multiple containers by association to the >> > tasks in that network namespace. We still want a way to attribute >> > these events to any potential containers. Keep a list per network >> > namespace to track these container identifiiers. >> > >> > Add/increment the container identifier on: >> > - initial setting of the container id via /proc >> > - clone/fork call that inherits a container identifier >> > - unshare call that inherits a container identifier >> > - setns call that inherits a container identifier >> > Delete/decrement the container identifier on: >> > - an inherited container id dropped when child set >> > - process exit >> > - unshare call that drops a net namespace >> > - setns call that drops a net namespace >> > >> > See: https://github.com/linux-audit/audit-kernel/issues/32 >> > See: https://github.com/linux-audit/audit-testsuite/issues/64 >> > Signed-off-by: Richard Guy Briggs >> > --- >> > include/linux/audit.h | 7 +++ >> > include/net/net_namespace.h | 12 >> > kernel/auditsc.c| 9 ++--- >> > kernel/nsproxy.c| 6 ++ >> > net/core/net_namespace.c| 45 >> > + >> > 5 files changed, 76 insertions(+), 3 deletions(-) ... >> > diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c >> > index f6c5d33..d9f1090 100644 >> > --- a/kernel/nsproxy.c >> > +++ b/kernel/nsproxy.c >> > @@ -140,6 +140,7 @@ int copy_namespaces(unsigned long flags, struct >> > task_struct *tsk) >> > struct nsproxy *old_ns = tsk->nsproxy; >> > struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns); >> > struct nsproxy *new_ns; >> > + u64 containerid = audit_get_containerid(tsk); >> > >> > if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC | >> > CLONE_NEWPID | CLONE_NEWNET | >> > @@ -167,6 +168,7 @@ int copy_namespaces(unsigned long flags, struct >> > task_struct *tsk) >> > return PTR_ERR(new_ns); >> > >> > tsk->nsproxy = new_ns; >> > + net_add_audit_containerid(new_ns->net_ns, containerid); >> > return 0; >> > } >> >> Hopefully we can handle this in audit_net_init(), we just need to >> figure out where we can get the correct task_struct for the audit >> container ID (some backpointer in the net struct?). > > I don't follow. This needs to happen on every task startup. > audit_net_init() is only called when a new network namespace starts up. Yep, sorry, my mistake. I must have confused myself when I was looking at the code. I'm thinking out loud here, bear with me ... Assuming we move the netns/audit-container-ID tracking to audit_net, and considering we already have an audit hook in copy_process() (it calls audit_alloc()), would this be better handled by the copy_process() hook? This ignores naming, audit_alloc() reuse, etc.; those can be easily fixed. I'm just thinking of ways to limit our impact on the core kernel and leverage our existing interaction points. -- paul moore www.paul-moore.com
Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces
On April 20, 2018 4:48:34 PM Richard Guy Briggs wrote: On 2018-04-20 16:22, Paul Moore wrote: On Fri, Apr 20, 2018 at 4:02 PM, Richard Guy Briggs wrote: On 2018-04-18 21:46, Paul Moore wrote: On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote: Audit events could happen in a network namespace outside of a task context due to packets received from the net that trigger an auditing rule prior to being associated with a running task. The network namespace could in use by multiple containers by association to the tasks in that network namespace. We still want a way to attribute these events to any potential containers. Keep a list per network namespace to track these container identifiiers. Add/increment the container identifier on: - initial setting of the container id via /proc - clone/fork call that inherits a container identifier - unshare call that inherits a container identifier - setns call that inherits a container identifier Delete/decrement the container identifier on: - an inherited container id dropped when child set - process exit - unshare call that drops a net namespace - setns call that drops a net namespace See: https://github.com/linux-audit/audit-kernel/issues/32 See: https://github.com/linux-audit/audit-testsuite/issues/64 Signed-off-by: Richard Guy Briggs --- include/linux/audit.h | 7 +++ include/net/net_namespace.h | 12 kernel/auditsc.c| 9 ++--- kernel/nsproxy.c| 6 ++ net/core/net_namespace.c| 45 + 5 files changed, 76 insertions(+), 3 deletions(-) ... diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c index f6c5d33..d9f1090 100644 --- a/kernel/nsproxy.c +++ b/kernel/nsproxy.c @@ -140,6 +140,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk) struct nsproxy *old_ns = tsk->nsproxy; struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns); struct nsproxy *new_ns; + u64 containerid = audit_get_containerid(tsk); if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC | CLONE_NEWPID | CLONE_NEWNET | @@ -167,6 +168,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk) return PTR_ERR(new_ns); tsk->nsproxy = new_ns; + net_add_audit_containerid(new_ns->net_ns, containerid); return 0; } Hopefully we can handle this in audit_net_init(), we just need to figure out where we can get the correct task_struct for the audit container ID (some backpointer in the net struct?). I don't follow. This needs to happen on every task startup. audit_net_init() is only called when a new network namespace starts up. Yep, sorry, my mistake. I must have confused myself when I was looking at the code. I'm thinking out loud here, bear with me ... Assuming we move the netns/audit-container-ID tracking to audit_net, and considering we already have an audit hook in copy_process() (it calls audit_alloc()), would this be better handled by the copy_process() hook? This ignores naming, audit_alloc() reuse, etc.; those can be easily fixed. I'm just thinking of ways to limit our impact on the core kernel and leverage our existing interaction points. The new namespace hasn't been cloned yet and this is the only function where we have access to both namespaces, so I don't see how that could work... I'll take another, closer look, with v3. paul moore - RGB -- Richard Guy Briggs Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635 -- paul moore www.paul-moore.com
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs wrote:
> On 2018-04-18 19:47, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
>> > Implement the proc fs write to set the audit container ID of a process,
>> > emitting an AUDIT_CONTAINER record to document the event.
>> >
>> > This is a write from the container orchestrator task to a proc entry of
>> > the form /proc/PID/containerid where PID is the process ID of the newly
>> > created task that is to become the first task in a container, or an
>> > additional task added to a container.
>> >
>> > The write expects up to a u64 value (unset: 18446744073709551615).
>> >
>> > This will produce a record such as this:
>> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
>> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0
>> > ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
>> >
>> > The "op" field indicates an initial set. The "pid" to "ses" fields are
>> > the orchestrator while the "opid" field is the object's PID, the process
>> > being "contained". Old and new container ID values are given in the
>> > "contid" fields, while res indicates its success.
>> >
>> > It is not permitted to self-set, unset or re-set the container ID. A
>> > child inherits its parent's container ID, but then can be set only once
>> > after.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >
>> > Signed-off-by: Richard Guy Briggs
>> > ---
>> > fs/proc/base.c | 37
>> > include/linux/audit.h | 16 +
>> > include/linux/init_task.h | 4 ++-
>> > include/linux/sched.h | 1 +
>> > include/uapi/linux/audit.h | 2 ++
>> > kernel/auditsc.c | 84
>> > ++
>> > 6 files changed, 143 insertions(+), 1 deletion(-)
...
>> > diff --git a/include/linux/sched.h b/include/linux/sched.h
>> > index d258826..1b82191 100644
>> > --- a/include/linux/sched.h
>> > +++ b/include/linux/sched.h
>> > @@ -796,6 +796,7 @@ struct task_struct {
>> > #ifdef CONFIG_AUDITSYSCALL
>> > kuid_t loginuid;
>> > unsigned intsessionid;
>> > + u64 containerid;
>>
>> This one line addition to the task_struct scares me the most of
>> anything in this patchset. Why? It's a field named "containerid" in
>> a perhaps one of the most widely used core kernel structures; the
>> possibilities for abuse are endless, and it's foolish to think we
>> would ever be able to adequately police this.
>
> Fair enough.
>
>> Unfortunately, we can't add the field to audit_context as things
>> currently stand because we don't always allocate an audit_context,
>> it's dependent on the system's configuration, and we need to track the
>> audit container ID for a given process, regardless of the audit
>> configuration. Pretty much the same reason why loginuid and sessionid
>> are located directly in task_struct now. As I stressed during the
>> design phase, I really want to keep this as an *audit* container ID
>> and not a general purpose kernel wide container ID. If the kernel
>> ever grows a general purpose container ID token, I'll be the first in
>> line to convert the audit code, but I don't want audit to be that
>> general purpose mechanism ... audit is hated enough as-is ;)
>
> When would we need an audit container ID when audit is not enabled
> enough to have an audit_context?
I'm thinking of the audit_alloc() case where audit_filter_task()
returns AUDIT_DISABLED.
I believe this is the same reason why loginuid and sessionid live
directly in the task_struct and not in the audit_context; they need to
persist for the lifetime of the task.
> If it is only used for audit, and audit is the only consumer, and audit
> can only use it when it is enabled, then we can just return success to
> any write to the proc filehandle, or not even present it. Nothing will
> be able to know that value wasn't used.
>
> When are loginuid and sessionid used now when audit is not enabled (or
> should I say, explicitly disabled)?
See above. I think that should answer these questions.
>> > diff --git a/include/uapi/linux/audit.h b/include/ua
Re: [PATCH 2/3] net/unix: hook unix_socketpair() into LSM
On Mon, Apr 23, 2018 at 9:30 AM, David Herrmann wrote:
> Use the newly created LSM-hook for unix_socketpair(). The default hook
> return-value is 0, so behavior stays the same unless LSMs start using
> this hook.
>
> Signed-off-by: David Herrmann
> ---
> net/unix/af_unix.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 68bb70a62afe..bc9705ace9b1 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1371,6 +1371,11 @@ static int unix_stream_connect(struct socket *sock,
> struct sockaddr *uaddr,
> static int unix_socketpair(struct socket *socka, struct socket *sockb)
> {
> struct sock *ska = socka->sk, *skb = sockb->sk;
> + int err;
> +
> + err = security_unix_stream_socketpair(ska, skb);
> + if (err)
> + return err;
I recognize that AF_UNIX is really the only protocol that supports
socketpair(2) at the moment, but I like to avoid protocol specific LSM
hooks whenever possible. Unless someone can think of a good
objection, I would prefer to see the hook placed in __sys_socketpair()
instead (and obviously drop the "unix_stream" portion from the hook
name).
> /* Join our sockets back to back */
> sock_hold(ska);
> --
> 2.17.0
--
paul moore
www.paul-moore.com
Re: [PATCH 2/3] net/unix: hook unix_socketpair() into LSM
On Tue, Apr 24, 2018 at 1:56 PM, David Miller wrote:
> From: Paul Moore
> Date: Tue, 24 Apr 2018 13:55:31 -0400
>
>> On Mon, Apr 23, 2018 at 9:30 AM, David Herrmann
>> wrote:
>>> Use the newly created LSM-hook for unix_socketpair(). The default hook
>>> return-value is 0, so behavior stays the same unless LSMs start using
>>> this hook.
>>>
>>> Signed-off-by: David Herrmann
>>> ---
>>> net/unix/af_unix.c | 5 +
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>>> index 68bb70a62afe..bc9705ace9b1 100644
>>> --- a/net/unix/af_unix.c
>>> +++ b/net/unix/af_unix.c
>>> @@ -1371,6 +1371,11 @@ static int unix_stream_connect(struct socket *sock,
>>> struct sockaddr *uaddr,
>>> static int unix_socketpair(struct socket *socka, struct socket *sockb)
>>> {
>>> struct sock *ska = socka->sk, *skb = sockb->sk;
>>> + int err;
>>> +
>>> + err = security_unix_stream_socketpair(ska, skb);
>>> + if (err)
>>> + return err;
>>
>> I recognize that AF_UNIX is really the only protocol that supports
>> socketpair(2) at the moment, but I like to avoid protocol specific LSM
>> hooks whenever possible. Unless someone can think of a good
>> objection, I would prefer to see the hook placed in __sys_socketpair()
>> instead (and obviously drop the "unix_stream" portion from the hook
>> name).
>
> The counterargument is that after 30 years no other protocol has grown
> usage of this operation. :-)
Call me a an optimist ;)
--
paul moore
www.paul-moore.com
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Mon, Apr 23, 2018 at 10:02 PM, Richard Guy Briggs wrote:
> On 2018-04-23 19:15, Paul Moore wrote:
>> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs wrote:
>> > On 2018-04-18 19:47, Paul Moore wrote:
>> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs
>> >> wrote:
>> >> > Implement the proc fs write to set the audit container ID of a process,
>> >> > emitting an AUDIT_CONTAINER record to document the event.
>> >> >
>> >> > This is a write from the container orchestrator task to a proc entry of
>> >> > the form /proc/PID/containerid where PID is the process ID of the newly
>> >> > created task that is to become the first task in a container, or an
>> >> > additional task added to a container.
>> >> >
>> >> > The write expects up to a u64 value (unset: 18446744073709551615).
>> >> >
>> >> > This will produce a record such as this:
>> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
>> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
>> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
>> >> > res=0
>> >> >
>> >> > The "op" field indicates an initial set. The "pid" to "ses" fields are
>> >> > the orchestrator while the "opid" field is the object's PID, the process
>> >> > being "contained". Old and new container ID values are given in the
>> >> > "contid" fields, while res indicates its success.
>> >> >
>> >> > It is not permitted to self-set, unset or re-set the container ID. A
>> >> > child inherits its parent's container ID, but then can be set only once
>> >> > after.
>> >> >
>> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >> >
>> >> > Signed-off-by: Richard Guy Briggs
>> >> > ---
>> >> > fs/proc/base.c | 37
>> >> > include/linux/audit.h | 16 +
>> >> > include/linux/init_task.h | 4 ++-
>> >> > include/linux/sched.h | 1 +
>> >> > include/uapi/linux/audit.h | 2 ++
>> >> > kernel/auditsc.c | 84
>> >> > ++
>> >> > 6 files changed, 143 insertions(+), 1 deletion(-)
...
>> >> > /* audit_rule_data supports filter rules with both integer and string
>> >> > * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> > index 4e0a4ac..29c8482 100644
>> >> > --- a/kernel/auditsc.c
>> >> > +++ b/kernel/auditsc.c
>> >> > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
>> >> > return rc;
>> >> > }
>> >> >
>> >> > +static int audit_set_containerid_perm(struct task_struct *task, u64
>> >> > containerid)
>> >> > +{
>> >> > + struct task_struct *parent;
>> >> > + u64 pcontainerid, ccontainerid;
>> >> > +
>> >> > + /* Don't allow to set our own containerid */
>> >> > + if (current == task)
>> >> > + return -EPERM;
>> >>
>> >> Why not? Is there some obvious security concern that I missing?
>> >
>> > We then lose the distinction in the AUDIT_CONTAINER record between the
>> > initiating PID and the target PID. This was outlined in the proposal.
>>
>> I just went back and reread the v3 proposal and I still don't see a
>> good explanation of this. Why is this bad? What's the security
>> concern?
>
> I don't remember, specifically. Maybe this has been addressed by the
> check for children/threads or identical parent container ID. So, I'm
> reluctantly willing to remove that check for now.
Okay. For the record, if someone can explain to me why this
restriction saves us from some terrible situation I'm all for leaving
it. I'm just opposed to restrictions without solid reasoning behind
them.
>> > Having said that, I'm still not sure we have protected sufficiently from
>> > a child turning around and setting it's parent's as yet unset or
>> > inherited audit containe
Re: [PATCH 0/3] Introduce LSM-hook for socketpair(2)
On Wed, Apr 25, 2018 at 2:44 PM, James Morris wrote: > On Mon, 23 Apr 2018, David Herrmann wrote: >> This patch series tries to close this gap and makes both behave the >> same. A new LSM-hook is added which allows LSMs to cache the correct >> peer information on newly created socket-pairs. > > Looks okay to me. > > Once it's respun with the Smack backend and maybe the hook name change, > I'll merge this unless DaveM wants it to go in via his networking tree. Note my objection to the hook placement in patch 2/3; I think we should move the hook out of the AF_UNIX layer and up into the socket layer. -- paul moore www.paul-moore.com
Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On Tue, Apr 24, 2018 at 8:40 PM, Richard Guy Briggs wrote:
> On 2018-04-24 15:01, Paul Moore wrote:
>> On Mon, Apr 23, 2018 at 10:02 PM, Richard Guy Briggs wrote:
>> > On 2018-04-23 19:15, Paul Moore wrote:
>> >> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs
>> >> wrote:
>> >> > On 2018-04-18 19:47, Paul Moore wrote:
>> >> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs
>> >> >> wrote:
>> >> >> > Implement the proc fs write to set the audit container ID of a
>> >> >> > process,
>> >> >> > emitting an AUDIT_CONTAINER record to document the event.
>> >> >> >
>> >> >> > This is a write from the container orchestrator task to a proc entry
>> >> >> > of
>> >> >> > the form /proc/PID/containerid where PID is the process ID of the
>> >> >> > newly
>> >> >> > created task that is to become the first task in a container, or an
>> >> >> > additional task added to a container.
>> >> >> >
>> >> >> > The write expects up to a u64 value (unset: 18446744073709551615).
>> >> >> >
>> >> >> > This will produce a record such as this:
>> >> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
>> >> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
>> >> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615
>> >> >> > contid=123455 res=0
>> >> >> >
>> >> >> > The "op" field indicates an initial set. The "pid" to "ses" fields
>> >> >> > are
>> >> >> > the orchestrator while the "opid" field is the object's PID, the
>> >> >> > process
>> >> >> > being "contained". Old and new container ID values are given in the
>> >> >> > "contid" fields, while res indicates its success.
>> >> >> >
>> >> >> > It is not permitted to self-set, unset or re-set the container ID. A
>> >> >> > child inherits its parent's container ID, but then can be set only
>> >> >> > once
>> >> >> > after.
>> >> >> >
>> >> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >> >> >
>> >> >> > Signed-off-by: Richard Guy Briggs
>> >> >> > ---
>> >> >> > fs/proc/base.c | 37
>> >> >> > include/linux/audit.h | 16 +
>> >> >> > include/linux/init_task.h | 4 ++-
>> >> >> > include/linux/sched.h | 1 +
>> >> >> > include/uapi/linux/audit.h | 2 ++
>> >> >> > kernel/auditsc.c | 84
>> >> >> > ++
>> >> >> > 6 files changed, 143 insertions(+), 1 deletion(-)
>>
>> ...
>>
>> >> >> > /* audit_rule_data supports filter rules with both integer and
>> >> >> > string
>> >> >> > * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> >> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> >> > index 4e0a4ac..29c8482 100644
>> >> >> > --- a/kernel/auditsc.c
>> >> >> > +++ b/kernel/auditsc.c
>> >> >> > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
>> >> >> > return rc;
>> >> >> > }
>> >> >> >
>> >> >> > +static int audit_set_containerid_perm(struct task_struct *task, u64
>> >> >> > containerid)
>> >> >> > +{
>> >> >> > + struct task_struct *parent;
>> >> >> > + u64 pcontainerid, ccontainerid;
>> >> >> > +
>> >> >> > + /* Don't allow to set our own containerid */
>> >> >> > + if (current == task)
>> >> >> > + return -EPERM;
>> >> >>
>> >> >> Why not? Is there some obvious security concern that I missing?
>> >> >
>> >> > We then lose the dist
New skb extension for use by LSMs (skb "security blob")?
Hello netdev, I was just made aware of the skb extension work, and it looks very appealing from a LSM perspective. As some of you probably remember, we (the LSM folks) have wanted a proper security blob in the skb for quite some time, but netdev has been resistant to this idea thus far. If I were to propose a patchset to add a SKB_EXT_SECURITY skb extension (a single extension ID to be shared among the different LSMs), would that be something that netdev would consider merging, or is there still a philosophical objection to things like this? -- paul moore www.paul-moore.com
Re: New skb extension for use by LSMs (skb "security blob")?
On Wed, Aug 21, 2019 at 6:50 PM David Miller wrote: > From: Paul Moore > Date: Wed, 21 Aug 2019 18:00:09 -0400 > > > I was just made aware of the skb extension work, and it looks very > > appealing from a LSM perspective. As some of you probably remember, > > we (the LSM folks) have wanted a proper security blob in the skb for > > quite some time, but netdev has been resistant to this idea thus far. > > > > If I were to propose a patchset to add a SKB_EXT_SECURITY skb > > extension (a single extension ID to be shared among the different > > LSMs), would that be something that netdev would consider merging, or > > is there still a philosophical objection to things like this? > > Unlike it's main intended user (MPTCP), it sounds like LSM's would use > this in a way such that it would be enabled on most systems all the > time. > > That really defeats the whole purpose of making it dynamic. :-/ I would be okay with only adding a skb extension when we needed it, which I'm currently thinking would only be when we had labeled networking actually configured at runtime and not just built into the kernel. In SELinux we do something similar today when it comes to our per-packet access controls; if labeled networking is not configured we bail out of the LSM hooks early to improve performance (we would just be comparing unlabeled_t to unlabeled_t anyway). I think the other LSMs would be okay with this usage as well. While a number of distros due enable some form of LSM and the labeled networking bits at build time, vary few (if any?) provide a default configuration so I would expect no additional overhead in the common case. Would that be acceptable? -- paul moore www.paul-moore.com
Re: New skb extension for use by LSMs (skb "security blob")?
On Thu, Aug 22, 2019 at 3:03 AM Florian Westphal wrote: > Paul Moore wrote: > > Hello netdev, > > > > I was just made aware of the skb extension work, and it looks very > > appealing from a LSM perspective. As some of you probably remember, > > we (the LSM folks) have wanted a proper security blob in the skb for > > quite some time, but netdev has been resistant to this idea thus far. > > Is that "blob" in addition to skb->secmark, or a replacement? That's a good question. While I thought about that, I wasn't sure if that was worth bringing up as previous attempts to trade the secmark field for a void pointer met with failure. Last time I played with it I was able to take the additional 32-bits from holes in the skb, and possibly even improve some of the cacheline groupings (but that is always going to be a dependent on use case I think), but that wasn't enough. I think we could consider freeing up the secmark in the main skb, and move it to a skb extension, but this would potentially increase the chances that we would need to add an extension to a skb. I don't have any hard numbers, but based on discussions and questions I suspect Secmark is more widely used than NetLabel and/or labeled IPsec; although I'm confident it is still a minor percentage of the overall Linux installed base. For me the big question is what would it take for us to get a security blob associated with the skb? Would moving the secmark into the skb extension be enough? Something else? Or is this simply never going to happen? I want to remain optimistic, but I've been trying for this off-and-on for over a decade and keep running into a brick wall ;) -- paul moore www.paul-moore.com
Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
On Fri, Aug 23, 2019 at 7:41 AM Jeffrey Vander Stoep wrote: > On Fri, Aug 23, 2019 at 1:19 AM David Miller wrote: > > From: Jeff Vander Stoep > > Date: Wed, 21 Aug 2019 15:45:47 +0200 > > > > > MAC addresses are often considered sensitive because they are > > > usually unique and can be used to identify/track a device or > > > user [1]. > > > > > > The MAC address is accessible via the RTM_NEWLINK message type of a > > > netlink route socket[2]. Ideally we could grant/deny access to the > > > MAC address on a case-by-case basis without blocking the entire > > > RTM_NEWLINK message type which contains a lot of other useful > > > information. This can be achieved using a new LSM hook on the netlink > > > message receive path. Using this new hook, individual LSMs can select > > > which processes are allowed access to the real MAC, otherwise a > > > default value of zeros is returned. Offloading access control > > > decisions like this to an LSM is convenient because it preserves the > > > status quo for most Linux users while giving the various LSMs > > > flexibility to make finer grained decisions on access to sensitive > > > data based on policy. > > > > > > [1] https://adamdrake.com/mac-addresses-udids-and-privacy.html > > > [2] Other access vectors like ioctl(SIOCGIFHWADDR) are already covered > > > by existing LSM hooks. > > > > > > Signed-off-by: Jeff Vander Stoep > > > > I'm sure the MAC address will escape into userspace via other means, > > dumping pieces of networking config in other contexts, etc. I mean, > > if I can get a link dump, I can dump the neighbor table as well. > > These are already gated by existing LSM hooks and capability checks. > They are not allowed on mandatory access control systems unless explicitly > granted. > > > I kinda think this is all very silly whack-a-mole kind of stuff, to > > be quite honest. > > We evaluated mechanisms for the MAC to reach unprivileged apps. > A number of researchers have published on this as well such as: > https://www.usenix.org/conference/usenixsecurity19/presentation/reardon > > Three "leaks" were identified, two have already been fixed. > -ioctl(SIOCGIFHWADDR). Fixed using finer grained LSM checks > on socket ioctls (similar to this change). > -IPv6 IP addresses. Fixed by no longer including the MAC as part > of the IP address. > -RTM_NEWLINK netlink route messages. The last mole to be whacked. > > > And like others have said, tomorrow you'll be like "oh crap, we should > > block X too" and we'll get another hook, another config knob, another > > rulset update, etc. > > This seems like an issue inherent with permissions/capabilities. I don’t > think we should abandon the concept of permissions because someone > can forget to add a check. Likewise, if someone adds new code to the > kernel and omits a capable(CAP_NET_*) check, I would expect it to be > fixed like any other bug without the idea of capability checks being tossed > out. > > We need to do something because this information is being abused. Any > recommendations? This seemed like the simplest approach, but I can > definitely appreciate that it has downsides. > > I could make this really generic by adding a single hook to the end of > sock_msgrecv() which would allow an LSM to modify the message to omit > the MAC address and any other information that we deem as sensitive in the > future. Basically what Casey was suggesting. Thoughts on that approach? I apologize for the delay in responding; I'm blaming LSS-NA travel. I'm also not a big fan of inserting the hook in rtnl_fill_ifinfo(); as presented it is way too specific for a LSM hook for me to be happy. However, I do agree that giving the LSMs some control over netlink messages makes sense. As others have pointed out, it's all a matter of where to place the hook. If we only care about netlink messages which leverage nlattrs I suppose one option that I haven't seen mentioned would be to place a hook in nla_put(). While it is a bit of an odd place for a hook, it would allow the LSM easy access to the skb and attribute type to make decisions, and all of the callers should already be checking the return code (although we would need to verify this). One notable drawback (not the only one) is that the hook is going to get hit multiple times for each message. -- paul moore www.paul-moore.com
Re: [PATCH 1/2] rtnetlink: gate MAC address with an LSM hook
On Thu, Aug 29, 2019 at 3:45 AM Michal Kubecek wrote: > On Tue, Aug 27, 2019 at 04:47:04PM -0400, Paul Moore wrote: > > > > I'm also not a big fan of inserting the hook in rtnl_fill_ifinfo(); as > > presented it is way too specific for a LSM hook for me to be happy. > > However, I do agree that giving the LSMs some control over netlink > > messages makes sense. As others have pointed out, it's all a matter > > of where to place the hook. > > > > If we only care about netlink messages which leverage nlattrs I > > suppose one option that I haven't seen mentioned would be to place a > > hook in nla_put(). While it is a bit of an odd place for a hook, it > > would allow the LSM easy access to the skb and attribute type to make > > decisions, and all of the callers should already be checking the > > return code (although we would need to verify this). One notable > > drawback (not the only one) is that the hook is going to get hit > > multiple times for each message. > > For most messages, "multiple times" would mean tens, for many even > hundreds of calls. For each, you would have to check corresponding > socket (and possibly also genetlink header) to see which netlink based > protocol it is and often even parse existing part of the message to get > the context (because the same numeric attribute type can mean something > completely different if it appears in a nested attribute). > > Also, nla_put() (or rather __nla_put()) is not used for all attributes, > one may also use nla_reserve() and then compose the attribute date in > place. I never said it was a great idea, just an idea ;) Honestly I'm just trying to spur some discussion on this so we can hopefully arrive at a solution which allows a LSM to control kernel generated netlink messages that we can all accept. -- paul moore www.paul-moore.com
Re: [PATCH v2 2/2] NETWORKING: avoid use IPCB in cipso_v4_error
On Mon, Feb 25, 2019 at 11:27 AM Nazarov Sergey wrote:
>
> Extract IP options in cipso_v4_error and use __icmp_send.
>
> Signed-off-by: Sergey Nazarov
> ---
> include/net/ip.h |2 ++
> net/ipv4/cipso_ipv4.c | 17 +++--
> net/ipv4/ip_options.c | 22 +-
> 3 files changed, 34 insertions(+), 7 deletions(-)
Thanks Sergey.
Acked-by: Paul Moore
> diff --git a/include/net/ip.h b/include/net/ip.h
> index 8866bfc..f0e8d06 100644
> --- a/include/net/ip.h
> +++ b/include/net/ip.h
> @@ -667,6 +667,8 @@ static inline int ip_options_echo(struct net *net, struct
> ip_options *dopt,
> }
>
> void ip_options_fragment(struct sk_buff *skb);
> +int __ip_options_compile(struct net *net, struct ip_options *opt,
> +struct sk_buff *skb, __be32 *info);
> int ip_options_compile(struct net *net, struct ip_options *opt,
>struct sk_buff *skb);
> int ip_options_get(struct net *net, struct ip_options_rcu **optp,
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 777fa3b..eff86a7 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1735,13 +1735,26 @@ int cipso_v4_validate(const struct sk_buff *skb,
> unsigned char **option)
> */
> void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
> {
> + unsigned char optbuf[sizeof(struct ip_options) + 40];
> + struct ip_options *opt = (struct ip_options *)optbuf;
> +
> if (ip_hdr(skb)->protocol == IPPROTO_ICMP || error != -EACCES)
> return;
>
> + /*
> +* We might be called above the IP layer,
> +* so we can not use icmp_send and IPCB here.
> +*/
> +
> + memset(opt, 0, sizeof(struct ip_options));
> + opt->optlen = ip_hdr(skb)->ihl*4 - sizeof(struct iphdr);
> + if (__ip_options_compile(dev_net(skb->dev), opt, skb, NULL))
> + return;
> +
> if (gateway)
> - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0);
> + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0, opt);
> else
> - icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0);
> + __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0, opt);
> }
>
> /**
> diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
> index ed194d4..32a3504 100644
> --- a/net/ipv4/ip_options.c
> +++ b/net/ipv4/ip_options.c
> @@ -251,8 +251,9 @@ static void spec_dst_fill(__be32 *spec_dst, struct
> sk_buff *skb)
> * If opt == NULL, then skb->data should point to IP header.
> */
>
> -int ip_options_compile(struct net *net,
> - struct ip_options *opt, struct sk_buff *skb)
> +int __ip_options_compile(struct net *net,
> +struct ip_options *opt, struct sk_buff *skb,
> +__be32 *info)
> {
> __be32 spec_dst = htonl(INADDR_ANY);
> unsigned char *pp_ptr = NULL;
> @@ -468,11 +469,22 @@ int ip_options_compile(struct net *net,
> return 0;
>
> error:
> - if (skb) {
> - icmp_send(skb, ICMP_PARAMETERPROB, 0,
> htonl((pp_ptr-iph)<<24));
> - }
> + if (info)
> + *info = htonl((pp_ptr-iph)<<24);
> return -EINVAL;
> }
> +
> +int ip_options_compile(struct net *net,
> + struct ip_options *opt, struct sk_buff *skb)
> +{
> + int ret;
> + __be32 info;
> +
> + ret = __ip_options_compile(net, opt, skb, &info);
> + if (ret != 0 && skb)
> + icmp_send(skb, ICMP_PARAMETERPROB, 0, info);
> + return ret;
> +}
> EXPORT_SYMBOL(ip_options_compile);
>
> /*
> ---
>
--
paul moore
www.paul-moore.com
Re: [PATCH v2 1/2] NETWORKING: avoid use IPCB in cipso_v4_error
On Mon, Feb 25, 2019 at 11:24 AM Nazarov Sergey wrote:
>
> Add __icmp_send function having ip_options struct parameter
>
> Signed-off-by: Sergey Nazarov
> ---
> include/net/icmp.h|9 -
> net/ipv4/icmp.c |7 ---
> 2 files changed, 12 insertions(+), 4 deletions(-)
Reviewed-by: Paul Moore
> diff --git a/include/net/icmp.h b/include/net/icmp.h
> index 6ac3a5b..e0f709d 100644
> --- a/include/net/icmp.h
> +++ b/include/net/icmp.h
> @@ -22,6 +22,7 @@
>
> #include
> #include
> +#include
>
> struct icmp_err {
>int errno;
> @@ -39,7 +40,13 @@ struct icmp_err {
> struct sk_buff;
> struct net;
>
> -void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info);
> +void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
> +const struct ip_options *opt);
> +static inline void icmp_send(struct sk_buff *skb_in, int type, int code,
> __be32 info)
> +{
> + __icmp_send(skb_in, type, code, info, &IPCB(skb_in)->opt);
> +}
> +
> int icmp_rcv(struct sk_buff *skb);
> int icmp_err(struct sk_buff *skb, u32 info);
> int icmp_init(void);
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 065997f..3f24414 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -570,7 +570,8 @@ static void icmp_reply(struct icmp_bxm *icmp_param,
> struct sk_buff *skb)
> * MUST reply to only the first fragment.
> */
>
> -void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
> +void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
> +const struct ip_options *opt)
> {
> struct iphdr *iph;
> int room;
> @@ -691,7 +692,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int
> code, __be32 info)
> iph->tos;
> mark = IP4_REPLY_MARK(net, skb_in->mark);
>
> - if (ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in))
> + if (__ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in,
> opt))
> goto out_unlock;
>
>
> @@ -742,7 +743,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int
> code, __be32 info)
> local_bh_enable();
> out:;
> }
> -EXPORT_SYMBOL(icmp_send);
> +EXPORT_SYMBOL(__icmp_send);
>
>
> static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
> ---
>
--
paul moore
www.paul-moore.com
Re: [PATCH v2 1/2] NETWORKING: avoid use IPCB in cipso_v4_error
On Mon, Feb 25, 2019 at 5:33 PM David Miller wrote: > From: Nazarov Sergey > Date: Mon, 25 Feb 2019 19:24:15 +0300 > > > Add __icmp_send function having ip_options struct parameter > > > > Signed-off-by: Sergey Nazarov > > Applied with Subject line fixes up. This commit doesn't even make > changes to cipse_v4_error(). > > Anyone who ACK'd this change or added their Reviewed-by did not read > this email and are just rubber stamping crap. I should have checked the subject line closer that's my fault (Gmail does interesting things to threads sometimes, and obscured the subject line), however, I did look at the content of patch before giving it my thumbs up. Claiming the email wasn't read isn't correct (although you could rightly argue I didn't read the subject line), or I'm "rubber stamping crap" isn't correct. -- paul moore www.paul-moore.com
[PATCH] netlabel: fix out-of-bounds memory accesses
There are two array out-of-bounds memory accesses, one in
cipso_v4_map_lvl_valid(), the other in netlbl_bitmap_walk(). Both
errors are embarassingly simple, and the fixes are straightforward.
As a FYI for anyone backporting this patch to kernels prior to v4.8,
you'll want to apply the netlbl_bitmap_walk() patch to
cipso_v4_bitmap_walk() as netlbl_bitmap_walk() doesn't exist before
Linux v4.8.
Reported-by: Jann Horn
Fixes: 446fda4f2682 ("[NetLabel]: CIPSOv4 engine")
Fixes: 3faa8f982f95 ("netlabel: Move bitmap manipulation functions to the
NetLabel core.")
Signed-off-by: Paul Moore
---
net/ipv4/cipso_ipv4.c|3 ++-
net/netlabel/netlabel_kapi.c |3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 777fa3b7fb13..f4b83de2263e 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -667,7 +667,8 @@ static int cipso_v4_map_lvl_valid(const struct cipso_v4_doi
*doi_def, u8 level)
case CIPSO_V4_MAP_PASS:
return 0;
case CIPSO_V4_MAP_TRANS:
- if (doi_def->map.std->lvl.cipso[level] < CIPSO_V4_INV_LVL)
+ if ((level < doi_def->map.std->lvl.cipso_size) &&
+ (doi_def->map.std->lvl.cipso[level] < CIPSO_V4_INV_LVL))
return 0;
break;
}
diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
index ea7c67050792..ee3e5b6471a6 100644
--- a/net/netlabel/netlabel_kapi.c
+++ b/net/netlabel/netlabel_kapi.c
@@ -903,7 +903,8 @@ int netlbl_bitmap_walk(const unsigned char *bitmap, u32
bitmap_len,
(state == 0 && (byte & bitmask) == 0))
return bit_spot;
- bit_spot++;
+ if (++bit_spot >= bitmap_len)
+ return -1;
bitmask >>= 1;
if (bitmask == 0) {
byte = bitmap[++byte_offset];
Re: [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create()
On Wed, Mar 6, 2019 at 9:44 PM Mao Wenan wrote:
>
> If netlbl_sock_setattr() is failed, it directly returns rc and forgets
> to free secattr.
>
> BUG: memory leak
> unreferenced object 0x8883c3ea4200 (size 2664):
> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> hex dump (first 32 bytes):
> 7f 00 00 01 7f 00 00 01 eb 7f ed 71 4e 24 00 00 ...qN$..
> 02 00 07 40 00 00 00 00 00 00 00 00 00 00 00 00 ...@
> backtrace:
> [<4c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> [<535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> [<9aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> [<4274b384>] sock_create net/socket.c:1315 [inline]
> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> [<4ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<f737e62f>] 0x
>
> BUG: memory leak
> unreferenced object 0x8883de23d570 (size 32):
> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> hex dump (first 32 bytes):
> 02 00 00 00 00 00 00 00 60 a9 40 ce 83 88 ff ff `.@.
> 01 00 00 00 03 00 00 00 10 00 00 00 00 00 00 00
> backtrace:
> [<35ba8b75>] security_sk_alloc+0x5e/0xb0 security/security.c:1473
> [<302cc426>] sk_prot_alloc+0x8e/0x290 net/core/sock.c:1472
> [<4c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> [<535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> [<9aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> [<4274b384>] sock_create net/socket.c:1315 [inline]
> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> [<4ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<f737e62f>] 0x
>
> BUG: memory leak
> unreferenced object 0x8883ce40a960 (size 64):
> comm "syz-executor.2", pid 8813, jiffies 4297264420 (age 156.089s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> backtrace:
> [<29add401>] selinux_netlbl_socket_post_create+0x68/0x130
> security/selinux/netlabel.c:416
> [<5368a19c>] selinux_socket_post_create+0x31a/0x7f0
> security/selinux/hooks.c:4597
> [<bd4730e2>] security_socket_post_create+0x70/0xc0
> security/security.c:1385
> [<671052a4>] __sock_create+0x5a6/0x790 net/socket.c:1291
> [<4274b384>] sock_create net/socket.c:1315 [inline]
> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> [<4ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [<f737e62f>] 0x
>
> Fixes: 389fb800ac8b("netlabel: Label incoming TCP connections correctly in
> SELinux")
> Reported-by: Hulk Robot
> Signed-off-by: Mao Wenan
> ---
> security/selinux/netlabel.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index 186e727b737b..f3da05338580 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -426,6 +426,9 @@ int selinux_netlbl_socket_post_create(struct sock *sk,
> u16 family)
> rc = 0;
> break;
> }
> + if (rc != 0) {
> + netlbl_secattr_free(secattr);
> + }
This is likely going to cause a problem as the
sock->sk_security->nlbl_secattr still has a reference to the secattr
pointer you are releasing here. Assuming things are working correctly
elsewhere, I believe freeing secattr here will result in a double free
when the network stacks cleans up after the failed socket creation
(via the sock_release() call in the error handling code).
It looks like you may have found this via a test tool (syzbot?), do
you have a reproducer you can share?
--
paul moore
www.paul-moore.com
Re: [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create()
On Fri, Mar 8, 2019 at 1:31 AM maowenan wrote:
> On 2019/3/8 4:36, Paul Moore wrote:
> > On Wed, Mar 6, 2019 at 9:44 PM Mao Wenan wrote:
> >>
> >> If netlbl_sock_setattr() is failed, it directly returns rc and forgets
> >> to free secattr.
> >>
> >> BUG: memory leak
> >> unreferenced object 0x8883c3ea4200 (size 2664):
> >> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> >> hex dump (first 32 bytes):
> >> 7f 00 00 01 7f 00 00 01 eb 7f ed 71 4e 24 00 00 ...qN$..
> >> 02 00 07 40 00 00 00 00 00 00 00 00 00 00 00 00 ...@
> >> backtrace:
> >> [<4c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> >> [<535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> >> [<9aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> >> [<4274b384>] sock_create net/socket.c:1315 [inline]
> >> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >> [<4ae3186e>] do_syscall_64+0xc8/0x580
> >> arch/x86/entry/common.c:290
> >> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >> [<f737e62f>] 0x
> >>
> >> BUG: memory leak
> >> unreferenced object 0x8883de23d570 (size 32):
> >> comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> >> hex dump (first 32 bytes):
> >> 02 00 00 00 00 00 00 00 60 a9 40 ce 83 88 ff ff `.@.
> >> 01 00 00 00 03 00 00 00 10 00 00 00 00 00 00 00
> >> backtrace:
> >> [<35ba8b75>] security_sk_alloc+0x5e/0xb0
> >> security/security.c:1473
> >> [<302cc426>] sk_prot_alloc+0x8e/0x290 net/core/sock.c:1472
> >> [<4c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> >> [<535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> >> [<9aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> >> [<4274b384>] sock_create net/socket.c:1315 [inline]
> >> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >> [<4ae3186e>] do_syscall_64+0xc8/0x580
> >> arch/x86/entry/common.c:290
> >> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >> [<f737e62f>] 0x
> >>
> >> BUG: memory leak
> >> unreferenced object 0x8883ce40a960 (size 64):
> >> comm "syz-executor.2", pid 8813, jiffies 4297264420 (age 156.089s)
> >> hex dump (first 32 bytes):
> >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> backtrace:
> >> [<29add401>] selinux_netlbl_socket_post_create+0x68/0x130
> >> security/selinux/netlabel.c:416
> >> [<5368a19c>] selinux_socket_post_create+0x31a/0x7f0
> >> security/selinux/hooks.c:4597
> >> [<bd4730e2>] security_socket_post_create+0x70/0xc0
> >> security/security.c:1385
> >> [<671052a4>] __sock_create+0x5a6/0x790 net/socket.c:1291
> >> [<4274b384>] sock_create net/socket.c:1315 [inline]
> >> [<4274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >> [<b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >> [<b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >> [<b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >> [<4ae3186e>] do_syscall_64+0xc8/0x580
> >> arch/x86/entry/common.c:290
> >> [<bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >> [<f737e62f>] 0x
> >>
> >> Fixes: 389fb800ac8b("netlabel: Label incoming TCP
Re: [PATCH net] selinux: add the missing walk_size + len check in selinux_sctp_bind_connect
On Fri, Mar 8, 2019 at 12:08 PM Marcelo Ricardo Leitner
wrote:
> On Sat, Mar 09, 2019 at 12:07:34AM +0800, Xin Long wrote:
> > As does in __sctp_connect(), when checking addrs in a while loop, after
> > get the addr len according to sa_family, it's necessary to do the check
> > walk_size + af->sockaddr_len > addrs_size to make sure it won't access
> > an out-of-bounds addr.
> >
> > The same thing is needed in selinux_sctp_bind_connect(), otherwise an
> > out-of-bounds issue can be triggered:
> >
> > [14548.772313] BUG: KASAN: slab-out-of-bounds in
> > selinux_sctp_bind_connect+0x1aa/0x1f0
> > [14548.927083] Call Trace:
> > [14548.938072] dump_stack+0x9a/0xe9
> > [14548.953015] print_address_description+0x65/0x22e
> > [14548.996524] kasan_report.cold.6+0x92/0x1a6
> > [14549.015335] selinux_sctp_bind_connect+0x1aa/0x1f0
> > [14549.036947] security_sctp_bind_connect+0x58/0x90
> > [14549.058142] __sctp_setsockopt_connectx+0x5a/0x150 [sctp]
> > [14549.081650] sctp_setsockopt.part.24+0x1322/0x3ce0 [sctp]
> >
> > Fixes: d452930fd3b9 ("selinux: Add SCTP support")
> > Reported-by: Chunyu Hu
> > Signed-off-by: Xin Long
>
> Reviewed-by: Marcelo Ricardo Leitner
Thanks, this patch looks good to me.
> Paul, how can we get this into -stable trees? SELinux process may be
> different from -net trees.
For patches that warrant inclusion in -stable, I merge them into the
current selinux/stable-X.Y and send it up to Linus once it passes some
basic sanity tests. Since we're still only half-way through the merge
window, and this is an obvious bug fix, I think this qualifies as
-stable material.
I'm traveling at the moment with not-so-great connectivity, but I'll
get this merged and verified early next week.
--
paul moore
www.paul-moore.com
Re: [PATCH net] selinux: add the missing walk_size + len check in selinux_sctp_bind_connect
On Fri, Mar 8, 2019 at 9:47 PM Paul Moore wrote:
> On Fri, Mar 8, 2019 at 12:08 PM Marcelo Ricardo Leitner
> wrote:
> > On Sat, Mar 09, 2019 at 12:07:34AM +0800, Xin Long wrote:
> > > As does in __sctp_connect(), when checking addrs in a while loop, after
> > > get the addr len according to sa_family, it's necessary to do the check
> > > walk_size + af->sockaddr_len > addrs_size to make sure it won't access
> > > an out-of-bounds addr.
> > >
> > > The same thing is needed in selinux_sctp_bind_connect(), otherwise an
> > > out-of-bounds issue can be triggered:
> > >
> > > [14548.772313] BUG: KASAN: slab-out-of-bounds in
> > > selinux_sctp_bind_connect+0x1aa/0x1f0
> > > [14548.927083] Call Trace:
> > > [14548.938072] dump_stack+0x9a/0xe9
> > > [14548.953015] print_address_description+0x65/0x22e
> > > [14548.996524] kasan_report.cold.6+0x92/0x1a6
> > > [14549.015335] selinux_sctp_bind_connect+0x1aa/0x1f0
> > > [14549.036947] security_sctp_bind_connect+0x58/0x90
> > > [14549.058142] __sctp_setsockopt_connectx+0x5a/0x150 [sctp]
> > > [14549.081650] sctp_setsockopt.part.24+0x1322/0x3ce0 [sctp]
> > >
> > > Fixes: d452930fd3b9 ("selinux: Add SCTP support")
> > > Reported-by: Chunyu Hu
> > > Signed-off-by: Xin Long
> >
> > Reviewed-by: Marcelo Ricardo Leitner
>
> Thanks, this patch looks good to me.
>
> > Paul, how can we get this into -stable trees? SELinux process may be
> > different from -net trees.
>
> For patches that warrant inclusion in -stable, I merge them into the
> current selinux/stable-X.Y and send it up to Linus once it passes some
> basic sanity tests. Since we're still only half-way through the merge
> window, and this is an obvious bug fix, I think this qualifies as
> -stable material.
>
> I'm traveling at the moment with not-so-great connectivity, but I'll
> get this merged and verified early next week.
FYI, I just merged this into selinux/stable-5.1. Assuming it tests
okay (and I can't imagine it would cause a regression), I'll send it
to Linus tomorrow.
--
paul moore
www.paul-moore.com
Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
On Wed, May 8, 2019 at 2:55 PM Stephen Smalley wrote:
> On 5/8/19 2:27 PM, Marcelo Ricardo Leitner wrote:
> > On Wed, May 08, 2019 at 02:13:17PM -0400, Stephen Smalley wrote:
> >> On 5/8/19 2:12 PM, Stephen Smalley wrote:
> >>> On 5/8/19 9:32 AM, Paolo Abeni wrote:
> >>>> calling connect(AF_UNSPEC) on an already connected TCP socket is an
> >>>> established way to disconnect() such socket. After commit 68741a8adab9
> >>>> ("selinux: Fix ltp test connect-syscall failure") it no longer works
> >>>> and, in the above scenario connect() fails with EAFNOSUPPORT.
> >>>>
> >>>> Fix the above falling back to the generic/old code when the address
> >>>> family
> >>>> is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has
> >>>> specific constraints.
> >>>>
> >>>> Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure")
> >>>> Reported-by: Tom Deseyn
> >>>> Signed-off-by: Paolo Abeni
> >>>> ---
> >>>>security/selinux/hooks.c | 8
> >>>>1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>>> index c61787b15f27..d82b87c16b0a 100644
> >>>> --- a/security/selinux/hooks.c
> >>>> +++ b/security/selinux/hooks.c
> >>>> @@ -4649,7 +4649,7 @@ static int
> >>>> selinux_socket_connect_helper(struct socket *sock,
> >>>>struct lsm_network_audit net = {0,};
> >>>>struct sockaddr_in *addr4 = NULL;
> >>>>struct sockaddr_in6 *addr6 = NULL;
> >>>> -unsigned short snum;
> >>>> +unsigned short snum = 0;
> >>>>u32 sid, perm;
> >>>>/* sctp_connectx(3) calls via selinux_sctp_bind_connect()
> >>>> @@ -4674,12 +4674,12 @@ static int
> >>>> selinux_socket_connect_helper(struct socket *sock,
> >>>>break;
> >>>>default:
> >>>>/* Note that SCTP services expect -EINVAL, whereas
> >>>> - * others expect -EAFNOSUPPORT.
> >>>> + * others must handle this at the protocol level:
> >>>> + * connect(AF_UNSPEC) on a connected socket is
> >>>> + * a documented way disconnect the socket.
> >>>> */
> >>>>if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> >>>>return -EINVAL;
> >>>> -else
> >>>> -return -EAFNOSUPPORT;
> >>>
> >>> I think we need to return 0 here. Otherwise, we'll fall through with an
> >>> uninitialized snum, triggering a random/bogus permission check.
> >>
> >> Sorry, I see that you initialize snum above. Nonetheless, I think the
> >> correct behavior here is to skip the check since this is a disconnect, not
> >> a
> >> connect.
> >
> > Skipping the check would make it less controllable. So should it
> > somehow re-use shutdown() stuff? It gets very confusing, and after
> > all, it still is, in essence, a connect() syscall.
>
> The function checks CONNECT permission on entry, before reaching this
> point. This logic is only in preparation for a further check
> (NAME_CONNECT) on the port. In this case, there is no further check to
> perform and we can just return.
I agree with Stephen, in the connect(AF_UNSPEC) case the right thing
to do is to simply return with no error.
I would also suggest that since this patch only touches the SELinux
code it really should go in via the SELinux tree and not netdev; this
will help avoid merge conflicts in the linux-next tree and during the
merge window. I think the right thing to do at this point is to
create a revert patch (or have DaveM do it, I'm not sure what he
prefers in situations like this) for this commit, make the adjustments
that Stephen mentioned and submit them for the SELinux tree.
Otherwise, thanks for catching this and submitting a fix :)
--
paul moore
www.paul-moore.com
Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
On Thu, May 9, 2019 at 4:40 AM Paolo Abeni wrote:
> On Wed, 2019-05-08 at 17:17 -0400, Paul Moore wrote:
> > On Wed, May 8, 2019 at 2:55 PM Stephen Smalley wrote:
> > > On 5/8/19 2:27 PM, Marcelo Ricardo Leitner wrote:
> > > > On Wed, May 08, 2019 at 02:13:17PM -0400, Stephen Smalley wrote:
> > > > > On 5/8/19 2:12 PM, Stephen Smalley wrote:
> > > > > > On 5/8/19 9:32 AM, Paolo Abeni wrote:
> > > > > > > calling connect(AF_UNSPEC) on an already connected TCP socket is
> > > > > > > an
> > > > > > > established way to disconnect() such socket. After commit
> > > > > > > 68741a8adab9
> > > > > > > ("selinux: Fix ltp test connect-syscall failure") it no longer
> > > > > > > works
> > > > > > > and, in the above scenario connect() fails with EAFNOSUPPORT.
> > > > > > >
> > > > > > > Fix the above falling back to the generic/old code when the
> > > > > > > address
> > > > > > > family
> > > > > > > is not AF_INET{4,6}, but leave the SCTP code path untouched, as
> > > > > > > it has
> > > > > > > specific constraints.
> > > > > > >
> > > > > > > Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall
> > > > > > > failure")
> > > > > > > Reported-by: Tom Deseyn
> > > > > > > Signed-off-by: Paolo Abeni
> > > > > > > ---
> > > > > > >security/selinux/hooks.c | 8
> > > > > > >1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > > > > index c61787b15f27..d82b87c16b0a 100644
> > > > > > > --- a/security/selinux/hooks.c
> > > > > > > +++ b/security/selinux/hooks.c
> > > > > > > @@ -4649,7 +4649,7 @@ static int
> > > > > > > selinux_socket_connect_helper(struct socket *sock,
> > > > > > >struct lsm_network_audit net = {0,};
> > > > > > >struct sockaddr_in *addr4 = NULL;
> > > > > > >struct sockaddr_in6 *addr6 = NULL;
> > > > > > > -unsigned short snum;
> > > > > > > +unsigned short snum = 0;
> > > > > > >u32 sid, perm;
> > > > > > >/* sctp_connectx(3) calls via
> > > > > > > selinux_sctp_bind_connect()
> > > > > > > @@ -4674,12 +4674,12 @@ static int
> > > > > > > selinux_socket_connect_helper(struct socket *sock,
> > > > > > >break;
> > > > > > >default:
> > > > > > >/* Note that SCTP services expect -EINVAL, whereas
> > > > > > > - * others expect -EAFNOSUPPORT.
> > > > > > > + * others must handle this at the protocol level:
> > > > > > > + * connect(AF_UNSPEC) on a connected socket is
> > > > > > > + * a documented way disconnect the socket.
> > > > > > > */
> > > > > > >if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> > > > > > >return -EINVAL;
> > > > > > > -else
> > > > > > > -return -EAFNOSUPPORT;
> > > > > >
> > > > > > I think we need to return 0 here. Otherwise, we'll fall through
> > > > > > with an
> > > > > > uninitialized snum, triggering a random/bogus permission check.
> > > > >
> > > > > Sorry, I see that you initialize snum above. Nonetheless, I think the
> > > > > correct behavior here is to skip the check since this is a
> > > > > disconnect, not a
> > > > > connect.
> > > >
> > > > Skipping the check would make it less controllable. So should it
> > > > somehow re-use shutdown() stuff? It gets very confusing, and after
> > > > all, it still is, in essence, a connect() syscall.
> > >
> > > The function checks CONNECT permission on entry, before reaching this
> > > point
Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()
On Sun, Jun 24, 2018 at 3:48 AM David Miller wrote:
>
> From: Al Viro
> Date: Sat, 23 Jun 2018 23:21:07 +0100
>
> > BTW, I wonder if the life would be simpler with do_ipv6_setsockopt() doing
> > the copy-in and verifying ipv6_optlen(*hdr) <= newoptlen; that would've
> > simplified ipv6_renew_option{,s}() quite a bit and completely eliminated
> > ipv6_renew_options_kern()...
>
> I agree that this makes things a lot simpler.
I had looked at moving the userspace copy up, but feared it was a bit
too invasive. It sounds like you are open to the idea so I'll code
something up.
> One thing that drives me crazy though is this inherit stuff:
>
> > + ipv6_renew_option(newtype == IPV6_HOPOPTS ? newopt :
> > + opt ? opt->hopopt : NULL,
>
> Why don't we pass the type into ipv6_renew_option() and have it
> do this pointer dance instead?
>
> That's going to definitely be easier to read.
I agree, that struck me as a little odd. I'll rework that too. I'll
send you guys something this week to take a look at.
Thanks.
> I don't know enough about this code to give feedback about the
> option length handling wrt. copies, sorry.
--
paul moore
www.paul-moore.com
[RFC PATCH] ipv6: make ipv6_renew_options() interrupt/kernel safe
From: Paul Moore
At present the ipv6_renew_options_kern() function ends up calling into
access_ok() which is problematic if done from inside an interrupt as
access_ok() calls WARN_ON_IN_IRQ() on some (all?) architectures
(x86-64 is affected). Example warning/backtrace is shown below:
WARNING: CPU: 1 PID: 3144 at lib/usercopy.c:11 _copy_from_user+0x85/0x90
...
Call Trace:
ipv6_renew_option+0xb2/0xf0
ipv6_renew_options+0x26a/0x340
ipv6_renew_options_kern+0x2c/0x40
calipso_req_setattr+0x72/0xe0
netlbl_req_setattr+0x126/0x1b0
selinux_netlbl_inet_conn_request+0x80/0x100
selinux_inet_conn_request+0x6d/0xb0
security_inet_conn_request+0x32/0x50
tcp_conn_request+0x35f/0xe00
? __lock_acquire+0x250/0x16c0
? selinux_socket_sock_rcv_skb+0x1ae/0x210
? tcp_rcv_state_process+0x289/0x106b
tcp_rcv_state_process+0x289/0x106b
? tcp_v6_do_rcv+0x1a7/0x3c0
tcp_v6_do_rcv+0x1a7/0x3c0
tcp_v6_rcv+0xc82/0xcf0
ip6_input_finish+0x10d/0x690
ip6_input+0x45/0x1e0
? ip6_rcv_finish+0x1d0/0x1d0
ipv6_rcv+0x32b/0x880
? ip6_make_skb+0x1e0/0x1e0
__netif_receive_skb_core+0x6f2/0xdf0
? process_backlog+0x85/0x250
? process_backlog+0x85/0x250
? process_backlog+0xec/0x250
process_backlog+0xec/0x250
net_rx_action+0x153/0x480
__do_softirq+0xd9/0x4f7
do_softirq_own_stack+0x2a/0x40
...
While not present in the backtrace, ipv6_renew_option() ends up calling
access_ok() via the following chain:
access_ok()
_copy_from_user()
copy_from_user()
ipv6_renew_option()
The fix presented in this patch is to perform the userspace copy
earlier in the call chain such that it is only called when the option
data is actually coming from userspace; that place is
do_ipv6_setsockopt(). Not only does this solve the problem seen in
the backtrace above, it also allows us to simplify the code quite a
bit by removing ipv6_renew_options_kern() completely. We also take
this opportunity to cleanup ipv6_renew_options()/ipv6_renew_option()
a small amount as well.
This patch is heavily based on a rough patch by Al Viro. I've taken
his original patch, converted a kmemdup() call in do_ipv6_setsockopt()
to a memdup_user() call, made better use of the e_inval jump target in
the same function, and cleaned up the use ipv6_renew_option() by
ipv6_renew_options().
CC: Al Viro
Signed-off-by: Paul Moore
---
include/net/ipv6.h |9
net/ipv6/calipso.c |9 +---
net/ipv6/exthdrs.c | 108 --
net/ipv6/ipv6_sockglue.c | 27
4 files changed, 50 insertions(+), 103 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 16475c269749..d02881e4ad1f 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -355,14 +355,7 @@ struct ipv6_txoptions *ipv6_dup_options(struct sock *sk,
struct ipv6_txoptions *ipv6_renew_options(struct sock *sk,
struct ipv6_txoptions *opt,
int newtype,
- struct ipv6_opt_hdr __user *newopt,
- int newoptlen);
-struct ipv6_txoptions *
-ipv6_renew_options_kern(struct sock *sk,
- struct ipv6_txoptions *opt,
- int newtype,
- struct ipv6_opt_hdr *newopt,
- int newoptlen);
+ struct ipv6_opt_hdr *newopt);
struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,
struct ipv6_txoptions *opt);
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index 1323b9679cf7..1c0bb9fb76e6 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -799,8 +799,7 @@ static int calipso_opt_update(struct sock *sk, struct
ipv6_opt_hdr *hop)
{
struct ipv6_txoptions *old = txopt_get(inet6_sk(sk)), *txopts;
- txopts = ipv6_renew_options_kern(sk, old, IPV6_HOPOPTS,
-hop, hop ? ipv6_optlen(hop) : 0);
+ txopts = ipv6_renew_options(sk, old, IPV6_HOPOPTS, hop);
txopt_put(old);
if (IS_ERR(txopts))
return PTR_ERR(txopts);
@@ -1222,8 +1221,7 @@ static int calipso_req_setattr(struct request_sock *req,
if (IS_ERR(new))
return PTR_ERR(new);
- txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-new, new ? ipv6_optlen(new) : 0);
+ txopts = ipv6_renew_options(sk, req_inet->ipv6_opt, IPV6_HOPOPTS, new);
kfree(new);
@@ -1260,8 +1258,7 @@ static void calipso_req_delattr(struct request_sock *req)
if (calipso_opt_del(req_inet->ipv6_opt->hopopt, &new))
return; /* Nothing to do */
- txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-new, new ? ipv6_optlen(n
Re: linux-next: manual merge of the selinux tree with the net-next tree
} else {
> + if (dlen < sizeof(struct in6_addr))
> + goto free;
>
> - /* Label connection socket for first association 1-to-many
> - * style for client sequence socket()->sendmsg(). This
> - * needs to be done before sctp_assoc_add_peer() as that will
> - * set up the initial packet that needs to account for any
> - * security ip options (CIPSO/CALIPSO) added to the packet.
> - */
> - af = sctp_get_af_specific(to.sa.sa_family);
> - if (!af) {
> - err = -EINVAL;
> - goto out_unlock;
> + dlen = sizeof(struct in6_addr);
> + daddr->v6.sin6_family = AF_INET6;
> + daddr->v6.sin6_port = htons(asoc->peer.port);
> + memcpy(&daddr->v6.sin6_addr, CMSG_DATA(cmsg), dlen);
> }
> - err = security_sctp_bind_connect(sk, SCTP_SENDMSG_CONNECT,
> - (struct sockaddr *)&to,
> - af->sockaddr_len);
> - if (err < 0)
> - goto out_unlock;
> + err = sctp_verify_addr(sk, daddr, sizeof(*daddr));
> + if (err)
> + goto free;
>
> - new_asoc = sctp_association_new(ep, sk, scope, GFP_KERNEL);
> - if (!new_asoc) {
> - err = -ENOMEM;
> - goto out_unlock;
> - }
> - asoc = new_asoc;
> - err = sctp_assoc_set_bind_addr_from_ep(asoc, scope,
> GFP_KERNEL);
> - if (err < 0) {
> - err = -ENOMEM;
> - goto out_free;
> + old = sctp_endpoint_lookup_assoc(ep, daddr, &transport);
> + if (old && old != asoc) {
> + if (old->state >= SCTP_STATE_ESTABLISHED)
> + err = -EISCONN;
> + else
> + err = -EALREADY;
> + goto free;
> }
>
> - /* If the SCTP_INIT ancillary data is specified, set all
> - * the association init values accordingly.
> - */
> - if (sinit) {
> - if (sinit->sinit_num_ostreams) {
> - __u16 outcnt = sinit->sinit_num_ostreams;
> -
> - asoc->c.sinit_num_ostreams = outcnt;
> - /* outcnt has been changed, so re-init stream
> */
> - err = sctp_stream_init(&asoc->stream, outcnt,
> 0,
> - GFP_KERNEL);
> - if (err)
> - goto out_free;
> - }
> - if (sinit->sinit_max_instreams) {
> - asoc->c.sinit_max_instreams =
> - sinit->sinit_max_instreams;
> - }
> - if (sinit->sinit_max_attempts) {
> - asoc->max_init_attempts
> - = sinit->sinit_max_attempts;
> - }
> - if (sinit->sinit_max_init_timeo) {
> - asoc->max_init_timeo =
> -
> msecs_to_jiffies(sinit->sinit_max_init_timeo);
> - }
> + if (sctp_endpoint_is_peeled_off(ep, daddr)) {
> + err = -EADDRNOTAVAIL;
> + goto free;
> }
>
> - /* Prime the peer's transport structures. */
> - transport = sctp_assoc_add_peer(asoc, &to, GFP_KERNEL,
> SCTP_UNKNOWN);
> + transport = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL,
> + SCTP_UNKNOWN);
> if (!transport) {
> err = -ENOMEM;
> - goto out_free;
> + goto free;
> }
> }
>
--
paul moore
www.paul-moore.com
Re: [PATCH V7 2/4] sctp: Add ip option support
On Wed, Feb 21, 2018 at 3:45 PM, Paul Moore wrote: > On February 21, 2018 9:33:51 AM Marcelo Ricardo Leitner > wrote: >> On Tue, Feb 20, 2018 at 07:15:27PM +, Richard Haines wrote: >>> Add ip option support to allow LSM security modules to utilise CIPSO/IPv4 >>> and CALIPSO/IPv6 services. >>> >>> Signed-off-by: Richard Haines >> >> LGTM too, thanks! >> >> Acked-by: Marcelo Ricardo Leitner > > I agree, thanks everyone for all the work, review, and patience behind this > patchset! I'll work on merging this into selinux/next and I'll send a note > when it's done. I just merged the four patches (1,3,4 from the v6 patchset, 2 from the v7 patchset) in selinux/next and did a quick sanity test on the kernel (booted, no basic SELinux regressions). Additional testing help is always appreciated ... * git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git * https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git -- paul moore www.paul-moore.com
Re: [PATCH V7 2/4] sctp: Add ip option support
On Thu, Feb 22, 2018 at 9:40 PM, Marcelo Ricardo Leitner wrote: > On Thu, Feb 22, 2018 at 06:08:05PM -0500, Paul Moore wrote: >> On Wed, Feb 21, 2018 at 3:45 PM, Paul Moore wrote: >> > On February 21, 2018 9:33:51 AM Marcelo Ricardo Leitner >> > wrote: >> >> On Tue, Feb 20, 2018 at 07:15:27PM +, Richard Haines wrote: >> >>> Add ip option support to allow LSM security modules to utilise CIPSO/IPv4 >> >>> and CALIPSO/IPv6 services. >> >>> >> >>> Signed-off-by: Richard Haines >> >> >> >> LGTM too, thanks! >> >> >> >> Acked-by: Marcelo Ricardo Leitner >> > >> > I agree, thanks everyone for all the work, review, and patience behind >> > this patchset! I'll work on merging this into selinux/next and I'll send >> > a note when it's done. >> >> I just merged the four patches (1,3,4 from the v6 patchset, 2 from the >> v7 patchset) in selinux/next and did a quick sanity test on the kernel >> (booted, no basic SELinux regressions). Additional testing help is >> always appreciated ... > > I'll try it early next week. > > Any ideas on when this is going to appear on Dave's net-next tree? > We have a lot of SCTP changes to be posted on this cycle and would be > nice if we could avoid merge conflicts. It's merged into the SELinux tree, next branch; see the links below. Last I checked DaveM doesn't pull the selinux/next into his net-next tree (that would be a little funny for historical reasons). Any idea on how bad the merge conflicts are? >> * git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git >> * https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git -- paul moore www.paul-moore.com
Re: [PATCH V8 2/4] sctp: Add ip option support
.sockaddr_len = sizeof(struct sockaddr_in),
> + .ip_options_len= sctp_v4_ip_options_len,
> #ifdef CONFIG_COMPAT
> .compat_setsockopt = compat_ip_setsockopt,
> .compat_getsockopt = compat_ip_getsockopt,
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index bf271f8..eb55c63 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3138,6 +3138,7 @@ static int sctp_setsockopt_mappedv4(struct sock *sk,
> char __user *optval, unsign
> static int sctp_setsockopt_maxseg(struct sock *sk, char __user *optval,
> unsigned int optlen)
> {
> struct sctp_sock *sp = sctp_sk(sk);
> + struct sctp_af *af = sp->pf->af;
> struct sctp_assoc_value params;
> struct sctp_association *asoc;
> int val;
> @@ -3162,7 +3163,8 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char
> __user *optval, unsigned
> if (val) {
> int min_len, max_len;
>
> - min_len = SCTP_DEFAULT_MINSEGMENT -
> sp->pf->af->net_header_len;
> + min_len = SCTP_DEFAULT_MINSEGMENT - af->net_header_len;
> + min_len -= af->ip_options_len(sk);
> min_len -= sizeof(struct sctphdr) +
>sizeof(struct sctp_data_chunk);
>
> @@ -3175,7 +3177,8 @@ static int sctp_setsockopt_maxseg(struct sock *sk, char
> __user *optval, unsigned
> asoc = sctp_id2assoc(sk, params.assoc_id);
> if (asoc) {
> if (val == 0) {
> - val = asoc->pathmtu - sp->pf->af->net_header_len;
> + val = asoc->pathmtu - af->net_header_len;
> + val -= af->ip_options_len(sk);
> val -= sizeof(struct sctphdr) +
>sctp_datachk_len(&asoc->stream);
> }
> @@ -5087,9 +5090,11 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id,
> struct socket **sockp)
> sctp_copy_sock(sock->sk, sk, asoc);
>
> /* Make peeled-off sockets more like 1-1 accepted sockets.
> -* Set the daddr and initialize id to something more random
> +* Set the daddr and initialize id to something more random and also
> +* copy over any ip options.
> */
> sp->pf->to_sk_daddr(&asoc->peer.primary_addr, sk);
> + sp->pf->copy_ip_options(sk, sock->sk);
>
> /* Populate the fields of the newsk from the oldsk and migrate the
> * asoc to the newsk.
> --
> 2.14.3
>
--
paul moore
www.paul-moore.com
Re: Regression found when running LTP connect01 on next-20180301
On Thu, Mar 1, 2018 at 3:33 AM, Anders Roxell wrote: > Hi, > > I was running LTP's testcase connect01 [1] and found a regression in > linux-next > (next-20180301). Bisect gave me this patch as the problematic patch (sha > d452930fd3b9 "selinux: Add SCTP support") on a x86 target. > > Output from the test(LTP release 20180118): > $ cd /opt/ltp/ > $ cat runtest/syscalls |grep connect01>runtest/connect-syscall > $ ./runltp -pq -f connect-syscall > " > Running tests... > connect011 TPASS : bad file descriptor successful > connect012 TPASS : invalid socket buffer successful > connect013 TPASS : invalid salen successful > connect014 TPASS : invalid socket successful > connect015 TPASS : already connected successful > connect016 TPASS : connection refused successful > connect017 TFAIL : connect01.c:146: invalid address family ; returned > -1 (expected -1), errno 22 (expected 97) > INFO: ltp-pan reported some tests FAIL > LTP Version: 20180118 > " > > The output from the test expected 97 and we received 22, can you please > elaborate on what have been changed? > > Cheers, > Anders > [1] > https://github.com/linux-test-project/ltp/blob/20180118/testcases/kernel/syscalls/connect/connect01.c#L146 Hi Anders, Thanks for the report. Out of curiosity, we're you running the full LTP test suite and this was the only failure, or did you just run the connect01 test? Either answer is fine, I'm just trying to understand the scope of the regression. Richard, are you able to look into this? If not, let me know and I'll dig a bit deeper (I'll likely take a quick look today, but if the failure is subtle it might require some digging). -- paul moore www.paul-moore.com
Re: Regression found when running LTP connect01 on next-20180301
On Thu, Mar 1, 2018 at 9:36 AM, Richard Haines wrote: > On Thu, 2018-03-01 at 08:42 -0500, Paul Moore wrote: >> On Thu, Mar 1, 2018 at 3:33 AM, Anders Roxell > rg> wrote: >> > Hi, >> > >> > I was running LTP's testcase connect01 [1] and found a regression >> > in linux-next >> > (next-20180301). Bisect gave me this patch as the problematic >> > patch (sha >> > d452930fd3b9 "selinux: Add SCTP support") on a x86 target. >> > >> > Output from the test(LTP release 20180118): >> > $ cd /opt/ltp/ >> > $ cat runtest/syscalls |grep connect01>runtest/connect-syscall >> > $ ./runltp -pq -f connect-syscall >> > " >> > Running tests... >> > connect011 TPASS : bad file descriptor successful >> > connect012 TPASS : invalid socket buffer successful >> > connect013 TPASS : invalid salen successful >> > connect014 TPASS : invalid socket successful >> > connect015 TPASS : already connected successful >> > connect016 TPASS : connection refused successful >> > connect017 TFAIL : connect01.c:146: invalid address family ; >> > returned -1 (expected -1), errno 22 (expected 97) >> > INFO: ltp-pan reported some tests FAIL >> > LTP Version: 20180118 >> > " >> > >> > The output from the test expected 97 and we received 22, can you >> > please >> > elaborate on what have been changed? >> > >> > Cheers, >> > Anders >> > [1] https://github.com/linux-test-project/ltp/blob/20180118/testcas >> > es/kernel/syscalls/connect/connect01.c#L146 >> >> Hi Anders, >> >> Thanks for the report. Out of curiosity, we're you running the full >> LTP test suite and this was the only failure, or did you just run the >> connect01 test? Either answer is fine, I'm just trying to understand >> the scope of the regression. >> >> Richard, are you able to look into this? If not, let me know and >> I'll >> dig a bit deeper (I'll likely take a quick look today, but if the >> failure is subtle it might require some digging). > > I'll have a look today. Thanks. Let me know if you get stuck. -- paul moore www.paul-moore.com
Re: Regression found when running LTP connect01 on next-20180301
On March 1, 2018 9:36:37 AM Richard Haines wrote: > On Thu, 2018-03-01 at 08:42 -0500, Paul Moore wrote: >> On Thu, Mar 1, 2018 at 3:33 AM, Anders Roxell > rg> wrote: >> > Hi, >> > >> > I was running LTP's testcase connect01 [1] and found a regression >> > in linux-next >> > (next-20180301). Bisect gave me this patch as the problematic >> > patch (sha >> > d452930fd3b9 "selinux: Add SCTP support") on a x86 target. >> > >> > Output from the test(LTP release 20180118): >> > $ cd /opt/ltp/ >> > $ cat runtest/syscalls |grep connect01>runtest/connect-syscall >> > $ ./runltp -pq -f connect-syscall >> > " >> > Running tests... >> > connect011 TPASS : bad file descriptor successful >> > connect012 TPASS : invalid socket buffer successful >> > connect013 TPASS : invalid salen successful >> > connect014 TPASS : invalid socket successful >> > connect015 TPASS : already connected successful >> > connect016 TPASS : connection refused successful >> > connect017 TFAIL : connect01.c:146: invalid address family ; >> > returned -1 (expected -1), errno 22 (expected 97) >> > INFO: ltp-pan reported some tests FAIL >> > LTP Version: 20180118 >> > " >> > >> > The output from the test expected 97 and we received 22, can you >> > please >> > elaborate on what have been changed? >> > >> > Cheers, >> > Anders >> > [1] https://github.com/linux-test-project/ltp/blob/20180118/testcas >> > es/kernel/syscalls/connect/connect01.c#L146 >> >> Hi Anders, >> >> Thanks for the report. Out of curiosity, we're you running the full >> LTP test suite and this was the only failure, or did you just run the >> connect01 test? Either answer is fine, I'm just trying to understand >> the scope of the regression. >> >> Richard, are you able to look into this? If not, let me know and >> I'll >> dig a bit deeper (I'll likely take a quick look today, but if the >> failure is subtle it might require some digging). > > I'll have a look today. One more thing I forgot to mention earlier, if there is a patch to fix this, could you please base it on top of the existing SELinux/SCTP patches that have already been merged, and not respin an earlier patch? Thank you. -- paul moore www.paul-moore.com
Re: Regression found when running LTP connect01 on next-20180301
On Thu, Mar 1, 2018 at 3:01 PM, Anders Roxell wrote: > On 1 March 2018 at 14:42, Paul Moore wrote: >> On Thu, Mar 1, 2018 at 3:33 AM, Anders Roxell >> wrote: >>> Hi, >>> >>> I was running LTP's testcase connect01 [1] and found a regression in >>> linux-next >>> (next-20180301). Bisect gave me this patch as the problematic patch (sha >>> d452930fd3b9 "selinux: Add SCTP support") on a x86 target. >>> >>> Output from the test(LTP release 20180118): >>> $ cd /opt/ltp/ >>> $ cat runtest/syscalls |grep connect01>runtest/connect-syscall >>> $ ./runltp -pq -f connect-syscall >>> " >>> Running tests... >>> connect011 TPASS : bad file descriptor successful >>> connect012 TPASS : invalid socket buffer successful >>> connect013 TPASS : invalid salen successful >>> connect014 TPASS : invalid socket successful >>> connect015 TPASS : already connected successful >>> connect016 TPASS : connection refused successful >>> connect017 TFAIL : connect01.c:146: invalid address family ; >>> returned -1 (expected -1), errno 22 (expected 97) >>> INFO: ltp-pan reported some tests FAIL >>> LTP Version: 20180118 >>> " >>> >>> The output from the test expected 97 and we received 22, can you please >>> elaborate on what have been changed? >>> >>> Cheers, >>> Anders >>> [1] >>> https://github.com/linux-test-project/ltp/blob/20180118/testcases/kernel/syscalls/connect/connect01.c#L146 >> >> Hi Anders, >> >> Thanks for the report. Out of curiosity, we're you running the full >> LTP test suite and this was the only failure, or did you just run the >> connect01 test? > > Normally we run all syscalls, but when we saw this regression I did the > bisect and only ran test connect01. > On every new push we ran 19 different sets of LTP tests, where > connect01 is part of the syscalls test set. So this means that only the connect01 test experienced failures? >> Either answer is fine, I'm just trying to understand >> the scope of the regression. >> >> Richard, are you able to look into this? If not, let me know and I'll >> dig a bit deeper (I'll likely take a quick look today, but if the >> failure is subtle it might require some digging). >> >> -- >> paul moore >> www.paul-moore.com -- paul moore www.paul-moore.com
Re: [RFC PATCH V1 01/12] audit: add container id
On Thu, Mar 1, 2018 at 8:41 PM, Richard Guy Briggs wrote: > On 2018-03-01 14:41, Richard Guy Briggs wrote: >> Implement the proc fs write to set the audit container ID of a process, >> emitting an AUDIT_CONTAINER record to document the event. >> >> This is a write from the container orchestrator task to a proc entry of >> the form /proc/PID/containerid where PID is the process ID of the newly >> created task that is to become the first task in a container, or an >> additional task added to a container. >> >> The write expects up to a u64 value (unset: 18446744073709551615). >> >> This will produce a record such as this: >> type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 >> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 >> ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0 >> >> The "op" field indicates an initial set. The "pid" to "ses" fields are >> the orchestrator while the "opid" field is the object's PID, the process >> being "contained". Old and new container ID values are given in the >> "contid" fields, while res indicates its success. >> >> It is not permitted to self-set, unset or re-set the container ID. A >> child inherits its parent's container ID, but then can be set only once >> after. > > There are more restrictions coming later: > - check that the child being set has no children or threads yet, or > forcibly set them all to the same container ID (assuming they all pass > the same tests). This will also prevent an orch from setting its > parent and other tit-for-tat games to circumvent the basic checks. FYI, I think you may have a problem with something in your outgoing mail path; I didn't receive the original patchset you are referencing and it doesn't appear in the mail archive either. -- paul moore www.paul-moore.com
Re: [RFC PATCH V1 01/12] audit: add container id
On Fri, Mar 2, 2018 at 1:23 PM, Matthew Wilcox wrote: > On Fri, Mar 02, 2018 at 10:48:42AM -0500, Paul Moore wrote: >> On Thu, Mar 1, 2018 at 8:41 PM, Richard Guy Briggs wrote: >> > On 2018-03-01 14:41, Richard Guy Briggs wrote: >> FYI, I think you may have a problem with something in your outgoing >> mail path; I didn't receive the original patchset you are referencing >> and it doesn't appear in the mail archive either. > > I have those patches. Which mail archive is missing them? The archive run by the linux-audit mailing list: * https://www.redhat.com/archives/linux-audit -- paul moore www.paul-moore.com
Re: [RFC PATCH V1 01/12] audit: add container id
On Fri, Mar 2, 2018 at 2:25 PM, Paul Moore wrote: > On Fri, Mar 2, 2018 at 1:23 PM, Matthew Wilcox wrote: >> On Fri, Mar 02, 2018 at 10:48:42AM -0500, Paul Moore wrote: >>> On Thu, Mar 1, 2018 at 8:41 PM, Richard Guy Briggs wrote: >>> > On 2018-03-01 14:41, Richard Guy Briggs wrote: >>> FYI, I think you may have a problem with something in your outgoing >>> mail path; I didn't receive the original patchset you are referencing >>> and it doesn't appear in the mail archive either. >> >> I have those patches. Which mail archive is missing them? > > The archive run by the linux-audit mailing list: > > * https://www.redhat.com/archives/linux-audit After having my reply get bounced from the linux-audit list I realized that Richard had gotten a little overzealous with the number of recipients (note to Richard, you easily could have dropped some of those lists/people from the To/CC line). I was able to get in and free those patches from the moderation queue, they should be arriving on the linux-audit list shortly. -- paul moore www.paul-moore.com
Re: [PATCH] selinux: Fix ltp test connect-syscall failure
On Fri, Mar 2, 2018 at 2:54 PM, Richard Haines
wrote:
> Fix the following error when running regression tests using LTP as follows:
> cd /opt/ltp/
> cat runtest/syscalls |grep connect01>runtest/connect-syscall
> ./runltp -pq -f connect-syscall
>
> Running tests...
> connect011 TPASS : bad file descriptor successful
> connect012 TPASS : invalid socket buffer successful
> connect013 TPASS : invalid salen successful
> connect014 TPASS : invalid socket successful
> connect015 TPASS : already connected successful
> connect016 TPASS : connection refused successful
> connect017 TFAIL : connect01.c:146: invalid address family ;
> returned -1 (expected -1), errno 22 (expected 97)
> INFO: ltp-pan reported some tests FAIL
> LTP Version: 20180118
>
> Reported-by: Anders Roxell
> Signed-off-by: Richard Haines
> ---
> security/selinux/hooks.c | 42 ++
> 1 file changed, 30 insertions(+), 12 deletions(-)
Merged, thanks guys.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 28a5c4e..d614df1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4470,22 +4470,29 @@ static int selinux_socket_bind(struct socket *sock,
> struct sockaddr *address, in
> * need to check address->sa_family as it is possible to have
> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
> */
> - if (address->sa_family == AF_INET) {
> - if (addrlen < sizeof(struct sockaddr_in)) {
> - err = -EINVAL;
> - goto out;
> - }
> + switch (address->sa_family) {
> + case AF_INET:
> + if (addrlen < sizeof(struct sockaddr_in))
> + return -EINVAL;
> addr4 = (struct sockaddr_in *)address;
> snum = ntohs(addr4->sin_port);
> addrp = (char *)&addr4->sin_addr.s_addr;
> - } else {
> - if (addrlen < SIN6_LEN_RFC2133) {
> - err = -EINVAL;
> - goto out;
> - }
> + break;
> + case AF_INET6:
> + if (addrlen < SIN6_LEN_RFC2133)
> + return -EINVAL;
> addr6 = (struct sockaddr_in6 *)address;
> snum = ntohs(addr6->sin6_port);
> addrp = (char *)&addr6->sin6_addr.s6_addr;
> + break;
> + default:
> + /* Note that SCTP services expect -EINVAL, whereas
> +* others expect -EAFNOSUPPORT.
> +*/
> + if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> + return -EINVAL;
> + else
> + return -EAFNOSUPPORT;
> }
>
> if (snum) {
> @@ -4589,16 +4596,27 @@ static int selinux_socket_connect_helper(struct
> socket *sock,
> * need to check address->sa_family as it is possible to have
> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
> */
> - if (address->sa_family == AF_INET) {
> + switch (address->sa_family) {
> + case AF_INET:
> addr4 = (struct sockaddr_in *)address;
> if (addrlen < sizeof(struct sockaddr_in))
> return -EINVAL;
> snum = ntohs(addr4->sin_port);
> - } else {
> + break;
> + case AF_INET6:
> addr6 = (struct sockaddr_in6 *)address;
> if (addrlen < SIN6_LEN_RFC2133)
> return -EINVAL;
> snum = ntohs(addr6->sin6_port);
> + break;
> + default:
> + /* Note that SCTP services expect -EINVAL, whereas
> +* others expect -EAFNOSUPPORT.
> + */
> + if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> + return -EINVAL;
> + else
> + return -EAFNOSUPPORT;
> }
>
> err = sel_netport_sid(sk->sk_protocol, snum, &sid);
> --
> 2.14.3
>
--
paul moore
www.paul-moore.com
Re: [RFC PATCH V1 01/12] audit: add container id
On Sat, Mar 3, 2018 at 4:19 AM, Serge E. Hallyn wrote:
> On Thu, Mar 01, 2018 at 02:41:04PM -0500, Richard Guy Briggs wrote:
> ...
>> +static inline bool audit_containerid_set(struct task_struct *tsk)
>
> Hi Richard,
>
> the calls to audit_containerid_set() confused me. Could you make it
> is_audit_containerid_set() or audit_containerid_isset()?
I haven't gone through the entire patchset yet, but I wanted to
quickly comment on this ... I really dislike the
function-names-as-sentences approach and would would greatly prefer
audit_containerid_isset().
>> +{
>> + return audit_get_containerid(tsk) != INVALID_CID;
>> +}
--
paul moore
www.paul-moore.com
[PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()
From: Paul Moore
Starting with v4.16-rc1 we've been seeing a higher than usual number
of requests for the kernel to load networking modules, even on events
which shouldn't trigger a module load (e.g. ioctl(TCGETS)). Stephen
Smalley suggested the problem may lie in commit 44c02a2c3dc5
("dev_ioctl(): move copyin/copyout to callers") which moves changes
the network dev_ioctl() function to always call dev_load(),
regardless of the requested ioctl.
This patch moves the dev_load() calls back into the individual ioctls
while preserving the rest of the original patch.
Reported-by: Dominick Grift
Suggested-by: Stephen Smalley
Signed-off-by: Paul Moore
---
net/core/dev_ioctl.c |7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 0ab1af04296c..a04e1e88bf3a 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -402,8 +402,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
if (colon)
*colon = 0;
- dev_load(net, ifr->ifr_name);
-
/*
* See which interface the caller is talking about.
*/
@@ -423,6 +421,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
case SIOCGIFMAP:
case SIOCGIFINDEX:
case SIOCGIFTXQLEN:
+ dev_load(net, ifr->ifr_name);
rcu_read_lock();
ret = dev_ifsioc_locked(net, ifr, cmd);
rcu_read_unlock();
@@ -431,6 +430,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
return ret;
case SIOCETHTOOL:
+ dev_load(net, ifr->ifr_name);
rtnl_lock();
ret = dev_ethtool(net, ifr);
rtnl_unlock();
@@ -447,6 +447,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
case SIOCGMIIPHY:
case SIOCGMIIREG:
case SIOCSIFNAME:
+ dev_load(net, ifr->ifr_name);
if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
return -EPERM;
rtnl_lock();
@@ -494,6 +495,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
/* fall through */
case SIOCBONDSLAVEINFOQUERY:
case SIOCBONDINFOQUERY:
+ dev_load(net, ifr->ifr_name);
rtnl_lock();
ret = dev_ifsioc(net, ifr, cmd);
rtnl_unlock();
@@ -518,6 +520,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
ifreq *ifr, bool *need_c
cmd == SIOCGHWTSTAMP ||
(cmd >= SIOCDEVPRIVATE &&
cmd <= SIOCDEVPRIVATE + 15)) {
+ dev_load(net, ifr->ifr_name);
rtnl_lock();
ret = dev_ifsioc(net, ifr, cmd);
rtnl_unlock();
Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()
On Tue, Mar 6, 2018 at 5:27 PM, Paul Moore wrote:
> From: Paul Moore
>
> Starting with v4.16-rc1 we've been seeing a higher than usual number
> of requests for the kernel to load networking modules, even on events
> which shouldn't trigger a module load (e.g. ioctl(TCGETS)). Stephen
> Smalley suggested the problem may lie in commit 44c02a2c3dc5
> ("dev_ioctl(): move copyin/copyout to callers") which moves changes
> the network dev_ioctl() function to always call dev_load(),
> regardless of the requested ioctl.
>
> This patch moves the dev_load() calls back into the individual ioctls
> while preserving the rest of the original patch.
>
> Reported-by: Dominick Grift
> Suggested-by: Stephen Smalley
> Signed-off-by: Paul Moore
> ---
> net/core/dev_ioctl.c |7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
In the interest of full disclosure, I've compiled this code but I
haven't booted it yet (test kernel building now). I just wanted to
post this sooner rather than later in case the networking folks, or
Al, had a different solution they would prefer.
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 0ab1af04296c..a04e1e88bf3a 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -402,8 +402,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> if (colon)
> *colon = 0;
>
> - dev_load(net, ifr->ifr_name);
> -
> /*
> * See which interface the caller is talking about.
> */
> @@ -423,6 +421,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> case SIOCGIFMAP:
> case SIOCGIFINDEX:
> case SIOCGIFTXQLEN:
> + dev_load(net, ifr->ifr_name);
> rcu_read_lock();
> ret = dev_ifsioc_locked(net, ifr, cmd);
> rcu_read_unlock();
> @@ -431,6 +430,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> return ret;
>
> case SIOCETHTOOL:
> + dev_load(net, ifr->ifr_name);
> rtnl_lock();
> ret = dev_ethtool(net, ifr);
> rtnl_unlock();
> @@ -447,6 +447,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> case SIOCGMIIPHY:
> case SIOCGMIIREG:
> case SIOCSIFNAME:
> + dev_load(net, ifr->ifr_name);
> if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> return -EPERM;
> rtnl_lock();
> @@ -494,6 +495,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> /* fall through */
> case SIOCBONDSLAVEINFOQUERY:
> case SIOCBONDINFOQUERY:
> + dev_load(net, ifr->ifr_name);
> rtnl_lock();
> ret = dev_ifsioc(net, ifr, cmd);
> rtnl_unlock();
> @@ -518,6 +520,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
> ifreq *ifr, bool *need_c
> cmd == SIOCGHWTSTAMP ||
> (cmd >= SIOCDEVPRIVATE &&
> cmd <= SIOCDEVPRIVATE + 15)) {
> + dev_load(net, ifr->ifr_name);
> rtnl_lock();
> ret = dev_ifsioc(net, ifr, cmd);
> rtnl_unlock();
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
paul moore
www.paul-moore.com
Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()
On Tue, Mar 6, 2018 at 6:59 PM, Stephen Hemminger
wrote:
> On Tue, 06 Mar 2018 17:27:44 -0500
> Paul Moore wrote:
>> From: Paul Moore
>>
>> Starting with v4.16-rc1 we've been seeing a higher than usual number
>> of requests for the kernel to load networking modules, even on events
>> which shouldn't trigger a module load (e.g. ioctl(TCGETS)). Stephen
>> Smalley suggested the problem may lie in commit 44c02a2c3dc5
>> ("dev_ioctl(): move copyin/copyout to callers") which moves changes
>> the network dev_ioctl() function to always call dev_load(),
>> regardless of the requested ioctl.
>>
>> This patch moves the dev_load() calls back into the individual ioctls
>> while preserving the rest of the original patch.
>>
>> Reported-by: Dominick Grift
>> Suggested-by: Stephen Smalley
>> Signed-off-by: Paul Moore
>> ---
>> net/core/dev_ioctl.c |7 +--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
>> index 0ab1af04296c..a04e1e88bf3a 100644
>> --- a/net/core/dev_ioctl.c
>> +++ b/net/core/dev_ioctl.c
>> @@ -402,8 +402,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct
>> ifreq *ifr, bool *need_c
>> if (colon)
>> *colon = 0;
>>
>> - dev_load(net, ifr->ifr_name);
>
> Actually dev_load by ethernet name is really a legacy thing that should just
> die,
>
> It was kept around so that some very tunnel configuration using special names.
>
> # ifconfig sit0
>
> which probably several web pages still tell users to do...
> We have much better control now with ip commands so that this is just
> baggage.
In an effort to get this regression fixed quickly, and not get tangled
up in a user education issue, can we at least restore the old ioctl()
behavior and worry about removing dev_load() later?
--
paul moore
www.paul-moore.com
Re: linux-next: manual merge of the selinux tree with the net-next tree
On Mon, Mar 5, 2018 at 2:03 AM, Xin Long wrote:
> On Mon, Mar 5, 2018 at 9:40 AM, Stephen Rothwell
> wrote:
>> Hi Paul,
>>
>> Today's linux-next merge of the selinux tree got a conflict in:
>>
>> net/sctp/socket.c
>>
>> between several refactoring commits from the net-next tree and commit:
>>
>> 2277c7cd75e3 ("sctp: Add LSM hooks")
>>
>> from the selinux tree.
>>
>> I fixed it up (I think - see below) and can carry the fix as
> The fixup is great! the same as I mentioned in:
> https://patchwork.ozlabs.org/patch/879898/
> for net-next.git
>
>> necessary. This is now fixed as far as linux-next is concerned, but any
>> non trivial conflicts should be mentioned to your upstream maintainer
>> when your tree is submitted for merging. You may also want to consider
>> cooperating with the maintainer of the conflicting tree to minimise any
>> particularly complex conflicts.
>
> [net-next,0/9] sctp: clean up sctp_sendmsg, this patchset was just applied
> in net-next. So I just guess it might not yet be there when selinux tree was
> being submitted.
The selinux/next branch is based on v4.16-rc1 and doesn't feed into
the netdev tree, it goes straight to Linus during the merge window so
unfortunately I think we may need to carry this for some time and
relay this fix-up patch up to Linus during the merge window.
--
paul moore
www.paul-moore.com
Re: linux-next: manual merge of the selinux tree with the net-next tree
On Wed, Mar 7, 2018 at 11:41 AM, David Miller wrote:
> From: Paul Moore
> Date: Wed, 7 Mar 2018 11:34:31 -0500
>> On Mon, Mar 5, 2018 at 2:03 AM, Xin Long wrote:
>>> On Mon, Mar 5, 2018 at 9:40 AM, Stephen Rothwell
>>> wrote:
>>>> Hi Paul,
>>>>
>>>> Today's linux-next merge of the selinux tree got a conflict in:
>>>>
>>>> net/sctp/socket.c
>>>>
>>>> between several refactoring commits from the net-next tree and commit:
>>>>
>>>> 2277c7cd75e3 ("sctp: Add LSM hooks")
>>>>
>>>> from the selinux tree.
>>>>
>>>> I fixed it up (I think - see below) and can carry the fix as
>>> The fixup is great! the same as I mentioned in:
>>> https://patchwork.ozlabs.org/patch/879898/
>>> for net-next.git
>>>
>>>> necessary. This is now fixed as far as linux-next is concerned, but any
>>>> non trivial conflicts should be mentioned to your upstream maintainer
>>>> when your tree is submitted for merging. You may also want to consider
>>>> cooperating with the maintainer of the conflicting tree to minimise any
>>>> particularly complex conflicts.
>>>
>>> [net-next,0/9] sctp: clean up sctp_sendmsg, this patchset was just applied
>>> in net-next. So I just guess it might not yet be there when selinux tree was
>>> being submitted.
>>
>> The selinux/next branch is based on v4.16-rc1 and doesn't feed into
>> the netdev tree, it goes straight to Linus during the merge window so
>> unfortunately I think we may need to carry this for some time and
>> relay this fix-up patch up to Linus during the merge window.
>
> What a mess.
>
> The SCTP option changes should have gone through my tree in retrospect.
It's unfortunate.
I'm not sure we could have cleanly separated the core network stack
changes from the rest of the SELinux/SCTP enablement, regardless it's
a bit late at this point. The only other thought would have been to
simply push Xin Long's cleanup patches until after the next merge
window, but that would only be worth considering if they truly were
just cleanup patches, and even then it doesn't seem very fair to Xin
Long to have to wait.
Thankfully stuff like this is rare (at least from a netdev/SELinux POV).
--
paul moore
www.paul-moore.com
Re: linux-next: manual merge of the selinux tree with the net-next tree
On Wed, Mar 7, 2018 at 12:45 PM, David Miller wrote: > From: Paul Moore > Date: Wed, 7 Mar 2018 12:27:52 -0500 > >> I'm not sure we could have cleanly separated the core network stack >> changes from the rest of the SELinux/SCTP enablement, regardless it's >> a bit late at this point. The only other thought would have been to >> simply push Xin Long's cleanup patches until after the next merge >> window, but that would only be worth considering if they truly were >> just cleanup patches, and even then it doesn't seem very fair to Xin >> Long to have to wait. > > I think you wanted to have more integration, rather than less. I'm not quite sure where you are going here, I think we *all* want integration - subtrees merge patches/trees, Linus merges subtrees, etc. - and I don't believe I've said anything to the contrary. > What others have done in the past, is they simply pull my networking > tree into their's. I only base the SELinux and audit trees on Linus' tree. Perhaps I'm wrong, but a quick look at net-next makes me believe you do the same. I think it is also worth mentioning that the SELinux/SCTP patches have been in the selinux/next branch for several days now; from what I can tell they predate these net-next cleanup patches. Not that it matters, I just don't believe that pulling net-next would have solved this problem; I suppose the right thing would have been for net-next to pull selinux/next, yes? > I never rebase, ever. I've learned that saying "never" (or "never X, ever" in this case) is a recipe for disaster, but if it works for you, go for it. FWIW, I try to avoid rebases as much as possible; it's the nuclear option as far as I'm concerned and the only time I regularly rebase the SELinux and audit trees is after the merge window (e.g. we need something in -rc1, or we are simply too far out of date). Looking quickly at net-next, it looks like net-next/master is refreshed/rebased on a regular basis too (it contains the selinux-pr-20180130 tag)... and perhaps rebase is a term you don't want to use, but I think we are on the same page here. > My tree often goes in reasonable early in the merge window. Generally speaking I send my pull request to Linus early in the merge window too. It obviously tends to vary on when he does the pull, but we generally haven't had any major problems. > So you would only have to wait until my tree went in before > sending your pull request. So you would want me to rebase selinux/next on top of Linus' tree in the middle of the merge window? I'm sure that isn't what you meant, but that's how I keep reading the above ... which can't be right, because in my experience that's one way to piss off Linus. Help me understand what you are saying. > That's really the way to handle something like this. -- paul moore www.paul-moore.com
Re: linux-next: manual merge of the selinux tree with the net-next tree
On Wed, Mar 7, 2018 at 3:26 PM, David Miller wrote: > From: Paul Moore > Date: Wed, 7 Mar 2018 15:20:33 -0500 > >>> So you would only have to wait until my tree went in before >>> sending your pull request. >> >> So you would want me to rebase selinux/next on top of Linus' tree in >> the middle of the merge window? I'm sure that isn't what you meant, >> but that's how I keep reading the above ... which can't be right, >> because in my experience that's one way to piss off Linus. Help me >> understand what you are saying. > > I never said you rebase anything. I wonder where you get that from. As I said, I was just trying to figure out what you were suggesting. Your email was not very clear in my opinion. > I'm saying, you just defer your pull request until Linus takes my > networking tree in. > > No changes or rebasing of your tree is necessary whatsoever. You just > ask him to pull your tree as-is. > > Again, this is what other smaller subsystem trees do when they have a > situation like this. Which gets us back to what I originally suggested in my first email of this thread: linux-next carries the fixup patch and when we send the pull requests to Linus we mention this fixup/thread. For what it's worth, if you mention the potential merge conflict, and the fixup that Stephen provided, it shouldn't matter when the pull requests are sent to Linus; he's a smart guy, he'll merge things in the order he wants. I've seen more than a few people get burned by deferring pull requests, I don't intend to have SELinux, or audit for that matter, run into the same problem. -- paul moore www.paul-moore.com
