On 12/22/2012 6:32 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> On 12/20/2012 6:02 AM, Tetsuo Handa wrote:
>>> Several bugfixes for v11 patchset.
>>>
>>> Fixed off-by-one bug in lsm_read().
>> Where is the off-by-one error?
>>
> list_for_each_entry(sop, &lsm_hooks[LSM_name], list[LSM_name]) {
> strcat(data, sop->name);
> strcat(data, ","); // <= Here. :-(
> }
>
> If COMPOSER_MAX == 1, COMPOSER_NAMES_MAX is SECURITY_NAME_MAX+1.
> If strlen(sop->name) == SECURITY_NAME_MAX,
> strcat(data, sop->name) writes data[0...COMPOSER_NAMES_MAX] and
> strcat(data, ",") writes data[COMPOSER_NAMES_MAX...COMPOSER_NAMES_MAX+1].
> data[] needs to be one byte larger for writing trailing '\0'.
Yup, there it is.
>>> (Changed to use PAGE_SIZE in preparation for LKM-based LSM modules.)
>> How does this help in the LKM-based case? I don't see it.
> If we use ((SECURITY_NAME_MAX + 1) * COMPOSER_MAX) + 1, only up to COMPOSER_MAX
> modules can be shown up. Users might load a few of LKM-based LSM modules, and
> I think that LKM-based LSM modules should also be shown up. I think PAGE_SIZE
> is practically sufficient, for nobody will load hundreds of LKM-based LSM
> modules.
Someone who wants an unreasonably large numbers of LSMs can always boost
COMPOSER_MAX to the value they desire.
> By the way, one entry per a line like
>
> # cat /sys/kernel/security/lsm
> smack
> tomoyo
> apparmor
>
> might look better. In this case, kmalloc()/kfree()/COMPOSER_NAMES_MAX/PAGE_SIZE
> are not needed.
It's a matter of taste between
smack,tomoyo,apparmor
and
smack
tomoyo
apparmor
I think which you prefer will depend on whether you're a fan of
grep, awk, sed, bash, perl, python, C or XML. I expect to see just
as many complaints with one as the other.
>
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -224,24 +224,32 @@ static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count,
> loff_t *ppos)
> {
> struct security_operations *sop;
> - char *data;
> - int len;
> -
> - data = kzalloc(COMPOSER_NAMES_MAX, GFP_KERNEL);
> - if (data == NULL)
> - return -ENOMEM;
> + int len = 0;
> + loff_t skip = 0;
> + const loff_t pos = *ppos;
>
> + if (pos < 0)
> + return -EINVAL;
> list_for_each_entry(sop, &lsm_hooks[LSM_name], list[LSM_name]) {
> - strcat(data, sop->name);
> - strcat(data, ",");
> + const char *cp = sop->name;
> + char c;
> + do {
> + c = *cp++;
> + if (!c)
> + c = '\n';
> + if (skip < pos)
> + skip++;
> + else if (!count--)
> + return len;
> + else if (put_user(c, buf))
> + return len ? len : -EFAULT;
> + else {
> + len++;
> + (*ppos)++;
> + buf++;
> + }
> + } while (c != '\n');
> }
> - len = strlen(data);
> - if (len > 1)
> - data[len-1] = '\n';
> -
> - len = simple_read_from_buffer(buf, count, ppos, data, len);
> - kfree(data);
> -
> return len;
> }
I think I'll leave this as is for the time being.
>>> Removed redundant specified_lsms[][].
>> It's __init data, and ensures the result is correct.
>> If there's a good reason to change the way boot ordering
>> is done, I'm open to it, but I don't see how your change
>> makes anything better.
> My steps for allowing LKM-based LSM is to change like below.
>
> (step 1) Remove __init from security_module_enable().
> (step 2) Change "char allowed_lsms[]" to "char *allowed_lsms".
> (step 3) Reset allowed_lsms to NULL after calling do_security_initcalls().
>
> Step 1, 2 and 3 mean that LSM modules which are specified via security=
> parameter are enabled before userspace starts. Whether to load other LKM-based
> LSM modules or not is determined by userspace, and permission for loading other
> LKM-based LSM modules after userspace starts is checked by already loaded LSM
> modules.
>
> (step 4) Add checks for whether that LSM module uses blobs or not, and
> increment lsm_count only when that LSM module uses blobs.
>
> Step 4 means that the number of LSM modules (including LKM-based LSM modules)
> is no longer limited to COMPOSER_MAX constraint as long as LSM modules do not
> use blobs.
I think you have more faith that new LSMs will not use blobs than I do.
Further, I fear the case where an LSM is changed from blobless to blobfull
and is expected to load and unload the way it always has. Maybe that's
an unreasonable fear, but I have learned to respect my fears, especially
the unreasonable ones.
>
> As of your v11 patchset,
>
> static __initdata char allowed_lsms[COMPOSER_NAMES_MAX];
>
> is used by only within
>
> static int __init choose_lsm(char *str)
>
> . But I will have to drop __initdata when doing steps shown above.
I would expect an implementation of loadable security modules to have
a different mechanism for identifying what modules can be loaded from
the mechanism used to determine which of the compiled in LSMs get used.
I would expect it to ignore allowed_lsms in any case.
>>> LSM modules which do not use lsm_get_blob()/lsm_set_blob() will be able to
>>> use ops->order == -1. By allowing LKM-based LSM modules, users can add
>>> LSM modules with ops->order == -1 regardless of COMPOSER_MAX constraint.)
>> An LSM that uses no blobs (e.g. Yama) does not care what value is in sop->order.
>> An LSM that uses any blobs needs a unique sop->order.
>>
> Exactly. This is important background for my plan. When step 4 is done,
> COMPOSER_MAX means "the max number of LSM modules which use blobs" rather than
> "the max number of LSM modules". Yama-like modules are freely added at runtime.
It's not relevant now, but I oppose the notion that not using blobs
means you don't get a slot for blobs.
I looked at the possibility of only allocating enough slots for the
LSMs that use blobs. The case that made me walk away from it was
reset_security_ops, which decreases the number. If someone did
security=selinux,apparmor
and SELinux withdrew itself with reset_security_ops you have to leave
a blank slot anyway.
If lsm_blobs are fixed size, you run out of slots. If they are variable size,
accessing the slots requires way too much checking. In either case they're
small. I prefer wasted space to run time checks.
>>> diff --git a/include/linux/lsm.h b/include/linux/lsm.h
>>> index 5f36b6b..fcf852e 100644
>>> --- a/include/linux/lsm.h
>>> +++ b/include/linux/lsm.h
>>> @@ -24,7 +24,6 @@
>>> * Maximum number of LSMs that can be used at a time.
>>> */
>>> #define COMPOSER_MAX CONFIG_SECURITY_COMPOSER_MAX
>>> -#define COMPOSER_NAMES_MAX ((SECURITY_NAME_MAX + 1) * COMPOSER_MAX)
>>>
>>> #include <linux/security.h>
>>>
>>> diff --git a/security/inode.c b/security/inode.c
>>> index 2c14313..8cebd4b 100644
>>> --- a/security/inode.c
>>> +++ b/security/inode.c
>> No. I'm keeping a safe constant for buffer sizes.
>>
> My opinion is we don't need COMPOSER_NAMES_MAX. I will do
>
> -static __initdata char allowed_lsms[((SECURITY_NAME_MAX + 1) * COMPOSER_MAX)];
> +static char *allowed_lsms;
>
> (or equivalent) in step 2.
>
> Even more,
>
> struct security_operations {
> struct list_head list[LSM_MAX_HOOKS];
> - char name[SECURITY_NAME_MAX + 1];
> + const char *name;
> int order;
>
> and remove SECURITY_NAME_MAX is possible (provided that name field does not
> change after registration).
>
> sop->order serves for determining blob slot index and priority for linking to
> the lsm_hooks list. For now, index of LKM-based LSM modules in the lsm_hooks
> list is larger than that of LSM modules specified via security= parameter.
> But maybe adding "int priority" for allowing control of priority for linking to
> the lsm_hooks list (like Apache's modules). In that case, sop->order will
> purely represent blob slot index.
I see the point. I think saving that for loadable modules is prudent.
>>> @@ -257,9 +261,10 @@ static ssize_t present_read(struct file *filp, char __user *buf, size_t count,
>>> char *raw;
>>> char *data;
>>> int len;
>>> + struct security_operations *ops = lsm_present;
>>>
>>> - if (lsm_present)
>>> - raw = lsm_present->name;
>>> + if (ops)
>>> + raw = ops->name;
>>> else
>>> raw = "(none)";
>>> len = strlen(raw);
>>> diff --git a/security/security.c b/security/security.c
>>> index 75e2f6e..48597d6 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>> Yes. Have I mentioned how I feel about reset_security_ops?
> If I didn't miss your mails, I think you haven't.
Ah. I dislike reset_security_ops. It is a special case interface.
If it were not present there are a number of issues that would have
never arisen.
>>> ops->getprocattr && ops->setprocattr in security_module_enable() should be
>>> ops->getprocattr || ops->setprocattr in case out-of-tree LSM modules provided
>>> only either one.
>> No. Think it through. It makes no sense whatever for an LSM to supply one
>> without the other. That, and it could cock up the race condition logic for
>> reset_security_ops and those two hooks.
> Oops. I forgot the fact that present_getprocattr() or present_setprocattr()
> triggers oops if we used ops->getprocattr || ops->setprocattr. But I still
> think that there could be out-of-tree LSM modules which provide only
> ops->getprocattr or ops->setprocattr.
That's fine, I suppose. Such an LSM can't be the presenting LSM. It will have to
use it's designated /proc/.../attr interfaces instead. If it's out of tree I
don't really much care. I'll worry about it when I know about it.
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
