[PATCH] SELinux: cleanup ipc_has_perm
This patch removes the sclass argument from ipc_has_perm in the
SELinux module, as it can be obtained from the ipc security structure.
The use of a separate argument was a legacy of the older precondition
function handling in SELinux and is obsolete. Please apply.
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
Signed-off-by: James Morris <[EMAIL PROTECTED]>
---
security/selinux/hooks.c | 21 -
1 files changed, 8 insertions(+), 13 deletions(-)
= security/selinux/hooks.c 1.95 vs edited =
--- 1.95/security/selinux/hooks.c 2005-04-01 16:30:16 -05:00
+++ edited/security/selinux/hooks.c 2005-04-08 10:37:42 -04:00
@@ -3666,7 +3666,7 @@ static void msg_msg_free_security(struct
}
static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
- u16 sclass, u32 perms)
+ u32 perms)
{
struct task_security_struct *tsec;
struct ipc_security_struct *isec;
@@ -3678,7 +3678,7 @@ static int ipc_has_perm(struct kern_ipc_
AVC_AUDIT_DATA_INIT(&ad, IPC);
ad.u.ipc_id = ipc_perms->key;
- return avc_has_perm(tsec->sid, isec->sid, sclass, perms, &ad);
+ return avc_has_perm(tsec->sid, isec->sid, isec->sclass, perms, &ad);
}
static int selinux_msg_msg_alloc_security(struct msg_msg *msg)
@@ -3763,7 +3763,7 @@ static int selinux_msg_queue_msgctl(stru
return 0;
}
- err = ipc_has_perm(&msq->q_perm, SECCLASS_MSGQ, perms);
+ err = ipc_has_perm(&msq->q_perm, perms);
return err;
}
@@ -3915,7 +3915,7 @@ static int selinux_shm_shmctl(struct shm
return 0;
}
- err = ipc_has_perm(&shp->shm_perm, SECCLASS_SHM, perms);
+ err = ipc_has_perm(&shp->shm_perm, perms);
return err;
}
@@ -3934,7 +3934,7 @@ static int selinux_shm_shmat(struct shmi
else
perms = SHM__READ | SHM__WRITE;
- return ipc_has_perm(&shp->shm_perm, SECCLASS_SHM, perms);
+ return ipc_has_perm(&shp->shm_perm, perms);
}
/* Semaphore security operations */
@@ -4023,7 +4023,7 @@ static int selinux_sem_semctl(struct sem
return 0;
}
- err = ipc_has_perm(&sma->sem_perm, SECCLASS_SEM, perms);
+ err = ipc_has_perm(&sma->sem_perm, perms);
return err;
}
@@ -4037,18 +4037,13 @@ static int selinux_sem_semop(struct sem_
else
perms = SEM__READ;
- return ipc_has_perm(&sma->sem_perm, SECCLASS_SEM, perms);
+ return ipc_has_perm(&sma->sem_perm, perms);
}
static int selinux_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
{
- struct ipc_security_struct *isec = ipcp->security;
- u16 sclass = SECCLASS_IPC;
u32 av = 0;
- if (isec && isec->magic == SELINUX_MAGIC)
- sclass = isec->sclass;
-
av = 0;
if (flag & S_IRUGO)
av |= IPC__UNIX_READ;
@@ -4058,7 +4053,7 @@ static int selinux_ipc_permission(struct
if (av == 0)
return 0;
- return ipc_has_perm(ipcp, sclass, av);
+ return ipc_has_perm(ipcp, av);
}
/* module stacking operations */
--
Stephen Smalley <[EMAIL PROTECTED]>
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] SELinux: fix deadlock on dcache lock
This patch fixes a deadlock on the dcache lock detected during testing
at IBM by moving the logging of the current executable information
from the SELinux avc_audit function to audit_log_exit (via an
audit_log_task_info helper) for processing upon syscall exit. For
consistency, the patch also removes the logging of other task-related
information from avc_audit, deferring handling to audit_log_exit
instead. This allows simplification of the avc_audit code, allows the
exe information to be obtained more reliably, always includes the comm
information (useful for scripts), and avoids including bogus task
information for checks performed from irq or softirq. Please apply.
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
Signed-off-by: James Morris <[EMAIL PROTECTED]>
--
kernel/auditsc.c | 28
security/selinux/avc.c | 34 --
2 files changed, 28 insertions(+), 34 deletions(-)
= kernel/auditsc.c 1.10 vs edited =
--- 1.10/kernel/auditsc.c 2005-03-17 11:33:20 -05:00
+++ edited/kernel/auditsc.c 2005-04-15 09:22:15 -04:00
@@ -610,6 +610,33 @@
printk(KERN_ERR "audit: freed %d contexts\n", count);
}
+static void audit_log_task_info(struct audit_buffer *ab)
+{
+ char name[sizeof(current->comm)];
+ struct mm_struct *mm = current->mm;
+ struct vm_area_struct *vma;
+
+ get_task_comm(name, current);
+ audit_log_format(ab, " comm=%s", name);
+
+ if (!mm)
+ return;
+
+ down_read(&mm->mmap_sem);
+ vma = mm->mmap;
+ while (vma) {
+ if ((vma->vm_flags & VM_EXECUTABLE) &&
+ vma->vm_file) {
+ audit_log_d_path(ab, "exe=",
+vma->vm_file->f_dentry,
+vma->vm_file->f_vfsmnt);
+ break;
+ }
+ vma = vma->vm_next;
+ }
+ up_read(&mm->mmap_sem);
+}
+
static void audit_log_exit(struct audit_context *context)
{
int i;
@@ -639,6 +666,7 @@
context->gid,
context->euid, context->suid, context->fsuid,
context->egid, context->sgid, context->fsgid);
+ audit_log_task_info(ab);
audit_log_end(ab);
while (context->aux) {
struct audit_aux_data *aux;
= security/selinux/avc.c 1.23 vs edited =
--- 1.23/security/selinux/avc.c 2005-03-28 17:21:18 -05:00
+++ edited/security/selinux/avc.c 2005-04-15 09:22:16 -04:00
@@ -532,7 +532,6 @@
u16 tclass, u32 requested,
struct av_decision *avd, int result, struct avc_audit_data *a)
{
- struct task_struct *tsk = current;
struct inode *inode = NULL;
u32 denied, audited;
struct audit_buffer *ab;
@@ -556,39 +555,6 @@
audit_log_format(ab, "avc: %s ", denied ? "denied" : "granted");
avc_dump_av(ab, tclass,audited);
audit_log_format(ab, " for ");
- if (a && a->tsk)
- tsk = a->tsk;
- if (tsk && tsk->pid) {
- struct mm_struct *mm;
- struct vm_area_struct *vma;
- audit_log_format(ab, " pid=%d", tsk->pid);
- if (tsk == current)
- mm = current->mm;
- else
- mm = get_task_mm(tsk);
- if (mm) {
- if (down_read_trylock(&mm->mmap_sem)) {
- vma = mm->mmap;
- while (vma) {
- if ((vma->vm_flags & VM_EXECUTABLE) &&
- vma->vm_file) {
- audit_log_d_path(ab, "exe=",
- vma->vm_file->f_dentry,
- vma->vm_file->f_vfsmnt);
- break;
- }
- vma = vma->vm_next;
- }
- up_read(&mm->mmap_sem);
- } else {
- audit_log_format(ab, " comm=%s", tsk->comm);
- }
- if (tsk != current)
- mmput(mm);
- } else {
- audit_log_format(ab, " comm=%s", tsk->comm);
- }
- }
if (a) {
switch (a->type) {
case AVC_AUDIT_DATA_IPC:
--
Stephen Smalley <[EMAIL PROTECTED]>
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/7] procfs privacy
On Mon, 2005-04-18 at 22:18 +0200, Lorenzo HernÃndez GarcÃa-Hierro wrote: > For this purpose I (re)submitted a patch originally made by Serge E. > Hallyn that adds a hook in order to catch task lookups, thus, providing > an easy way to handle and determine when a task can lookup'ed. > > It's at: > http://pearls.tuxedo-es.org/patches/lsm/lsm-task_lookup-hook.patch > > vSecurity currently provides support for it (optional). > > SELinux policy can handle in a much more fine-grained these > restrictions, just that it's still something that not all people can > deploy without some special effort and "tweak up" (if their system > doesn't provide support for it, of course, currently Red Hat has done a > great job in that terms). To be precise, SELinux assigns security labels to /proc inodes (/proc/pid inodes are labeled based on the associated task label, and other /proc inodes are labeled based on the policy configuration), and controls access based on the policy. It can e.g. prevent a process in one security domain from accessing anything under /proc/ for a process in a different domain, but not from seeing the top-level entry in /proc itself (as it doesn't do any kind of directory filtering). -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH][SELINUX] Fix selinux_inode_setattr hook
This patch against 2.6.11-rc3 fixes the selinux_inode_setattr hook function to honor the ATTR_FORCE flag, skipping any permission checking in that case. Otherwise, it is possible though unlikely for a denial from the hook to prevent proper updating, e.g. for remove_suid upon writing to a file. This would only occur if the process had write permission to a suid file but lacked setattr permission to it. Please apply. Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]> Signed-off-by: James Morris <[EMAIL PROTECTED]> security/selinux/hooks.c |3 +++ 1 files changed, 3 insertions(+) Index: linux-2.6/security/selinux/hooks.c === RCS file: /nfshome/pal/CVS/linux-2.6/security/selinux/hooks.c,v retrieving revision 1.150 diff -u -p -r1.150 hooks.c --- linux-2.6/security/selinux/hooks.c 26 Jan 2005 21:20:59 - 1.150 +++ linux-2.6/security/selinux/hooks.c 4 Feb 2005 16:39:23 - @@ -2142,6 +2142,9 @@ static int selinux_inode_setattr(struct if (rc) return rc; + if (iattr->ia_valid & ATTR_FORCE) + return 0; + if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_ATIME_SET | ATTR_MTIME_SET)) return dentry_has_perm(current, NULL, dentry, FILE__SETATTR); -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][SELINUX] Fix selinux_inode_setattr hook
On Fri, 2005-02-04 at 13:14, Chris Wright wrote: > * Stephen Smalley ([EMAIL PROTECTED]) wrote: > > This patch against 2.6.11-rc3 fixes the selinux_inode_setattr hook > > function to honor the ATTR_FORCE flag, skipping any permission checking > > in that case. Otherwise, it is possible though unlikely for a denial > > from the hook to prevent proper updating, e.g. for remove_suid upon > > writing to a file. This would only occur if the process had write > > permission to a suid file but lacked setattr permission to it. Please > > apply. > > Is there any reason not to promote this to the framework? I wasn't sure if a security module might still want to be notified of forced changes (e.g. to adjust some state in its own security structure), even if it skips permission checking on such changes. -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] Real-Time Preemption, -RT-2.6.11-rc3-V0.7.38-01
On Tue, 2005-02-08 at 16:58, William Weston wrote: > Hi Ingo, > > Great work on the -RT kernel! Here's a status report from my Athlon box > w/ kernel -RT-2.6.11-rc3-V0.7.38-03, realtime-lsm-0.8.5, jack-0.99.48, > alsa-1.0.8, and latencytest-0.5.5: > A couple BUGs are being logged (see below), but without any ill effect > other than taking up space on my /var. > Network interface (via rhine) startup triggers these two BUGs: > > BUG: sleeping function called from invalid context ksoftirqd/0(2) at > kernel/rt.c:1448 > in_atomic():1 [0001], irqs_disabled():0 > [] dump_stack+0x17/0x20 (12) > [] __might_sleep+0xd9/0xf0 (40) > [] __spin_lock+0x36/0x50 (24) > [] kmem_cache_alloc+0x34/0x120 (44) > [] sel_netif_lookup+0x63/0x150 (28) > [] sel_netif_sids+0x2d/0xb0 (28) > [] selinux_socket_sock_rcv_skb+0xac/0x230 (144) I'm not sure I understand, as sel_netif_lookup passes GFP_ATOMIC to kmalloc. > [] udp_queue_rcv_skb+0xb8/0x280 (28) > [] udp_rcv+0x192/0x3e0 (100) > [] ip_local_deliver+0x64/0x1c0 (32) > [] ip_rcv+0x215/0x3f0 (56) > [] netif_receive_skb+0x12c/0x160 (40) > [] process_backlog+0x7e/0x110 (32) > [] net_rx_action+0x72/0x130 (24) > [] ___do_softirq+0x48/0xd0 (40) > [] _do_softirq+0x1b/0x30 (8) > [] ksoftirqd+0xa0/0xf0 (28) > [] kthread+0x8b/0xc0 (36) > [] kernel_thread_helper+0x5/0x10 (537116692) > --- > | preempt count: 0002 ] > | 2-level deep critical section nesting: > > .. [] __do_IRQ+0xef/0x180 > .[] .. ( <= do_IRQ+0x56/0xa0) > .. [] print_traces+0x10/0x40 > .[] .. ( <= dump_stack+0x17/0x20) -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[patch] selinux: Fix address length checks in connect hook
This patch fixes the address length checks in the selinux_socket_connect
hook to be no more restrictive than the underlying ipv4 and ipv6 code;
otherwise, this hook can reject valid connect calls. This patch is in
response to a bug report where an application was calling connect on an
INET6 socket with an address that didn't include the optional scope id
and failing due to these checks. Please apply. To 2.6.13, if possible.
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
Signed-off-by: James Morris <[EMAIL PROTECTED]>
---
security/selinux/hooks.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff -rup linux-2.6.13-rc3-mm3/security/selinux/hooks.c
linux-2.6.13-rc3-mm3-fix/security/selinux/hooks.c
--- linux-2.6.13-rc3-mm3/security/selinux/hooks.c 2005-07-28
14:59:58.0 -0400
+++ linux-2.6.13-rc3-mm3-fix/security/selinux/hooks.c 2005-07-28
14:56:58.0 -0400
@@ -3073,12 +3073,12 @@ static int selinux_socket_connect(struct
if (sk->sk_family == PF_INET) {
addr4 = (struct sockaddr_in *)address;
- if (addrlen != sizeof(struct sockaddr_in))
+ if (addrlen < sizeof(struct sockaddr_in))
return -EINVAL;
snum = ntohs(addr4->sin_port);
} else {
addr6 = (struct sockaddr_in6 *)address;
- if (addrlen != sizeof(struct sockaddr_in6))
+ if (addrlen < SIN6_LEN_RFC2133)
return -EINVAL;
snum = ntohs(addr6->sin6_port);
}
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] securityfs
On Wed, 2005-07-06 at 02:52 -0400, James Morris wrote: > On Sun, 3 Jul 2005, Greg KH wrote: > > > Good idea. Here's a patch to do just that (compile tested only...) > > > > Comments? > > Looks promising so far. > > I'm currently porting selinuxfs funtionality to securityfs, although I'm > not sure if we'll be ok during early initialization. selinuxfs is > currently kern_mounted via an initcall. We may need an initcall > kern_mount() of securityfs before SELinux kicks in. > > Stephen: opinions on this? The reason for creating a kernel mount of selinuxfs at that point is so that the selinuxfs_mount vfsmount and selinux_null dentry are available for flush_unauthorized_files to use. > Otherwise, it looks like it'll allow SELinux to drop some code. Generally > it will mean that other LSM components won't have to create their own > filesystems, and their subdirectories will be hanging off /security (or > wherever selinuxfs is mounted), rather than scattered across / > > Some of the SELinux code may be useful as part of securityfs later, as > well. We need to be able to assign specific security labels to specific inodes in selinuxfs, particularly for the booleans, but ideally for any of them. Userspace compatibility is obviously a concern for such a change. libselinux determines where selinuxfs is mounted during library initialization by checking /proc/mounts for selinuxfs, and rc.sysinit does likewise. /sbin/init performs the initial mount of selinuxfs prior to initial policy load. Further, the existence of selinuxfs in /proc/filesystems is used as a test of whether SELinux was enabled in the kernel (e.g. is_selinux_enabled in libselinux). I'm not sure such a change is worthwhile for SELinux; large amount of disruption for little real gain. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] securityfs
On Wed, 2005-07-06 at 11:35 -0400, James Morris wrote: > When exactly is this needed? The securityfs mountpoint will be available > via a core_initcall, after which we can initialize the selinux subtree. As long as it occurs prior to initial policy load, so that should be fine. > With securityfs, we'd have /sys/kernel/security/selinux configured during > kernel initialization. It still has to be mounted by userspace, right? So /sbin/init still needs to know to mount securityfs, and where the selinux nodes live under it, so you are still talking about changing it (and libselinux and rc.sysinit), and risking compatibility breakage for existing systems when they update their kernels. > Could be a simple change to look for the presence of > /sys/kernel/security/selinux Slightly different. That would correspond to checking for the presence of selinuxfs in /proc/mounts (i.e. has it been mounted), vs. the current check of selinuxfs in /proc/filesystems (i.e. has it been registered). The latter allows testing whether SELinux is enabled at all in the kernel, regardless of whether selinuxfs has been mounted in the process' namespace. In practice, the difference likely only matters for init and you can deal with distinction there. But again, this requires code change to libselinux, /sbin/init, and rc.sysinit at least, with coordinated update with the kernel. Certainly possible, but experience suggests it isn't likely to go smoothly. > I think it should reduce and simplify the SELinux kernel code, with less > filesystems in the kernel, consolidating several potential projects into > the same security filesystem. If there are several such projects in the first place... -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 5/12] lsm stacking v0.2: actual stacker module
On Thu, 2005-06-30 at 14:50 -0500, [EMAIL PROTECTED] wrote:
> Adds the actual stacker LSM.
> +static int stacker_inode_getsecurity(struct inode *inode, const char *name,
> void *buffer, size_t size)
> +{
> +
> RETURN_ERROR_IF_ANY_ERROR(inode_getsecurity,inode_getsecurity(inode,name,buffer,size));
> +}
> +
> +static int stacker_inode_setsecurity(struct inode *inode, const char *name,
> const void *value, size_t size, int flags)
> +{
> +
> RETURN_ERROR_IF_ANY_ERROR(inode_setsecurity,inode_setsecurity(inode,name,value,size,flags));
> +}
> +
> +static int stacker_inode_listsecurity(struct inode *inode, char *buffer,
> size_t buffer_size)
> +{
> +
> RETURN_ERROR_IF_ANY_ERROR(inode_listsecurity,inode_listsecurity(inode,buffer,
> buffer_size));
> +}
These hooks pose a similar problem for stacking as with the
[gs]etprocattr hooks, although [gs]etsecurity have the benefit of
already taking a distinguishing name suffix (the part after the
security. prefix). Note also that inode_getsecurity returns the number
of bytes used/required on success.
The proposed inode_init_security hook will likewise have an issue for
stacking.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 5/12] lsm stacking v0.2: actual stacker module
On Mon, 2005-07-11 at 12:51 -0500, [EMAIL PROTECTED] wrote: > I can imagine a few ways of fixing this: > > 1. We simply expect that only one module use xattrs. This > is probably unacceptable, as we will want both EVM and selinux > to store xattrs. Note that these particular hooks are only used for filesystems like devpts and tmpfs where there is no underlying storage for the security xattrs but we still need a way to [gs]et the incore inode security label from userspace. > 2. A module registers an xattr name when it registers > itself. Then only the registered module is consulted on one of > these calls. If no module is registered, all are consulted as > they are now. SELinux already checks the name suffix in inode_getsecurity and inode_setsecurity, and returns -EOPNOTSUPP if it isn't selinux. Hence, stacker could just iterate through the modules until it gets a result other than -EOPNOTSUPP, relying on the modules to check the name. listsecurity is different, as it is supposed to yield the list of attribute names concatenated together, but that shouldn't be difficult for stacker to construct, similar to your getprocattr logic but without the need to add tags. > This prevents a module like capability from deciding > based on its own credentials whether another module's hook > should be called. Is that a good or bad thing? These hooks aren't supposed to be doing permission checking; that is handled by the separate security_inode_*xattr hooks. They are just for getting/setting the incore inode security label. > This might have the added bonus of obviating the need > for a separate cap_stack module. I don't think so - different hooks are involved (inode_setxattr vs. inode_setsecurity). -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [2.6 patch] selinux: cleanups
On Sun, 2005-03-13 at 04:01 +0100, Adrian Bunk wrote:
> The patch below contains the following possible cleanups:
> - make needlessly global code static
> - remove the following unused global functions:
> - avc.c: avc_ss_grant
> - avc.c: avc_ss_try_revoke
> - avc.c: avc_ss_revoke
> - avc.c: avc_ss_set_auditallow
> - avc.c: avc_ss_set_auditdeny
> - ss/avtab.c: avtab_map
> - ss/ebitmap.c: ebitmap_or
> - ss/hashtab.c: hashtab_remove
> - ss/hashtab.c: hashtab_replace
> - ss/hashtab.c: hashtab_map_remove_on_error
> - ss/sidtab.c: sidtab_remove
> - remove the following unused static functions:
> - avc.c: avc_update_cache
> - avc.c: avc_control
>
>
> Please review and comment on which of these changes are correct and
> which conflict with pending patches for in-kernel users of the functions
> affected.
>
>
> diffstat output:
> security/selinux/avc.c | 174
> security/selinux/hooks.c| 40 +++---
> security/selinux/include/avc.h |7 -
> security/selinux/include/avc_ss.h | 13 --
> security/selinux/include/objsec.h |2
> security/selinux/include/security.h |3
> security/selinux/selinuxfs.c|4
> security/selinux/ss/avtab.c | 27
> security/selinux/ss/avtab.h |6
> security/selinux/ss/conditional.c |2
> security/selinux/ss/ebitmap.c | 43 --
> security/selinux/ss/ebitmap.h |1
> security/selinux/ss/hashtab.c | 113 --
> security/selinux/ss/hashtab.h | 38 --
> security/selinux/ss/mls.c |2
> security/selinux/ss/policydb.c | 10 -
> security/selinux/ss/policydb.h |3
> security/selinux/ss/services.c | 23 ---
> security/selinux/ss/sidtab.c| 36 -
> 19 files changed, 34 insertions(+), 513 deletions(-)
>
>
> Signed-off-by: Adrian Bunk <[EMAIL PROTECTED]>
Looks fine to me (although your diffstat output is stale). Re-diff
against 2.6.11-mm3 is below, feel free to send along to Andrew Morton.
Acked-by: Stephen Smalley <[EMAIL PROTECTED]>
security/selinux/avc.c| 174 --
security/selinux/hooks.c | 40
security/selinux/include/avc.h|7 -
security/selinux/include/avc_ss.h | 13 --
security/selinux/include/objsec.h |2
security/selinux/selinuxfs.c |4
security/selinux/ss/avtab.c | 29 --
security/selinux/ss/avtab.h |6 -
security/selinux/ss/conditional.c |2
security/selinux/ss/ebitmap.c | 43 -
security/selinux/ss/ebitmap.h |1
security/selinux/ss/hashtab.c | 113
security/selinux/ss/hashtab.h | 38
security/selinux/ss/policydb.c| 10 +-
security/selinux/ss/policydb.h|3
security/selinux/ss/services.c| 18 +--
security/selinux/ss/services.h|6 -
security/selinux/ss/sidtab.c | 36 ---
18 files changed, 42 insertions(+), 503 deletions(-)
diff -X /home/sds/dontdiff -rup linux-2.6.11-mm3/security/selinux/avc.c
linux-2.6.11-mm3-adrian/security/selinux/avc.c
--- linux-2.6.11-mm3/security/selinux/avc.c 2005-03-02 02:38:17.0
-0500
+++ linux-2.6.11-mm3-adrian/security/selinux/avc.c 2005-03-14
14:08:28.0 -0500
@@ -139,7 +139,7 @@ static inline int avc_hash(u32 ssid, u32
* @tclass: target security class
* @av: access vector
*/
-void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
+static void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
{
const char **common_pts = NULL;
u32 common_base = 0;
@@ -199,7 +199,7 @@ void avc_dump_av(struct audit_buffer *ab
* @tsid: target security identifier
* @tclass: target security class
*/
-void avc_dump_query(struct audit_buffer *ab, u32 ssid, u32 tsid, u16 tclass)
+static void avc_dump_query(struct audit_buffer *ab, u32 ssid, u32 tsid, u16
tclass)
{
int rc;
char *scontext;
@@ -828,136 +828,6 @@ out:
return rc;
}
-static int avc_update_cache(u32 event, u32 ssid, u32 tsid,
-u16 tclass, u32 perms)
-{
- struct avc_node *node;
- int i;
-
- rcu_read_lock();
-
- if (ssid == SECSID_WILD || tsid == SECSID_WILD) {
- /* apply to all matching nodes */
- for (i = 0; i < AVC_CACHE_SLOTS; i++) {
- list_for_each_entry_rcu(node, &avc_cache.slots[i],
list) {
- if (avc_sidcmp(ssid, node->ae.ssid) &&
- avc_sidcmp(tsid, node->ae.tsid) &&
- tclass == node->
[PATCH][SELINUX] Allow mounting of filesystems with invalid root inode context
This patch alters the SELinux handling of inodes with invalid security contexts so that a filesystem with a root inode that has an invalid security context can still be mounted for administrative recovery without disabling SELinux altogether. Please apply. Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]> Signed-off-by: James Morris <[EMAIL PROTECTED]> security/selinux/hooks.c |4 +++- 1 files changed, 3 insertions(+), 1 deletion(-) Index: linux-2.6/security/selinux/hooks.c === RCS file: /nfshome/pal/CVS/linux-2.6/security/selinux/hooks.c,v retrieving revision 1.157 diff -u -p -r1.157 hooks.c --- linux-2.6/security/selinux/hooks.c 14 Mar 2005 19:56:52 - 1.157 +++ linux-2.6/security/selinux/hooks.c 18 Mar 2005 20:39:03 - @@ -828,7 +828,9 @@ static int inode_doinit_with_dentry(stru __FUNCTION__, context, -rc, inode->i_sb->s_id, inode->i_ino); kfree(context); - goto out; + /* Leave with the unlabeled SID */ + rc = 0; + break; } } kfree(context); -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH][SELINUX] Make code static and remove unused code
This patch from Adrian Bunk makes needlessly global code static and
removes a number of unused global and static functions from SELinux.
Please apply.
Author: Adrian Bunk <[EMAIL PROTECTED]>
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
security/selinux/avc.c| 174 --
security/selinux/hooks.c | 40
security/selinux/include/avc.h|7 -
security/selinux/include/avc_ss.h | 13 --
security/selinux/include/objsec.h |2
security/selinux/selinuxfs.c |4
security/selinux/ss/avtab.c | 29 --
security/selinux/ss/avtab.h |6 -
security/selinux/ss/conditional.c |2
security/selinux/ss/ebitmap.c | 43 -
security/selinux/ss/ebitmap.h |1
security/selinux/ss/hashtab.c | 113
security/selinux/ss/hashtab.h | 38
security/selinux/ss/policydb.c| 10 +-
security/selinux/ss/policydb.h|3
security/selinux/ss/services.c| 18 +--
security/selinux/ss/services.h|6 -
security/selinux/ss/sidtab.c | 36 ---
18 files changed, 42 insertions(+), 503 deletions(-)
diff -X /home/sds/dontdiff -rup linux-2.6.11-mm3/security/selinux/avc.c
linux-2.6.11-mm3-adrian/security/selinux/avc.c
--- linux-2.6.11-mm3/security/selinux/avc.c 2005-03-02 02:38:17.0
-0500
+++ linux-2.6.11-mm3-adrian/security/selinux/avc.c 2005-03-14
14:08:28.0 -0500
@@ -139,7 +139,7 @@ static inline int avc_hash(u32 ssid, u32
* @tclass: target security class
* @av: access vector
*/
-void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
+static void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
{
const char **common_pts = NULL;
u32 common_base = 0;
@@ -199,7 +199,7 @@ void avc_dump_av(struct audit_buffer *ab
* @tsid: target security identifier
* @tclass: target security class
*/
-void avc_dump_query(struct audit_buffer *ab, u32 ssid, u32 tsid, u16 tclass)
+static void avc_dump_query(struct audit_buffer *ab, u32 ssid, u32 tsid, u16
tclass)
{
int rc;
char *scontext;
@@ -828,136 +828,6 @@ out:
return rc;
}
-static int avc_update_cache(u32 event, u32 ssid, u32 tsid,
-u16 tclass, u32 perms)
-{
- struct avc_node *node;
- int i;
-
- rcu_read_lock();
-
- if (ssid == SECSID_WILD || tsid == SECSID_WILD) {
- /* apply to all matching nodes */
- for (i = 0; i < AVC_CACHE_SLOTS; i++) {
- list_for_each_entry_rcu(node, &avc_cache.slots[i],
list) {
- if (avc_sidcmp(ssid, node->ae.ssid) &&
- avc_sidcmp(tsid, node->ae.tsid) &&
- tclass == node->ae.tclass ) {
- avc_update_node(event, perms,
node->ae.ssid,
- node->ae.tsid,
node->ae.tclass);
- }
- }
- }
- } else {
- /* apply to one node */
- avc_update_node(event, perms, ssid, tsid, tclass);
- }
-
- rcu_read_unlock();
-
- return 0;
-}
-
-static int avc_control(u32 event, u32 ssid, u32 tsid,
- u16 tclass, u32 perms,
- u32 seqno, u32 *out_retained)
-{
- struct avc_callback_node *c;
- u32 tretained = 0, cretained = 0;
- int rc = 0;
-
- /*
-* try_revoke only removes permissions from the cache
-* state if they are not retained by the object manager.
-* Hence, try_revoke must wait until after the callbacks have
-* been invoked to update the cache state.
-*/
- if (event != AVC_CALLBACK_TRY_REVOKE)
- avc_update_cache(event,ssid,tsid,tclass,perms);
-
- for (c = avc_callbacks; c; c = c->next)
- {
- if ((c->events & event) &&
- avc_sidcmp(c->ssid, ssid) &&
- avc_sidcmp(c->tsid, tsid) &&
- c->tclass == tclass &&
- (c->perms & perms)) {
- cretained = 0;
- rc = c->callback(event, ssid, tsid, tclass,
-(c->perms & perms),
-&cretained);
- if (rc)
- goto out;
- tretained |= cretained;
- }
- }
-
- if (event == AVC_CALLBACK_TRY_REVOKE) {
- /* revoke any unretained permissions */
- perms &= ~tretained;
- avc_update_cache(event,ssid,tsid,tclass,perms);
- *out_retained = tretained;
- }
-
-
[PATCH][SELINUX] Audit unrecognized netlink messages
This patch changes SELinux to audit any unrecognized netlink messages
in controlled classes rather than silently rejecting them, and to
allow them if in permissive mode. Please apply.
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
Signed-off-by: James Morris <[EMAIL PROTECTED]>
security/selinux/hooks.c | 10 ++
1 files changed, 10 insertions(+)
Index: linux-2.6/security/selinux/hooks.c
===
RCS file: /nfshome/pal/CVS/linux-2.6/security/selinux/hooks.c,v
retrieving revision 1.158
diff -u -p -r1.158 hooks.c
--- linux-2.6/security/selinux/hooks.c 21 Mar 2005 15:56:37 - 1.158
+++ linux-2.6/security/selinux/hooks.c 21 Mar 2005 16:24:06 -
@@ -67,6 +67,7 @@
#include
#include
#include
+#include
#include "avc.h"
#include "objsec.h"
@@ -3377,6 +3378,15 @@ static int selinux_nlmsg_perm(struct soc
err = selinux_nlmsg_lookup(isec->sclass, nlh->nlmsg_type, &perm);
if (err) {
+ if (err == -EINVAL) {
+ audit_log(current->audit_context,
+ "SELinux: unrecognized netlink message"
+ " type=%hu for sclass=%hu\n",
+ nlh->nlmsg_type, isec->sclass);
+ if (!selinux_enforcing)
+ err = 0;
+ }
+
/* Ignore */
if (err == -ENOENT)
err = 0;
--
Stephen Smalley <[EMAIL PROTECTED]>
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] don't do pointless NULL checks and casts before kfree() in security/
On Sun, 2005-03-20 at 13:29 +0100, Jesper Juhl wrote: > kfree() handles NULL pointers, so checking a pointer for NULL before > calling kfree() on it is pointless. kfree() takes a void* argument and > changing the type of a pointer before kfree()'ing it is equally pointless. > This patch removes the pointless checks for NULL and needless mucking > about with the pointer types before kfree() for all files in security/* > where I could locate such code. > > The following files are modified by this patch: > security/keys/key.c > security/keys/user_defined.c > security/selinux/hooks.c > security/selinux/selinuxfs.c > security/selinux/ss/conditional.c > security/selinux/ss/policydb.c > security/selinux/ss/services.c > > (please keep me on CC if you reply) > > > Signed-off-by: Jesper Juhl <[EMAIL PROTECTED]> The diffs to selinux look fine to me, and the resulting kernel seems to be operating without problem. Feel free to send along to Andrew Morton. Acked-by: Stephen Smalley <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4 with proper signed-off] security/selinux/ss/policydb.c: fix sparse warnings
On Sun, 2005-03-20 at 12:59 +0100, Domen Puncer wrote:
> Signed-off-by: Alexey Dobriyan <[EMAIL PROTECTED]>
> Signed-off-by: Domen Puncer <[EMAIL PROTECTED]>
> ---
>
>
> kj-domen/security/selinux/ss/policydb.c | 35
> ++--
> 1 files changed, 20 insertions(+), 15 deletions(-)
>
> diff -puN security/selinux/ss/policydb.c~sparse-security_selinux_ss_policydb
> security/selinux/ss/policydb.c
> --- kj/security/selinux/ss/policydb.c~sparse-security_selinux_ss_policydb
> 2005-03-20 12:11:25.0 +0100
> +++ kj-domen/security/selinux/ss/policydb.c 2005-03-20 12:11:25.0
> +0100
> @@ -1494,9 +1497,11 @@ int policydb_read(struct policydb *p, vo
> goto bad;
> }
>
> - if (buf[2] != info->sym_num || buf[3] != info->ocon_num) {
> + if (le32_to_cpu(buf[2]) != info->sym_num ||
> + le32_to_cpu(buf[3]) != info->ocon_num) {
> printk(KERN_ERR "security: policydb table sizes (%d,%d) do "
> -"not match mine (%d,%d)\n", buf[2], buf[3],
> +"not match mine (%d,%d)\n",
> +le32_to_cpu(buf[2]), le32_to_cpu(buf[3]),
> info->sym_num, info->ocon_num);
> goto bad;
> }
> _
You didn't remove the loop that already converted these values to little
endian already (no that isn't the same as the earlier loop that you did
remove), so now you are converting them twice. And why is this new code
better even if you fix this omission?
--
Stephen Smalley <[EMAIL PROTECTED]>
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/4 with proper signed-off] security/selinux/ss/policydb.c: fix sparse warnings
On Tue, 2005-03-22 at 10:19 -0500, Stephen Smalley wrote: > You didn't remove the loop that already converted these values to little s/ to / from / > endian already (no that isn't the same as the earlier loop that you did > remove), so now you are converting them twice. And why is this new code > better even if you fix this omission? Note btw that you would also need to modify usage of buf[0] and buf[1] if you do remove that loop. But I'm still not clear on the benefit of the change (silencing warnings generated by a checker doesn't count unless they point to a real bug). -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH][SELINUX] Add name_connect permission check
This patch adds a name_connect permission check to SELinux to provide
control over outbound TCP connections to particular ports distinct
from the general controls over sending and receiving packets. Please
apply.
security/selinux/hooks.c | 48 ++-
security/selinux/include/av_perm_to_string.h |1
security/selinux/include/av_permissions.h|1
3 files changed, 49 insertions(+), 1 deletion(-)
Index: linux-2.6/security/selinux/hooks.c
===
RCS file: /nfshome/pal/CVS/linux-2.6/security/selinux/hooks.c,v
retrieving revision 1.160
diff -u -p -r1.160 hooks.c
--- linux-2.6/security/selinux/hooks.c 22 Mar 2005 17:30:12 - 1.160
+++ linux-2.6/security/selinux/hooks.c 23 Mar 2005 14:17:26 -
@@ -3085,7 +3085,53 @@ out:
static int selinux_socket_connect(struct socket *sock, struct sockaddr
*address, int addrlen)
{
- return socket_has_perm(current, sock, SOCKET__CONNECT);
+ struct inode_security_struct *isec;
+ int err;
+
+ err = socket_has_perm(current, sock, SOCKET__CONNECT);
+ if (err)
+ return err;
+
+ /*
+* If a TCP socket, check name_connect permission for the port.
+*/
+ isec = SOCK_INODE(sock)->i_security;
+ if (isec->sclass == SECCLASS_TCP_SOCKET) {
+ struct sock *sk = sock->sk;
+ struct avc_audit_data ad;
+ struct sockaddr_in *addr4 = NULL;
+ struct sockaddr_in6 *addr6 = NULL;
+ unsigned short snum;
+ u32 sid;
+
+ if (sk->sk_family == PF_INET) {
+ addr4 = (struct sockaddr_in *)address;
+ if (addrlen != sizeof(struct sockaddr_in))
+ return -EINVAL;
+ snum = ntohs(addr4->sin_port);
+ } else {
+ addr6 = (struct sockaddr_in6 *)address;
+ if (addrlen != sizeof(struct sockaddr_in6))
+ return -EINVAL;
+ snum = ntohs(addr6->sin6_port);
+ }
+
+ err = security_port_sid(sk->sk_family, sk->sk_type,
+ sk->sk_protocol, snum, &sid);
+ if (err)
+ goto out;
+
+ AVC_AUDIT_DATA_INIT(&ad,NET);
+ ad.u.net.dport = htons(snum);
+ ad.u.net.family = sk->sk_family;
+ err = avc_has_perm(isec->sid, sid, isec->sclass,
+ TCP_SOCKET__NAME_CONNECT, &ad);
+ if (err)
+ goto out;
+ }
+
+out:
+ return err;
}
static int selinux_socket_listen(struct socket *sock, int backlog)
Index: linux-2.6/security/selinux/include/av_perm_to_string.h
===
RCS file:
/nfshome/pal/CVS/linux-2.6/security/selinux/include/av_perm_to_string.h,v
retrieving revision 1.23
diff -u -p -r1.23 av_perm_to_string.h
--- linux-2.6/security/selinux/include/av_perm_to_string.h 23 Feb 2005
20:26:54 - 1.23
+++ linux-2.6/security/selinux/include/av_perm_to_string.h 22 Mar 2005
20:29:05 -
@@ -25,6 +25,7 @@
S_(SECCLASS_TCP_SOCKET, TCP_SOCKET__NEWCONN, "newconn")
S_(SECCLASS_TCP_SOCKET, TCP_SOCKET__ACCEPTFROM, "acceptfrom")
S_(SECCLASS_TCP_SOCKET, TCP_SOCKET__NODE_BIND, "node_bind")
+ S_(SECCLASS_TCP_SOCKET, TCP_SOCKET__NAME_CONNECT, "name_connect")
S_(SECCLASS_UDP_SOCKET, UDP_SOCKET__NODE_BIND, "node_bind")
S_(SECCLASS_RAWIP_SOCKET, RAWIP_SOCKET__NODE_BIND, "node_bind")
S_(SECCLASS_NODE, NODE__TCP_RECV, "tcp_recv")
Index: linux-2.6/security/selinux/include/av_permissions.h
===
RCS file: /nfshome/pal/CVS/linux-2.6/security/selinux/include/av_permissions.h,v
retrieving revision 1.22
diff -u -p -r1.22 av_permissions.h
--- linux-2.6/security/selinux/include/av_permissions.h 23 Feb 2005 20:26:54
- 1.22
+++ linux-2.6/security/selinux/include/av_permissions.h 22 Mar 2005 20:29:05
-
@@ -253,6 +253,7 @@
#define TCP_SOCKET__NEWCONN 0x0080UL
#define TCP_SOCKET__ACCEPTFROM0x0100UL
#define TCP_SOCKET__NODE_BIND 0x0200UL
+#define TCP_SOCKET__NAME_CONNECT 0x0400UL
#define UDP_SOCKET__IOCTL 0x0001UL
#define UDP_SOCKET__READ 0x0002UL
--
Stephen Smalley <[EMAIL PROTECTED]>
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][SELINUX] Add name_connect permission check
On Wed, 2005-03-23 at 09:40 -0500, Stephen Smalley wrote: > This patch adds a name_connect permission check to SELinux to provide > control over outbound TCP connections to particular ports distinct > from the general controls over sending and receiving packets. Please > apply. > > security/selinux/hooks.c | 48 > ++- > security/selinux/include/av_perm_to_string.h |1 > security/selinux/include/av_permissions.h|1 > 3 files changed, 49 insertions(+), 1 deletion(-) Ah, sorry - forgot the Signed-off-by lines. Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]> Signed-off-by: James Morris <[EMAIL PROTECTED]> -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Fix SELinux for removal of i_sock
Hi,
This patch against -bk eliminates the use of i_sock by SELinux as it
appears to have been removed recently, breaking the build of SELinux in
-bk. Simply replacing the i_sock test with an S_ISSOCK test would be
unsafe in the SELinux code, as the latter will also return true for the
inodes of socket files in the filesystem, not just the actual socket
objects IIUC. Hence this patch reworks the SELinux code to avoid the
need to apply such a test in the first place, part of which was
obsoleted anyway by earlier changes to SELinux. Please apply.
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
Signed-off-by: James Morris <[EMAIL PROTECTED]>
security/selinux/hooks.c | 21 +++--
1 files changed, 3 insertions(+), 18 deletions(-)
= security/selinux/hooks.c 1.93 vs edited =
--- 1.93/security/selinux/hooks.c 2005-03-28 17:21:19 -05:00
+++ edited/security/selinux/hooks.c 2005-04-01 15:01:58 -05:00
@@ -877,18 +877,8 @@ static int inode_doinit_with_dentry(stru
isec->initialized = 1;
out:
- if (inode->i_sock) {
- struct socket *sock = SOCKET_I(inode);
- if (sock->sk) {
- isec->sclass =
socket_type_to_security_class(sock->sk->sk_family,
-
sock->sk->sk_type,
-
sock->sk->sk_protocol);
- } else {
- isec->sclass = SECCLASS_SOCKET;
- }
- } else {
+ if (isec->sclass == SECCLASS_FILE)
isec->sclass = inode_mode_to_security_class(inode->i_mode);
- }
if (hold_sem)
up(&isec->sem);
@@ -2979,18 +2969,15 @@ out:
static void selinux_socket_post_create(struct socket *sock, int family,
int type, int protocol, int kern)
{
- int err;
struct inode_security_struct *isec;
struct task_security_struct *tsec;
- err = inode_doinit(SOCK_INODE(sock));
- if (err < 0)
- return;
isec = SOCK_INODE(sock)->i_security;
tsec = current->security;
isec->sclass = socket_type_to_security_class(family, type, protocol);
isec->sid = kern ? SECINITSID_KERNEL : tsec->sid;
+ isec->initialized = 1;
return;
}
@@ -3158,14 +3145,12 @@ static int selinux_socket_accept(struct
if (err)
return err;
- err = inode_doinit(SOCK_INODE(newsock));
- if (err < 0)
- return err;
newisec = SOCK_INODE(newsock)->i_security;
isec = SOCK_INODE(sock)->i_security;
newisec->sclass = isec->sclass;
newisec->sid = isec->sid;
+ newisec->initialized = 1;
return 0;
}
--
Stephen Smalley <[EMAIL PROTECTED]>
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix SELinux for removal of i_sock
On Fri, 2005-04-01 at 12:35 -0800, David S. Miller wrote: > On Fri, 01 Apr 2005 15:06:37 -0500 > Stephen Smalley <[EMAIL PROTECTED]> wrote: > > > This patch against -bk eliminates the use of i_sock by SELinux as it > > appears to have been removed recently, breaking the build of SELinux in > > -bk. Simply replacing the i_sock test with an S_ISSOCK test would be > > unsafe in the SELinux code, as the latter will also return true for the > > inodes of socket files in the filesystem, not just the actual socket > > objects IIUC. Hence this patch reworks the SELinux code to avoid the > > need to apply such a test in the first place, part of which was > > obsoleted anyway by earlier changes to SELinux. Please apply. > > > > Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]> > > Signed-off-by: James Morris <[EMAIL PROTECTED]> > > Applied, thanks Stephen. So, just for clarification, since a S_ISSOCK test is not necessarily equivalent to an i_sock test (in the case of inodes of socket files in the filesystem), was removing i_sock truly the right choice? It may not be an issue for typical users of i_sock since you can't open a descriptor to such a socket file, so any code that was acting on an open file shouldn't have to deal with this ambiguity, but could possibly lead to an erroneous use of SOCKET_I on the inode of a socket file in other code (which is what would have happened in SELinux if we had just changed the i_sock test to an ISSOCK test). Thanks, just trying to avoid confusion in the kernel in the future... -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Thoughts on the "No Linux Security Modules framework" old claims
On Wed, 2005-02-16 at 08:29, Lorenzo HernÃndez GarcÃa-Hierro wrote: > Yes, there are many cases that apply to such scenario and context, this > may be worth to work on, but think it's main shortcoming is that it cuts > performance and adds further overlapping to the DAC checks, that should > be the first ones being called (as most times they do) and then apply > the LSM basis, so, post-processing will be only required if the DAC > checks get in override or passed, without adding too-much overhead to > the current behavior. > > So, I just agree partially, but yes, maybe modifying the DAC checks > themselves and add what-ever-else helper function to handle by-default > auditing in certain operations could be interesting. Audit is being handled by a separate audit framework, not by LSM. There is already support in the Linux 2.6 kernel for auditing at syscall exit (thereby guaranteeing that you capture the final return value in all cases), with the ability of an LSM to enable such auditing for a particular event from its hook functions. Further, there is ongoing work (see the linux-audit mailing list) for a set of audit-related hooks that will allow auditing based on object identity and the requested mode separate from any particular LSM. -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: idr_remove
On Tue, 2005-02-22 at 13:22 -0500, Jim Houston wrote: > I spent time looking at the pty and selinux code yesterday. > I had little luck finding where the selinux code hooks into > the pty code. The call to lookup_one_len() by fs/devpts/inode.c:get_node() ultimately calls permission(...,MAY_EXEC,...) on the devpts root directory, which will call security_inode_permission() -> selinux_inode_permission() to check search access to the directory. Hence, get_node() can fail if SELinux is enabled and the process has no permission at all to search /dev/pts. If it can get that far, then the inode will ultimately have its security label set upon the d_instantiate() call (via security_d_instantiate() -> selinux_d_instantiate()), and be subsequently checked for opens/reads/writes via the selinux_inode_permission() and selinux_file_permission() hook functions. -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Thoughts on the "No Linux Security Modules framework" old claims
On Wed, 2005-02-23 at 13:37 -0800, Crispin Cowan wrote: > You are confused. It is Secure Computing Corporation that holds patents > that threaten SELinux > http://www.securecomputing.com/pdf/Statement_of_Assurance.pdf The NSA made a statement in response to that statement a long time ago, and in any event, the patents in question have expired AFAICS. -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SELinux: Leak in error path
On Tue, 2005-03-01 at 01:32 +0100, Alexander Nyberg wrote: > There's a leak here in the first error path. > > Found by the Coverity tool. > > Signed-off-by: Alexander Nyberg <[EMAIL PROTECTED]> Acked-by: Stephen Smalley <[EMAIL PROTECTED]> -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SELinux: null dereference in error path
On Tue, 2005-03-01 at 01:32 +0100, Alexander Nyberg wrote: > The 'bad' label will call function that unconditionally dereferences > the NULL pointer. > > Found by the Coverity tool > > Signed-off-by: Alexander Nyberg <[EMAIL PROTECTED]> Acked-by: Stephen Smalley <[EMAIL PROTECTED]> -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/4] vfs: adds the S_PRIVATE flag and adds use to security
On Fri, 2005-03-04 at 21:28 -0800, Andrew Morton wrote: > Jeffrey Mahoney <[EMAIL PROTECTED]> wrote: > > > > This patch adds an S_PRIVATE flag to inode->i_flags to mark an inode as > > filesystem-internal. As such, it should be excepted from the security > > infrastructure to allow the filesystem to perform its own access control. > > OK, thanks. I'll assume that the other three patches are unchanged. > > I don't think we've heard from the SELinux team regarding these patches? > > (See http://www.zip.com.au/~akpm/linux/patches/stuff/selinux-reiserfs/) Acked-by: Stephen Smalley <[EMAIL PROTECTED]> -- Stephen Smalley <[EMAIL PROTECTED]> National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH][LSM/SELINUX] Pass requested protection to security_file_mmap/mprotect hooks
This patch adds a reqprot parameter to the security_file_mmap and
security_file_mprotect hooks that is the original requested protection
value prior to any modification for read-implies-exec, and changes the
SELinux module to allow a mode of operation (controllable via a
checkreqprot setting) where it applies checks based on that protection
value rather than the protection that will be applied by the kernel,
effectively restoring SELinux's original behavior prior to the
introduction of the read-implies-exec logic in the mainline kernel.
The patch also disables execmem and execmod checking entirely on
PPC32, as the PPC32 ELF ABI presently requires RWE segments per Ulrich
Drepper. The patch is relative to the enhanced MLS support patch submitted
against 2.6.11-mm1.
At present, the read-implies-exec logic causes SELinux to see every
mmap/mprotect read request by legacy binaries or binaries marked with
PT_GNU_STACK RWE as a read|execute request, which tends to distort
policy even if it reflects what is ultimately possible. The
checkreqprot setting allows one to set the desired behavior for
SELinux, so either the current behavior or the original behavior is
possible. The checkreqprot value has a compile-time configurable
default value and can also be set via boot parameter or at runtime via
/selinux/checkreqprot if allowed by policy. Thanks to Chris Wright,
James Morris, and Colin Walters for comments on an earlier version of
the patch.
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
Signed-off-by: James Morris <[EMAIL PROTECTED]>
include/linux/security.h | 25 +++
mm/mmap.c|4 -
mm/mprotect.c|6 +-
security/dummy.c |7 ++-
security/selinux/Kconfig | 19
security/selinux/hooks.c | 18 ++--
security/selinux/include/av_perm_to_string.h |1
security/selinux/include/av_permissions.h|1
security/selinux/include/objsec.h|2
security/selinux/selinuxfs.c | 60 +++
10 files changed, 126 insertions(+), 17 deletions(-)
diff -X /home/sds/dontdiff -rup linux-2.6.11-mm1-mls/include/linux/security.h
linux-2.6.11-mm1-mls-reqprot/include/linux/security.h
--- linux-2.6.11-mm1-mls/include/linux/security.h 2005-03-07
13:06:54.0 -0500
+++ linux-2.6.11-mm1-mls-reqprot/include/linux/security.h 2005-03-07
12:21:48.0 -0500
@@ -458,13 +458,15 @@ struct swap_info_struct;
* Check permissions for a mmap operation. The @file may be NULL, e.g.
* if mapping anonymous memory.
* @file contains the file structure for file to map (may be NULL).
- * @prot contains the requested permissions.
+ * @reqprot contains the protection requested by the application.
+ * @prot contains the protection that will be applied by the kernel.
* @flags contains the operational flags.
* Return 0 if permission is granted.
* @file_mprotect:
* Check permissions before changing memory access permissions.
* @vma contains the memory region to modify.
- * @prot contains the requested permissions.
+ * @reqprot contains the protection requested by the application.
+ * @prot contains the protection that will be applied by the kernel.
* Return 0 if permission is granted.
* @file_lock:
* Check permission before performing file locking operations.
@@ -1128,9 +1130,12 @@ struct security_operations {
void (*file_free_security) (struct file * file);
int (*file_ioctl) (struct file * file, unsigned int cmd,
unsigned long arg);
- int (*file_mmap) (struct file * file,
+ int (*file_mmap) (struct file * file,
+ unsigned long reqprot,
unsigned long prot, unsigned long flags);
- int (*file_mprotect) (struct vm_area_struct * vma, unsigned long prot);
+ int (*file_mprotect) (struct vm_area_struct * vma,
+ unsigned long reqprot,
+ unsigned long prot);
int (*file_lock) (struct file * file, unsigned int cmd);
int (*file_fcntl) (struct file * file, unsigned int cmd,
unsigned long arg);
@@ -1631,16 +1636,18 @@ static inline int security_file_ioctl (s
return security_ops->file_ioctl (file, cmd, arg);
}
-static inline int security_file_mmap (struct file *file, unsigned long prot,
+static inline int security_file_mmap (struct file *file, unsigned long reqprot,
+ unsigned long prot,
unsigned long flags)
{
- return security_ops->file_mmap (file, prot, flags);
+ return security_ops->file_mmap (file, reqprot, prot, flags);
}
static inline int security_file_mprotect (
Re: [PATCH][LSM/SELINUX] Pass requested protection to security_file_mmap/mprotect hooks
On Mon, 2005-03-07 at 16:14 -0800, Andrew Morton wrote:
> Stephen Smalley <[EMAIL PROTECTED]> wrote:
> >
> > +__setup("checkreqprot=", checkreqprot_setup);
>
> Can we have an update to Documentation/kernel-parameters.txt, please?
Ok, how does the patch below look? Includes descriptions of the other
two SELinux-related parameters as well.
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
Documentation/kernel-parameters.txt | 26 ++
1 files changed, 26 insertions(+)
--- linux-2.6.11-mm2/Documentation/kernel-parameters.txt2005-03-08
07:46:07.491966080 -0500
+++ linux-2.6.11-mm2-sel/Documentation/kernel-parameters.txt2005-03-08
08:21:11.179157016 -0500
@@ -67,6 +67,7 @@
SCSIAppropriate SCSI support is enabled.
A lot of drivers has their options described inside of
Documentation/scsi/.
+ SELINUX SELinux support is enabled.
SERIAL Serial support is enabled.
SMP The kernel is an SMP kernel.
SPARC Sparc architecture is enabled.
@@ -296,6 +297,14 @@
See header of drivers/cdrom/cdu31a.c.
chandev=[HW,NET] Generic channel device initialisation
+
+ checkreqprot[SELINUX] Set initial checkreqprot flag value.
+ Format: { "0" | "1" }
+ See security/selinux/Kconfig help text.
+ 0 -- check protection applied by kernel (includes any
implied execute protection).
+ 1 -- check protection requested by application.
+ Default value is set via a kernel config option.
+ Value can be changed at runtime via
/selinux/checkreqprot.
clock= [BUGS=IA-32, HW] gettimeofday timesource override.
Forces specified timesource (if avaliable) to be used
@@ -444,6 +453,14 @@
See Documentation/block/as-iosched.txt
and Documentation/block/deadline-iosched.txt for
details.
+ enforcing [SELINUX] Set initial enforcing status.
+ Format: {"0" | "1"}
+ See security/selinux/Kconfig help text.
+ 0 -- permissive (log only, no denials).
+ 1 -- enforcing (deny and log).
+ Default value is 0.
+ Value can be changed at runtime via /selinux/enforce.
+
es1370= [HW,OSS]
Format: [,]
See also header of sound/oss/es1370.c.
@@ -1187,6 +1204,15 @@
scsi_logging= [SCSI]
+ selinux [SELINUX] Disable or enable SELinux at boot time.
+ Format: { "0" | "1" }
+ See security/selinux/Kconfig help text.
+ 0 -- disable.
+ 1 -- enable.
+ Default value is set via kernel config option.
+ If enabled at boot time, /selinux/disable can be used
+ later to disable prior to initial policy load.
+
serialnumber[BUGS=IA-32]
sg_def_reserved_size=
--
Stephen Smalley <[EMAIL PROTECTED]>
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH][SELINUX] Fix selinux_setprocattr
This patch against 2.6.11-mm2 changes the selinux_setprocattr hook function
(which
handles writes to nodes in the /proc/pid/attr directory) to ignore an
optional terminating newline at the end of the value, and to handle a
value beginning with a newline or a null in the same manner as a zero
length value (clearing the attribute for the process and resetting it
to using the default policy behavior). This change is to address the
divergence from POSIX in the existing API, as POSIX says that write(2)
with a zero count will return zero with no other effect, as well as to
simplify use of the API from scripts (although that isn't
recommended). Please apply.
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
Signed-off-by: James Morris <[EMAIL PROTECTED]>
security/selinux/hooks.c |8 ++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff -X /home/sds/dontdiff -ru linux-2.6.11-mm2/security/selinux/hooks.c
linux-2.6.11-mm2-sel/security/selinux/hooks.c
--- linux-2.6.11-mm2/security/selinux/hooks.c 2005-03-08 08:43:52.867139656
-0500
+++ linux-2.6.11-mm2-sel/security/selinux/hooks.c 2005-03-08
08:44:02.733639720 -0500
@@ -4106,6 +4106,7 @@
struct task_security_struct *tsec;
u32 sid = 0;
int error;
+ char *str = value;
if (current != p) {
/* SELinux only allows a process to change its own
@@ -4130,8 +4131,11 @@
return error;
/* Obtain a SID for the context, if one was specified. */
- if (size) {
- int error;
+ if (size && str[1] && str[1] != '\n') {
+ if (str[size-1] == '\n') {
+ str[size-1] = 0;
+ size--;
+ }
error = security_context_to_sid(value, size, &sid);
if (error)
return error;
--
Stephen Smalley <[EMAIL PROTECTED]>
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/1] SELinux AVC audit log ipaddr field support (for task_struct->curr_ip)
On Thu, 2005-03-10 at 16:05 +0100, Lorenzo HernÃndez GarcÃa-Hierro
wrote:
> Provides support for a new field ipaddr within the SELinux
> AVC audit log, relying in task_struct->curr_ip (ipv4 only)
> provided by the task-curr_ip or grSecurity patch to be applied
> before.It was first implemented by Joshua Brindle (a.k.a Method)
> from the Hardened Gentoo project.
>
> An example of the audit messages with ipaddr field:
> audit(1110432234.161:0): avc: denied { search } for pid=19057
> exe=/usr/bin/wget name=portage dev=hda3 ino=1024647 ipaddr=192.168.1.30
> scontext=root:sysadm_r:portage_fetch_t
> tcontext=system_u:object_r:portage_tmp_t tclass=dir
Even if the basic idea were sound (doubtful), this would need to be
generalized (i.e. not ipv4-specific). Also, I think I'd rather see
extensions to the audit data be incorporated into the audit framework,
not the AVC-specific audit code, and some of the existing avc_audit()
code migrated into the audit framework (e.g. the exe= information
currently generated by avc_audit could be done by audit_log_exit
instead).
--
Stephen Smalley <[EMAIL PROTECTED]>
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: AppArmor FAQ
On Wed, 2007-04-18 at 12:41 -0700, Crispin Cowan wrote: > James Morris wrote: > > On Tue, 17 Apr 2007, Alan Cox wrote: > > > >> I'm not sure if AppArmor can be made good security for the general case, > >> but it is a model that works in the limited http environment > >> (eg .htaccess) and is something people can play with and hack on and may > >> be possible to configure to be very secure. > >> > > Perhaps -- until your httpd is compromised via a buffer overflow or > > simply misbehaves due to a software or configuration flaw, then the > > assumptions being made about its use of pathnames and their security > > properties are out the window. > > > How is it that you think a buffer overflow in httpd could allow an > attacker to break out of an AppArmor profile? This is exactly what > AppArmor was designed to do, and without specifics, this is just FUD. > > > Without security labeling of the objects being accessed, you can't protect > > against software flaws, which has been a pretty fundamental and widely > > understood requirement in general computing for at least a decade. > > > Please explain why labels are necessary for effective confinement. Many > systems besides AppArmor have used non-label schemes for effective > confinement: TRON, Janus, LIDS, Systrace, BSD Jail, EROS, PSOS, KeyOS, > AS400, to name just a few. This claim seems bogus. Labels may be your > method of choice for confinement, but they are far from the only way. Confinement in its traditional sense (e.g. the 1973 Lampson paper, ACM Vol 16 No 10) means information flow control, which you have agreed AppArmor does not and cannot provide. Yes? As to the (genuine) capability-based systems, have a look at the DTOS General System Security and Assurability Assessment Report for why we have concerns with their approach. But that has nothing to do with AppArmor. Look, if you would just refrain from making misleading statements about SELinux (not only in this FAQ but throughout your documents and talks), especially when we've previously refuted those same statements (as in the first AppArmor submission and its discussion), and honestly acknowledged the limitations of your approach without trying to spin them as strengths, I think that there would be nothing to discuss here. We could just agree to disagree, and you could just focus on addressing the issues raised by the vfs folks about how to get your changes into an acceptable form. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: AppArmor FAQ
On Wed, 2007-04-18 at 13:15 -0700, David Lang wrote: > On Wed, 18 Apr 2007, James Morris wrote: > > > On Tue, 17 Apr 2007, Alan Cox wrote: > > > >> I'm not sure if AppArmor can be made good security for the general case, > >> but it is a model that works in the limited http environment > >> (eg .htaccess) and is something people can play with and hack on and may > >> be possible to configure to be very secure. > > > > Perhaps -- until your httpd is compromised via a buffer overflow or > > simply misbehaves due to a software or configuration flaw, then the > > assumptions being made about its use of pathnames and their security > > properties are out the window. > > since AA defines a whitelist of files that httpd is allowed to access, a > comprimised one may be able to mess up it's files, but it's still not going > to > be able to touch other files on the system. > > > Without security labeling of the objects being accessed, you can't protect > > against software flaws, which has been a pretty fundamental and widely > > understood requirement in general computing for at least a decade. > > this is not true. you don't need to label all object and chunks of memory, > you > just need to have a way to list (and enforce) the objects and memory that the > program is allowed to use. labeling them is one way of doing this, but not > the > only way. You need a way of providing global and persistent security guarantees for the data, and per-program profiles based on pathname don't get you there. There is no system view in AA, just a bunch of disconnected profiles. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: AppArmor FAQ
On Thu, 2007-04-19 at 16:35 +, David Wagner wrote: > James Morris wrote: > >On Wed, 18 Apr 2007, Crispin Cowan wrote: > >> How is it that you think a buffer overflow in httpd could allow an > >> attacker to break out of an AppArmor profile? > > > >Because you can change the behavior of the application and then bypass > >policy entirely by utilizing any mechanism other than direct filesystem > >access: IPC, shared memory, Unix domain sockets, local IP networking, > >remote networking etc. > > Any halfway decent jail will let you control access to all of those > things, thereby preventing an 0wned httpd from breaking out of the jail. > (For instance, Janus did. So does Systrace.) > > Are you saying AppArmor does not allow that kind of control? Specifics > would be useful. Just look at their code and their own description of AppArmor. It does not provide that level of control, and by your own metric, it is thus not a halfway decent jail. Which begs the question - if that is the kind of approach you want, why not use a real jail/containers mechanism for it? > >Also worth noting here is that you have to consider any limited > >environment as enforcing security policy, and thus its configuration > >becomes an additional component of security policy. > > I don't understand what you are saying. Yes, the AppArmor policy > file is part of policy. Is that what you mean? I think he means the dependencies on which AppArmor relies, not just its policy, e.g. since it is name-based, it presumes the filesystem namespace has been set up by a trusted agent and is correct. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: AppArmor FAQ
On Tue, 2007-04-17 at 20:05 +0200, Andi Kleen wrote: > Karl MacMillan <[EMAIL PROTECTED]> writes: > > > No - the real fix is to change the applications or to run under a policy > > that confines all applications. Most of the problems with resolv.conf, > > mtab, etc. stem from admin processes (e.g., editors or shell scripts) > > all running under the same unconfined domain. > > > > In some cases applications need modification as only the application has > > enough information to determine the correct label. Usually this means > > preserving labels from input files or separating the output into > > distinct directories so type transitions or label inheritance will work. > > > > restorecond is just a hack not a requirement or a sign that something is > > wrong with the model. That is why it is a userspace application and not > > integrated into the kernel mechanism. > > You nicely show one of the major disadvantages of the label model vs the path > model here: it requires modification of a lot of applications. It is true that many applications that already deal with mode bits need to become aware of labels, just as with ACLs, and that this makes it a bit harder and slower to roll out something that is label-based. But the right solution is rarely quick and easy, and a lot of work has already happened to integrate such support into userland. To look at it in a slightly different way, the AA emphasis on not modifying applications could be viewed as a limitation. Ultimately, users have security goals that go beyond just what the OS can directly enforce and at least some applications (notably things like X, D-BUS, PostgreSQL, etc) need to likewise support strong domain separation and controlled information flow through their own internal objects and operations. SELinux provides APIs and infrastructure for such applications, and has already done quite a bit of work in that space (D-BUS support, XACE/XSELinux, SE-PostgreSQL), whereas AA seems to have no interest in going there (and would have to recant its emphasis on no application mods to do so). If you actually want to truly confine a desktop application, you can't limit yourself to the kernel. And the label model provides a unifying abstraction for dealing with all of these various objects, whereas the path/"natural abstraction" model has no unifying abstraction at all. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: AppArmor FAQ
On Tue, 2007-04-17 at 16:09 -0700, Crispin Cowan wrote: > David Safford wrote: > > On Mon, 2007-04-16 at 20:20 -0400, James Morris wrote: > > > >> On Mon, 16 Apr 2007, John Johansen wrote: > >> > >>> Label-based security (exemplified by SELinux, and its predecessors in > >>> MLS systems) attaches security policy to the data. As the data flows > >>> through the system, the label sticks to the data, and so security > >>> policy with respect to this data stays intact. This is a good approach > >>> for ensuring secrecy, the kind of problem that intelligence agencies have. > >>> > >> Labels are also a good approach for ensuring integrity, which is one of > >> the most fundamental aspects of the security model implemented by SELinux. > >> > >> > >> Some may infer otherwise from your document. > >> > > In fact, I am not sure how you can provide integrity support without > > labels. AppArmor confines a process, but does not effectively confine > > its output files, precisely because the output files are not labeled. > > Other processes are free to access the unlabeled, potentially malicious > > output files without restriction. > > > That depends on what you mean by "integrity." It is true that AppArmor > does not directly manage information flow. AppArmor assumes that if you > granted write permission to /etc/resolv.conf, that you meant to do that. > Whether any other process is permitted to access /etc/resolv.conf is > determined by those other process's respective profiles. Integrity protection requires information flow control; you can't protect a high integrity process from being corrupted by a low integrity process if you don't control the flow of information. Plenty of attacks take the form of a untrusted process injecting data that will ultimately be used by a more trusted process with a surprising side effect. And you can't do information flow control if you can't provide global and persistent protection of the data, which requires labeling it and preserving that label for its lifetime. > What AppArmor provides is a way for administrators to confine software > that they have to run but do not trust. The use of pathnames means that > the administrator can understand the exact meaning of the security > policy, without having to do a complete labeling of the file system. The > flip side of not managing information flow is that each profile is > independent of every other profile. They aren't truly independent; the composition may lead to surprising results where each individual program is "confined" exactly as you specified, but in combination, one is able to corrupt the higher integrity subject by actions taken by the lower integrity subject. Particularly in the fun area of publically writable directories, where pathnames are largely useless as an indicator. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: AppArmor FAQ
On Thu, 2007-04-19 at 20:08 +, David Wagner wrote: > Stephen Smalley wrote: > >Confinement in its traditional sense (e.g. the 1973 Lampson paper, ACM > >Vol 16 No 10) means information flow control, which you have agreed > >AppArmor does not and cannot provide. > > Right, that's how I understand it, too. > > However, I think some more caveats are in order. In all honesty, > I don't think SELinux solves Lampson's problem, either. > > It is useful to distinguish between "bit-confinement" (confining the > flow of information, a la Lampson) vs "authority-confinement" (confining > the flow of privileges and the ability of the untrusted app to cause > side effects on the rest of the system). > > No Linux system provides bit-confinement, if the confined app is > malicious. AppArmor does not provide bit-confinement. Neither does > SELinux. SELinux can stop some kinds of accidental leakage of secrets, > but it cannot prevent deliberate attempts to leak the secrets that are > known to malicious apps. The reason is that, in every system under > consideration, it is easy for a malicious app to leak any secrets it might > have to the outside world by using covert channels (e.g., wall-banging). > In practical terms, Lampson's bit-confinement problem is just not > solvable. Oh well, so it goes. > > A good jail needs to provide authority-confinement, but thankfully, > it doesn't need to provide bit-confinement. I don't know enough about > AppArmor to know whether it is able to do a good job of providing > authority-confinement. If it cannot, then it deserves criticism on > those grounds. > > Often the pragmatic solution to the covert channel problem is to ensure > that untrusted apps are never given access to critical secrets in the > first place. They can't leak something they don't know. This solves the > confidentiality problem by avoiding any attempt to tackle the unsolvable > bit-confinement problem. > > Note that the problem of building a good jail is a little different from > the information flow control problem. First, I think there is practical value in providing confidentiality control on the overt channels, even if covert channels remain. We have to start somewhere. Second, information flow is not just a confidentiality issue - see my other email. It is quite important as well for integrity, and integrity corruption in order to assume control over a privileged subject or trick it into abusing its power can be done solely via a data channel - it doesn't require explicit flow of authority. Lastly, if you want to judge AA as a jail mechanism, I think you'll find it fails there too. So where does that leave it? An easy-to-use yet inadequate solution for MAC or jail. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: AppArmor FAQ
On Thu, 2007-04-19 at 20:54 +, David Wagner wrote:
> Stephen Smalley wrote:
> >Integrity protection requires information flow control; you can't
> >protect a high integrity process from being corrupted by a low integrity
> >process if you don't control the flow of information. Plenty of attacks
> >take the form of a untrusted process injecting data that will ultimately
> >be used by a more trusted process with a surprising side effect.
>
> I don't agree with this blanket statement. In a number of cases
> of practical interest, useful integrity protection can be achieved
> without full information flow control. Suppose you have a malicious
> ("low integrity") process A, and a target ("high integrity") process B.
> We want to prevent A from attacking B. One way to do that is to ensure
> that A has no overt channel it can use to attack process B, by severely
> restricting A's ability to cause side effects on the rest of the world.
> This is often sufficient to contain the damage that A can do.
If you could do that, I'd call that information flow control - I wasn't
saying you had to eliminate covert channels. As you said, we don't deal
with those even in SELinux. The point is that AA can't even do that,
not only because it has incomplete controls but because it bases its
decisions on unreliable identifiers (paths) that doesn't let it provide
global and persistent protection of the data.
> Of course, if the intended functionality of the system requires A to
> communicate data to B, and if you don't trust B's ability to handle
> that data carefully enough, and if A is malicious, then you've got a
> serious problem.
>
> But in a number of cases (enough cases to be useful), you can provide
> a useful level of security without needing information flow control and
> without needing global, persistent labels.
Without a reliable way of identifying the data in a system view, you
can't say anything at all about the data flows. The labels provide you
with a way of doing that. The paths are ambiguous, highly mutable, and
often meaningless (particularly for runtime files, temporary files, etc)
from a security pov.
Simple example: malicious symlink attacks.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][Patch 2/6] integrity: fs hook placement
On Thu, 2007-03-08 at 12:01 -0600, Serge E. Hallyn wrote: > Quoting Chris Wright ([EMAIL PROTECTED]): > > * Serge E. Hallyn ([EMAIL PROTECTED]) wrote: > > > Are you objecting only to the duplication at the callsites, so that an > > > fsnotify-type of consolidation of security and integrity hooks would be > > > ok? Or are you complaining that the security_inode_setxattr and > > > integrity_inode_setxattr hooks are too similar anyway, and integrity > > > modules should just use some lsm hooks for anything which will be > > > authoritative? > > > > It's duplication of callsites with many identical implementations > > that's the problem. > > Yes it's ugly... > > But I guess it gets a point across :) > > > > (I could see an argument that integirty subsystem should be purely for > > > measuring and hence its hooks should never return a value. Only hitch > > > there is that if integrity subsystem hits ENOMEM it should be able to > > > refuse the action...) > > > > Right, that's what I was expecting to see, just the measurement > > infrastructure. > > So what you are saying is EVM would stay an LSM, with a cooperating > integrity subsystem *just* doing measurements? > > That's kind of what i was expecting too, however that doesn't fit as > well with the idea that an integrity subsystem prevents the need for lsm > stacking. I think the idea was that evm would still be able to enforce > integrity of selinux xattrs without it stack with selinux. So I can see > where this approach came from. The enforcement mechanism should be directly integrated into SELinux, not stacked as a separate module. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[patch 1/1] selinux: always initialize arguments to security_sid_to_context (Was: Re: [GIT PULL] audit patches)
On Thu, 2007-02-22 at 13:19 -0800, Andrew Morton wrote:
> > On Thu, 22 Feb 2007 08:22:47 -0500 Stephen Smalley <[EMAIL PROTECTED]>
> > wrote:
> > On Wed, 2007-02-21 at 16:03 -0800, Andrew Morton wrote:
> > >
> > > Looking at the changes to audit_receive_msg():
> > >
> > >
> > > if (sid) {
> > > if (selinux_sid_to_string(
> > > sid, &ctx, &len)) {
> > > audit_log_format(ab,
> > > " ssid=%u", sid);
> > > /* Maybe call audit_panic? */
> > > } else
> > > audit_log_format(ab,
> > > " subj=%s", ctx);
> > > kfree(ctx);
> > > }
> > >
> > > This is assuming that selinux_sid_to_string() always initialises `ctx'.
> > >
> > > But AFAICT there are two error paths in security_sid_to_context() which
> > > forget to do that, so we end up doing kfree(uninitialised-local).
> > >
> > > I'd consider that a shortcoming in security_sid_to_context(), so not a
> > > problem in this patch, as long as people agree with my blaming above.
> >
> > I wouldn't assume that the function initializes an argument if it
> > returns an error, and at least some of the callers (in auditsc.c) appear
> > to correctly initialize ctx to NULL themselves before calling
> > selinux_sid_to_string(). But if you'd prefer the function to always
> > handle it, we can do that.
> >
>
> Well we now have (at least) one caller which assumes that *ctx is
> initialied in error cases.
>
> And I think it's sane to make it do that: safer, and will simplify coding
> in the callers.
Ok, patch below.
Always initialize *scontext and *scontext_len in security_sid_to_context.
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
---
security/selinux/ss/services.c |3 +++
1 file changed, 3 insertions(+)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index ca9154d..1e52356 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -609,6 +609,9 @@ int security_sid_to_context(u32 sid, char **scontext, u32
*scontext_len)
struct context *context;
int rc = 0;
+ *scontext = NULL;
+ *scontext_len = 0;
+
if (!ss_initialized) {
if (sid <= SECINITSID_NUM) {
char *scontextp;
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 0/28] Patches to pass vfsmount to LSM inode security hooks
On Mon, 2007-02-05 at 18:13 -0800, Andreas Gruenbacher wrote: > On Monday 05 February 2007 10:44, Christoph Hellwig wrote: > > Looking at the actual patches I see you're lazy in a lot of places. > > Please make sure that when you introduce a vfsmount argument somewhere > > that it is _always_ passed and not just when it's conveniant. Yes, that's > > more work, but then again if you're not consistant anyone half-serious > > will laught at a security model using this infrasturcture. > > It may appear like laziness, but it's not. Let's look at where we're passing > NULL at the moment: > > fs/hpfs/namei.c > > In hpfs_unlink, hpfs truncates one of its own inodes through > notify_change(). You definitely don't want any lsms to interfere here, > pathname based or not; hpfs should probably truncate its inode itself > instead. But given that hpfs goes via the vfs, we at least pass NULL > to indicate that this file really has no meaningful paths to it > anymore. (In addition, we don't really have a vfsmount at this > point anymore, and neither would it make sense to pass it there.) > > To play more nicely with other lsms, hpfs could mark the inode as > private before attempting the truncate. > > fs/reiserfs/xattr.c > > The directories an files that reiserfs uses internally to store xattrs > are hanging off ".reiserfs_priv/xattrs" in the filesystem. This part > of the namespace is not accessible or visible from user space though > except through the xattr syscalls. > > Reiserfs should probably just mark all its xattr inodes as private in > order > to play nicely with other lsms. As far as pathname based lsms are > concerned, > pathnames to those fs-internal objects are meaningless though, and so we > pass NULL here. That should be handled by the current marking of reiserfs xattr inodes with S_PRIVATE and the tests for IS_PRIVATE in include/linux/security.h (and in one instance, within SELinux itself). -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.
On Tue, 2007-02-06 at 14:21 -0700, Eric W. Biederman wrote:
> This time instead of generating the generating the paths from proc_dir_entries
> generate the labels from the names in the sysctl ctl_tables themselves. This
> removes an unnecessary layer of indirection, allows this to work even when
> procfs support is not compiled into the kernel, and especially allows it
> to work now that ctl_tables no longer have a proc_dir_entry field.
Thanks, looks sane.
> I continue passing "proc" into genfs sid although that is complete nonsense
> to allow existing selinux policies to work without modification.
>
> Signed-off-by: Eric W. Biederman <[EMAIL PROTECTED]>
Acked-by: Stephen Smalley <[EMAIL PROTECTED]>
> ---
> security/selinux/hooks.c | 43 +--
> 1 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3a36057..c17a8dd 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1424,6 +1424,41 @@ static int selinux_capable(struct task_struct *tsk,
> int cap)
> return task_has_capability(tsk,cap);
> }
>
> +static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
> +{
> + int buflen, rc;
> + char *buffer, *path, *end;
> +
> + rc = -ENOMEM;
> + buffer = (char*)__get_free_page(GFP_KERNEL);
> + if (!buffer)
> + goto out;
> +
> + buflen = PAGE_SIZE;
> + end = buffer+buflen;
> + *--end = '\0';
> + buflen--;
> + path = end-1;
> + *path = '/';
> + while (table) {
> + const char *name = table->procname;
> + size_t namelen = strlen(name);
> + buflen -= namelen + 1;
> + if (buflen < 0)
> + goto out_free;
> + end -= namelen;
> + memcpy(end, name, namelen);
> + *--end = '/';
> + path = end;
> + table = table->parent;
> + }
> + rc = security_genfs_sid("proc", path, tclass, sid);
> +out_free:
> + free_page((unsigned long)buffer);
> +out:
> + return rc;
> +}
> +
> static int selinux_sysctl(ctl_table *table, int op)
> {
> int error = 0;
> @@ -1438,8 +1473,12 @@ static int selinux_sysctl(ctl_table *table, int op)
>
> tsec = current->security;
>
> - /* Use the well-defined sysctl SID. */
> - tsid = SECINITSID_SYSCTL;
> + rc = selinux_sysctl_get_sid(table, (op == 0001) ?
> + SECCLASS_DIR : SECCLASS_FILE, &tsid);
> + if (rc) {
> + /* Default to the well-defined sysctl SID. */
> + tsid = SECINITSID_SYSCTL;
> + }
>
> /* The op values are "defined" in sysctl.c, thereby creating
>* a bad coupling between this module and sysctl.c */
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC 0/28] Patches to pass vfsmount to LSM inode security hooks
On Wed, 2007-02-07 at 07:43 -0800, Chris Wright wrote: > * Andreas Gruenbacher ([EMAIL PROTECTED]) wrote: > > Reiserfs currently only marks the ".reiserfs_priv" directory as private, > > but > > not the files below it -- how about the attached patch to fix that? > > I don't think that's right. Look at ->create or ->lookup. Both of those > properly set the private flag. This patch looks like a step backwards, > sprinkling the init in so many places. Yes, I thought that this was already covered by the existing inheritance of the private flag from the parent directory. On a separate note, I believe that the current problem with using reiserfs and selinux is just that reiserfs hasn't been updated to call security_inode_init_security() and set the security xattr when creating a new file; see ext3 or jfs for an example. But I don't know of anyone using reiserfs with selinux presently. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.
On Wed, 2007-02-07 at 13:24 -0500, Stephen Smalley wrote:
> On Tue, 2007-02-06 at 14:21 -0700, Eric W. Biederman wrote:
> > This time instead of generating the generating the paths from
> > proc_dir_entries
> > generate the labels from the names in the sysctl ctl_tables themselves.
> > This
> > removes an unnecessary layer of indirection, allows this to work even when
> > procfs support is not compiled into the kernel, and especially allows it
> > to work now that ctl_tables no longer have a proc_dir_entry field.
>
> Thanks, looks sane.
>
> > I continue passing "proc" into genfs sid although that is complete nonsense
> > to allow existing selinux policies to work without modification.
> >
> > Signed-off-by: Eric W. Biederman <[EMAIL PROTECTED]>
>
> Acked-by: Stephen Smalley <[EMAIL PROTECTED]>
Hmmm...but in testing the patch, I don't seem to (consistently) reach
these checks when accessing via /proc/sys. I see that you are caching
the mode information and using it in some cases rather than calling the
sysctl_perm function.
One related but separate issue is that the /proc/sys inode labeling is
also affected by the sysctl patch series. Those inodes used to be
labeled by selinux_proc_get_sid (from selinux_d_instantiate), but that
no longer works, so they now fall back to the superblock SID (generic
proc label). That changes the inode permission checks on an attempt to
access a /proc/sys node and will likely cause denials under current
policy for confined domains since one wouldn't generally be writing to
the generic proc label. If you always called sysctl_perm from the proc
sysctl code, we could possibly dispense with inode permission checking
on those inodes, e.g. marking them private.
>
> > ---
> > security/selinux/hooks.c | 43 +--
> > 1 files changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 3a36057..c17a8dd 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1424,6 +1424,41 @@ static int selinux_capable(struct task_struct *tsk,
> > int cap)
> > return task_has_capability(tsk,cap);
> > }
> >
> > +static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
> > +{
> > + int buflen, rc;
> > + char *buffer, *path, *end;
> > +
> > + rc = -ENOMEM;
> > + buffer = (char*)__get_free_page(GFP_KERNEL);
> > + if (!buffer)
> > + goto out;
> > +
> > + buflen = PAGE_SIZE;
> > + end = buffer+buflen;
> > + *--end = '\0';
> > + buflen--;
> > + path = end-1;
> > + *path = '/';
> > + while (table) {
> > + const char *name = table->procname;
> > + size_t namelen = strlen(name);
> > + buflen -= namelen + 1;
> > + if (buflen < 0)
> > + goto out_free;
> > + end -= namelen;
> > + memcpy(end, name, namelen);
> > + *--end = '/';
> > + path = end;
> > + table = table->parent;
> > + }
> > + rc = security_genfs_sid("proc", path, tclass, sid);
> > +out_free:
> > + free_page((unsigned long)buffer);
> > +out:
> > + return rc;
> > +}
> > +
> > static int selinux_sysctl(ctl_table *table, int op)
> > {
> > int error = 0;
> > @@ -1438,8 +1473,12 @@ static int selinux_sysctl(ctl_table *table, int op)
> >
> > tsec = current->security;
> >
> > - /* Use the well-defined sysctl SID. */
> > - tsid = SECINITSID_SYSCTL;
> > + rc = selinux_sysctl_get_sid(table, (op == 0001) ?
> > + SECCLASS_DIR : SECCLASS_FILE, &tsid);
> > + if (rc) {
> > + /* Default to the well-defined sysctl SID. */
> > + tsid = SECINITSID_SYSCTL;
> > + }
> >
> > /* The op values are "defined" in sysctl.c, thereby creating
> > * a bad coupling between this module and sysctl.c */
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.
On Wed, 2007-02-07 at 16:12 -0500, Stephen Smalley wrote: > On Wed, 2007-02-07 at 13:24 -0500, Stephen Smalley wrote: > > On Tue, 2007-02-06 at 14:21 -0700, Eric W. Biederman wrote: > > > This time instead of generating the generating the paths from > > > proc_dir_entries > > > generate the labels from the names in the sysctl ctl_tables themselves. > > > This > > > removes an unnecessary layer of indirection, allows this to work even when > > > procfs support is not compiled into the kernel, and especially allows it > > > to work now that ctl_tables no longer have a proc_dir_entry field. > > > > Thanks, looks sane. > > > > > I continue passing "proc" into genfs sid although that is complete > > > nonsense > > > to allow existing selinux policies to work without modification. > > > > > > Signed-off-by: Eric W. Biederman <[EMAIL PROTECTED]> > > > > Acked-by: Stephen Smalley <[EMAIL PROTECTED]> > > Hmmm...but in testing the patch, I don't seem to (consistently) reach > these checks when accessing via /proc/sys. I see that you are caching > the mode information and using it in some cases rather than calling the > sysctl_perm function. Actually, on further inspection, it looks like the real issue is the "path" name generation; "cat /proc/sys/kernel/modprobe" yields a call to security_genfs_sid() with just "/modprobe" rather than the expected "/sys/kernel/modprobe". Which likewise leaves us with the generic proc label, just as with the inode permission check, so I end up seeing checks against it only. > One related but separate issue is that the /proc/sys inode labeling is > also affected by the sysctl patch series. Those inodes used to be > labeled by selinux_proc_get_sid (from selinux_d_instantiate), but that > no longer works, so they now fall back to the superblock SID (generic > proc label). That changes the inode permission checks on an attempt to > access a /proc/sys node and will likely cause denials under current > policy for confined domains since one wouldn't generally be writing to > the generic proc label. If you always called sysctl_perm from the proc > sysctl code, we could possibly dispense with inode permission checking > on those inodes, e.g. marking them private. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.
On Wed, 2007-02-07 at 18:57 -0700, Eric W. Biederman wrote:
> Stephen Smalley <[EMAIL PROTECTED]> writes:
>
> >
> > One related but separate issue is that the /proc/sys inode labeling is
> > also affected by the sysctl patch series. Those inodes used to be
> > labeled by selinux_proc_get_sid (from selinux_d_instantiate), but that
> > no longer works, so they now fall back to the superblock SID (generic
> > proc label). That changes the inode permission checks on an attempt to
> > access a /proc/sys node and will likely cause denials under current
> > policy for confined domains since one wouldn't generally be writing to
> > the generic proc label. If you always called sysctl_perm from the proc
> > sysctl code, we could possibly dispense with inode permission checking
> > on those inodes, e.g. marking them private.
>
> Like this?
>
> It seems a little weird but I'm happy with it if you are.
>
> Eric
>
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index b9d59c0..7d6f7c7 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -47,6 +47,7 @@ static struct inode *proc_sys_make_inode(struct inode *dir,
> struct ctl_table *ta
> inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
> inode->i_op = &proc_sys_inode_operations;
> inode->i_fop = &proc_sys_file_operations;
> + inode->i_flags |= S_PRIVATE; /* tell selinux to ignore this inode */
> proc_sys_refresh_inode(inode, table);
> out:
> return inode;
Hmmm...turns out to not be quite enough, as the /proc/sys inodes aren't
truly private to the fs, so we can run into them in a variety of
security hooks beyond just the inode hooks, such as
security_file_permission (when reading and writing them via the vfs
helpers), security_sb_mount (when mounting other filesystems on
directories in proc like binfmt_misc), and deeper within the security
module itself (as in flush_unauthorized_files upon inheritance across
execve). So I think we have to add an IS_PRIVATE() guard within
SELinux, as below. Note however that the use of the private flag here
could be confusing, as these inodes are _not_ private to the fs, are
exposed to userspace, and security modules must implement the sysctl
hook to get any access control over them.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 65fb5e8..21bf2f0 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1078,6 +1077,9 @@ static int inode_has_perm(struct task_st
struct inode_security_struct *isec;
struct avc_audit_data ad;
+ if (unlikely (IS_PRIVATE (inode)))
+ return 0;
+
tsec = tsk->security;
isec = inode->i_security;
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.
On Wed, 2007-02-07 at 15:21 -0700, Eric W. Biederman wrote:
> Stephen Smalley <[EMAIL PROTECTED]> writes:
>
> > Actually, on further inspection, it looks like the real issue is the
> > "path" name generation; "cat /proc/sys/kernel/modprobe" yields a call to
> > security_genfs_sid() with just "/modprobe" rather than the expected
> > "/sys/kernel/modprobe". Which likewise leaves us with the generic proc
> > label, just as with the inode permission check, so I end up seeing
> > checks against it only.
>
> Ok. It looks like two silly thing are going on here.
> I failed to register the root sysctl table, so none of the parent
> pointers got set.
>
> I didn't prepend /sys in the compatibility code, so for something with
> the parent pointers set you would have gotten "/kernel/modprobe" instead
> of /sys/kernel/modprobe"
>
> Sorry about that.
>
> I think the patch below will fix it.
Yes, thanks - this appears to correct the name generation and thus the
resulting SID computation for the selinux sysctl checks.
> Eric
>
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index 24f36f1..f316854 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -929,8 +929,6 @@ extern struct ctl_table_header *sysctl_head_next(struct
> ctl_table_header *prev);
> extern void sysctl_head_finish(struct ctl_table_header *prev);
> extern int sysctl_perm(struct ctl_table *table, int op);
>
> -extern void sysctl_init(void);
> -
> typedef struct ctl_table ctl_table;
>
> typedef int ctl_handler (ctl_table *table, int __user *name, int nlen,
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 0a5499f..0bb2c5f 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1241,6 +1241,14 @@ static void sysctl_set_parent(struct ctl_table
> *parent, struct ctl_table *table)
> }
> }
>
> +static __init int sysctl_init(void)
> +{
> + sysctl_set_parent(NULL, root_table);
> + return 0;
> +}
> +
> +core_initcall(sysctl_init);
> +
> /**
> * register_sysctl_table - register a sysctl hierarchy
> * @table: the top-level table structure
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c17a8dd..aad2697 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1452,6 +1452,12 @@ static int selinux_sysctl_get_sid(ctl_table *table,
> u16 tclass, u32 *sid)
> path = end;
> table = table->parent;
> }
> + buflen -= 4;
> + if (buflen < 0)
> + goto out_free;
> + end -= 4;
> + memcpy(end, "/sys", 4);
> + path = end;
> rc = security_genfs_sid("proc", path, tclass, sid);
> out_free:
> free_page((unsigned long)buffer);
>
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] sysctl: Restore the selinux path based label lookup for sysctls.
On Thu, 2007-02-08 at 10:53 -0700, Eric W. Biederman wrote: > Stephen Smalley <[EMAIL PROTECTED]> writes: > > > > > Hmmm...turns out to not be quite enough, as the /proc/sys inodes aren't > > truly private to the fs, so we can run into them in a variety of > > security hooks beyond just the inode hooks, such as > > security_file_permission (when reading and writing them via the vfs > > helpers), security_sb_mount (when mounting other filesystems on > > directories in proc like binfmt_misc), and deeper within the security > > module itself (as in flush_unauthorized_files upon inheritance across > > execve). So I think we have to add an IS_PRIVATE() guard within > > SELinux, as below. Note however that the use of the private flag here > > could be confusing, as these inodes are _not_ private to the fs, are > > exposed to userspace, and security modules must implement the sysctl > > hook to get any access control over them. > > Agreed, the naming is confusing, and using private here doesn't quite > feel right. > > A practical question is: Will we ever encounter these inodes > in the inode_init() path from superblock_init? Possibly, during setup upon initial policy load (initiated by /sbin/init these days) from selinux_complete_init, as early userspace may have already been accessing them. > If all of the accesses > that we care about go through inode_doinit_with_dentry we can just > walk the dcache to get the names, and that should work for the normal > proc case as well. Walking the proc_dir_entry tree (or the ctl_table tree) is preferable as it is a stable, user-immutable representation. Also avoids taking the dcache lock. > A somewhat related question: How do you handle security labels for > sysfs? No fine grained security yet. Right, they are all mapped to a single label presently. I was thinking of handling that from userspace after introducing a setxattr handler for sysfs and a way to preserve the SID on the entry (likely caching it in the sysfs_dirent and propagating that to the inode when the inode is populated from the sysfs_dirent). Then early userspace could walk sysfs and apply finer-grained labeling from a configuration. > If it doesn't look easy to solve this another way I will certainly > go with marking the inodes private. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/5] sysctl: Fix the selinux_sysctl_get_sid
On Thu, 2007-02-08 at 15:55 -0700, Eric W. Biederman wrote:
> I goofed and when reenabling the fine grained selinux labels for
> sysctls and forgot to add the "/sys" prefix before consulting
> the policy database. When computing the same path using
> proc_dir_entries we got the "/sys" for free as it was part
> of the tree, but it isn't true for clt_table trees.
>
> Signed-off-by: Eric W. Biederman <[EMAIL PROTECTED]>
Acked-by: Stephen Smalley <[EMAIL PROTECTED]>
> ---
> security/selinux/hooks.c |6 ++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 47fb937..de16b9f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1445,6 +1445,12 @@ static int selinux_sysctl_get_sid(ctl_table *table,
> u16 tclass, u32 *sid)
> path = end;
> table = table->parent;
> }
> + buflen -= 4;
> + if (buflen < 0)
> + goto out_free;
> + end -= 4;
> + memcpy(end, "/sys", 4);
> + path = end;
> rc = security_genfs_sid("proc", path, tclass, sid);
> out_free:
> free_page((unsigned long)buffer);
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/5] selinux: Enhance selinux to always ignore private inodes.
On Thu, 2007-02-08 at 16:02 -0700, Eric W. Biederman wrote: > From: Stephen Smalley <[EMAIL PROTECTED]> > > Hmmm...turns out to not be quite enough, as the /proc/sys inodes aren't > truly private to the fs, so we can run into them in a variety of > security hooks beyond just the inode hooks, such as > security_file_permission (when reading and writing them via the vfs > helpers), security_sb_mount (when mounting other filesystems on > directories in proc like binfmt_misc), and deeper within the security > module itself (as in flush_unauthorized_files upon inheritance across > execve). So I think we have to add an IS_PRIVATE() guard within > SELinux, as below. Note however that the use of the private flag here > could be confusing, as these inodes are _not_ private to the fs, are > exposed to userspace, and security modules must implement the sysctl > hook to get any access control over them. Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]> > Signed-off-by: Eric W. Biederman <[EMAIL PROTECTED]> > > --- > security/selinux/hooks.c |3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index de16b9f..ff9fccc 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -1077,6 +1077,9 @@ static int inode_has_perm(struct task_struct *tsk, > struct inode_security_struct *isec; > struct avc_audit_data ad; > > + if (unlikely (IS_PRIVATE (inode))) > + return 0; > + > tsec = tsk->security; > isec = inode->i_security; > -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] [PATCH -mm] file caps: make on-disk capabilities future-proof
On Mon, 2007-02-19 at 11:01 -0600, Serge E. Hallyn wrote:
> From: Serge E. Hallyn <[EMAIL PROTECTED]>
> Subject: [PATCH -mm] file caps: make on-disk capabilities future-proof
>
> Stephen Smalley has pointed out that the current file capabilities
> will eventually pose a problem.
>
> As the capability set changes and distributions start tagging
> binaries with capabilities, we would like for running an older
> kernel to not necessarily make those binaries unusable. To
> that end,
>
> 1. If capabilities are specified which we don't know
> about, just ignore them, do not return -EPERM as we
> were doing before.
I didn't advocate that change - it is a separate issue from allowing the
capability bitmaps to grow in size in a backward compatible manner. In
the one case, you have a binary that needs a capability that is unknown
to the kernel, so running it could lead to unexpected failure. In the
other case, you simply have a binary labeled by newer userspace with a
newer on-disk representation that supports larger bitmaps, but the
binary might only have capabilities set that are known to the kernel.
> 2. Specify a size with the on-disk capability implementation.
> In this implementation the size is the number of __u32's
> used for each of (eff,perm,inh). For now, sz is 1.
> When we move to 64-bit capabilities, it becomes 2.
You could alternatively split them into separate xattrs, e.g.
security.cap.eff, security.cap.perm, security.cap.inh, and determine the
bitmap size from the xattr length rather than a separate field.
> diff --git a/security/commoncap.c b/security/commoncap.c
> index be86acb..dc8bf4f 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -148,50 +145,65 @@ static int set_file_caps(struct linux_bi
> {
> struct dentry *dentry;
> ssize_t rc;
> - struct vfs_cap_data_disk dcaps;
> + struct vfs_cap_data_disk *dcaps;
> struct vfs_cap_data caps;
> struct inode *inode;
> - int err;
>
> if (bprm->file->f_vfsmnt->mnt_flags & MNT_NOSUID)
> return 0;
>
> dentry = dget(bprm->file->f_dentry);
> inode = dentry->d_inode;
> - if (!inode->i_op || !inode->i_op->getxattr) {
> - dput(dentry);
> - return 0;
> + rc = 0;
> + if (!inode->i_op || !inode->i_op->getxattr)
> + goto out;
> +
> + rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, NULL, 0);
> + if (rc == -ENODATA) {
> + rc = 0;
> + goto out;
> + }
I'd allocate an initial buffer with an expected size and try first to
avoid always having to make the two ->getxattr calls in the common case.
> + if (rc < 0)
> + goto out;
> + if (rc < sizeof(struct vfs_cap_data_disk)) {
You could make this a bit stricter, as you know that it will have at
least three additional u32 values beyond the header.
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + dcaps = kmalloc(rc, GFP_KERNEL);
> + if (!dcaps) {
> + rc = -ENOMEM;
> + goto out;
> + }
> + rc = inode->i_op->getxattr(dentry, XATTR_NAME_CAPS, dcaps,
> + XATTR_CAPS_SZ);
I'm confused - you just asked for the actual length of the xattr and
allocated a buffer for it, and then don't use the length in this second
call to ->getxattr. And since you said you were organizing it as
eff[0..sz-1],perm[0..sz-1],inh[0..sz-1], you do need to read the entire
thing to get all three values even if you only use the lower 32 bits of
each. Or if you change the organization to avoid the need to read the
entire thing, you don't need the first getxattr call at all, and you
need to change how cap_from_disk extracts the values.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] audit patches
On Wed, 2007-02-21 at 16:03 -0800, Andrew Morton wrote:
> On Sun, 18 Feb 2007 04:01:27 + Al Viro <[EMAIL PROTECTED]> wrote:
>
> > Misc audit patches (resend again...); the most intrusive one is
> > AUDIT_FD_PAIR,
> > allowing to log descriptor numbers from syscalls that do not return them in
> > usual way (i.e. pipe() and socketpair()). It took some massage of
> > the failure exits in sys_socketpair(); the rest is absolutely trivial.
> > Please, pull from
> > git.kernel.org/pub/scm/linux/kernel/git/viro/audit-current.git/ audit.b37
>
> Please send patches to the list for review if practical? In this case it
> was. I trust davem has had a look at the non-trivial changes to
> sys_socketpair().
>
>
>
> Looking at the changes to audit_receive_msg():
>
>
> if (sid) {
> if (selinux_sid_to_string(
> sid, &ctx, &len)) {
> audit_log_format(ab,
> " ssid=%u", sid);
> /* Maybe call audit_panic? */
> } else
> audit_log_format(ab,
> " subj=%s", ctx);
> kfree(ctx);
> }
>
> This is assuming that selinux_sid_to_string() always initialises `ctx'.
>
> But AFAICT there are two error paths in security_sid_to_context() which
> forget to do that, so we end up doing kfree(uninitialised-local).
>
> I'd consider that a shortcoming in security_sid_to_context(), so not a
> problem in this patch, as long as people agree with my blaming above.
I wouldn't assume that the function initializes an argument if it
returns an error, and at least some of the callers (in auditsc.c) appear
to correctly initialize ctx to NULL themselves before calling
selinux_sid_to_string(). But if you'd prefer the function to always
handle it, we can do that.
>
> The coding style in there is a bit odd-looking.
>
> The new __audit_fd_pair() has unneeded braces in it.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: oprofile / selinux / security_port_sid
On Tue, 2007-03-27 at 13:06 +0300, Sami Farin wrote: > is there room for improvement in security_port_sid() ? Yes, lots of room. Also, it won't get called per-packet if you enable secmark (echo 0 > /selinux/compat_net or boot with selinux_compat_net=0 or build with SECURITY_SELINUX_ENABLE_SECMARK_DEFAULT), although that may require updating your policy. > little test with dns queries (dnsfilter (the client) on local host > using poll() and dnscache (the server) using epoll (at max 4000 concurrent > queries): > (stats for only vmlinux) > > CPU: P4 / Xeon, speed 2797.32 MHz (estimated) > Counted GLOBAL_POWER_EVENTS events (time during which processor is not > stopped) with a unit mask of 0x01 (mandatory) count 45000 > Counted FSB_DATA_ACTIVITY events (DRDY or DBSY events on the front side bus) > with a unit mask of 0x03 (multiple flags) count 45000 > Counted BRANCH_RETIRED events (retired branches) with a unit mask of 0x05 > (multiple flags) count 45000 > Counted BRANCH_RETIRED events (retired branches) with a unit mask of 0x0a > (multiple flags) count 45000 > samples %samples %samples %samples % > symbol name > 220663 10.2181 6704 17.9737 5735 7.5171 271.1989 > datagram_poll > 1400866.4869 3239 8.6839 3786 4.9624 241.0657 > sock_poll > 1196365.5399 2172 5.8232 7168 9.3954 241.0657 > do_poll > 1015124.7006 3987 10.6893 812 1.0643 140.6217 > udp_get_port > 71008 3.2881 1017 2.7266 2694 3.5311 397 17.6288 > security_port_sid > 64350 2.9798 144 0.3861 1912 2.5061 6 0.2664 > add_wait_queue > 60815 2.8161 187 0.5014 3246 4.2546 2 0.0888 > remove_wait_queue > 47456 2.1975 1823 4.8875 476 0.6239 311.3766 > udp_v4_lookup_longway > > if dnsfilter had used epoll, security_port_sid would > probably (?) be number one (or two or three) CPU user in kernel. > > also note that 17.6% of mispredicted branches occurr in security_port_sid. > -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[patch][-mm] selinux: Reduce memory use by avtab
This patch improves memory use by SELinux by both reducing the avtab
node size and reducing the number of avtab nodes. The memory savings
are substantial, e.g. on a 64-bit system after boot, James Morris
reported the following data for the targeted and strict policies:
#objs objsize kernmem
Targeted:
Before: 237888 40 9.1MB
After:19968 24 468KB
Strict:
Before: 571680 40 21.81MB
After: 221052 245.06MB
The improvement in memory use comes at a cost in the speed of security
server computations of access vectors, but these computations are only
required on AVC cache misses, and performance measurements by James
Morris using a number of benchmarks have shown that the change does
not cause any significant degradation.
Note that a rebuilt policy via an updated policy toolchain
(libsepol/checkpolicy) is required in order to gain the full benefits
of this patch, although some memory savings benefits are immediately
applied even to older policies (in particular, the reduction in avtab
node size). Sources for the updated toolchain are presently available
from the sourceforge CVS tree
(http://sourceforge.net/cvs/?group_id=21266), and tarballs are available
from http://www.flux.utah.edu/~sds.
Please add this patch to -mm for wider testing in preparation for
eventual merging for 2.6.14. Thanks.
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
Signed-off-by: James Morris <[EMAIL PROTECTED]>
---
security/selinux/include/security.h |3
security/selinux/ss/avtab.c | 194 +-
security/selinux/ss/avtab.h | 37 +++---
security/selinux/ss/conditional.c | 205
security/selinux/ss/ebitmap.h | 30 +
security/selinux/ss/mls.c | 42 ---
security/selinux/ss/policydb.c | 47
security/selinux/ss/policydb.h |3
security/selinux/ss/services.c | 76 +++--
9 files changed, 401 insertions(+), 236 deletions(-)
diff -X /home/sds/dontdiff -rup
linux-2.6.13-rc5-mm1/security/selinux/include/security.h
linux-2.6.13-rc5-mm1-avtab/security/selinux/include/security.h
--- linux-2.6.13-rc5-mm1/security/selinux/include/security.h2005-08-11
13:20:28.0 -0400
+++ linux-2.6.13-rc5-mm1-avtab/security/selinux/include/security.h
2005-08-11 12:13:29.0 -0400
@@ -23,10 +23,11 @@
#define POLICYDB_VERSION_NLCLASS 18
#define POLICYDB_VERSION_VALIDATETRANS 19
#define POLICYDB_VERSION_MLS 19
+#define POLICYDB_VERSION_AVTAB 20
/* Range of policy versions we understand*/
#define POLICYDB_VERSION_MIN POLICYDB_VERSION_BASE
-#define POLICYDB_VERSION_MAX POLICYDB_VERSION_MLS
+#define POLICYDB_VERSION_MAX POLICYDB_VERSION_AVTAB
#ifdef CONFIG_SECURITY_SELINUX_BOOTPARAM
extern int selinux_enabled;
diff -X /home/sds/dontdiff -rup
linux-2.6.13-rc5-mm1/security/selinux/ss/avtab.c
linux-2.6.13-rc5-mm1-avtab/security/selinux/ss/avtab.c
--- linux-2.6.13-rc5-mm1/security/selinux/ss/avtab.c2005-08-11
13:20:28.0 -0400
+++ linux-2.6.13-rc5-mm1-avtab/security/selinux/ss/avtab.c 2005-08-11
12:13:29.0 -0400
@@ -58,6 +58,7 @@ static int avtab_insert(struct avtab *h,
{
int hvalue;
struct avtab_node *prev, *cur, *newnode;
+ u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
if (!h)
return -EINVAL;
@@ -69,7 +70,7 @@ static int avtab_insert(struct avtab *h,
if (key->source_type == cur->key.source_type &&
key->target_type == cur->key.target_type &&
key->target_class == cur->key.target_class &&
- (datum->specified & cur->datum.specified))
+ (specified & cur->key.specified))
return -EEXIST;
if (key->source_type < cur->key.source_type)
break;
@@ -98,6 +99,7 @@ avtab_insert_nonunique(struct avtab * h,
{
int hvalue;
struct avtab_node *prev, *cur, *newnode;
+ u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
if (!h)
return NULL;
@@ -108,7 +110,7 @@ avtab_insert_nonunique(struct avtab * h,
if (key->source_type == cur->key.source_type &&
key->target_type == cur->key.target_type &&
key->target_class == cur->key.target_class &&
- (datum->specified & cur->datum.specified))
+ (specified & cur->key.specified))
break;
if (key->source_type < cur->key.source_type)
break;
@@ -125,10 +127,11 @@ avtab_insert_nonunique(struct avtab * h,
return newnode;
}
Re: [patch][-mm] selinux: Reduce memory use by avtab
On Fri, 2005-08-12 at 00:34 +0400, Alexey Dobriyan wrote:
> On Thu, Aug 11, 2005 at 03:32:24PM -0400, Stephen Smalley wrote:
> > This patch improves memory use by SELinux by both reducing the avtab
> > node size and reducing the number of avtab nodes.
>
> > +int avtab_read_item(void *fp, u32 vers, struct avtab *a,
> > + int (*insertf)(struct avtab *a, struct avtab_key *k,
> > + struct avtab_datum *d, void *p),
> > + void *p)
> > +{
> > + u16 buf16[4],
>
> __le16
>
> > + u32 buf32[7],
>
> __le32
>
> > + items2 = le32_to_cpu(buf32[0]);
>
> > + key.source_type = le16_to_cpu(buf16[items++]);
>
> > +static int cond_read_av_list(struct policydb *p, void *fp, struct
> > cond_av_list **ret_list, struct cond_av_list *other)
> > +{
>
> > + u32 buf[1]
>
> __le32
>
> > + len = le32_to_cpu(buf[0]);
>
> Stephen, "[PATCH] selinux: endian annotations" you ACKed in June is hopelessly
> lost or thoroughly queued for inclusion?
Hmmm...sorry about that - I do have that patch, but seem to have failed
to follow up on it. I'll make a patch relative to this one that fixes
up any sparse bitwise warnings unless you have a particular desire to
update your original one. Should -Wbitwise be included in CHECKFLAGS in
the kernel Makefile by default?
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[patch][-mm] selinux: endian notations
This patch adds endian notations to the SELinux code.
It is relative to my prior patch, and is just an updated version of
Alexey's original patch (I hope) adjusted for the new code.
Please add it to -mm as well. Thanks.
From: Alexey Dobriyan <[EMAIL PROTECTED]>
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
---
security/selinux/avc.c|4 +-
security/selinux/ss/avtab.c |8 +++--
security/selinux/ss/conditional.c | 12 +--
security/selinux/ss/ebitmap.c |5 +--
security/selinux/ss/policydb.c| 60 +-
5 files changed, 52 insertions(+), 37 deletions(-)
diff -X /home/sds/dontdiff -rup
linux-2.6.13-rc5-mm1-avtab/security/selinux/avc.c
linux-2.6.13-rc5-endian/security/selinux/avc.c
--- linux-2.6.13-rc5-mm1-avtab/security/selinux/avc.c 2005-08-12
09:07:42.0 -0400
+++ linux-2.6.13-rc5-endian/security/selinux/avc.c 2005-08-12
09:04:48.0 -0400
@@ -490,7 +490,7 @@ out:
}
static inline void avc_print_ipv6_addr(struct audit_buffer *ab,
- struct in6_addr *addr, u16 port,
+ struct in6_addr *addr, __be16 port,
char *name1, char *name2)
{
if (!ipv6_addr_any(addr))
@@ -501,7 +501,7 @@ static inline void avc_print_ipv6_addr(s
}
static inline void avc_print_ipv4_addr(struct audit_buffer *ab, u32 addr,
- u16 port, char *name1, char *name2)
+ __be16 port, char *name1, char *name2)
{
if (addr)
audit_log_format(ab, " %s=%d.%d.%d.%d", name1, NIPQUAD(addr));
diff -X /home/sds/dontdiff -rup
linux-2.6.13-rc5-mm1-avtab/security/selinux/ss/avtab.c
linux-2.6.13-rc5-endian/security/selinux/ss/avtab.c
--- linux-2.6.13-rc5-mm1-avtab/security/selinux/ss/avtab.c 2005-08-12
09:15:51.0 -0400
+++ linux-2.6.13-rc5-endian/security/selinux/ss/avtab.c 2005-08-12
09:02:56.0 -0400
@@ -297,8 +297,10 @@ int avtab_read_item(void *fp, u32 vers,
struct avtab_datum *d, void *p),
void *p)
{
- u16 buf16[4], enabled;
- u32 buf32[7], items, items2, val;
+ __le16 buf16[4];
+ u16 enabled;
+ __le32 buf32[7];
+ u32 items, items2, val;
struct avtab_key key;
struct avtab_datum datum;
int i, rc;
@@ -403,7 +405,7 @@ static int avtab_insertf(struct avtab *a
int avtab_read(struct avtab *a, void *fp, u32 vers)
{
int rc;
- u32 buf[1];
+ __le32 buf[1];
u32 nel, i;
diff -X /home/sds/dontdiff -rup
linux-2.6.13-rc5-mm1-avtab/security/selinux/ss/conditional.c
linux-2.6.13-rc5-endian/security/selinux/ss/conditional.c
--- linux-2.6.13-rc5-mm1-avtab/security/selinux/ss/conditional.c
2005-08-12 09:15:51.0 -0400
+++ linux-2.6.13-rc5-endian/security/selinux/ss/conditional.c 2005-08-12
09:08:27.0 -0400
@@ -216,7 +216,8 @@ int cond_read_bool(struct policydb *p, s
{
char *key = NULL;
struct cond_bool_datum *booldatum;
- u32 buf[3], len;
+ __le32 buf[3];
+ u32 len;
int rc;
booldatum = kmalloc(sizeof(struct cond_bool_datum), GFP_KERNEL);
@@ -342,7 +343,8 @@ err:
static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list
**ret_list, struct cond_av_list *other)
{
int i, rc;
- u32 buf[1], len;
+ __le32 buf[1];
+ u32 len;
struct cond_insertf_data data;
*ret_list = NULL;
@@ -388,7 +390,8 @@ static int expr_isvalid(struct policydb
static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
{
- u32 buf[2], len, i;
+ __le32 buf[2];
+ u32 len, i;
int rc;
struct cond_expr *expr = NULL, *last = NULL;
@@ -446,7 +449,8 @@ err:
int cond_read_list(struct policydb *p, void *fp)
{
struct cond_node *node, *last = NULL;
- u32 buf[1], i, len;
+ __le32 buf[1];
+ u32 i, len;
int rc;
rc = next_entry(buf, fp, sizeof buf);
diff -X /home/sds/dontdiff -rup
linux-2.6.13-rc5-mm1-avtab/security/selinux/ss/ebitmap.c
linux-2.6.13-rc5-endian/security/selinux/ss/ebitmap.c
--- linux-2.6.13-rc5-mm1-avtab/security/selinux/ss/ebitmap.c2005-08-12
09:07:42.0 -0400
+++ linux-2.6.13-rc5-endian/security/selinux/ss/ebitmap.c 2005-08-12
08:43:33.0 -0400
@@ -196,8 +196,9 @@ int ebitmap_read(struct ebitmap *e, void
{
int rc;
struct ebitmap_node *n, *l;
- u32 buf[3], mapsize, count, i;
- u64 map;
+ __le32 buf[3];
+ u32 mapsize, count, i;
+ __le64 map;
ebitmap_init(e);
diff -X /home/sds/dontdiff -rup
linux-2.6.13-rc5-mm1-avtab/security/selinux/ss/policydb.c
linux-2.6.13-rc5-endian/security/selinux/ss/policydb.c
--- linux-2.6.13-rc5-mm1-avtab/security/seli
Re: SELinux policies, memory protections
ce, by labeling such objects appropriately, you can limit this permission to particular objects. The benefit of having these checks: http://marc.theaimsgroup.com/?l=selinux&m=111348610311179&w=2 More recently, some additional checks have been introduced: http://marc.theaimsgroup.com/?l=bk-commits-head&m=111974870402956&w=2 http://marc.theaimsgroup.com/?l=bk-commits-head&m=111974871308442&w=2 These checks provide some restrictions in the event that execmem must be allowed to a process, e.g. for runtime code generation, by still disabling the ability to make the primary stack executable or the heap executable. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2.6.13-rc6 2/2] New Syscall: set rlimits of any process (update)
On Thu, 2005-08-18 at 03:02 +0200, Wieland Gmeiner wrote: > diff -uprN -X linux-2.6.13-rc6-vanilla/Documentation/dontdiff > linux-2.6.13-rc6-getprlimit/security/selinux/hooks.c > linux-2.6.13-rc6-setprlimit/security/selinux/hooks.c > --- linux-2.6.13-rc6-getprlimit/security/selinux/hooks.c 2005-08-17 > 03:11:33.0 +0200 > +++ linux-2.6.13-rc6-setprlimit/security/selinux/hooks.c 2005-08-18 > 01:28:11.0 +0200 > @@ -2717,11 +2721,22 @@ static int selinux_task_setrlimit(unsign > later be used as a safe reset point for the soft limit > upon context transitions. See selinux_bprm_apply_creds. */ > if (old_rlim->rlim_max != new_rlim->rlim_max) > - return task_has_perm(current, current, PROCESS__SETRLIMIT); > + return task_has_perm(current, task, PROCESS__SETRLIMIT); > > return 0; > } This isn't sufficient for mandatory access control over setprlimit. As long as the setrlimit operation could only be performed by the process on itself, we were only concerned with controlling attempts to modify the hard limit (not the soft limit), as that is being used as a safe reset point for the soft limit upon security context transitions. But for the case of setprlimit where the target process may differ, we need to be able to control setting of any limit, soft or hard, in order to control the ability of a process in one security context to modify the state of a process in a different security context. Further, we would need a parallel check on the getprlimit side, to control the ability of a process in one security context to observe the state of a process in a different security context. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/5] Remove unnecesary capability hooks in rootplug.
On Thu, 2005-08-25 at 09:38 -0500, [EMAIL PROTECTED] wrote: > Ok, with the attached patch SELinux seems to work correctly. You'll > probably want to make it a little prettier :) Note I have NOT ran the > ltp tests for correctness. I'll do some performance runs, though > unfortunately can't do so on ppc right now. Note that the selinux tests there _only_ test the SELinux checking. So if these changes interfere with proper stacking of SELinux with capabilities, that won't show up there. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/5] Remove unnecesary capability hooks in rootplug.
On Thu, 2005-08-25 at 09:21 -0700, Chris Wright wrote: > * Stephen Smalley ([EMAIL PROTECTED]) wrote: > > On Thu, 2005-08-25 at 09:38 -0500, [EMAIL PROTECTED] wrote: > > > Ok, with the attached patch SELinux seems to work correctly. You'll > > > probably want to make it a little prettier :) Note I have NOT ran the > > > ltp tests for correctness. I'll do some performance runs, though > > > unfortunately can't do so on ppc right now. > > > > Note that the selinux tests there _only_ test the SELinux checking. So > > if these changes interfere with proper stacking of SELinux with > > capabilities, that won't show up there. > > Sorry, I'm not parsing that? e.g. if secondary_ops->capable is null, the SELinux tests aren't going to show that, because they will still see that the SELinux permission checks are working correctly. They only test failure/success for the SELinux permission checks, not for the capability checks, so if you unhook capabilities, they won't notice. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH][-mm] Generic VFS fallback for security xattrs
This patch modifies the VFS setxattr, getxattr, and listxattr code to
fall back to the security module for security xattrs if the filesystem
does not support xattrs natively. This allows security modules to
export the incore inode security label information to userspace even
if the filesystem does not provide xattr storage, and eliminates the
need to individually patch various pseudo filesystem types to provide
such access. The patch removes the existing xattr code from devpts
and tmpfs as it is then no longer needed.
The patch restructures the code flow slightly to reduce duplication
between the normal path and the fallback path, but this should only
have one user-visible side effect - a program may get -EACCES rather
than -EOPNOTSUPP if policy denied access but the filesystem didn't
support the operation anyway. Note that the post_setxattr hook call
is not needed in the fallback case, as the inode_setsecurity hook call
handles the incore inode security state update directly. In contrast,
we do call fsnotify in both cases.
Please include in -mm for wider testing prior to merging in 2.6.14.
---
fs/Kconfig | 43 --
fs/devpts/Makefile |1
fs/devpts/inode.c | 21 ---
fs/devpts/xattr_security.c | 47
fs/xattr.c | 80 +-
mm/shmem.c | 85 -
6 files changed, 49 insertions(+), 228 deletions(-)
diff -X /home/sds/dontdiff -Nrup linux-2.6.13-rc6-mm2/fs/devpts/inode.c
linux-2.6.13-rc6-mm2-xattr/fs/devpts/inode.c
--- linux-2.6.13-rc6-mm2/fs/devpts/inode.c 2005-06-17 15:48:29.0
-0400
+++ linux-2.6.13-rc6-mm2-xattr/fs/devpts/inode.c2005-08-24
07:57:51.0 -0400
@@ -18,28 +18,9 @@
#include
#include
#include
-#include
#define DEVPTS_SUPER_MAGIC 0x1cd1
-extern struct xattr_handler devpts_xattr_security_handler;
-
-static struct xattr_handler *devpts_xattr_handlers[] = {
-#ifdef CONFIG_DEVPTS_FS_SECURITY
- &devpts_xattr_security_handler,
-#endif
- NULL
-};
-
-static struct inode_operations devpts_file_inode_operations = {
-#ifdef CONFIG_DEVPTS_FS_XATTR
- .setxattr = generic_setxattr,
- .getxattr = generic_getxattr,
- .listxattr = generic_listxattr,
- .removexattr= generic_removexattr,
-#endif
-};
-
static struct vfsmount *devpts_mnt;
static struct dentry *devpts_root;
@@ -102,7 +83,6 @@ devpts_fill_super(struct super_block *s,
s->s_blocksize_bits = 10;
s->s_magic = DEVPTS_SUPER_MAGIC;
s->s_op = &devpts_sops;
- s->s_xattr = devpts_xattr_handlers;
s->s_time_gran = 1;
inode = new_inode(s);
@@ -175,7 +155,6 @@ int devpts_pty_new(struct tty_struct *tt
inode->i_gid = config.setgid ? config.gid : current->fsgid;
inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
init_special_inode(inode, S_IFCHR|config.mode, device);
- inode->i_op = &devpts_file_inode_operations;
inode->u.generic_ip = tty;
dentry = get_node(number);
diff -X /home/sds/dontdiff -Nrup linux-2.6.13-rc6-mm2/fs/devpts/Makefile
linux-2.6.13-rc6-mm2-xattr/fs/devpts/Makefile
--- linux-2.6.13-rc6-mm2/fs/devpts/Makefile 2005-06-17 15:48:29.0
-0400
+++ linux-2.6.13-rc6-mm2-xattr/fs/devpts/Makefile 2005-08-24
07:57:51.0 -0400
@@ -5,4 +5,3 @@
obj-$(CONFIG_UNIX98_PTYS) += devpts.o
devpts-$(CONFIG_UNIX98_PTYS) := inode.o
-devpts-$(CONFIG_DEVPTS_FS_SECURITY)+= xattr_security.o
diff -X /home/sds/dontdiff -Nrup
linux-2.6.13-rc6-mm2/fs/devpts/xattr_security.c
linux-2.6.13-rc6-mm2-xattr/fs/devpts/xattr_security.c
--- linux-2.6.13-rc6-mm2/fs/devpts/xattr_security.c 2005-06-17
15:48:29.0 -0400
+++ linux-2.6.13-rc6-mm2-xattr/fs/devpts/xattr_security.c 1969-12-31
19:00:00.0 -0500
@@ -1,47 +0,0 @@
-/*
- * Security xattr support for devpts.
- *
- * Author: Stephen Smalley <[EMAIL PROTECTED]>
- * Copyright (c) 2004 Red Hat, Inc., James Morris <[EMAIL PROTECTED]>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the Free
- * Software Foundation; either version 2 of the License, or (at your option)
- * any later version.
- */
-#include
-#include
-#include
-#include
-
-static size_t
-devpts_xattr_security_list(struct inode *inode, char *list, size_t list_len,
- const char *name, size_t name_len)
-{
- return security_inode_listsecurity(inode, list, list_len);
-}
-
-static int
-devpts_xattr_security_get(struct inode *inode, const char *name,
- void *buffer, size_t size)
-{
- if (strcmp(name, "") == 0)
- return -EINVAL;
-
Re: [PATCH][-mm] Generic VFS fallback for security xattrs
On Thu, 2005-08-25 at 13:43 -0400, Stephen Smalley wrote: > This patch modifies the VFS setxattr, getxattr, and listxattr code to > fall back to the security module for security xattrs if the filesystem > does not support xattrs natively. This allows security modules to > export the incore inode security label information to userspace even > if the filesystem does not provide xattr storage, and eliminates the > need to individually patch various pseudo filesystem types to provide > such access. The patch removes the existing xattr code from devpts > and tmpfs as it is then no longer needed. > > The patch restructures the code flow slightly to reduce duplication > between the normal path and the fallback path, but this should only > have one user-visible side effect - a program may get -EACCES rather > than -EOPNOTSUPP if policy denied access but the filesystem didn't > support the operation anyway. Note that the post_setxattr hook call > is not needed in the fallback case, as the inode_setsecurity hook call > handles the incore inode security state update directly. In contrast, > we do call fsnotify in both cases. > > Please include in -mm for wider testing prior to merging in 2.6.14. > > --- > > fs/Kconfig | 43 -- > fs/devpts/Makefile |1 > fs/devpts/inode.c | 21 --- > fs/devpts/xattr_security.c | 47 > fs/xattr.c | 80 +- > mm/shmem.c | 85 > - > 6 files changed, 49 insertions(+), 228 deletions(-) Sorry, forgot to explicitly sign off on the patch: Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]> -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] NFS: Ensure we support selinux xattrs
On Wed, 2007-01-24 at 11:54 -0800, Trond Myklebust wrote:
> From: Trond Myklebust <[EMAIL PROTECTED]>
>
> Some programs (e.g. GNU "cp") are currently crashing because NFS
> allows you to inquire about SELinux security attributes (vfs_getxattr()
> automatically handles this), but returns -EOPNOTSUPP when they later
> attempt to set the SELinux security attributes.
>
> The following patch just ensures that we pass the request to set security
> attributes on to the default SELinux handler.
That seems like a bug in cp, and it can happen under other circumstances
as well (mount with context= option).
I don't think you want to just set the incore inode security state on
the client side, as that won't be preserved.
> Signed-off-by: Trond Myklebust <[EMAIL PROTECTED]>
> ---
>
> fs/nfs/nfs3acl.c | 13 +
> fs/nfs/nfs4proc.c | 14 ++
> 2 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> index 7322da4..5970e7b 100644
> --- a/fs/nfs/nfs3acl.c
> +++ b/fs/nfs/nfs3acl.c
> @@ -4,6 +4,9 @@ #include
> #include
> #include
> #include
> +#include
> +#include
> +#include
>
> #define NFSDBG_FACILITY NFSDBG_PROC
>
> @@ -82,6 +85,16 @@ int nfs3_setxattr(struct dentry *dentry,
> struct posix_acl *acl;
> int type, error;
>
> + /* Selinux support */
> + if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
> + const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> + error = security_inode_setsecurity(inode, suffix, value,
> + size, flags);
> + if (!error)
> + fsnotify_xattr(dentry);
> + return error;
> + }
> +
> if (strcmp(name, POSIX_ACL_XATTR_ACCESS) == 0)
> type = ACL_TYPE_ACCESS;
> else if (strcmp(name, POSIX_ACL_XATTR_DEFAULT) == 0)
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index b3fd29b..ac1082b 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -48,6 +48,9 @@ #include
> #include
> #include
> #include
> +#include
> +#include
> +#include
>
> #include "nfs4_fs.h"
> #include "delegation.h"
> @@ -3547,6 +3550,17 @@ int nfs4_setxattr(struct dentry *dentry,
> {
> struct inode *inode = dentry->d_inode;
>
> + /* Selinux support */
> + if (!strncmp(key, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
> + const char *suffix = key + XATTR_SECURITY_PREFIX_LEN;
> + int error;
> + error = security_inode_setsecurity(inode, suffix, buf,
> + buflen, flags);
> + if (!error)
> + fsnotify_xattr(dentry);
> + return error;
> + }
> +
> if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
> return -EOPNOTSUPP;
>
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/5] LSM hook updates
On Fri, 2005-08-26 at 04:23 -0500, [EMAIL PROTECTED] wrote: > Here are some numbers on a 4way x86 - PIII 700Mhz with 1G memory (hmm, > highmem not enabled). I should hopefully have a 2way ppc available > later today for a pair of runs. > > dbench and tbench were run 50 times each, kernbench and reaim 10 times > each. Results are mean +/- 95% confidence half-interval. Kernel had > selinux and capabilities compiled in. > > A little surprising: kernbench is improved, but dbench and tbench > are worse - though within the 95% CI. Might be interesting to roll in Chris' patch (sent separately to lsm and selinux list) for "remove selinux stacked ops" in place of your patch, as that will avoid the indirect call through the secondary_ops in SELinux. At that point, you can also disable the capability module altogether, as SELinux will just directly use the built-in cap_ functions from commoncap. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/5] Rework stubs in security.h
On Fri, 2005-08-26 at 10:31 -0700, Tony Jones wrote:
> On Wed, Aug 24, 2005 at 06:20:30PM -0700, Chris Wright wrote:
>
> > static inline int security_ptrace (struct task_struct * parent, struct
> > task_struct * child)
> > {
> > +#ifdef CONFIG_SECURITY
> > return security_ops->ptrace (parent, child);
> > +#else
> > + return cap_ptrace (parent, child);
> > +#endif
> > +
> > }
With the third patch applied, it looks like this instead:
static inline int security_ptrace (struct task_struct * parent, struct
task_struct * child)
{
#ifdef CONFIG_SECURITY
if (security_ops->ptrace)
return security_ops->ptrace(parent, child);
#endif
return cap_ptrace (parent, child);
}
> The discussion about composing with commoncap made me think about whether
> this is the best way to do this. It seems that we're heading towards a
> requirement that every module internally compose with commoncap.
>
>
> If so (apart from the obvious correctness issues when they don't) it's work
> for each module and composing N of them under stacker obviously creates
> overhead.
Only matters if there are two or more modules that need to be used
together and both need to override/supplement the capability logic for a
given hook.
> Would the following not be a better approach?
>
> static inline int security_ptrace (struct task_struct * parent, struct
> task_struct * child)
> {
> int ret;
> ret=cap_ptrace (parent, child);
> #ifdef CONFIG_SECURITY
> if (!ret && security_ops->ptrace)
> ret=security_ops->ptrace(parent, child);
> #endif
> return ret;
> }
That makes capability part of the core kernel again, just like DAC,
which means that you can never override a capability denial in your
module. We sometimes want to override the capability implementation,
not just apply further restrictions after it. cap_inode_setxattr and
cap_inode_removexattr are examples; they prohibit any access to _all_
security attributes without CAP_SYS_ADMIN, whereas SELinux wants to
allow access to security.selinux if you pass a certain set of its own
permission checks. vm_enough_memory is another problem area due to vm
accounting handled internally.
> If every module is already internally composing, there shouldn't be a
> performance cost for the additional branch inside the #ifdef.
>
> I havn't looked at every single hook and it's users to see if this would
> cause a problem. I noticed SELinux calls sec->capget() post rather than pre
> it's processing which may be an issue.
That one isn't so much an issue as the xattr ones and vm_enough_memory
case. But more generally, if you think about moving toward a place
where one can grant privileges to processes based solely on their
role/domain, you'll need the same ability for capable and other hooks
too. Naturally, that can't be done safely without a lot more work on
userspace and policy, but it is a long term goal.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl selinux: Don't look at table->de
On Sun, 2007-01-28 at 12:21 -0700, Eric W. Biederman wrote:
> With the sysctl cleanups sysctl is not really a part of proc
> it just shows up there, and any path based approach will not
> adequately describe the data as sysctl is essentially a
> union mount underneath the covers. As designed this mechanism
> is viewer dependent so trying to be path based gets even worse.
>
> However the permissions in sys_sysctl are currently immutable
> and going through proc does not change the permission checks
> when accessing sysctl. So we might as well stick with the well
> defined sysctl sid, as that is what selinux uses when proc is
> not compiled in.
>
> I.e. I see no hope for salvaging the selinux_proc_get_sid call
> in selinux_sysctl so I'm removing it.
>
> Signed-off-by: Eric W. Biederman <[EMAIL PROTECTED]>
> ---
> security/selinux/hooks.c |8 ++--
> 1 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 7b38372..3a36057 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1438,12 +1438,8 @@ static int selinux_sysctl(ctl_table *table, int op)
>
> tsec = current->security;
>
> - rc = selinux_proc_get_sid(table->de, (op == 001) ?
> - SECCLASS_DIR : SECCLASS_FILE, &tsid);
> - if (rc) {
> - /* Default to the well-defined sysctl SID. */
> - tsid = SECINITSID_SYSCTL;
> - }
> + /* Use the well-defined sysctl SID. */
> + tsid = SECINITSID_SYSCTL;
>
> /* The op values are "defined" in sysctl.c, thereby creating
>* a bad coupling between this module and sysctl.c */
NAK. Mapping all sysctls to a single security label prevents any kind
of fine-grained security on sysctls, and current policies already make
use of the current distinctions to limit access to particular sets of
sysctls to particular processes. As is, I'd expect breakage of current
systems running SELinux from this patch, because (confined) processes
that formerly only required access to specific sysctl labels will
suddenly run into denials on the generic fallback label.
If the ctl_table supplied more information about the functional purpose
and the security sensitivity of the sysctl, then we could leverage that
information instead, as long as we can at least derive the current
labelings from that information for compatibility.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/7] breaking the global file_list_lock
On Sun, 2007-01-28 at 15:11 +, Christoph Hellwig wrote: > On Sun, Jan 28, 2007 at 02:43:25PM +, Christoph Hellwig wrote: > > On Sun, Jan 28, 2007 at 12:51:18PM +0100, Peter Zijlstra wrote: > > > This patch-set breaks up the global file_list_lock which was found to be a > > > severe contention point under basically any filesystem intensive workload. > > > > Benchmarks, please. Where exactly do you see contention for this? > > > > > > filesystem intensive workload apparently means namespace operation heavy > > workload, right? The biggest bottleneck I've seen with those is dcache > > lock. > > > > Even if this is becoming a real problem there must be simpler ways to fix > > this than introducing various new locking primitives and all kinds of > > complexity. > > One good way to fix scalability without all this braindamage is > to get rid of sb->s_files. Current uses are: > > - fs/dquot.c:add_dquot_ref() > > This performs it's actual operation on inodes. We should > be able to check inode->i_writecount to see which inodes > need quota initialization. > > - fs/file_table.c:fs_may_remount_ro() > > This one is gone in Dave Hansens per-mountpoint r/o patchkit > > - fs/proc/generic.c:proc_kill_inodes() > > This can be done with a list inside procfs. > > - fs/super.c:mark_files_ro() > > This one is only used for do_emergency_remount(), which is > and utter hack. It might be much better to just deny any > kind of write access through a superblock flag here. > > - fs/selinuxfs.c:sel_remove_bools() > > Utter madness. I have no idea how this ever got merged. > Maybe the selinux folks can explain what crack they were > on when writing this. The problem would go away with > a generic rewoke infrastructure. It was modeled after proc_kill_inodes(), as noted in the comments. So if you have a suitable replacement for proc_kill_inodes(), you can apply the same approach to sel_remove_bools(). > > Once sb->s_files is gone we can also kill of fu_list entirely and > replace it by a list head entirely in the tty code and make the lock > for it per-tty. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl selinux: Don't look at table->de
On Mon, 2007-01-29 at 10:43 -0700, Eric W. Biederman wrote:
> Stephen Smalley <[EMAIL PROTECTED]> writes:
>
> > On Sun, 2007-01-28 at 12:21 -0700, Eric W. Biederman wrote:
> >> With the sysctl cleanups sysctl is not really a part of proc
> >> it just shows up there, and any path based approach will not
> >> adequately describe the data as sysctl is essentially a
> >> union mount underneath the covers. As designed this mechanism
> >> is viewer dependent so trying to be path based gets even worse.
> >>
> >> However the permissions in sys_sysctl are currently immutable
> >> and going through proc does not change the permission checks
> >> when accessing sysctl. So we might as well stick with the well
> >> defined sysctl sid, as that is what selinux uses when proc is
> >> not compiled in.
> >>
> >> I.e. I see no hope for salvaging the selinux_proc_get_sid call
> >> in selinux_sysctl so I'm removing it.
> >>
> >> Signed-off-by: Eric W. Biederman <[EMAIL PROTECTED]>
> >> ---
> >> security/selinux/hooks.c |8 ++--
> >> 1 files changed, 2 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index 7b38372..3a36057 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -1438,12 +1438,8 @@ static int selinux_sysctl(ctl_table *table, int op)
> >>
> >>tsec = current->security;
> >>
> >> - rc = selinux_proc_get_sid(table->de, (op == 001) ?
> >> -SECCLASS_DIR : SECCLASS_FILE, &tsid);
> >> - if (rc) {
> >> - /* Default to the well-defined sysctl SID. */
> >> - tsid = SECINITSID_SYSCTL;
> >> - }
> >> + /* Use the well-defined sysctl SID. */
> >> + tsid = SECINITSID_SYSCTL;
> >>
> >>/* The op values are "defined" in sysctl.c, thereby creating
> >> * a bad coupling between this module and sysctl.c */
> >
> > NAK. Mapping all sysctls to a single security label prevents any kind
> > of fine-grained security on sysctls, and current policies already make
> > use of the current distinctions to limit access to particular sets of
> > sysctls to particular processes. As is, I'd expect breakage of current
> > systems running SELinux from this patch, because (confined) processes
> > that formerly only required access to specific sysctl labels will
> > suddenly run into denials on the generic fallback label.
>
> Reasonable. There is the issue that your code already had this code
> path for when /proc was compiled out.
True, but a system that disables proc is likely a system with a custom
policy anyway, and dependency on proc is fairly basic to selinux these
days (due to reliance on /proc/self/attr for process attribute
manipulation in place of the old selinux syscalls). Possibly we should
just make selinux depend on proc and drop the #ifdef there.
> > If the ctl_table supplied more information about the functional purpose
> > and the security sensitivity of the sysctl, then we could leverage that
> > information instead, as long as we can at least derive the current
> > labelings from that information for compatibility.
>
> What do information do you need to do need? Do you need extra fields in
> sysctl?
> I am more than willing to help but I am not familiar enough with selinux
> to do a reasonable job on my own.
At present, we map the sysctls into functional groups (e.g. net, vm,
fs, ...) that parallel the sysctl hierarchy so that we can limit access
to only those programs/processes that need access for their purpose, and
further partition where it makes sense to do so. We also separate out
particularly security sensitive ones like modprobe and hotplug. So if
the ctl_table carried some indication of functional grouping and
security relevance (for some relatively small number of equivalence
classes), then we could map those to labels instead of the current
scheme. And if we could have the ctl_table inherit the information from
its logical "parent" in the hierarchy by default, then it shouldn't
require too invasive a patch.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl selinux: Don't look at table->de
On Mon, 2007-01-29 at 10:55 -0700, Eric W. Biederman wrote: > James Morris <[EMAIL PROTECTED]> writes: > > > On Mon, 29 Jan 2007, Stephen Smalley wrote: > > > >> NAK. Mapping all sysctls to a single security label prevents any kind > >> of fine-grained security on sysctls, and current policies already make > >> use of the current distinctions to limit access to particular sets of > >> sysctls to particular processes. As is, I'd expect breakage of current > >> systems running SELinux from this patch, because (confined) processes > >> that formerly only required access to specific sysctl labels will > >> suddenly run into denials on the generic fallback label. > > > > Agreed, 100% NACK. > > > > Please don't just simply remove long-researched & analyzed MAC security > > which has been in the kernel for years, which is being used in the field > > for high assurance systems, because you neglected to consider it during a > > code cleanup. > > Please don't shoot the messenger when a weakness is found in your code. I'm not sure how breaking our code with your set of patches qualifies as finding a weakness. I will agree that the current handling of sysctl in selinux is fragile and can be improved, but it did work (prior to your patches). > Systems that increase security without worry that their implementation > is correct do not impress me, but I do understand that security has > little to do with correctness and everything to do with making it > _expensive_ for the other guy to do what he isn't supposed to. I think you misunderstand; we are concerned about the correctness of our implementation. I think that possibly you are misunderstanding one of the SELinux FAQ answers outside of its historical context (the initial release of a proof-of-concept implementation of flexible MAC in Dec 2000). > This code path was always in the selinux code for when /proc was > compiled out. I could see no way to preserve it so I removed > it. > > Not knowing if it was a problem, or if we needed to do something more > I copied the people who did, at the first available opportunity. > Before this code makes it's way into peoples production systems. Which we appreciate, although it would be nice if you tried building with selinux (and ideally testing with it) in the first place. > Of course after all of the rants against path based security I was > amazed to find a code path that was using exactly that in selinux. To clarify, in this case, the pathname (relative to the root of proc) is derived from the proc_dir_entry hierarchy and is thus not ambiguous or mutable by userspace, unlike the pathname-based approaches that we have criticized. There is a difference. But we are open to improving the approach via explicit marking of the ctl_table entries with sufficient information. > I'm trying to make things correct, and simple and will be happy to > work with you in a way to achieve what you need in a way that does > not conflict with the rest of the kernel. Good. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sysctl selinux: Don't look at table->de
On Mon, 2007-01-29 at 11:08 -0800, Casey Schaufler wrote: > --- Stephen Smalley <[EMAIL PROTECTED]> wrote: > > > True, but a system that disables proc is likely a > > system with a custom > > policy anyway, and dependency on proc is fairly > > basic to selinux these > > days (due to reliance on /proc/self/attr for process > > attribute > > manipulation in place of the old selinux syscalls). > > Possibly we should > > just make selinux depend on proc and drop the #ifdef > > there. > > Alternativly you could move the SELinux specific > bits out of /proc/self/attr into an equivalent > /selinux/self/attr and avoid that /proc dependency. We could, but I don't see any compelling reason to do so. We were specifically told to use proc for the selinux process attributes when we refactored the selinux api for 2.6 inclusion, as they are per-process state and fit naturally into proc. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: -mm merge plans for 2.6.20
On Mon, 2006-12-04 at 20:40 -0800, Andrew Morton wrote: > mprotect-patch-for-use-by-slim.patch > integrity-service-api-and-dummy-provider.patch > integrity-service-api-and-dummy-provider-cleanup-use-of-configh.patch > integrity-service-api-and-dummy-provider-compilation-warning-fix.patch > slim-main-patch.patch > slim-main-patch-socket_post_create-hook-return-code.patch > slim-main-patch-misc-cleanups-requested-at-inclusion-time.patch > slim-main-patch-handle-failure-to-register.patch > slim-main-patch-fix-bug-with-mm_users-usage.patch > slim-main-patch-security-slim-slm_mainc-make-2-functions-static.patch > slim-secfs-patch.patch > slim-secfs-patch-slim-correct-use-of-snprintf.patch > slim-secfs-patch-cleanup-use-of-configh.patch > slim-make-and-config-stuff.patch > slim-make-and-config-stuff-makefile-fix.patch > slim-debug-output.patch > slim-fix-security-issue-with-the-task_post_setuid-hook.patch > slim-secfs-inode-i_private-build-fix.patch > slim-documentation.patch > fdtable-make-fdarray-and-fdsets-equal-in-size-slim.patch > > Shall hold in -mm. Why? I haven't seen any evidence that prior review comments have been addressed so far, and a fresh patch set would be beneficial anyway to facilitate full review of the updated code and to allow them to fix their patch descriptions as well (as they were wrong in some instances, describing older versions of the code). -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: How is SELinux integrated into kernel 2.6?
On Thu, 2005-09-08 at 02:46 -0400, Xin Zhao wrote: > Sorry if this question is dumb. > > SELinux is included in 2.6. But I think it works by putting LSM hooks a lot > of place in Linux and then it can define its own policy enforcement codes. > > However, I cannot find hooks in kernel 2.6.9 and 2.6.11. How can > SELinux work with kernel 2.6 to protect system without hooks? > > Thanks in advance for your help! The hooks are there, but possibly you are confused by the out-of-date documentation (e.g. Documentation/DocBook/lsm.tmpl still says to look for "security_ops->" in the core kernel for the hook calls, but they have long since been replaced with calls to static inline functions defined in include/linux/security.h). As an example, fs/namei.c:permission calls security_inode_permission, which is defined in include/linux/security.h and will invoke the corresponding hook if CONFIG_SECURITY=y. SELinux provides its implementations of the hook functions in security/selinux/hooks.c, e.g. selinux_inode_permission. Hence, you should be looking for calls to functions with the security_ prefix instead of explicit references to security_ops in the core kernel. Chris - feel free to rip out lsm.tmpl and replace it with something more up-to-date and complete. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH v2 1/3] mm: Create utility function for accessing a tasks commandline value
On 01/13/2014 12:02 PM, William Roberts wrote:
> introduce get_cmdline() for retreiving the value of a processes
> proc/self/cmdline value.
>
> Signed-off-by: William Roberts
Acked-by: Stephen Smalley
> ---
> include/linux/mm.h |1 +
> mm/util.c | 48
> 2 files changed, 49 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 3552717..01e7970 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1134,6 +1134,7 @@ void account_page_writeback(struct page *page);
> int set_page_dirty(struct page *page);
> int set_page_dirty_lock(struct page *page);
> int clear_page_dirty_for_io(struct page *page);
> +int get_cmdline(struct task_struct *task, char *buffer, int buflen);
>
> /* Is the vma a continuation of the stack vma above it? */
> static inline int vma_growsdown(struct vm_area_struct *vma, unsigned long
> addr)
> diff --git a/mm/util.c b/mm/util.c
> index f7bc209..5285ff0 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -410,6 +410,54 @@ unsigned long vm_commit_limit(void)
> * sysctl_overcommit_ratio / 100) + total_swap_pages;
> }
>
> +/**
> + * get_cmdline() - copy the cmdline value to a buffer.
> + * @task: the task whose cmdline value to copy.
> + * @buffer: the buffer to copy to.
> + * @buflen: the length of the buffer. Larger cmdline values are truncated
> + *to this length.
> + * Returns the size of the cmdline field copied. Note that the copy does
> + * not guarantee an ending NULL byte.
> + */
> +int get_cmdline(struct task_struct *task, char *buffer, int buflen)
> +{
> + int res = 0;
> + unsigned int len;
> + struct mm_struct *mm = get_task_mm(task);
> + if (!mm)
> + goto out;
> + if (!mm->arg_end)
> + goto out_mm;/* Shh! No looking before we're done */
> +
> + len = mm->arg_end - mm->arg_start;
> +
> + if (len > buflen)
> + len = buflen;
> +
> + res = access_process_vm(task, mm->arg_start, buffer, len, 0);
> +
> + /*
> + * If the nul at the end of args has been overwritten, then
> + * assume application is using setproctitle(3).
> + */
> + if (res > 0 && buffer[res-1] != '\0' && len < buflen) {
> + len = strnlen(buffer, res);
> + if (len < res) {
> + res = len;
> + } else {
> + len = mm->env_end - mm->env_start;
> + if (len > buflen - res)
> + len = buflen - res;
> + res += access_process_vm(task, mm->env_start,
> + buffer+res, len, 0);
> + res = strnlen(buffer, res);
> + }
> + }
> +out_mm:
> + mmput(mm);
> +out:
> + return res;
> +}
>
> /* Tracepoints definitions. */
> EXPORT_TRACEPOINT_SYMBOL(kmalloc);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC][PATCH v2 2/3] proc: Update get proc_pid_cmdline() to use mm.h helpers
On 01/13/2014 12:02 PM, William Roberts wrote:
> Re-factor proc_pid_cmdline() to use get_cmdline() helper
> from mm.h.
>
> Signed-off-by: William Roberts
Acked-by: Stephen Smalley
> ---
> fs/proc/base.c | 36 ++--
> 1 file changed, 2 insertions(+), 34 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 03c8d74..cfd178d 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -200,41 +200,9 @@ static int proc_root_link(struct dentry *dentry, struct
> path *path)
> return result;
> }
>
> -static int proc_pid_cmdline(struct task_struct *task, char * buffer)
> +static int proc_pid_cmdline(struct task_struct *task, char *buffer)
> {
> - int res = 0;
> - unsigned int len;
> - struct mm_struct *mm = get_task_mm(task);
> - if (!mm)
> - goto out;
> - if (!mm->arg_end)
> - goto out_mm;/* Shh! No looking before we're done */
> -
> - len = mm->arg_end - mm->arg_start;
> -
> - if (len > PAGE_SIZE)
> - len = PAGE_SIZE;
> -
> - res = access_process_vm(task, mm->arg_start, buffer, len, 0);
> -
> - // If the nul at the end of args has been overwritten, then
> - // assume application is using setproctitle(3).
> - if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
> - len = strnlen(buffer, res);
> - if (len < res) {
> - res = len;
> - } else {
> - len = mm->env_end - mm->env_start;
> - if (len > PAGE_SIZE - res)
> - len = PAGE_SIZE - res;
> - res += access_process_vm(task, mm->env_start,
> buffer+res, len, 0);
> - res = strnlen(buffer, res);
> - }
> - }
> -out_mm:
> - mmput(mm);
> -out:
> - return res;
> + return get_cmdline(task, buffer, PAGE_SIZE);
> }
>
> static int proc_pid_auxv(struct task_struct *task, char *buffer)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] conditionally reschedule while loading selinux policy.
On 09/30/2013 01:24 PM, Dave Jones wrote: > On Mon, Sep 16, 2013 at 02:40:30PM -0400, Dave Jones wrote: > > On a slow machine (with debugging enabled), upgrading selinux policy may > take > > a considerable amount of time. Long enough that the softlockup detector > > gets triggered. > > > > The backtrace looks like this.. > > > > > BUG: soft lockup - CPU#2 stuck for 23s! [load_policy:19045] > > > Call Trace: > > > [] symcmp+0xf/0x20 > > > [] hashtab_search+0x47/0x80 > > > [] mls_convert_context+0xdc/0x1c0 > > > [] convert_context+0x378/0x460 > > > [] ? security_context_to_sid_core+0x240/0x240 > > > [] sidtab_map+0x45/0x80 > > > [] security_load_policy+0x3ff/0x580 > > With that patch applied, the problem seems to have moved elsewhere.. > > BUG: soft lockup - CPU#3 stuck for 22s! [load_policy:8119] > irq event stamp: 1590886 > hardirqs last enabled at (1590885): [] > __slab_alloc.constprop.78+0x4c0/0x4d7 > hardirqs last disabled at (1590886): [] > apic_timer_interrupt+0x6a/0x80 > softirqs last enabled at (1590336): [] > __do_softirq+0x169/0x200 > softirqs last disabled at (1590331): [] > irq_exit+0x11d/0x140 > RIP: 0010:[] [] > hashtab_insert+0x62/0x110 > > Call Trace: > [] policydb_read+0xc25/0x1200 > [] ? is_module_text_address+0x19/0x40 > [] security_load_policy+0x10e/0x580 > [] ? sched_clock_cpu+0xa8/0x100 > [] ? sched_clock_local+0x1d/0x80 > [] ? sched_clock_cpu+0xa8/0x100 > [] ? sched_clock_local+0x1d/0x80 > [] ? sched_clock_cpu+0xa8/0x100 > [] ? __change_page_attr_set_clr+0x82a/0xa50 > [] ? sched_clock_cpu+0xa8/0x100 > [] ? retint_restore_args+0xe/0xe > [] ? trace_hardirqs_on_caller+0xfd/0x1c0 > [] ? trace_hardirqs_on_thunk+0x3a/0x3f > [] ? rcu_irq_exit+0x68/0xb0 > [] ? retint_restore_args+0xe/0xe > [] sel_write_load+0xa7/0x770 > [] ? vfs_write+0x1c3/0x200 > [] ? security_file_permission+0x1e/0xa0 > [] vfs_write+0xbb/0x200 > [] ? fget_light+0x397/0x4b0 > [] SyS_write+0x47/0xa0 > [] tracesys+0xdd/0xe2 > > We're holding a bunch of locks here, so we can't just cond_resched. Thoughts > ? Sorry, what locks are we holding there? You ought to be able to do a cond_resched() anywhere during policydb_read() AFAIK; it is loading the policy into a new structure that isn't being accessed by anything else yet and the policy_rwlock is only held by security_load_policy after calling policydb_read and only to switch it into place as the active policydb. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] SELinux: reduce overhead of mls_level_isvalid() function call
On 06/05/2013 05:15 PM, Waiman Long wrote:
diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index 30f119b..100b3e6 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -213,7 +213,12 @@ netlbl_import_failure:
}
#endif /* CONFIG_NETLABEL */
-int ebitmap_contains(struct ebitmap *e1, struct ebitmap *e2)
+/*
+ * Check to see if all the bits set in e2 are also set in e1. Optionally,
+ * if last_e2bit is non-zero, the highest set bit in e2 cannot exceed
+ * last_e2bit.
+ */
+int ebitmap_contains(struct ebitmap *e1, struct ebitmap *e2, u32 last_e2bit)
{
struct ebitmap_node *n1, *n2;
int i;
@@ -223,6 +228,33 @@ int ebitmap_contains(struct ebitmap *e1, struct ebitmap
*e2)
n1 = e1->node;
n2 = e2->node;
+ if (last_e2bit) {
+ while (n1 && n2 && (n1->startbit <= n2->startbit)) {
+ int lastsetbit = -1;
+
+ if (n1->startbit < n2->startbit) {
+ n1 = n1->next;
+ continue;
+ }
+ for (i = EBITMAP_UNIT_NUMS - 1; i >= 0; i--) {
+ if (!n2->maps[i])
+ continue;
+ if ((n1->maps[i] & n2->maps[i]) != n2->maps[i])
+ return 0;
+ if (lastsetbit < 0)
+ lastsetbit = n2->startbit +
+i * EBITMAP_UNIT_SIZE +
+__fls(n2->maps[i]);
+ }
+ if ((lastsetbit >= 0) && (lastsetbit > last_e2bit))
+ return 0;
+
+ n1 = n1->next;
+ n2 = n2->next;
+ }
+ goto done;
+ }
+
Can't you unify this logic with the nearly identical logic below?
while (n1 && n2 && (n1->startbit <= n2->startbit)) {
if (n1->startbit < n2->startbit) {
n1 = n1->next;
@@ -237,6 +269,7 @@ int ebitmap_contains(struct ebitmap *e1, struct ebitmap *e2)
n2 = n2->next;
}
+done:
if (n2)
return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v5] SELinux: Reduce overhead of mls_level_isvalid() function call
On 06/10/2013 01:55 PM, Waiman Long wrote:
v4->v5:
- Fix scripts/checkpatch.pl warning.
v3->v4:
- Merge the 2 separate while loops in ebitmap_contains() into
a single one.
v2->v3:
- Remove unused local variables i, node from mls_level_isvalid().
v1->v2:
- Move the new ebitmap comparison logic from mls_level_isvalid()
into the ebitmap_contains() helper function.
- Rerun perf and performance tests on the latest v3.10-rc4 kernel.
While running the high_systime workload of the AIM7 benchmark on
a 2-socket 12-core Westmere x86-64 machine running 3.10-rc4 kernel
(with HT on), it was found that a pretty sizable amount of time was
spent in the SELinux code. Below was the perf trace of the "perf
record -a -s" of a test run at 1500 users:
5.04%ls [kernel.kallsyms] [k] ebitmap_get_bit
1.96%ls [kernel.kallsyms] [k] mls_level_isvalid
1.95%ls [kernel.kallsyms] [k] find_next_bit
The ebitmap_get_bit() was the hottest function in the perf-report
output. Both the ebitmap_get_bit() and find_next_bit() functions
were, in fact, called by mls_level_isvalid(). As a result, the
mls_level_isvalid() call consumed 8.95% of the total CPU time of
all the 24 virtual CPUs which is quite a lot. The majority of the
mls_level_isvalid() function invocations come from the socket creation
system call.
Looking at the mls_level_isvalid() function, it is checking to see
if all the bits set in one of the ebitmap structure are also set in
another one as well as the highest set bit is no bigger than the one
specified by the given policydb data structure. It is doing it in
a bit-by-bit manner. So if the ebitmap structure has many bits set,
the iteration loop will be done many times.
The current code can be rewritten to use a similar algorithm as the
ebitmap_contains() function with an additional check for the
highest set bit. The ebitmap_contains() function was extended to
cover an optional additional check for the highest set bit, and the
mls_level_isvalid() function was modified to call ebitmap_contains().
With that change, the perf trace showed that the used CPU time drop
down to just 0.08% (ebitmap_contains + mls_level_isvalid) of the
total which is about 100X less than before.
0.07%ls [kernel.kallsyms] [k] ebitmap_contains
0.05%ls [kernel.kallsyms] [k] ebitmap_get_bit
0.01%ls [kernel.kallsyms] [k] mls_level_isvalid
0.01%ls [kernel.kallsyms] [k] find_next_bit
The remaining ebitmap_get_bit() and find_next_bit() functions calls
are made by other kernel routines as the new mls_level_isvalid()
function will not call them anymore.
This patch also improves the high_systime AIM7 benchmark result,
though the improvement is not as impressive as is suggested by the
reduction in CPU time spent in the ebitmap functions. The table below
shows the performance change on the 2-socket x86-64 system (with HT
on) mentioned above.
+--+---++-+
| Workload | mean % change | mean % change | mean % change |
| | 10-100 users | 200-1000 users | 1100-2000 users |
+--+---++-+
| high_systime | +0.1% | +0.9% | +2.6% |
+--+---++-+
Signed-off-by: Waiman Long
Acked-by: Stephen Smalley
---
security/selinux/ss/ebitmap.c | 20 ++--
security/selinux/ss/ebitmap.h |2 +-
security/selinux/ss/mls.c | 22 +++---
security/selinux/ss/mls_types.h |2 +-
4 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index 30f119b..820313a 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -213,7 +213,12 @@ netlbl_import_failure:
}
#endif /* CONFIG_NETLABEL */
-int ebitmap_contains(struct ebitmap *e1, struct ebitmap *e2)
+/*
+ * Check to see if all the bits set in e2 are also set in e1. Optionally,
+ * if last_e2bit is non-zero, the highest set bit in e2 cannot exceed
+ * last_e2bit.
+ */
+int ebitmap_contains(struct ebitmap *e1, struct ebitmap *e2, u32 last_e2bit)
{
struct ebitmap_node *n1, *n2;
int i;
@@ -223,14 +228,25 @@ int ebitmap_contains(struct ebitmap *e1, struct ebitmap
*e2)
n1 = e1->node;
n2 = e2->node;
+
while (n1 && n2 && (n1->startbit <= n2->startbit)) {
if (n1->startbit < n2->startbit) {
n1 = n1->next;
continue;
}
- for (i = 0; i < EBITMAP_UNIT_NUMS; i++) {
+ for (i = EBITMAP_UNIT_NUMS - 1; (i >= 0) && !n2->maps[i]; )
+ i--;/* Skip trailing NULL map entries */
+
Re: [PATCH v2 2/2] SELinux: Increase ebitmap_node size for 64-bit configuration
On 06/05/2013 05:15 PM, Waiman Long wrote: Currently, the ebitmap_node structure has a fixed size of 32 bytes. On a 32-bit system, the overhead is 8 bytes, leaving 24 bytes for being used as bitmaps. The overhead ratio is 1/4. On a 64-bit system, the overhead is 16 bytes. Therefore, only 16 bytes are left for bitmap purpose and the overhead ratio is 1/2. With a 3.8.2 kernel, a boot-up operation will cause the ebitmap_get_bit() function to be called about 9 million times. The average number of ebitmap_node traversal is about 3.7. This patch increases the size of the ebitmap_node structure to 64 bytes for 64-bit system to keep the overhead ratio at 1/4. This may also improve performance a little bit by making node to node traversal less frequent (< 2) as more bits are available in each node. Signed-off-by: Waiman Long Acked-by: Stephen Smalley --- security/selinux/ss/ebitmap.h |8 +++- 1 files changed, 7 insertions(+), 1 deletions(-) diff --git a/security/selinux/ss/ebitmap.h b/security/selinux/ss/ebitmap.h index e7eb3a9..712c8a7 100644 --- a/security/selinux/ss/ebitmap.h +++ b/security/selinux/ss/ebitmap.h @@ -16,7 +16,13 @@ #include -#define EBITMAP_UNIT_NUMS ((32 - sizeof(void *) - sizeof(u32))\ +#ifdef CONFIG_64BIT +#defineEBITMAP_NODE_SIZE 64 +#else +#defineEBITMAP_NODE_SIZE 32 +#endif + +#define EBITMAP_UNIT_NUMS ((EBITMAP_NODE_SIZE-sizeof(void *)-sizeof(u32))\ / sizeof(unsigned long)) #define EBITMAP_UNIT_SIZE BITS_PER_LONG #define EBITMAP_SIZE (EBITMAP_UNIT_NUMS * EBITMAP_UNIT_SIZE) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: selinux_msg_queue_msgrcv() oops
On 02/06/2013 07:56 AM, Tommi Rantala wrote:
Hello,
I'm hitting an oops in selinux_msg_queue_msgrcv() when fuzzing with
Trinity as the root user (in a qemu VM):
NULL msg->security at that point is a bug in the ipc subsystem; SELinux
is just the messenger. Normally msg->security is set for every
allocated msg by load_msg() -> security_msg_msg_alloc() ->
selinux_msg_msg_alloc_security(), and freed/cleared upon free_msg() ->
security_msg_msg_free() -> selinux_msg_msg_free_security(). Looking
around, I see copy_msg() introduced for checkpoint-restore initializes
dst->security to NULL but never sets it properly?
[12578.053111] BUG: unable to handle kernel NULL pointer dereference
at (null)
[12578.054025] IP: [] selinux_msg_queue_msgrcv+0xda/0x1e0
[12578.054025] PGD 29961067 PUD 34dc5067 PMD 0
[12578.054025] Oops: [#2] SMP
[12578.054025] CPU 1
[12578.054025] Pid: 23453, comm: trinity-child23 Tainted: G D W
3.8.0-rc6+ #31 Bochs Bochs
[12578.054025] RIP: 0010:[] []
selinux_msg_queue_msgrcv+0xda/0x1e0
[12578.054025] RSP: 0018:88002b6b5e18 EFLAGS: 00010246
[12578.054025] RAX: RBX: 88003132d410 RCX: 0001
[12578.054025] RDX: 88000e8bc560 RSI: 0001 RDI: 0246
[12578.054025] RBP: 88002b6b5e68 R08: R09:
[12578.054025] R10: 88000e8bc560 R11: R12: 0001
[12578.054025] R13: R14: 880006449500 R15: 88003132d410
[12578.054025] FS: 7f7385059700() GS:88003e20()
knlGS:
[12578.054025] CS: 0010 DS: ES: CR0: 80050033
[12578.054025] CR2: CR3: 303a2000 CR4: 06e0
[12578.054025] DR0: DR1: DR2:
[12578.054025] DR3: DR6: 0ff0 DR7: 0400
[12578.054025] Process trinity-child23 (pid: 23453, threadinfo
88002b6b4000, task 88000e8bc560)
[12578.054025] Stack:
[12578.054025] 8131e105 81313f69 88002b6b5e04
[12578.054025] 812fd6f5 88003a89c1c0
0001
[12578.054025] 88003132d4c0 88002b6b5e78
81314086
[12578.054025] Call Trace:
[12578.054025] [] ? selinux_msg_queue_msgrcv+0x5/0x1e0
[12578.054025] [] ? security_ipc_permission+0x19/0x20
[12578.054025] [] ? ipc_lock+0x5/0x1c0
[12578.054025] [] security_msg_queue_msgrcv+0x16/0x20
[12578.054025] [] do_msgrcv+0x1ef/0x6e0
[12578.054025] [] ? load_msg+0x180/0x180
[12578.054025] [] ? lockdep_sys_exit_thunk+0x35/0x67
[12578.054025] [] ? trace_hardirqs_on_caller+0x16/0x1a0
[12578.054025] [] ? trace_hardirqs_on_thunk+0x3a/0x3f
[12578.054025] [] sys_msgrcv+0x15/0x20
[12578.054025] [] system_call_fastpath+0x16/0x1b
[12578.054025] Code: 4c 8d 45 c0 45 31 c9 b9 10 00 00 00 44 89 e7 4d
8b 6d 28 c6 45 c0 04 89 55 c8 8b 70 04 ba 1b 00 00 00 e8 fa 7a ff ff
85 c0 75 1d <41> 8b 75 00 4c 8d 45 c0 45 31 c9 b9 02 00 00 00 ba 1a 00
00 00
[12578.054025] RIP [] selinux_msg_queue_msgrcv+0xda/0x1e0
[12578.054025] RSP
[12578.054025] CR2:
[12578.142292] ---[ end trace 36aee1c7bfea7f83 ]---
After adding:
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 54aaa72..20cec57 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4982,9 +4982,12 @@ static int selinux_msg_queue_msgrcv(struct
msg_queue *msq, struct msg_msg *msg,
rc = avc_has_perm(sid, isec->sid,
SECCLASS_MSGQ, MSGQ__READ, &ad);
- if (!rc)
+ if (!rc) {
+ WARN(msec == NULL, "msec is NULL!");
+
rc = avc_has_perm(sid, msec->sid,
SECCLASS_MSG, MSG__RECEIVE, &ad);
+ }
return rc;
}
I see:
[ 43.103283] [ cut here ]
[ 43.104236] WARNING: at
/home/ttrantal/git/linux-2.6/security/selinux/hooks.c:4986
selinux_msg_queue_msgrcv+0x1ff/0x210()
[ 43.106088] Hardware name: Bochs
[ 43.106640] msec is NULL!Pid: 2387, comm: trinity-child9 Not
tainted 3.8.0-rc6+ #37
[ 43.107950] Call Trace:
[ 43.108393] [] ? selinux_msg_queue_msgrcv+0x1ff/0x210
[ 43.109534] [] warn_slowpath_common+0x7a/0xb0
[ 43.110565] [] warn_slowpath_fmt+0x46/0x50
[ 43.111561] [] selinux_msg_queue_msgrcv+0x1ff/0x210
[ 43.112677] [] ? selinux_msg_queue_msgrcv+0x5/0x210
[ 43.113808] [] ? security_ipc_permission+0x19/0x20
[ 43.114919] [] ? ipc_lock+0x5/0x1c0
[ 43.115817] [] security_msg_queue_msgrcv+0x16/0x20
[ 43.116929] [] do_msgrcv+0x1ef/0x6e0
[ 43.117909] [] ? load_msg+0x180/0x180
[ 43.118850] [] ? trace_hardirqs_on_caller+0x10d/0x1a0
[ 43.120019] [] ? trace_hardirqs_on_thunk+0x3a/0x3f
[ 43.121126] [] sys_msgrcv+0x15/0x20
[ 43.122001] [] system_call_fastpath+0x16/0x1b
[ 43.123044] ---[ end trace db5952f0fa3bedc7 ]---
[ 43.123815]
[ 43.124096]
Re: selinux_msg_queue_msgrcv() oops
On 02/06/2013 10:21 AM, Tommi Rantala wrote:
2013/2/6 Stephen Smalley :
On 02/06/2013 07:56 AM, Tommi Rantala wrote:
Hello,
I'm hitting an oops in selinux_msg_queue_msgrcv() when fuzzing with
Trinity as the root user (in a qemu VM):
NULL msg->security at that point is a bug in the ipc subsystem; SELinux is
just the messenger. Normally msg->security is set for every allocated msg
by load_msg() -> security_msg_msg_alloc() ->
selinux_msg_msg_alloc_security(), and freed/cleared upon free_msg() ->
security_msg_msg_free() -> selinux_msg_msg_free_security(). Looking around,
I see copy_msg() introduced for checkpoint-restore initializes dst->security
to NULL but never sets it properly?
I am indeed building with CONFIG_CHECKPOINT_RESTORE=y, so your
analysis seems to be correct.
(cc originator of the bug)
If I am reading this correctly, then when the copy msg was created, a
msg security struct was already allocated
(prepare_copy->load_msg->security_msg_msg_alloc). So having copy_msg()
clear dst->security is also a memory leak in addition to leading to this
oops. Attached is a possible, un-tested fix.
[12578.053111] BUG: unable to handle kernel NULL pointer dereference
at (null)
[12578.054025] IP: []
selinux_msg_queue_msgrcv+0xda/0x1e0
[12578.054025] PGD 29961067 PUD 34dc5067 PMD 0
[12578.054025] Oops: [#2] SMP
[12578.054025] CPU 1
[12578.054025] Pid: 23453, comm: trinity-child23 Tainted: G D W
3.8.0-rc6+ #31 Bochs Bochs
[12578.054025] RIP: 0010:[] []
selinux_msg_queue_msgrcv+0xda/0x1e0
[12578.054025] RSP: 0018:88002b6b5e18 EFLAGS: 00010246
[12578.054025] RAX: RBX: 88003132d410 RCX:
0001
[12578.054025] RDX: 88000e8bc560 RSI: 0001 RDI:
0246
[12578.054025] RBP: 88002b6b5e68 R08: R09:
[12578.054025] R10: 88000e8bc560 R11: R12:
0001
[12578.054025] R13: R14: 880006449500 R15:
88003132d410
[12578.054025] FS: 7f7385059700() GS:88003e20()
knlGS:
[12578.054025] CS: 0010 DS: ES: CR0: 80050033
[12578.054025] CR2: CR3: 303a2000 CR4:
06e0
[12578.054025] DR0: DR1: DR2:
[12578.054025] DR3: DR6: 0ff0 DR7:
0400
[12578.054025] Process trinity-child23 (pid: 23453, threadinfo
88002b6b4000, task 88000e8bc560)
[12578.054025] Stack:
[12578.054025] 8131e105 81313f69 88002b6b5e04
[12578.054025] 812fd6f5 88003a89c1c0
0001
[12578.054025] 88003132d4c0 88002b6b5e78
81314086
[12578.054025] Call Trace:
[12578.054025] [] ? selinux_msg_queue_msgrcv+0x5/0x1e0
[12578.054025] [] ? security_ipc_permission+0x19/0x20
[12578.054025] [] ? ipc_lock+0x5/0x1c0
[12578.054025] [] security_msg_queue_msgrcv+0x16/0x20
[12578.054025] [] do_msgrcv+0x1ef/0x6e0
[12578.054025] [] ? load_msg+0x180/0x180
[12578.054025] [] ? lockdep_sys_exit_thunk+0x35/0x67
[12578.054025] [] ? trace_hardirqs_on_caller+0x16/0x1a0
[12578.054025] [] ? trace_hardirqs_on_thunk+0x3a/0x3f
[12578.054025] [] sys_msgrcv+0x15/0x20
[12578.054025] [] system_call_fastpath+0x16/0x1b
[12578.054025] Code: 4c 8d 45 c0 45 31 c9 b9 10 00 00 00 44 89 e7 4d
8b 6d 28 c6 45 c0 04 89 55 c8 8b 70 04 ba 1b 00 00 00 e8 fa 7a ff ff
85 c0 75 1d <41> 8b 75 00 4c 8d 45 c0 45 31 c9 b9 02 00 00 00 ba 1a 00
00 00
[12578.054025] RIP []
selinux_msg_queue_msgrcv+0xda/0x1e0
[12578.054025] RSP
[12578.054025] CR2:
[12578.142292] ---[ end trace 36aee1c7bfea7f83 ]---
After adding:
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 54aaa72..20cec57 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4982,9 +4982,12 @@ static int selinux_msg_queue_msgrcv(struct
msg_queue *msq, struct msg_msg *msg,
rc = avc_has_perm(sid, isec->sid,
SECCLASS_MSGQ, MSGQ__READ, &ad);
- if (!rc)
+ if (!rc) {
+ WARN(msec == NULL, "msec is NULL!");
+
rc = avc_has_perm(sid, msec->sid,
SECCLASS_MSG, MSG__RECEIVE, &ad);
+ }
return rc;
}
I see:
[ 43.103283] [ cut here ]
[ 43.104236] WARNING: at
/home/ttrantal/git/linux-2.6/security/selinux/hooks.c:4986
selinux_msg_queue_msgrcv+0x1ff/0x210()
[ 43.106088] Hardware name: Bochs
[ 43.106640] msec is NULL!Pid: 2387, comm: trinity-child9 Not
tainted 3.8.0-rc6+ #37
[ 43.107950] Call Trace:
[ 43.108393] [] ?
selinux_msg_queue_msgrcv+0x1ff/0x210
[ 43.109534] [] warn_slowpath_common+0x7a/0xb0
[ 43.110565] [] warn_slowpath_fmt+0x46/0x50
[ 43.111561] [] selinux_msg_queue_msgrcv+0x
Re: lockup during selinux policy load.
On 09/16/2013 01:30 PM, Dave Jones wrote:
> On a slow machine (with debugging enabled), during a yum update I get
> the soft lockup detector kicking in when it gets to reloading the selinux
> policy.
> It looks like this..
>
>
> BUG: soft lockup - CPU#2 stuck for 23s! [load_policy:19045]
> irq event stamp: 2368864
> hardirqs last enabled at (2368863): []
> __slab_alloc.constprop.78+0x4c0/0x4d7
> hardirqs last disabled at (2368864): []
> apic_timer_interrupt+0x6a/0x80
> softirqs last enabled at (2368554): []
> __do_softirq+0x169/0x200
> softirqs last disabled at (2368539): [] irq_exit+0x11d/0x140
> CPU: 2 PID: 19045 Comm: load_policy Not tainted 3.11.0+ #16
> Hardware name: /D510MO, BIOS
> MOPNV10J.86A.0175.2010.0308.0620 03/08/2010
> task: 88005ab38000 ti: 88001962 task.ti: 88001962
> RIP: 0010:[] [] strcmp+0x23/0x40
> RSP: 0018:880019621818 EFLAGS: 0246
> RAX: 0063 RBX: 880018090ca8 RCX: 6070
> RDX: 88000781d8f0 RSI: 88000781d8f1 RDI: 880079caed21
> RBP: 880019621818 R08: 88006345a290 R09: 880018091680
> R10: 0001 R11: 0001 R12: 0292
> R13: 000180160016 R14: 88007b804488 R15: 81221430
> FS: 7f73e1212800() GS:88007e60() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: 7f73e0857aa0 CR3: 76fea000 CR4: 07e0
> Stack:
> 880019621828 81221ddf 880019621850 81221c27
> 8800180ac6d8 880019621b88 82759620 8800196218a0
> 8122e96c 000181221e05 880018091680 0080
> Call Trace:
> [] symcmp+0xf/0x20
> [] hashtab_search+0x47/0x80
> [] mls_convert_context+0xdc/0x1c0
> [] convert_context+0x378/0x460
> [] ? security_context_to_sid_core+0x240/0x240
> [] sidtab_map+0x45/0x80
> [] security_load_policy+0x3ff/0x580
> [] ? sched_clock_cpu+0xa8/0x100
> [] ? sched_clock_local+0x1d/0x80
> [] ? sched_clock_cpu+0xa8/0x100
> [] ? __change_page_attr_set_clr+0x82a/0xa50
> [] ? sched_clock_local+0x1d/0x80
> [] ? sched_clock_cpu+0xa8/0x100
> [] ? __change_page_attr_set_clr+0x82a/0xa50
> [] ? sched_clock_cpu+0xa8/0x100
> [] ? retint_restore_args+0xe/0xe
> [] ? trace_hardirqs_on_caller+0xfd/0x1c0
> [] ? trace_hardirqs_on_thunk+0x3a/0x3f
> [] ? rcu_irq_exit+0x68/0xb0
> [] ? retint_restore_args+0xe/0xe
> [] sel_write_load+0xa7/0x770
> [] ? vfs_write+0x1c3/0x200
> [] ? security_file_permission+0x1e/0xa0
> [] vfs_write+0xbb/0x200
> [] ? fget_light+0x397/0x4b0
> [] SyS_write+0x47/0xa0
> [] tracesys+0xdd/0xe2
> Code: 0f 1f 84 00 00 00 00 00 55 48 89 e5 eb 0e 66 2e 0f 1f 84 00 00 00 00 00
> 84 c0 74 1c 48 83 c7 01 0f b6 47 ff 48 83 c6 01 3a 46 ff <74> eb 19 c0 83 c8
> 01 5d c3 0f 1f 40 00 31 c0 5d c3 66 66 66 2e
>
>
> 23s in the kernel is an eternity. Short of rearchitecting how policy loads
> are done,
> perhaps we could do something like this ? (untested, and 1 is arbitarily
> chosen, may need to be adjusted)
>
> thoughts ?
Maybe put a cond_resched() within the ebitmap_for_each_positive_bit()
loop in mls_convert_context()?
>
> Dave
>
> diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c
> index 933e735..69a0587 100644
> --- a/security/selinux/ss/hashtab.c
> +++ b/security/selinux/ss/hashtab.c
> @@ -75,14 +75,21 @@ void *hashtab_search(struct hashtab *h, const void *key)
> {
> u32 hvalue;
> struct hashtab_node *cur;
> + int count;
>
> if (!h)
> return NULL;
>
> hvalue = h->hash_value(h, key);
> cur = h->htable[hvalue];
> - while (cur && h->keycmp(h, key, cur->key) > 0)
> + while (cur && h->keycmp(h, key, cur->key) > 0) {
> cur = cur->next;
> + count++;
> + if (count == 1) {
> + touch_softlockup_watchdog();
> + count = 0;
> + }
> + }
>
> if (cur == NULL || (h->keycmp(h, key, cur->key) != 0))
> return NULL;
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] conditionally reschedule while loading selinux policy.
On 09/16/2013 02:40 PM, Dave Jones wrote: > On a slow machine (with debugging enabled), upgrading selinux policy may take > a considerable amount of time. Long enough that the softlockup detector > gets triggered. > > The backtrace looks like this.. > > > BUG: soft lockup - CPU#2 stuck for 23s! [load_policy:19045] > > Call Trace: > > [] symcmp+0xf/0x20 > > [] hashtab_search+0x47/0x80 > > [] mls_convert_context+0xdc/0x1c0 > > [] convert_context+0x378/0x460 > > [] ? security_context_to_sid_core+0x240/0x240 > > [] sidtab_map+0x45/0x80 > > [] security_load_policy+0x3ff/0x580 > > [] ? sched_clock_cpu+0xa8/0x100 > > [] ? sched_clock_local+0x1d/0x80 > > [] ? sched_clock_cpu+0xa8/0x100 > > [] ? __change_page_attr_set_clr+0x82a/0xa50 > > [] ? sched_clock_local+0x1d/0x80 > > [] ? sched_clock_cpu+0xa8/0x100 > > [] ? __change_page_attr_set_clr+0x82a/0xa50 > > [] ? sched_clock_cpu+0xa8/0x100 > > [] ? retint_restore_args+0xe/0xe > > [] ? trace_hardirqs_on_caller+0xfd/0x1c0 > > [] ? trace_hardirqs_on_thunk+0x3a/0x3f > > [] ? rcu_irq_exit+0x68/0xb0 > > [] ? retint_restore_args+0xe/0xe > > [] sel_write_load+0xa7/0x770 > > [] ? vfs_write+0x1c3/0x200 > > [] ? security_file_permission+0x1e/0xa0 > > [] vfs_write+0xbb/0x200 > > [] ? fget_light+0x397/0x4b0 > > [] SyS_write+0x47/0xa0 > > [] tracesys+0xdd/0xe2 > > Stephen Smalley suggested: > > > Maybe put a cond_resched() within the ebitmap_for_each_positive_bit() > > loop in mls_convert_context()? > > That seems to do the trick. Tested by downgrading and re-upgrading > selinux-policy-targeted. > > Signed-off-by: Dave Jones Acked-by: Stephen Smalley > > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c > index 40de8d3..9ef8e51 100644 > --- a/security/selinux/ss/mls.c > +++ b/security/selinux/ss/mls.c > @@ -500,6 +500,8 @@ int mls_convert_context(struct policydb *oldp, > rc = ebitmap_set_bit(&bitmap, catdatum->value - 1, 1); > if (rc) > return rc; > + > + cond_resched(); > } > ebitmap_destroy(&c->range.level[l].cat); > c->range.level[l].cat = bitmap; > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] LSM: Pass comm name via get_task_comm() [was: Re: [PATCH] Change task_struct->comm to use RCU.]
On 03/27/2014 01:20 PM, Richard Guy Briggs wrote: > On 14/03/12, James Morris wrote: >> On Tue, 11 Mar 2014, Tetsuo Handa wrote: >> >>> And the same phrase goes to James Morris... >>> >>> If you are sure that it is safe to use get_task_comm() from >>> dump_common_audit_data() and you prefer locked version, please pick up below >>> patch via your git tree. >>> >>> If you are unsure or prefer lockless version, I'll make a lockless version >>> using do_get_task_comm() proposed in this thread. >> >> If you can't understand whether your patch is correct or not, don't ask me >> to apply it to my tree. >> >> If you're unsure, get it reviewed first. > > Steve (see https://lkml.org/lkml/2014/3/11/218 ) and James, > > Are the labels on data output in LSM_AUDIT_DATA_TASK even right? The > general case gives pid and comm of current. Then the > LSM_AUDIT_DATA_TASK case gives pid and comm from the task handed in in > the struct common_audit_data pointer. They are a duplicate of the > general case without generating a new message. I expect this will cause > ausearch to ignore those latter two fields. Should the latter two be > renamed to something like ad_pid= and ad_comm= ? Hmmm..only seems to be used by Smack. SELinux had a tsk field in common_audit_data that was removed by b466066. This other tsk field seems to have been added for Smack by 6e837fb. That said, it would be nice to have pid/comm info for the target of a signal check as well as current. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] audit: get comm using lock to avoid race in string printing
On 03/15/2014 07:29 PM, Richard Guy Briggs wrote:
> ---
> kernel/audit.c |5 ++---
> kernel/auditsc.c |9 +
> 2 files changed, 7 insertions(+), 7 deletions(-)
Doesn't this also need to be fixed (twice) in security/lsm_audit.c?
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 9239e5e..5b600c8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1883,7 +1883,7 @@ EXPORT_SYMBOL(audit_log_task_context);
> void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
> {
> const struct cred *cred;
> - char name[sizeof(tsk->comm)];
> + char comm[sizeof(tsk->comm)];
> struct mm_struct *mm = tsk->mm;
> char *tty;
>
> @@ -1917,9 +1917,8 @@ void audit_log_task_info(struct audit_buffer *ab,
> struct task_struct *tsk)
>from_kgid(&init_user_ns, cred->fsgid),
>tty, audit_get_sessionid(tsk));
>
> - get_task_comm(name, tsk);
> audit_log_format(ab, " comm=");
> - audit_log_untrustedstring(ab, name);
> + audit_log_untrustedstring(ab, get_task_comm(name, tsk));
>
> if (mm) {
> down_read(&mm->mmap_sem);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 8fffca8..b789e0c 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2289,7 +2289,7 @@ void __audit_ptrace(struct task_struct *t)
> context->target_uid = task_uid(t);
> context->target_sessionid = audit_get_sessionid(t);
> security_task_getsecid(t, &context->target_sid);
> - memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
> + get_task_comm(context->target_comm, t);
> }
>
> /**
> @@ -2328,7 +2328,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
> ctx->target_uid = t_uid;
> ctx->target_sessionid = audit_get_sessionid(t);
> security_task_getsecid(t, &ctx->target_sid);
> - memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
> + get_task_comm(ctx->target_comm, t);
> return 0;
> }
>
> @@ -2349,7 +2349,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
> axp->target_uid[axp->pid_count] = t_uid;
> axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
> security_task_getsecid(t, &axp->target_sid[axp->pid_count]);
> - memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
> + get_task_comm(axp->target_comm[axp->pid_count], t);
> axp->pid_count++;
>
> return 0;
> @@ -2435,6 +2435,7 @@ static void audit_log_task(struct audit_buffer *ab)
> kgid_t gid;
> unsigned int sessionid;
> struct mm_struct *mm = current->mm;
> + char comm[sizeof(current->comm)];
>
> auid = audit_get_loginuid(current);
> sessionid = audit_get_sessionid(current);
> @@ -2447,7 +2448,7 @@ static void audit_log_task(struct audit_buffer *ab)
>sessionid);
> audit_log_task_context(ab);
> audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
> - audit_log_untrustedstring(ab, current->comm);
> + audit_log_untrustedstring(ab, get_task_comm(comm, current);
> if (mm) {
> down_read(&mm->mmap_sem);
> if (mm->exe_file)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v5] SELinux: Reduce overhead of mls_level_isvalid() function call
On 07/05/2013 01:10 PM, Waiman Long wrote: On 06/11/2013 07:49 AM, Stephen Smalley wrote: On 06/10/2013 01:55 PM, Waiman Long wrote: v4->v5: - Fix scripts/checkpatch.pl warning. v3->v4: - Merge the 2 separate while loops in ebitmap_contains() into a single one. v2->v3: - Remove unused local variables i, node from mls_level_isvalid(). v1->v2: - Move the new ebitmap comparison logic from mls_level_isvalid() into the ebitmap_contains() helper function. - Rerun perf and performance tests on the latest v3.10-rc4 kernel. While running the high_systime workload of the AIM7 benchmark on a 2-socket 12-core Westmere x86-64 machine running 3.10-rc4 kernel (with HT on), it was found that a pretty sizable amount of time was spent in the SELinux code. Below was the perf trace of the "perf record -a -s" of a test run at 1500 users: 5.04%ls [kernel.kallsyms] [k] ebitmap_get_bit 1.96%ls [kernel.kallsyms] [k] mls_level_isvalid 1.95%ls [kernel.kallsyms] [k] find_next_bit The ebitmap_get_bit() was the hottest function in the perf-report output. Both the ebitmap_get_bit() and find_next_bit() functions were, in fact, called by mls_level_isvalid(). As a result, the mls_level_isvalid() call consumed 8.95% of the total CPU time of all the 24 virtual CPUs which is quite a lot. The majority of the mls_level_isvalid() function invocations come from the socket creation system call. Looking at the mls_level_isvalid() function, it is checking to see if all the bits set in one of the ebitmap structure are also set in another one as well as the highest set bit is no bigger than the one specified by the given policydb data structure. It is doing it in a bit-by-bit manner. So if the ebitmap structure has many bits set, the iteration loop will be done many times. The current code can be rewritten to use a similar algorithm as the ebitmap_contains() function with an additional check for the highest set bit. The ebitmap_contains() function was extended to cover an optional additional check for the highest set bit, and the mls_level_isvalid() function was modified to call ebitmap_contains(). With that change, the perf trace showed that the used CPU time drop down to just 0.08% (ebitmap_contains + mls_level_isvalid) of the total which is about 100X less than before. 0.07%ls [kernel.kallsyms] [k] ebitmap_contains 0.05%ls [kernel.kallsyms] [k] ebitmap_get_bit 0.01%ls [kernel.kallsyms] [k] mls_level_isvalid 0.01%ls [kernel.kallsyms] [k] find_next_bit The remaining ebitmap_get_bit() and find_next_bit() functions calls are made by other kernel routines as the new mls_level_isvalid() function will not call them anymore. This patch also improves the high_systime AIM7 benchmark result, though the improvement is not as impressive as is suggested by the reduction in CPU time spent in the ebitmap functions. The table below shows the performance change on the 2-socket x86-64 system (with HT on) mentioned above. +--+---++-+ | Workload | mean % change | mean % change | mean % change | | | 10-100 users | 200-1000 users | 1100-2000 users | +--+---++-+ | high_systime | +0.1% | +0.9% | +2.6% | +--+---++-+ Signed-off-by: Waiman Long Acked-by: Stephen Smalley Thank for the Ack. Will that patch go into v3.11? I hope so, but that's up to Eric Paris, who maintains the kernel tree for SELinux changes these days. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC 1/2] SELinux: reduce overhead of mls_level_isvalid() function call
On 05/03/2013 10:07 AM, Waiman Long wrote:
On 04/10/2013 02:26 PM, Waiman Long wrote:
While running the high_systime workload of the AIM7 benchmark on
a 2-socket 12-core Westmere x86-64 machine running 3.8.2 kernel,
it was found that a pretty sizable amount of time was spent in the
SELinux code. Below was the perf trace of the "perf record -a -s"
of a test run at 1500 users:
3.96%ls [kernel.kallsyms] [k] ebitmap_get_bit
1.44%ls [kernel.kallsyms] [k] mls_level_isvalid
1.33%ls [kernel.kallsyms] [k] find_next_bit
The ebitmap_get_bit() was the hottest function in the perf-report
output. Both the ebitmap_get_bit() and find_next_bit() functions
were, in fact, called by mls_level_isvalid(). As a result, the
mls_level_isvalid() call consumed 6.73% of the total CPU time of all
the 24 virtual CPUs which is quite a lot.
Looking at the mls_level_isvalid() function, it is checking to see
if all the bits set in one of the ebitmap structure are also set in
another one as well as the highest set bit is no bigger than the one
specified by the given policydb data structure. It is doing it in
a bit-by-bit manner. So if the ebitmap structure has many bits set,
the iteration loop will be done many times.
The current code can be rewritten to use a similar algorithm as the
ebitmap_contains() function with an additional check for the highest
set bit. With that change, the perf trace showed that the used CPU
time drop down to just 0.09% of the total which is about 100X less
than before.
0.04%ls [kernel.kallsyms] [k] ebitmap_get_bit
0.04%ls [kernel.kallsyms] [k] mls_level_isvalid
0.01%ls [kernel.kallsyms] [k] find_next_bit
Actually, the remaining ebitmap_get_bit() and find_next_bit() function
calls are made by other kernel routines as the new mls_level_isvalid()
function will not call them anymore.
This patch also improves the high_systime AIM7 benchmark result,
though the improvement is not as impressive as is suggested by the
reduction in CPU time. The table below shows the performance change
on the 2-socket x86-64 system mentioned above.
+--+---++-+
| Workload | mean % change | mean % change | mean % change |
| | 10-100 users | 200-1000 users | 1100-2000 users |
+--+---++-+
| high_systime | +0.2% | +1.1% | +2.4% |
+--+---++-+
Signed-off-by: Waiman Long
---
security/selinux/ss/mls.c | 38 +++---
1 files changed, 27 insertions(+), 11 deletions(-)
diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 40de8d3..ce02803 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -160,8 +160,7 @@ void mls_sid_to_context(struct context *context,
int mls_level_isvalid(struct policydb *p, struct mls_level *l)
{
struct level_datum *levdatum;
-struct ebitmap_node *node;
-int i;
+struct ebitmap_node *nodel, *noded;
if (!l->sens || l->sens> p->p_levels.nprim)
return 0;
@@ -170,16 +169,33 @@ int mls_level_isvalid(struct policydb *p, struct
mls_level *l)
if (!levdatum)
return 0;
-ebitmap_for_each_positive_bit(&l->cat, node, i) {
-if (i> p->p_cats.nprim)
-return 0;
-if (!ebitmap_get_bit(&levdatum->level->cat, i)) {
-/*
- * Category may not be associated with
- * sensitivity.
- */
-return 0;
+/*
+ * Return 1 iff
+ * 1. l->cat.node is NULL, or
+ * 2. all the bits set in l->cat are also set in
levdatum->level->cat,
+ *and
+ * 3. the last bit set in l->cat should not be larger than
+ *p->p_cats.nprim.
+ */
+noded = levdatum->level->cat.node;
+for (nodel = l->cat.node ; nodel ; nodel = nodel->next) {
+int i, lastsetbit = -1;
+
+for (i = EBITMAP_UNIT_NUMS - 1 ; i>= 0 ; i--) {
+if (!nodel->maps[i])
+continue;
+if (!noded ||
+ ((nodel->maps[i]&noded->maps[i]) != nodel->maps[i]))
+return 0;
+if (lastsetbit< 0)
+lastsetbit = nodel->startbit +
+ i * EBITMAP_UNIT_SIZE +
+ __fls(nodel->maps[i]);
}
+if ((lastsetbit>= 0)&& (lastsetbit> p->p_cats.nprim))
+return 0;
+if (noded)
+noded = noded->next;
}
return 1;
Would you mind giving me some feedback on what you think about this patch?
Can you take the core logic into a helper function within ebitmap.c?
Otherwise you are directly exposing ebitmap internals to the mls code.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to m
Re: [1/2] conditionally reschedule in mls_convert_context while loading selinux policy.
On 05/15/2014 03:02 PM, Dave Jones wrote: > On a slow machine (with debugging enabled), upgrading selinux policy may take > a considerable amount of time. Long enough that the softlockup detector > gets triggered. > > The backtrace looks like this.. > > > BUG: soft lockup - CPU#2 stuck for 23s! [load_policy:19045] > > Call Trace: > > [] symcmp+0xf/0x20 > > [] hashtab_search+0x47/0x80 > > [] mls_convert_context+0xdc/0x1c0 > > [] convert_context+0x378/0x460 > > [] ? security_context_to_sid_core+0x240/0x240 > > [] sidtab_map+0x45/0x80 > > [] security_load_policy+0x3ff/0x580 > > [] ? sched_clock_cpu+0xa8/0x100 > > [] ? sched_clock_local+0x1d/0x80 > > [] ? sched_clock_cpu+0xa8/0x100 > > [] ? __change_page_attr_set_clr+0x82a/0xa50 > > [] ? sched_clock_local+0x1d/0x80 > > [] ? sched_clock_cpu+0xa8/0x100 > > [] ? __change_page_attr_set_clr+0x82a/0xa50 > > [] ? sched_clock_cpu+0xa8/0x100 > > [] ? retint_restore_args+0xe/0xe > > [] ? trace_hardirqs_on_caller+0xfd/0x1c0 > > [] ? trace_hardirqs_on_thunk+0x3a/0x3f > > [] ? rcu_irq_exit+0x68/0xb0 > > [] ? retint_restore_args+0xe/0xe > > [] sel_write_load+0xa7/0x770 > > [] ? vfs_write+0x1c3/0x200 > > [] ? security_file_permission+0x1e/0xa0 > > [] vfs_write+0xbb/0x200 > > [] ? fget_light+0x397/0x4b0 > > [] SyS_write+0x47/0xa0 > > [] tracesys+0xdd/0xe2 > > Stephen Smalley suggested: > > > Maybe put a cond_resched() within the ebitmap_for_each_positive_bit() > > loop in mls_convert_context()? > > That seems to do the trick. Tested by downgrading and re-upgrading > selinux-policy-targeted. > > Signed-off-by: Dave Jones Acked-by: Stephen Smalley > > --- > security/selinux/ss/mls.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c > index c85bc1ec040c..d307b37ddc2b 100644 > --- a/security/selinux/ss/mls.c > +++ b/security/selinux/ss/mls.c > @@ -492,6 +492,8 @@ int mls_convert_context(struct policydb *oldp, > rc = ebitmap_set_bit(&bitmap, catdatum->value - 1, 1); > if (rc) > return rc; > + > + cond_resched(); > } > ebitmap_destroy(&c->range.level[l].cat); > c->range.level[l].cat = bitmap; > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [2/2] conditionally reschedule in hashtab_insert while loading selinux policy.
On 05/15/2014 03:03 PM, Dave Jones wrote: > After silencing the sleeping warning in mls_convert_context() I started > seeing similar traces from hashtab_insert. Do a cond_resched there too. > > Signed-off-by: Dave Jones Acked-by: Stephen Smalley > > diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c > index 933e735bb185..2cc496149842 100644 > --- a/security/selinux/ss/hashtab.c > +++ b/security/selinux/ss/hashtab.c > @@ -6,6 +6,7 @@ > #include > #include > #include > +#include > #include "hashtab.h" > > struct hashtab *hashtab_create(u32 (*hash_value)(struct hashtab *h, const > void *key), > @@ -40,6 +41,8 @@ int hashtab_insert(struct hashtab *h, void *key, void > *datum) > u32 hvalue; > struct hashtab_node *prev, *cur, *newnode; > > + cond_resched(); > + > if (!h || h->nel == HASHTAB_MAX_NODES) > return -EINVAL; > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: lgetxattr()/getxattr() return different values on a file labelled with selinux disabled
On 03/15/2013 06:54 AM, Thomas COUDRAY wrote: Hi, I encounter trouble that I can't explain when labelling my files. Here are steps to reproduce (on both 3.2.37 and 3.7.3, with selinux, on an ext4 fs): 0 - have a regular file "f", with a "before_t" security.selinux attribute 1 - reboot with selinux=0 2 - change the label to "after_t" (setfattr or chcon) 3 - both "ls -Z" (who calls lgetxattr(2)) and "getfattr -n security.selinux" (who calls getxattr(2)) show "after_t" 4 - reboot with selinux enabled 5 - now ls prints "before_t", and getfattr "after_t". I ran a small test that calls both syscalls (lgetxattr/getxattr), I get "before_t" as expected If I touch /.autorelabel, both ls/getfattr give "before_t". f is truly a regular file and not a symlink pointing to a regular file? before_t and after_t are both defined in the policy? before_t and after_t are not type aliases of each other? What are the credentials (capabilities and SELinux security context/permissions) of the process running the ls and getfattr commands? Any relevant messages from SELinux in dmesg output? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: lgetxattr()/getxattr() return different values on a file labelled with selinux disabled
On 03/15/2013 11:24 AM, Thomas COUDRAY wrote: 2013/3/15 Stephen Smalley : f is truly a regular file and not a symlink pointing to a regular file? f is a truly regular file. before_t and after_t are both defined in the policy? Only before_t was defined in the policy. If not defined in policy, then kernel should remap to unlabeled sid context. When I define after_t in the policy, both commands return the same label (after_t). But I wouldn't expect this to make a difference in the output of both commands (as the only visible difference is lgetxattr() vs getxattr()) getxattr security.* results are supplied by the security module rather than the filesystem to allow the value to be canonicalized. But this should happen the same for lgetxattr and getxattr; those should only differ if the file is a symlink. before_t and after_t are not type aliases of each other? They are not. What are the credentials (capabilities and SELinux security context/permissions) of the process running the ls and getfattr commands? It has unconfined_u:unconfined_r:before_t label with before_t type. Same as the file f. The process has full SELinux rights on both command and file. Did it run as root? Does it have :capability2 mac_override permission? Any relevant messages from SELinux in dmesg output? No avc warnings in dmesg and audit.log. All looks good. What about SELinux: messages? e.g. SELinux: Context ... is not valid (left unmapped). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] SELinux: Fix possible NULL pointer dereference in selinux_inode_permission()
On 01/09/2014 11:05 AM, Eric Paris wrote: > [adding lsm and selinux] > > Am I just crazy, or was this bug discussed (and obviously not fixed) > some time ago? > > VFS can still use inodes after security_inode_free_security() was > called... I didn't know that was the case; originally when we added the hook it was not possible. I have seen a Red Hat bugzilla report about it, but no upstream discussion. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] proc: Update get proc_pid_cmdline() to use mm.h helpers
On 12/02/2013 04:10 PM, William Roberts wrote:
> Re-factor proc_pid_cmdline() to use get_cmdline_length() and
> copy_cmdline() helpers from mm.h
>
> Signed-off-by: William Roberts
> ---
> fs/proc/base.c | 35 ++-
> 1 file changed, 10 insertions(+), 25 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 03c8d74..fb4eda5 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -203,37 +203,22 @@ static int proc_root_link(struct dentry *dentry, struct
> path *path)
> static int proc_pid_cmdline(struct task_struct *task, char * buffer)
> {
> int res = 0;
> - unsigned int len;
> + unsigned int len = 0;
Why? You set len below before first use, so this is redundant.
> struct mm_struct *mm = get_task_mm(task);
> if (!mm)
> - goto out;
> - if (!mm->arg_end)
> - goto out_mm;/* Shh! No looking before we're done */
> + return 0;
Equivalent to goto out in the original code, so why change it? Don't
make unnecessary changes.
Also, I think the get_task_mm() ought to move into the helper (or all of
proc_pid_cmdline() should just become the helper). In what situation
will you not be calling get_task_mm() and mmput() around every call to
the helper?
>
> - len = mm->arg_end - mm->arg_start;
> -
> + len = get_cmdline_length(mm);
> + if (!len)
> + goto mm_out;
Could be moved into the helper. Not sure how the inline function helps
readability or maintainability.
> +
> + /*The caller of this allocates a page */
> if (len > PAGE_SIZE)
> len = PAGE_SIZE;
If the capping of len is handled by the caller, then pass an int to your
helper rather than an unsigned int to avoid problems later with
access_process_vm().
> -out_mm:
> +
> + res = copy_cmdline(task, mm, buffer, len);
> +mm_out:
> mmput(mm);
Odd style. If there is only one exit path, just call it out; if there
are two, keep them as out_mm and out.
> -out:
> return res;
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] mm: Create utility functions for accessing a tasks commandline value
On 12/02/2013 04:10 PM, William Roberts wrote:
> Add two new functions to mm.h:
> * copy_cmdline()
> * get_cmdline_length()
>
> Signed-off-by: William Roberts
> ---
> include/linux/mm.h |7 +++
> mm/util.c | 48
> 2 files changed, 55 insertions(+)
>
> index f7bc209..c8cad32 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -410,6 +411,53 @@ unsigned long vm_commit_limit(void)
> * sysctl_overcommit_ratio / 100) + total_swap_pages;
> }
>
> +/**
> + * copy_cmdline - Copy's the tasks commandline value to a buffer
spelling: Copies, task's, command-line or command line
> + * @task: The task whose command line to copy
is to be copied?
> + * @mm: The mm struct refering to task with proper semaphores held
referring
> + * @buf: The buffer to copy the value into
> + * @buflen: The length og the buffer. It trucates the value to
of, truncates
> + * buflen.
> + * @return: The number of chars copied.
> + */
> +int copy_cmdline(struct task_struct *task, struct mm_struct *mm,
> + char *buf, unsigned int buflen)
> +{
> + int res = 0;
> + unsigned int len;
> +
> + if (!task || !mm || !buf)
> + return -1;
Typically these kinds of tests are frowned upon in the kernel unless
there is a legal usage where NULL is valid. Otherwise you may just be
covering up a bug.
Also, why not just get_task_mm(task) within the function rather than
pass it in by the caller?
> +
> + res = access_process_vm(task, mm->arg_start, buf, buflen, 0);
Unsigned int buflen passed as int len argument without a range check?
Note that in the proc_pid_cmdline() code, they first cap it at PAGE_SIZE
before passing it.
The closer you can keep your code to the original proc_pid_cmdline()
code, the better (less chance for new bugs to be introduced).
> + if (res <= 0)
> + return 0;
> +
> + if (res > buflen)
> + res = buflen;
Is this a possible condition? Under what circumstances?
> + /*
> + * If the nul at the end of args had been overwritten, then
> + * assume application is using setproctitle(3).
> + */
> + if (buf[res-1] != '\0') {
Lost the len < PAGE_SIZE check from proc_pid_cmdline() here, and that
isn't the same as the check above.
> + /* Nul between start and end of vm space?
> +If so then truncate */
Not sure where these comments are coming from. Isn't the issue that
lack of NUL at the end of args indicates that the cmdline extends
further into the environ and thus they need to copy in the rest?
> + len = strnlen(buf, res);
> + if (len < res) {
> + res = len;
> + } else {
> + /* No nul, truncate buflen if to big */
It isn't truncating buflen but rather copying the remainder of the
cmdline from the environ, right?
> + len = mm->env_end - mm->env_start;
> + if (len > buflen - res)
> + len = buflen - res;
> + /* Copy any remaining data */
> + res += access_process_vm(task, mm->env_start, buf+res,
> + len, 0);
> + res = strnlen(buf, res);
> + }
> + }
> + return res;
> +}
I think you are better off just copying proc_pid_cmdline() exactly as is
into a common helper function and then reusing it for audit. Far less
work, and far less potential for mistakes.
>
> /* Tracepoints definitions. */
> EXPORT_TRACEPOINT_SYMBOL(kmalloc);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] mm: Create utility functions for accessing a tasks commandline value
On 12/13/2013 09:51 AM, William Roberts wrote: > On Fri, Dec 13, 2013 at 9:12 AM, Stephen Smalley wrote: >> Also, why not just get_task_mm(task) within the function rather than >> pass it in by the caller? >> > > Yes I was debating whether or not to drop the pointer checks... np > > WRT the locking and moving it into the function. You need to take the lock > to determine the size of the cmdline area. The idea on the interface is you > would take the locks, acquire the size via the inline func, alloc memory and > then call the copy function. In some cases, like proc/pid/cmdline, they just > alloc a page and truncate on that boundary. However, one may with to truncate > on an arbitrary boundry, especially when cacheing the values, as you don't > want > to allocate too much. So inbetween functions calls that get the length and > copy, > one can make a decision based on their allocation scheme. Moving the locks > to the functions would require multiple locks and unlocks in the common case. I don't think it is a good idea to split it up, as what happens if the range changes between the time you compute the length and the time you copy? And your current callers appear to always get_task_mm(), compute len, call the helper, and mmput. So just take it all to the helper (at which point the helper essentially becomes proc_pid_cmdline). >> Unsigned int buflen passed as int len argument without a range check? >> Note that in the proc_pid_cmdline() code, they first cap it at PAGE_SIZE >> before passing it. >> > > buflen is passed by the caller. So if you look in the following patch > introducing its > use in proc/fs/base.c, their is a check. > /*The caller of this allocates a page */ > if (len > PAGE_SIZE) > len = PAGE_SIZE; > > res = copy_cmdline(task, mm, buffer, len); I understand that, but you are making correct operation of the helper dependent on the caller already having applied such a cap to the length. Which is unsafe practice and may not hold true for future callers. >>> + if (res <= 0) >>> + return 0; >>> + >>> + if (res > buflen) >>> + res = buflen; >> >> Is this a possible condition? Under what circumstances? > > for (res <= 0), in that case, the underlying call > to __access_remote_vm() returns an int. Most of the mm functions look > like they are using > ints for probably some historical reason I am not aware of. I tried to > pick the strongest invariant, > however, I don't think < 0 is possible. > > For the res > buflen check, that might might be an artifact from the > PAGE_SIZE cap from the original > code. It would only be possible if a process was able to write to > their mm when the semaphores are held. > I am assuming the case of: > kernel gets size > kernel allocs buffer > kernel copys but size has differed. I guess if I broke the locking out > it could happen, you need size and copy > to be autonomous. Sorry, you misunderstood. The <=0 case is clearly possible; I was only asking about the res > buflen check, which seems impossible as you provided buflen as the max for the access_process_vm() call. That one does not make sense to me and has no equivalent in the original proc_pid_cmdline() code. >> I think you are better off just copying proc_pid_cmdline() exactly as is >> into a common helper function and then reusing it for audit. Far less >> work, and far less potential for mistakes. > > I don't like caching a whole page in that audit context. So most of > the complexity relates to > determining the size of the cache. Steve Grub was in favor of limiting > the cmdline value to > PATH_MAX. So if that is an acceptable cache size, we can take the > existing code from > procfs/base.c and just add an argument indicating the size of the > buffer. procfs will be > PAGE_SIZE and audit will be PATH_MAX. Thoughts? Yes, that seems reasonable to me. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] selinux: simple cleanup for cond_read_node()
On 06/18/2014 03:36 PM, Paul Moore wrote: > On Sunday, June 15, 2014 01:19:01 AM Namhyung Kim wrote: >> The node->cur_state and len can be read in a single call of next_entry(). >> And setting len before reading is a dead write so can be eliminated. >> >> Signed-off-by: Namhyung Kim >> --- >> security/selinux/ss/conditional.c | 9 ++--- >> 1 file changed, 2 insertions(+), 7 deletions(-) >> >> diff --git a/security/selinux/ss/conditional.c >> b/security/selinux/ss/conditional.c index 377d148e7157..4766a38fae9a 100644 >> --- a/security/selinux/ss/conditional.c >> +++ b/security/selinux/ss/conditional.c >> @@ -402,19 +402,14 @@ static int cond_read_node(struct policydb *p, struct >> cond_node *node, void *fp) int rc; >> struct cond_expr *expr = NULL, *last = NULL; >> >> -rc = next_entry(buf, fp, sizeof(u32)); >> +rc = next_entry(buf, fp, sizeof(buf)); > > This is a bit nit-picky, but how about using "sizeof(u32) * 2"? It is more > consistent with the rest of the function and helps underscore that we are Concur - I don't want to assume that the buf size is always the same as the next read size (e.g. we sometimes use the same buf for multiple reads). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] selinux: no recursive read_lock of policy_rwlock in security_genfs_sid()
On 06/20/2014 01:45 PM, Waiman Long wrote:
> With introduction of fair queued rwlock, recursive read_lock() may hang
> the offending process if there is a write_lock() somewhere in between.
>
> With recursive read_lock checking enabled, the following error was
> reported:
>
> =
> [ INFO: possible recursive locking detected ]
> 3.16.0-rc1 #2 Tainted: GE
> -
> load_policy/708 is trying to acquire lock:
> (policy_rwlock){.+.+..}, at: []
> security_genfs_sid+0x3a/0x170
>
> but task is already holding lock:
> (policy_rwlock){.+.+..}, at: [] security_fs_use+0x2c/0x110
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
>CPU0
>
> lock(policy_rwlock);
> lock(policy_rwlock);
>
> This patch fixes the occurrence of recursive read_lock() of
> policy_rwlock in security_genfs_sid() by adding a 5th argument to
> indicate if the rwlock has been taken.
>
> Signed-off-by: Waiman Long
Thanks, but I'd prefer to instead create a static helper function in
services.c that does not take the lock at all, use that function from
security_fs_use, and leave the extern function unmodified.
> ---
> security/selinux/hooks.c|2 +-
> security/selinux/include/security.h |2 +-
> security/selinux/selinuxfs.c|3 ++-
> security/selinux/ss/services.c | 13 +
> 4 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 83d06db..430035a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1248,7 +1248,7 @@ static int selinux_proc_get_sid(struct dentry *dentry,
> path[1] = '/';
> path++;
> }
> - rc = security_genfs_sid("proc", path, tclass, sid);
> + rc = security_genfs_sid("proc", path, tclass, sid, false);
> }
> free_page((unsigned long)buffer);
> return rc;
> diff --git a/security/selinux/include/security.h
> b/security/selinux/include/security.h
> index ce7852c..6bc5b2f 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -180,7 +180,7 @@ int security_get_allow_unknown(void);
> int security_fs_use(struct super_block *sb);
>
> int security_genfs_sid(const char *fstype, char *name, u16 sclass,
> - u32 *sid);
> + u32 *sid, int locked);
>
> #ifdef CONFIG_NETLABEL
> int security_netlbl_secattr_to_sid(struct netlbl_lsm_secattr *secattr,
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index c71737f..405799e 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1273,7 +1273,8 @@ static int sel_make_bools(void)
> goto out;
>
> isec = (struct inode_security_struct *)inode->i_security;
> - ret = security_genfs_sid("selinuxfs", page, SECCLASS_FILE,
> &sid);
> + ret = security_genfs_sid("selinuxfs", page, SECCLASS_FILE,
> + &sid, false);
> if (ret)
> goto out;
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 4bca494..2b23c2c 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2282,6 +2282,7 @@ out:
> * @path: path from root of mount
> * @sclass: file security class
> * @sid: SID for path
> + * @locked: true if policy_rwlock taken
> *
> * Obtain a SID to use for a file in a filesystem that
> * cannot support xattr or use a fixed labeling behavior like
> @@ -2290,7 +2291,8 @@ out:
> int security_genfs_sid(const char *fstype,
> char *path,
> u16 orig_sclass,
> -u32 *sid)
> +u32 *sid,
> +int locked)
> {
> int len;
> u16 sclass;
> @@ -2301,7 +2303,8 @@ int security_genfs_sid(const char *fstype,
> while (path[0] == '/' && path[1] == '/')
> path++;
>
> - read_lock(&policy_rwlock);
> + if (!locked)
> + read_lock(&policy_rwlock);
>
> sclass = unmap_class(orig_sclass);
> *sid = SECINITSID_UNLABELED;
> @@ -2336,7 +2339,8 @@ int security_genfs_sid(const char *fstype,
> *sid = c->sid[0];
> rc = 0;
> out:
> - read_unlock(&policy_rwlock);
> + if (!locked)
> + read_unlock(&policy_rwlock);
> return rc;
> }
>
> @@ -2370,7 +2374,8 @@ int security_fs_use(struct super_block *sb)
> }
> sbsec->sid = c->sid[0];
> } else {
> - rc = security_genfs_sid(fstype, "/", SECCLASS_DIR, &sbsec->sid);
> + rc = security_genfs_sid(fstype, "/", SECCLASS_DIR, &sbsec->sid,
> + true);
> if (rc) {
> sbsec
Re: [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file
On 11/05/2014 10:43 AM, David Howells wrote:
> The copy-up operation must have read permission on the lower file for the task
> that caused the copy-up. This helps prevent overlayfs from being used to
> access something it shouldn't.
>
> Signed-off-by: David Howells
> ---
>
> security/selinux/hooks.c |3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f43f07fdc028..57f9c641779f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3144,7 +3144,8 @@ static void selinux_inode_getsecid(const struct inode
> *inode, u32 *secid)
>
> static int selinux_inode_copy_up(struct dentry *src, struct dentry *dst)
> {
> - return 0;
> + const struct cred *cred = current_cred();
> + return dentry_has_perm(cred, src, FILE__OPEN | FILE__READ);
> }
Won't this get checked anyway when overlayfs calls vfs helpers to open
the source and those vfs helpers call the security hooks and apply the
usual checks?
Or, if not, where do you check permissions for the destination?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/7] SELinux: Handle opening of a unioned file
On 11/05/2014 10:43 AM, David Howells wrote:
> Handle the opening of a unioned file by trying to derive the label that would
> be attached to the union-layer inode if it doesn't exist.
>
> If the union-layer inode does exist (as it necessarily does in overlayfs, but
> not in unionmount), we assume that it has the right label and use that.
> Otherwise we try to get it from the superblock.
>
> If the superblock has a globally-applied label, we use that, otherwise we try
> to transition to an appropriate label. This union label is then stored in the
> file_security_struct.
>
> We then perform an additional check to make sure that the calling task is
> granted permission by the union-layer inode label to open the file in addition
> to a check to make sure that the task is granted permission to open the lower
> file with the lower inode label.
>
> Signed-off-by: David Howells
> ---
>
> security/selinux/hooks.c | 56
> +
> security/selinux/include/objsec.h |1 +
> 2 files changed, 57 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6fd8090cc7a5..f43f07fdc028 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3431,6 +3431,58 @@ static int selinux_file_receive(struct file *file)
> return file_has_perm(cred, file, file_to_av(file));
> }
>
> +/*
> + * We have a file opened on a unioned file system that falls through to a
> file
> + * on a lower layer. If there is a union inode, we try to get the label from
> + * that, otherwise we need to get it from the superblock.
> + */
> +static int selinux_file_open_union(struct file *file,
> +const struct path *union_path,
> +struct file_security_struct *fsec,
> +const struct cred *cred)
> +{
> + const struct superblock_security_struct *sbsec;
> + const struct inode_security_struct *isec, *dsec;
> + const struct task_security_struct *tsec = current_security();
> + struct common_audit_data ad;
> + const struct inode *inode = union_path->dentry->d_inode;
> + struct dentry *dir;
> + int rc;
> +
> + sbsec = union_path->dentry->d_sb->s_security;
> +
> + if (inode) {
> + isec = inode->i_security;
> + fsec->union_isid = isec->sid;
> + } else if ((sbsec->flags & SE_SBINITIALIZED) &&
> +(sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> + fsec->union_isid = sbsec->mntpoint_sid;
> + } else {
> + dir = dget_parent(union_path->dentry);
> + dsec = dir->d_inode->i_security;
> +
> + rc = security_transition_sid(
> + tsec->sid, dsec->sid,
> + inode_mode_to_security_class(file_inode(file)->i_mode),
> + &union_path->dentry->d_name,
> + &fsec->union_isid);
> + dput(dir);
> + if (rc) {
> + printk(KERN_WARNING "%s: "
> +"security_transition_sid failed, rc=%d
> (name=%pD)\n",
> +__func__, -rc, file);
> + return rc;
> + }
> + }
How do we know that this union_isid will bear any relation to the actual
SID assigned to the union inode when it is created? If the union inode
does not already exist, when/where does it get created?
Also, would be good to create a common helper for use here, by
selinux_dentry_init_security(), selinux_inode_init_security(), and
may_create(). Already some seeming potential for inconsistencies there.
> +
> + /* We need to check that the union file is allowed to be opened as well
> + * as checking that the lower file is allowed to be opened.
> + */
> + ad.type = LSM_AUDIT_DATA_PATH;
> + ad.u.path = *union_path;
> + return inode_has_perm(cred, file_inode(file), fsec->union_isid, &ad);
Something is seriously wrong here; you are passing fsec->union_isid
where we expect a permissions bitmap / access vector.
> +}
> +
> static int selinux_file_open(struct file *file, const struct path
> *union_path,
>const struct cred *cred)
> {
> @@ -3456,6 +3508,10 @@ static int selinux_file_open(struct file *file, const
> struct path *union_path,
>* new inode label or new policy.
>* This check is not redundant - do not remove.
>*/
> +
> + if (union_path->dentry != file->f_path.dentry)
> + selinux_file_open_union(file, union_path, fsec, cred);
Ignored return value.
> +
> return file_path_has_perm(cred, file, open_file_to_av(file));
> }
>
> diff --git a/security/selinux/include/objsec.h
> b/security/selinux/include/objsec.h
> index 81fa718d5cb3..f088c080aa9e 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -54,6 +54,7 @@ struct file_security_struct {
> u32
Re: [PATCH 6/7] SELinux: The copy-up operation must have read permission on the lower file
On 11/05/2014 11:43 AM, Stephen Smalley wrote:
> On 11/05/2014 10:43 AM, David Howells wrote:
>> The copy-up operation must have read permission on the lower file for the
>> task
>> that caused the copy-up. This helps prevent overlayfs from being used to
>> access something it shouldn't.
>>
>> Signed-off-by: David Howells
>> ---
>>
>> security/selinux/hooks.c |3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index f43f07fdc028..57f9c641779f 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3144,7 +3144,8 @@ static void selinux_inode_getsecid(const struct inode
>> *inode, u32 *secid)
>>
>> static int selinux_inode_copy_up(struct dentry *src, struct dentry *dst)
>> {
>> -return 0;
>> +const struct cred *cred = current_cred();
>> +return dentry_has_perm(cred, src, FILE__OPEN | FILE__READ);
>> }
>
> Won't this get checked anyway when overlayfs calls vfs helpers to open
> the source and those vfs helpers call the security hooks and apply the
> usual checks?
>
> Or, if not, where do you check permissions for the destination?
BTW, a quick look at overlayfs suggests further problems with regard to
permission checking, e.g. ovl_copy_up_one() does:
/*
* CAP_SYS_ADMIN for copying up extended attributes
* CAP_DAC_OVERRIDE for create
* CAP_FOWNER for chmod, timestamp update
* CAP_FSETID for chmod
* CAP_CHOWN for chown
* CAP_MKNOD for mknod
*/
cap_raise(override_cred->cap_effective, CAP_SYS_ADMIN);
cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE);
cap_raise(override_cred->cap_effective, CAP_FOWNER);
cap_raise(override_cred->cap_effective, CAP_FSETID);
cap_raise(override_cred->cap_effective, CAP_CHOWN);
cap_raise(override_cred->cap_effective, CAP_MKNOD);
old_cred = override_creds(override_cred);
This means that it expects to trigger those capability checks as part of
its subsequent actions. Raising those capabilities temporarily in its
credentials will pass the capability module checks but won't address the
corresponding SELinux checks (both capability and file-based), so you'll
end up triggering an entire set of checks against the current process'
credentials. This same pattern is repeated elsewhere in overlayfs.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
