From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757172Ab1HaWy2 (ORCPT ); Wed, 31 Aug 2011 18:54:28 -0400 Received: from nm24-vm0.bullet.mail.sp2.yahoo.com ([98.139.91.226]:35673 "HELO nm24-vm0.bullet.mail.sp2.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1757087Ab1HaWy1 (ORCPT ); Wed, 31 Aug 2011 18:54:27 -0400 X-Yahoo-Newman-Id: 123979.7108.bm@omp1025.mail.sp2.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: IDbzUDYVM1kCEyX7wKG1XZuKGod9lumz18dR5gburBst5al B1g1HLZTknfZbC6rkl0WSBR7o5xkV87_5L3BO8p3gg2wzFy7PbOaD9qxnyvV BFTR4K8Zsk5qlu5GrpofecOcfWX2GWwmSWX89Xr2PQipl_mHfVDA3JpnELKA UVPTQH7VE4hswawxleNCMb61t34Ro5LvIitUhhB7HvyzB2WOBiHqkh72dO52 y7xqi8qdQ6wRH9FIO4xg1YoZ6D5hQL_QGo9jgJ3_C.z.maCUcXt0EEVC9j0D ohQg_ThJf14fDrLGeut9nUfiSVnAqBSLSvREyNHqE0LSKWJ_R9eFj_rQ8QVT 2jUDl1j1mYcTo4A2lxJGvvgSUkPbCXVpUZlzQVGY_Jzp2ieXPHZehOGUwhP_ boHd.zQ-- X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- Message-ID: <4E5EBBA1.3040202@schaufler-ca.com> Date: Wed, 31 Aug 2011 15:54:25 -0700 From: Casey Schaufler User-Agent: Mozilla/5.0 (Windows NT 6.0; rv:6.0) Gecko/20110812 Thunderbird/6.0 MIME-Version: 1.0 To: Jarkko Sakkinen CC: linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, Casey Schaufler Subject: Re: [PATCH] Smack: check permissions from user space References: <1314596111-2781-1-git-send-email-jarkko.sakkinen@intel.com> In-Reply-To: <1314596111-2781-1-git-send-email-jarkko.sakkinen@intel.com> X-Enigmail-Version: 1.3.1 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 8/28/2011 10:35 PM, Jarkko Sakkinen wrote: > Adds a new file into SmackFS called 'access'. Wanted > Smack permission is written into /smack/access. > After that result can be read from the opened file. > If access applies result contains 1 and otherwise > 0. File access is protected from race conditions > by using simple_transaction_get()/set() API. I confess to liking this less than the ioctl approach, but as there has been less contention over this mechanism I'm leaning in it's direction. Any comments beyond "ioctls are bad" lurking out there? > > Signed-off-by: Jarkko Sakkinen > --- > security/smack/smack.h | 1 + > security/smack/smackfs.c | 181 ++++++++++++++++++++++++++++++---------------- > 2 files changed, 119 insertions(+), 63 deletions(-) > > diff --git a/security/smack/smack.h b/security/smack/smack.h > index 2b6c6a5..82c5a30 100644 > --- a/security/smack/smack.h > +++ b/security/smack/smack.h > @@ -189,6 +189,7 @@ struct smk_audit_info { > struct common_audit_data a; > #endif > }; > + > /* > * These functions are in smack_lsm.c > */ > diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c > index f934601..5a87075 100644 > --- a/security/smack/smackfs.c > +++ b/security/smack/smackfs.c > @@ -27,6 +27,7 @@ > #include > #include > #include > +#include > #include "smack.h" > > /* > @@ -44,6 +45,7 @@ enum smk_inos { > SMK_ONLYCAP = 9, /* the only "capable" label */ > SMK_LOGGING = 10, /* logging */ > SMK_LOAD_SELF = 11, /* task specific rules */ > + SMK_ACCESSES = 12, /* access policy */ > }; > > /* > @@ -176,71 +178,19 @@ static int smk_set_access(struct smack_rule *srp, struct list_head *rule_list, > } > > /** > - * smk_write_load_list - write() for any /smack/load > - * @file: file pointer, not actually used > - * @buf: where to get the data from > - * @count: bytes sent > - * @ppos: where to start - must be 0 > - * @rule_list: the list of rules to write to > - * @rule_lock: lock for the rule list > - * > - * Get one smack access rule from above. > - * The format is exactly: > - * char subject[SMK_LABELLEN] > - * char object[SMK_LABELLEN] > - * char access[SMK_ACCESSLEN] > - * > - * writes must be SMK_LABELLEN+SMK_LABELLEN+SMK_ACCESSLEN bytes. > + * smk_parse_rule - parse subject, object and access type > + * @data: string to be parsed whose size is SMK_LOADLEN > + * @rule: parsed entities are stored in here > */ > -static ssize_t smk_write_load_list(struct file *file, const char __user *buf, > - size_t count, loff_t *ppos, > - struct list_head *rule_list, > - struct mutex *rule_lock) > +static int smk_parse_rule(const char *data, struct smack_rule *rule) > { > - struct smack_rule *rule; > - char *data; > - int rc = -EINVAL; > - > - /* > - * No partial writes. > - * Enough data must be present. > - */ > - if (*ppos != 0) > - return -EINVAL; > - /* > - * Minor hack for backward compatibility > - */ > - if (count < (SMK_OLOADLEN) || count > SMK_LOADLEN) > - return -EINVAL; > - > - data = kzalloc(SMK_LOADLEN, GFP_KERNEL); > - if (data == NULL) > - return -ENOMEM; > - > - if (copy_from_user(data, buf, count) != 0) { > - rc = -EFAULT; > - goto out; > - } > - > - /* > - * More on the minor hack for backward compatibility > - */ > - if (count == (SMK_OLOADLEN)) > - data[SMK_OLOADLEN] = '-'; > - > - rule = kzalloc(sizeof(*rule), GFP_KERNEL); > - if (rule == NULL) { > - rc = -ENOMEM; > - goto out; > - } > - > rule->smk_subject = smk_import(data, 0); > if (rule->smk_subject == NULL) > - goto out_free_rule; > + return -1; > > rule->smk_object = smk_import(data + SMK_LABELLEN, 0); > if (rule->smk_object == NULL) > - goto out_free_rule; > + return -1; > > rule->smk_access = 0; > > @@ -252,7 +202,7 @@ static ssize_t smk_write_load_list(struct file *file, const char __user *buf, > rule->smk_access |= MAY_READ; > break; > default: > - goto out_free_rule; > + return -1; > } > > switch (data[SMK_LABELLEN + SMK_LABELLEN + 1]) { > @@ -263,7 +213,7 @@ static ssize_t smk_write_load_list(struct file *file, const char __user *buf, > rule->smk_access |= MAY_WRITE; > break; > default: > - goto out_free_rule; > + return -1; > } > > switch (data[SMK_LABELLEN + SMK_LABELLEN + 2]) { > @@ -274,7 +224,7 @@ static ssize_t smk_write_load_list(struct file *file, const char __user *buf, > rule->smk_access |= MAY_EXEC; > break; > default: > - goto out_free_rule; > + return -1; > } > > switch (data[SMK_LABELLEN + SMK_LABELLEN + 3]) { > @@ -285,7 +235,7 @@ static ssize_t smk_write_load_list(struct file *file, const char __user *buf, > rule->smk_access |= MAY_APPEND; > break; > default: > - goto out_free_rule; > + return -1; > } > > switch (data[SMK_LABELLEN + SMK_LABELLEN + 4]) { > @@ -296,9 +246,74 @@ static ssize_t smk_write_load_list(struct file *file, const char __user *buf, > rule->smk_access |= MAY_TRANSMUTE; > break; > default: > - goto out_free_rule; > + return -1; > } > > + return 0; > +} > + > +/** > + * smk_write_load_list - write() for any /smack/load > + * @file: file pointer, not actually used > + * @buf: where to get the data from > + * @count: bytes sent > + * @ppos: where to start - must be 0 > + * @rule_list: the list of rules to write to > + * @rule_lock: lock for the rule list > + * > + * Get one smack access rule from above. > + * The format is exactly: > + * char subject[SMK_LABELLEN] > + * char object[SMK_LABELLEN] > + * char access[SMK_ACCESSLEN] > + * > + * writes must be SMK_LABELLEN+SMK_LABELLEN+SMK_ACCESSLEN bytes. > + */ > +static ssize_t smk_write_load_list(struct file *file, const char __user *buf, > + size_t count, loff_t *ppos, > + struct list_head *rule_list, > + struct mutex *rule_lock) > +{ > + struct smack_rule *rule; > + char *data; > + int rc = -EINVAL; > + > + /* > + * No partial writes. > + * Enough data must be present. > + */ > + if (*ppos != 0) > + return -EINVAL; > + /* > + * Minor hack for backward compatibility > + */ > + if (count < (SMK_OLOADLEN) || count > SMK_LOADLEN) > + return -EINVAL; > + > + data = kzalloc(SMK_LOADLEN, GFP_KERNEL); > + if (data == NULL) > + return -ENOMEM; > + > + if (copy_from_user(data, buf, count) != 0) { > + rc = -EFAULT; > + goto out; > + } > + > + /* > + * More on the minor hack for backward compatibility > + */ > + if (count == (SMK_OLOADLEN)) > + data[SMK_OLOADLEN] = '-'; > + > + rule = kzalloc(sizeof(*rule), GFP_KERNEL); > + if (rule == NULL) { > + rc = -ENOMEM; > + goto out; > + } > + > + if (smk_parse_rule(data, rule)) > + goto out_free_rule; > + > rc = count; > /* > * smk_set_access returns true if there was already a rule > @@ -1425,6 +1440,44 @@ static const struct file_operations smk_load_self_ops = { > .write = smk_write_load_self, > .release = seq_release, > }; > + > +/** > + * smk_write_access - handle access check transaction > + * @file: file pointer > + * @buf: data from user space > + * @count: bytes sent > + * @ppos: where to start - must be 0 > + */ > +static ssize_t smk_write_access(struct file *file, const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + struct smack_rule rule; > + char *data; > + > + if (!capable(CAP_MAC_ADMIN)) > + return -EPERM; > + > + data = simple_transaction_get(file, buf, count); > + if (IS_ERR(data)) > + return PTR_ERR(data); > + > + if (count < SMK_LOADLEN || smk_parse_rule(data, &rule)) > + return -EINVAL; > + > + data[0] = smk_access(rule.smk_subject, rule.smk_object, > + rule.smk_access, NULL) == 0; > + > + simple_transaction_set(file, 1); > + return SMK_LOADLEN; > +} > + > +static const struct file_operations smk_access_ops = { > + .write = smk_write_access, > + .read = simple_transaction_read, > + .release = simple_transaction_release, > + .llseek = generic_file_llseek, > +}; > + > /** > * smk_fill_super - fill the /smackfs superblock > * @sb: the empty superblock > @@ -1459,6 +1512,8 @@ static int smk_fill_super(struct super_block *sb, void *data, int silent) > "logging", &smk_logging_ops, S_IRUGO|S_IWUSR}, > [SMK_LOAD_SELF] = { > "load-self", &smk_load_self_ops, S_IRUGO|S_IWUGO}, > + [SMK_ACCESSES] = { > + "access", &smk_access_ops, S_IRUGO|S_IWUSR}, > /* last one */ > {""} > };