[TOMOYO #12 (2.6.28-rc2-mm1) 00/11] TOMOYO Linux - Kernel

This is a discussion on [TOMOYO #12 (2.6.28-rc2-mm1) 00/11] TOMOYO Linux - Kernel ; Andrew Morton wrote: > > +static bool is_byte_range(const char *str) > > +{ > > + return *str >= '0' && *str++ > > + *str >= '0' && *str++ > > + *str >= '0' && *str > > ...

+ Reply to Thread
Page 2 of 2 FirstFirst 1 2
Results 21 to 27 of 27

Thread: [TOMOYO #12 (2.6.28-rc2-mm1) 00/11] TOMOYO Linux

  1. Re: [TOMOYO #12 (2.6.28-rc2-mm1) 06/11] Common functions for TOMOYO Linux.

    Andrew Morton wrote:
    > > +static bool is_byte_range(const char *str)
    > > +{
    > > + return *str >= '0' && *str++ <= '3' &&
    > > + *str >= '0' && *str++ <= '7' &&
    > > + *str >= '0' && *str <= '7';
    > > +}

    >
    > Well... why?
    >
    > I cannot think of any kernel interfaces which use octal strings. What
    > is special about Tomoyo?
    >

    TOMOYO uses \ooo style representation for 0x01 - 0x20 and 0x7F - 0xFF.
    This function verifies that \ooo is in valid range.

    > > +static bool is_decimal(const char c)
    > > +{
    > > + return c >= '0' && c <= '9';
    > > +}

    >
    > This duplicates a standard ctype.h function.
    >

    Replaced "is_decimal" by "isdigit".

    > > +static bool is_hexadecimal(const char c)
    > > +{
    > > + return (c >= '0' && c <= '9') ||
    > > + (c >= 'A' && c <= 'F') ||
    > > + (c >= 'a' && c <= 'f');
    > > +}

    >
    > And so does this.
    >

    Replaced "is_hexadecimal" by "isxdigit".

    > > +static bool is_alphabet_char(const char c)
    > > +{
    > > + return (c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f');
    > > +}

    >
    > As does this.
    >

    Oops! "(c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f');" was wrong. X-p
    It is "(c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');".
    But, not found in a standard ctype.h function.

    > > +static bool str_starts(char **src, const char *find)
    > > +{
    > > + const int len = strlen(find);
    > > + char *tmp = *src;
    > > +
    > > + if (strncmp(tmp, find, len))
    > > + return false;
    > > + tmp += len;
    > > + *src = tmp;
    > > + return true;
    > > +}

    >
    > hrm. Isn't there a standard string.h way of doing this?
    >
    > If not, it looks like a pretty common thing. I'd suggest that it a) be
    > coded to not do two passes across the input and b) proposed as a
    > generic addition to the kernel's string library functions.
    >
    > > +static void normalize_line(unsigned char *buffer)
    > > +{
    > > + unsigned char *sp = buffer;
    > > + unsigned char *dp = buffer;
    > > + bool first = true;
    > > +
    > > + while (*sp && (*sp <= ' ' || *sp >= 127))
    > > + sp++;
    > > + while (*sp) {
    > > + if (!first)
    > > + *dp++ = ' ';
    > > + first = false;
    > > + while (*sp > ' ' && *sp < 127)
    > > + *dp++ = *sp++;
    > > + while (*sp && (*sp <= ' ' || *sp >= 127))
    > > + sp++;
    > > + }
    > > + *dp = '\0';
    > > +}

    >
    > that looks pretty generic as well.
    >

    If you think TOMOYO's way of string representation (described below) is useful,
    I'd like to propose these functions as generic functions.

    > It seems to have duplicated isprint() in lots of places.
    >

    It is different from "isprint()".

    TOMOYO uses only 0x21 - 0x7E (as printable characters) and 0x20 (as word
    delimiter) and 0x0A (as line delimiter).

    > What happens if I have a filename which includes a character in the
    > 128->255 range?


    0x01 - 0x20 and 0x80 - 0xFF will be handled in \ooo style representation.
    The reason to use \ooo is to guarantee that "%s" won't damage logs.
    Userland program can request

    open("/tmp/file granted.\nAccess /tmp/file ", O_WRONLY | O_CREAT)

    and auditing such crazy pathname using "Access %s denied.\n" format
    will results in "fabrication of audit logs" like

    Access /tmp/file granted.
    Access /tmp/file rejected.

    TOMOYO converts such characters to \ooo so that the auditing will generate

    Access /tmp/file\040granted.\012Access\040/tmp/file rejected.

    and the administrator can read the audited logs safely using /bin/cat .

    > > +struct domain_info *tmy_find_domain(const char *domainname)
    > > +{
    > > + struct domain_info *domain;
    > > + struct path_info name;
    > > +
    > > + name.name = domainname;
    > > + tmy_fill_path_info(&name);
    > > + list1_for_each_entry(domain, &domain_list, list) {
    > > + if (!domain->is_deleted &&
    > > + !tmy_pathcmp(&name, domain->domainname))
    > > + return domain;
    > > + }
    > > + return NULL;
    > > +}

    >
    > No lock was taken to protect that list.
    >

    No lock needed for protecting "list1" list.
    list1 was reviewed by Paul E. McKenney. ( http://lkml.org/lkml/2008/10/20/4 ).

    > If the caller must take some lock then that precondition should be
    > documented in the function's comment.
    >

    To your surprise, most functions are lock free, due to use of
    "append only singly linked list" named "list1".
    Only tmy_real_domain() requires the caller to take "tasklist_lock".
    tmy_read_control() and tmy_write_control take "struct tmy_io_buffer"->io_sem
    before calling "struct tmy_io_buffer"->read and "struct tmy_io_buffer"->write
    methods.
    All other functions don't require the caller to take some lock.

    > > +/**
    > > + * path_depth - Evaluate the number of '/' in a string.
    > > + *
    > > + * @pathname: The string to evaluate.
    > > + *
    > > + * Returns path depth of the string.
    > > + *
    > > + * I score 2 for each of the '/' in the @pathname
    > > + * and score 1 if the @pathname ends with '/'.
    > > + */
    > > +static int path_depth(const char *pathname)
    > > +{
    > > + int i = 0;
    > > +
    > > + if (pathname) {
    > > + char *ep = strchr(pathname, '\0');

    >
    > what? Does that even work? strchr(p, 0) should always return NULL:
    >
    > RETURN VALUE
    > The strchr() and strrchr() functions return a pointer to the matched
    > character or NULL if the character is not found.
    >
    >
    > Using
    >
    > pathname + strlen(pathname)
    >
    > would be saner, no?
    >

    strchr(p, 0) returns the location of '\0', not NULL.
    But, "p + strlen(p)" could generate faster assembly code than "strchr(p, 0)".
    Replaced "strchr(p, 0)" by "p + strlen(p)".

    > > + if (pathname < ep--) {
    > > + if (*ep != '/')
    > > + i++;
    > > + while (pathname <= ep)
    > > + if (*ep-- == '/')
    > > + i += 2;
    > > + }
    > > + }
    > > + return i;
    > > +}

    >
    > I cannot imagine why this function exists
    >

    To hash string for faster comparison.

    > > [vast amounts of string hacking snipped]

    >
    > This seems like madness, sorry.
    >
    > Why the heck is so much string bashing going on in here???
    >

    Yeah, You would go crazy with functions that handle string data.
    But these functions are needed to stay inside the kernel for validating,
    hashing and comparing string data.

    > > +/**
    > > + * tmy_io_printf - Transactional printf() to "struct tmy_io_buffer" structure.
    > > + *
    > > + * @head: Pointer to "struct tmy_io_buffer".
    > > + * @fmt: The printf()'s format string, followed by parameters.
    > > + *
    > > + * Returns true on success, false otherwise.

    >
    > This comment should explain what the terms "success" and "failure"
    > refer to. Perhaps "success"=="the output didn't overflow" or something.
    >

    Replaced "Returns true on success" by "Returns true if output was written".

    > > +static const char *tmy_get_exe(void)
    > > +{
    > > + struct mm_struct *mm = current->mm;
    > > + struct vm_area_struct *vma;
    > > + const char *cp = NULL;
    > > +
    > > + if (!mm)
    > > + return NULL;
    > > + down_read(&mm->mmap_sem);
    > > + for (vma = mm->mmap; vma; vma = vma->vm_next) {
    > > + if ((vma->vm_flags & VM_EXECUTABLE) && vma->vm_file) {
    > > + cp = tmy_realpath_from_path(&vma->vm_file->f_path);
    > > + break;
    > > + }
    > > + }
    > > + up_read(&mm->mmap_sem);
    > > + return cp;
    > > +}

    >
    > What guarantees that the first executable mapping in the mapping list
    > is the correct one for the executable?
    >

    It is just for auditing. Not for security decision.

    > What prevents us from accidentally breaking that guarantee in the
    > future? I wasn't even aware that this was the case.
    >
    > What happens if the executable was unlinked?
    >

    audit_log_task_info() is doing the same thing.

    > > +/**
    > > + * tmy_check_flags - Check mode for specified functionality.
    > > + *
    > > + * @domain: Pointer to "struct domain_info".
    > > + * @index: The functionality to check mode.
    > > + *
    > > + * Returns the mode of specified functionality.

    >
    > That description is rather meaningless.
    >

    Removed the description.

    > > +unsigned int tmy_check_flags(const struct domain_info *domain, const u8 index)
    > > +{
    > > + const u8 profile = domain->profile;
    > > +
    > > + if (unlikely(in_interrupt())) {
    > > + static u8 count = 20;
    > > + if (count) {
    > > + count--;
    > > + printk(KERN_ERR "BUG: sleeping function called "
    > > + "from invalid context.\n");
    > > + dump_stack();
    > > + }
    > > + return 0;
    > > + }

    >
    > a) WARN_ON is preferred
    >
    > b) WARN_ON_ONCE might be usable here
    >
    > c) what on earth is this code doing??
    >

    Replaced "count" with "WARN_ON".

    TOMOYO checks only process context.
    This code disables TOMOYO's enforcement in case the function is called from
    interrupt context.

    > > + return sbin_init_started && index < TMY_MAX_CONTROL_INDEX
    > > +#if MAX_PROFILES != 256
    > > + && profile < MAX_PROFILES
    > > +#endif
    > > + && profile_ptr[profile] ?
    > > + profile_ptr[profile]->value[index] : 0;
    > > +}

    >
    > And this. I cannot imagine why Tomoyo cares whether /sbin/init has
    > started yet. This sort of thing should be commented!
    >

    TOMOYO loads policy using /sbin/tomoyo-init when /sbin/init starts.
    Replaced "sbin_init_started" by "tmy_policy_loaded".

    > What happens in a cgroups environment where there will be multiple
    > /sbin/inits running?

    Nothing. /sbin/tomoyo-init is called only for the first time /sbin/init
    is requested and /sbin/tomoyo-init exists.

    > > +/**
    > > + * tmy_check_domain_quota - Check for domain's quota.
    > > + *
    > > + * @domain: Pointer to "struct domain_info".
    > > + *
    > > + * Returns true if the domain is not exceeded quota, false otherwise.
    > > + */

    >
    > This function is poorly named.
    >

    Replaced "tmy_check_domain_quota" by "tomoyo_domain_quota_is_ok".

    > > +/**
    > > + * tmy_find_or_assign_new_profile - Create a new profile.
    > > + *
    > > + * @profile: Profile number to create.
    > > + *
    > > + * Returns pointer to "struct profile" on success, NULL otherwise.
    > > + */
    > > +static struct profile *tmy_find_or_assign_new_profile(const unsigned int
    > > + profile)
    > > +{
    > > + static DEFINE_MUTEX(lock);
    > > + struct profile *ptr = NULL;
    > > +
    > > + /***** EXCLUSIVE SECTION START *****/
    > > + mutex_lock(&lock);
    > > + if (profile < MAX_PROFILES) {

    >
    > This check didn't need to be inside the lock.
    >

    Moved the "if" to before the lock.

    > > +/**
    > > + * write_profile - Write profile table.

    >
    > where to?
    >

    Write to profile table. Fixed.

    > > +/**
    > > + * read_profile - Read profile table.

    >
    > Where from?
    >

    Read from profile table. Fixed.

    > These functions appear to be implementing more userspace interfaces.
    >
    > The userspace interface is the most important part of any kernel code.
    > We can change all the internal details, but the interfaces will live
    > forever.
    >
    > Hence we should review the proposed interfaces before even looking at
    > the code. Indeed, before even writing the code.
    >
    > What are the Tomoyo kernel interfaces?


    I'll describe it in other posting.

    > > +/* Structure for policy manager. */
    > > +struct policy_manager_entry {
    > > + struct list1_head list;
    > > + /* A path to program or a domainname. */
    > > + const struct path_info *manager;
    > > + bool is_domain; /* True if manager is a domainname. */
    > > + bool is_deleted; /* True if this entry is deleted. */
    > > +};
    > > +
    > > +/*
    > > + * The list for "struct policy_manager_entry".
    > > + *
    > > + * This list is updated only inside update_manager_entry(), thus
    > > + * no global mutex exists.
    > > + */
    > > +static LIST1_HEAD(policy_manager_list);
    > > +
    > > +/**
    > > + * update_manager_entry - Add a manager entry.
    > > + *
    > > + * @manager: The path to manager or the domainnamme.
    > > + * @is_delete: True if it is a delete request.
    > > + *
    > > + * Returns 0 on success, negative value otherwise.
    > > + */

    >
    > eh? So deleted entries get their "is_deleted" flag set but they are
    > never actually removed from the list nor freed? So over time the list
    > gets longer and longer and consumes more and more memory?
    >

    Right. Most of elements are allocated when /sbin/init starts, and they
    are referred during lifetime of the kernel. Deleted entries get their
    "is_deleted" flag set but they are never actually removed from the list
    nor freed. But don't worry. The amount of memory used by TOMOYO is quite
    small (about 1MB or so).

    > > +/**
    > > + * write_manager_policy - Write manager policy.
    > > + *
    > > + * @head: Pointer to "struct tmy_io_buffer"
    > > + *
    > > + * Returns 0 on success, negative value otherwise.
    > > + */

    >
    > More userspace ABI proposals?
    >

    Yes. But, it is only for TOMOYO's management tools.
    TOMOYO requires no modification of existing userland programs
    and provides no API for existing userland programs.

    > > +/**
    > > + * is_policy_manager - Check whether the current process is a policy manager.
    > > + *
    > > + * Returns true if the current process is permitted to modify policy
    > > + * via /sys/kernel/security/tomoyo/ interface.
    > > + */
    > > +static bool is_policy_manager(void)
    > > +{
    > > + struct policy_manager_entry *ptr;
    > > + const char *exe;
    > > + const struct task_struct *task = current;
    > > + const struct path_info *domainname = tmy_domain()->domainname;
    > > + bool found = false;
    > > +
    > > + if (!sbin_init_started)
    > > + return true;
    > > + if (!manage_by_non_root && (task->cred->uid || task->cred->euid))
    > > + return false;

    >
    > What happens in a containerised environment where uids are non-unique
    > and where there are multiple /sbin/inits?
    >

    This interface is designed to be accessed by processes having init_pid_ns
    namespace.

    > > + if (!found) { /* Reduce error messages. */
    > > + static pid_t last_pid;
    > > + const pid_t pid = current->pid;
    > > + if (last_pid != pid) {
    > > + printk(KERN_WARNING "%s ( %s ) is not permitted to "
    > > + "update policies.\n", domainname->name, exe);

    >
    > It appears that unprivileged userspace can cause this messge to be
    > printed at will. That can cause the logs to fill and is considered to
    > be a small denial of service security hole.
    >

    By default, this pseudo file is "root:root" and it's permission is 0600.

    > > +static bool is_select_one(struct tmy_io_buffer *head, const char *data)
    > > +{
    > > + unsigned int pid;
    > > + struct domain_info *domain = NULL;
    > > +
    > > + if (sscanf(data, "pid=%u", &pid) == 1) {

    >
    > PIDs are no longer system-wide unique, and here we appear to be
    > implementing new userspace ABIs using PIDs.
    >

    This interface is designed to be accessed by processes having init_pid_ns
    namespace.

    > > +static int write_domain_profile(struct tmy_io_buffer *head)
    > > +{
    > > + char *data = head->write_buf;
    > > + char *cp = strchr(data, ' ');
    > > + struct domain_info *domain;
    > > + unsigned long profile;
    > > +
    > > + if (!cp)
    > > + return -EINVAL;
    > > + *cp = '\0';
    > > + domain = tmy_find_domain(cp + 1);
    > > + strict_strtoul(data, 10, &profile);

    >
    > Unchecked return value?
    >

    Added checking.

    > > +/* path to policy loader */
    > > +static const char *tmy_loader = "/sbin/tomoyo-init";

    >
    > hm, hard-wired knowledge of filesytem layout.
    >
    > We did this in a few places already, reluctantly. We did at least make
    > them configurable (eg: /proc/sys/kernel/modprobe).
    >
    > It's rather ugly to be doing this sort of thing.
    >

    This pathname is embedded into the kernel to avoid modification of
    userland program.
    /proc/sys/kernel/tmy_loader seems redundant. Should we use __setup()?

    > > +static bool policy_loader_exists(void)
    > > +{
    > > + /*
    > > + * Don't activate MAC if the policy loader doesn't exist.
    > > + * If the initrd includes /sbin/init but real-root-dev has not
    > > + * mounted on / yet, activating MAC will block the system since
    > > + * policies are not loaded yet.
    > > + * Thus, let do_execve() call this function everytime.
    > > + */
    > > + struct nameidata nd;
    > > +
    > > + if (path_lookup(tmy_loader, LOOKUP_FOLLOW, &nd)) {
    > > + printk(KERN_INFO "Not activating Mandatory Access Control now "
    > > + "since %s doesn't exist.\n", tmy_loader);
    > > + return false;
    > > + }
    > > + path_put(&nd.path);
    > > + return true;
    > > +}

    >
    > If you really really have to do this then a simple call to sys_access()
    > might suffice.
    >

    To use sys_access(), we need to add get_fs()/set_fs() stuff.
    It's not simple.

    > But it is of course racy against concurrent rename, unlink, etc.
    >

    Racing is not a problem. Policy loader is called *only once* upon boot.

    > > +void tmy_load_policy(const char *filename)
    > > +{
    > > + char *argv[2];
    > > + char *envp[3];
    > > +
    > > + if (sbin_init_started)
    > > + return;
    > > + /*
    > > + * Check filename is /sbin/init or /sbin/tomoyo-start.
    > > + * /sbin/tomoyo-start is a dummy filename in case where /sbin/init can't
    > > + * be passed.
    > > + * You can create /sbin/tomoyo-start by
    > > + * "ln -s /bin/true /sbin/tomoyo-start".
    > > + */
    > > + if (strcmp(filename, "/sbin/init") &&
    > > + strcmp(filename, "/sbin/tomoyo-start"))
    > > + return;
    > > + if (!policy_loader_exists())
    > > + return;

    >
    > Why do this? call_usermodehelper() will simply fail if the file isn't here.
    >

    Without policy_loader_exists(), the system will panic() if
    /sbin/init is requested but the policy loader doesn't exist.

    > > + printk(KERN_INFO "Calling %s to load policy. Please wait.\n",
    > > + tmy_loader);
    > > + argv[0] = (char *) tmy_loader;
    > > + argv[1] = NULL;
    > > + envp[0] = "HOME=/";
    > > + envp[1] = "PATH=/sbin:/bin:/usr/sbin:/usr/bin";
    > > + envp[2] = NULL;
    > > + call_usermodehelper(argv[0], argv, envp, 1);
    > > +
    > > + printk(KERN_INFO "TOMOYO: 2.2.0-pre 2008/10/10\n");
    > > + printk(KERN_INFO "Mandatory Access Control activated.\n");
    > > + sbin_init_started = true;
    > > + { /* Check all profiles currently assigned to domains are defined. */
    > > + struct domain_info *domain;
    > > + list1_for_each_entry(domain, &domain_list, list) {
    > > + const u8 profile = domain->profile;
    > > + if (profile_ptr[profile])
    > > + continue;
    > > + panic("Profile %u (used by '%s') not defined.\n",
    > > + profile, domain->domainname->name);
    > > + }
    > > + }
    > > +}


    > > +/**
    > > + * read_updates_counter - Check for policy change counter.
    > > + *
    > > + * @head: Pointer to "struct tmy_io_buffer".
    > > + *
    > > + * Returns how many times policy has changed since the previous check.
    > > + */
    > > +static int read_updates_counter(struct tmy_io_buffer *head)
    > > +{
    > > + if (head->read_eof)
    > > + return 0;
    > > + tmy_io_printf(head,
    > > + "/sys/kernel/security/tomoyo/domain_policy: %10u\n"
    > > + "/sys/kernel/security/tomoyo/exception_policy: %10u\n"
    > > + "/sys/kernel/security/tomoyo/profile: %10u\n"
    > > + "/sys/kernel/security/tomoyo/manager: %10u\n",
    > > + atomic_xchg(&updates_counter
    > > + [TMY_UPDATES_COUNTER_DOMAIN_POLICY], 0),
    > > + atomic_xchg(&updates_counter
    > > + [TMY_UPDATES_COUNTER_EXCEPTION_POLICY], 0),
    > > + atomic_xchg(&updates_counter
    > > + [TMY_UPDATES_COUNTER_PROFILE], 0),
    > > + atomic_xchg(&updates_counter
    > > + [TMY_UPDATES_COUNTER_MANAGER], 0));
    > > + head->read_eof = true;
    > > + return 0;
    > > +}

    >
    > What is this doing? We print the absolute pathnames of sysfs files via
    > another sysfs file?
    >

    This is an interface to allow GUI management tool to examine policy changes.
    But removed because GUI management tool is not ready to support this version.

    > > +static int tmy_read_control(struct file *file, char __user *buffer,
    > > + const int buffer_len)
    > > +{
    > > + int len = 0;
    > > + struct tmy_io_buffer *head = file->private_data;
    > > + char *cp;
    > > +
    > > + if (!head->read)
    > > + return -ENOSYS;
    > > + if (!access_ok(VERIFY_WRITE, buffer, buffer_len))

    >
    > Unneeded - copy_to_user() checks this.
    >

    Removed.

    > > +/**
    > > + * tmy_write_control - write() for /sys/kernel/security/tomoyo/ interface.
    > > + *
    > > + * @file: Pointer to "struct file".
    > > + * @buffer: Pointer to buffer to read from.
    > > + * @buffer_len: Size of @buffer.
    > > + *
    > > + * Returns @buffer_len on success, negative value otherwise.
    > > + */
    > > +static int tmy_write_control(struct file *file, const char __user *buffer,
    > > + const int buffer_len)
    > > +{
    > > + struct tmy_io_buffer *head = file->private_data;
    > > + int error = buffer_len;
    > > + int avail_len = buffer_len;
    > > + char *cp0 = head->write_buf;
    > > +
    > > + if (!head->write)
    > > + return -ENOSYS;
    > > + if (!access_ok(VERIFY_READ, buffer, buffer_len))

    >
    > Unneeded.
    >

    I know. But to avoid partial copy, I check here too.

    > > +/* Common header for holding ACL entries. */
    > > +struct acl_info {
    > > + struct list1_head list;
    > > + /*
    > > + * Type of this ACL entry.
    > > + *
    > > + * MSB is is_deleted flag.
    > > + */
    > > + u8 type;
    > > +} __attribute__((__packed__));

    >
    > I cannot tell from reading the code why this is packed. Hence a comment
    > is needed.
    >

    Packing "struct acl_info" allows "single_path_acl_record" to embed "u16" and
    "struct double_path_acl_record" to embed "u8" without enlarging their structure
    size. I added a comment.

    > Please use __packed.

    Replaced "__attribute__((__packed__))" with "__packed".

    > > +/* The kernel's domain. */
    > > +extern struct domain_info KERNEL_DOMAIN;

    >
    > Why upper-case?
    >

    Replaced "KERNEL_DOMAIN" by "kernel_domain".

    > Many of the symbols which this header defines have quite
    > generic-sounding names. it would be better if their names were to
    > identify the symbols as being part of Tomoyo.
    >

    Added "tomoyo_" to all symbols.


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [TOMOYO #12 (2.6.28-rc2-mm1) 05/11] Memory and pathname management functions.

    Andrew Morton wrote:
    > > +/**
    > > + * round_up - Round up an integer so that the returned pointers are appropriately aligned.
    > > + *
    > > + * @size: Size in bytes.
    > > + *
    > > + * Returns rounded value of @size.
    > > + *
    > > + * FIXME: Are there more requirements that is needed for assigning value
    > > + * atomically?
    > > + */

    >
    > Can PTR_ALIGN be used?
    >
    > If not, please prefer to avoid implementing generic helpers down in
    > specific code. It is better to add the helpers in a kernel-wide
    > fashion in an early patch, then to use those halpers in the
    > subsyste-specific patches.


    Replaced by "roundup(size, max(sizeof(void *), sizeof(long)))".

    > > +/* Structure for string data. */
    > > +struct name_entry {
    > > + struct list1_head list;
    > > + struct path_info entry;
    > > +};
    > > +

    >
    > You didn't need to invent list1_head for this application. This is
    > *exactly* what the existing hlist_head is designed for.


    hlist_head omits ->pprev, but hlist_node doesn't.
    Since TOMOYO uses this list as Write-Once-Read-Many,
    TOMOYO doesn't use ->pprev for list elements.

    > > +/**
    > > + * tmy_save_name - Allocate permanent memory for string data.
    > > + *
    > > + * @name: The string to store into the permernent memory.
    > > + *
    > > + * Returns pointer to "struct path_info" on success, NULL otherwise.
    > > + *
    > > + * The RAM is shared, so NEVER try to modify or kfree() the returned name.
    > > + */

    >
    > Nothing ever gets removed from fmb_list. How odd.
    >
    > If this is not a bug, I'd suggest that a code comment be added
    > explaining what all this code does and why it does it and how it does
    > it.


    fmb contains memory reserved by TOMOYO for future requests.
    fmb is removed from the fmb_list when fmb->len becomes 0.
    So, this is not a bug. I added a comment.

    > > +/* Memory allocated for temporal purpose. */
    > > +static atomic_t dynamic_memory_size;

    >
    > The correct word is "temporary". This needs fixing in at least one
    > other place.


    Replaced "temporal" by "temporary". Thanks.

    > Is this counter really useful? If not, I'd suggest that it be removed
    > and that all calls to tmy_alloc() simply be replaced by calls to
    > kmalloc().
    >

    This counter was introduced by user's request so that the administrator can
    know how much memory is used by TOMOYO module. So, I want to keep this counter.

    > A better way to perform memory accounting would be to create slab
    > caches for commonly-used objects and to reply uponthe existing
    > accounting in /proc/slabinfo.
    >

    Hmm, memory allocated for temporary purpose is not a fixed size.

    > > +/**
    > > + * tmy_alloc - Allocate memory for temporal purpose.
    > > + *
    > > + * @size: Size in bytes.
    > > + *
    > > + * Returns pointer to allocated memory on success, NULL otherwise.
    > > + */
    > > +void *tmy_alloc(const size_t size)
    > > +{
    > > + void *p = kzalloc(size, GFP_KERNEL);
    > > + if (p)
    > > + atomic_add(ksize(p), &dynamic_memory_size);
    > > + return p;
    > > +}

    >
    > Note that I said "kmalloc", not "kzalloc". This function zeroes
    > everything all the time, and surely that is not necessary. It's just a
    > waste of CPU time.
    >

    Callers of tmy_alloc assume that allocated memory is zeroed.

    > > +static int tmy_print_ascii(const char *sp, const char *cp,
    > > + int *buflen0, char **end0)

    >
    > I look at this and wonder "hm, does that duplicate any facility which
    > the kernel provides"? But I can't tell, because I don't know what this
    > function does, and I shouldn't have to sit down with a pencil and paper
    > decrypting it.
    >

    I splitted this function into "prepend()" part and "convert a string to
    TOMOYO's string representation rule" part. And I renamed the latter from
    "tmy_print_ascii" to "tmy_encode".

    > > +/* tmy_realpath_from_path2() for "struct ctl_table". */
    > > +static int tmy_sysctl_path(struct ctl_table *table, char *buffer, int buflen)

    >
    > Is this needed if CONFIG_SYSCTL=n? Does it compile if CONFIG_SYSCTL=n?
    >

    Added "#ifdef CONFIG_SYSCTL" and moved to security/tomoyo/tomoyo.c .

    > > +/**
    > > + * tmy_read_memory_counter - Check for memory usage.
    > > + *
    > > + * @head: Pointer to "struct tmy_io_buffer".
    > > + *
    > > + * Returns memory usage.

    >
    > In what units? Megabytes?
    >

    In bytes.

    > > +int tmy_read_memory_counter(struct tmy_io_buffer *head)

    >
    > This (I assume) is part of an implementation of a userspace interface.
    > We care a lot about userspace interfaces. Please describe the Tomoyo
    > userspace interfaces so that we can review them for suitability and
    > maintainability.
    >

    I'll describe it in other posting.

    > Surely this function should return a 64-bit quantity?
    >

    I believe 32-bit is enough. TOMOYO uses only 1MB or so. Never 4GB or more.

    > Again, we would like to see a complete decription of the proposed
    > userspace ABI. This one looks fairly ugly. Do I really have to write
    > 'S' 'h' 'a' 'r' 'e' 'd' ':' ' ' into some pseudo file?
    >
    > A better interface would be two suitably-named pseudo files each of
    > which takes a bare integer string. None of this funny colon-based
    > prefixing stuff.
    >

    Creating pseudo files for each variables is fine, though I don't see
    advantage by changing from
    "echo Shared: 16777216 > /sys/kernel/security/tomoyo/meminfo" to
    "echo 16777216 > /sys/kernel/security/tomoyo/quota/shared_memory".


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [TOMOYO #12 (2.6.28-rc2-mm1) 06/11] Common functions for TOMOYOLinux.

    Quoting Tetsuo Handa (penguin-kernel@i-love.sakura.ne.jp):
    > Hello.
    >
    > Serge E. Hallyn wrote:
    > > > I need to clarify reachability of "struct task_struct".
    > > >
    > > > A process inside a virtualized environment cannot reach "struct task_struct"
    > > > which belongs to outside the virtualized environment.
    > > >
    > > > A process outside virtualized environments can reach "struct task_struct"
    > > > which belongs to inside virtualized environments, can't it?

    > >
    > > To be precise, there isn't a real 'inside' and 'outside' virtualized
    > > environements. Rather pid namespaces are hierarchical.
    > >

    > So, processes which have non-topmost namespace cannot see processes which have
    > topmost namespace (like chroot()).
    > Then, it might be preferable if TOMOYO can prevent processes which have
    > non-topmost namespace from modifying policy information.
    > Do you think TOMOYO should do "current->nsproxy->pid_ns == &init_pid_ns"
    > checking like below one?


    Nah, I actually don't. There is nothing to stop init or getty from
    doing an unshare(CLONE_NEWNS|CLONE_NEWPID) so noone would be able to
    make modifications. The important thing is that the kernel won't
    use another task than what userspace intended, so I think you're ok.

    > static bool tomoyo_is_policy_manager(void)
    > {
    > struct tomoyo_policy_manager_entry *ptr;
    > const char *exe;
    > const struct task_struct *task = current;
    > const struct tomoyo_path_info *domainname = tomoyo_domain()->domainname;
    > bool found = false;
    >
    > if (!tomoyo_policy_loaded)
    > return true;
    > if (!tomoyo_manage_by_non_root && (task->cred->uid || task->cred->euid))


    Now the point of LSM is to let you decide on your own policy, so this
    is entirely up to you, but wouldn't it be nicer to use CAP_MAC_ADMIN's
    presence like SMACK does?

    > return false;
    > /* Don't allow modifying policy by processes not having init_pid_ns. */
    > if (task->nsproxy->pid_ns != &init_pid_ns)
    > return false;


    No, I think it's far better to keep this out. (Plus you'd have to use
    task_nsproxy() under rcu_read_lock.)

    I know at this point it might seem like I'm changing my mind left and
    right (sorry), so here is the guiding principle: pids are appropriate
    for userspace to communicate to userspace and, if done right, to the
    kernel. But the kernel must not cache them internally.

    (...and, if getting or sending them from/to user-space, must be careful
    about the pidns, of course.)

    So you're fine.

    thanks,
    -serge

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [TOMOYO #12 (2.6.28-rc2-mm1) 05/11] Memory and pathname management functions.

    On Mon, 10 Nov 2008 19:34:17 +0900 Kentaro Takeda wrote:

    >
    > ...
    >
    > > > +/**
    > > > + * tmy_alloc - Allocate memory for temporal purpose.
    > > > + *
    > > > + * @size: Size in bytes.
    > > > + *
    > > > + * Returns pointer to allocated memory on success, NULL otherwise.
    > > > + */
    > > > +void *tmy_alloc(const size_t size)
    > > > +{
    > > > + void *p = kzalloc(size, GFP_KERNEL);
    > > > + if (p)
    > > > + atomic_add(ksize(p), &dynamic_memory_size);
    > > > + return p;
    > > > +}

    > >
    > > Note that I said "kmalloc", not "kzalloc". This function zeroes
    > > everything all the time, and surely that is not necessary. It's just a
    > > waste of CPU time.
    > >

    > Callers of tmy_alloc assume that allocated memory is zeroed.


    That isn't the point. For programmer convenience we could make
    __alloc_pages() and kmalloc() zero all the memory too. But we don't
    because it is slow.

    > > > +/**
    > > > + * tmy_read_memory_counter - Check for memory usage.
    > > > + *
    > > > + * @head: Pointer to "struct tmy_io_buffer".
    > > > + *
    > > > + * Returns memory usage.

    > >
    > > In what units? Megabytes?
    > >

    > In bytes.


    Let me rephrase:

    The comment over tmy_read_memory_counter() fails to tell the reader
    what units are used for the return value. It should do so.

    > > Again, we would like to see a complete decription of the proposed
    > > userspace ABI. This one looks fairly ugly. Do I really have to write
    > > 'S' 'h' 'a' 'r' 'e' 'd' ':' ' ' into some pseudo file?
    > >
    > > A better interface would be two suitably-named pseudo files each of
    > > which takes a bare integer string. None of this funny colon-based
    > > prefixing stuff.
    > >

    > Creating pseudo files for each variables is fine, though I don't see
    > advantage by changing from
    > "echo Shared: 16777216 > /sys/kernel/security/tomoyo/meminfo" to
    > "echo 16777216 > /sys/kernel/security/tomoyo/quota/shared_memory".


    Well for starters, the existing interface is ugly as sin and will make
    kernel developers unhappy.

    There is a pretty strict one-value-per-file rule in sysfs files, and
    "multiple tagged values in one file" violates that a lot.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [TOMOYO #12 (2.6.28-rc2-mm1) 05/11] Memory and pathname management functions.

    Andrew Morton wrote:
    >>> Note that I said "kmalloc", not "kzalloc". This function zeroes
    >>> everything all the time, and surely that is not necessary. It's just a
    >>> waste of CPU time.
    >>>

    >> Callers of tmy_alloc assume that allocated memory is zeroed.

    >
    > That isn't the point. For programmer convenience we could make
    > __alloc_pages() and kmalloc() zero all the memory too. But we don't
    > because it is slow.

    Are you saying "make the callers of tmy_alloc() tolerable with
    uninitialized memory"?

    >>>> +/**
    >>>> + * tmy_read_memory_counter - Check for memory usage.
    >>>> + *
    >>>> + * @head: Pointer to "struct tmy_io_buffer".
    >>>> + *
    >>>> + * Returns memory usage.
    >>> In what units? Megabytes?
    >>>

    >> In bytes.

    >
    > Let me rephrase:
    >
    > The comment over tmy_read_memory_counter() fails to tell the reader
    > what units are used for the return value. It should do so.

    I see. Replaced "Check for memory usage." by "Check for memory usage in bytes".
    Thanks.

    >> Creating pseudo files for each variables is fine, though I don't see
    >> advantage by changing from
    >> "echo Shared: 16777216 > /sys/kernel/security/tomoyo/meminfo" to
    >> "echo 16777216 > /sys/kernel/security/tomoyo/quota/shared_memory".

    >
    > Well for starters, the existing interface is ugly as sin and will make
    > kernel developers unhappy.
    >
    > There is a pretty strict one-value-per-file rule in sysfs files, and
    > "multiple tagged values in one file" violates that a lot.

    /sys/kernel/security/ is not sysfs but securityfs.
    Does "one-value-per-file rule" also apply to securityfs?

    Regards,


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [TOMOYO #12 (2.6.28-rc2-mm1) 05/11] Memory and pathname management functions.

    On Tue, 11 Nov 2008 15:34:39 +0900 Kentaro Takeda wrote:

    > Andrew Morton wrote:
    > >>> Note that I said "kmalloc", not "kzalloc". This function zeroes
    > >>> everything all the time, and surely that is not necessary. It's just a
    > >>> waste of CPU time.
    > >>>
    > >> Callers of tmy_alloc assume that allocated memory is zeroed.

    > >
    > > That isn't the point. For programmer convenience we could make
    > > __alloc_pages() and kmalloc() zero all the memory too. But we don't
    > > because it is slow.

    > Are you saying "make the callers of tmy_alloc() tolerable with
    > uninitialized memory"?


    Well. That would be a desirable objective. I can understand the
    reasons for taking the easy way out. Given that Tomoyo doesn't seem to
    ever free memory again, one hopes that this function doesn't get called
    a lot, so the performance impact of zeroing out all that memory should
    be negligible.

    I think. Maybe I misinterpreted tmy_alloc(), and perhaps it _is_
    called frequently?

    > >> Creating pseudo files for each variables is fine, though I don't see
    > >> advantage by changing from
    > >> "echo Shared: 16777216 > /sys/kernel/security/tomoyo/meminfo" to
    > >> "echo 16777216 > /sys/kernel/security/tomoyo/quota/shared_memory".

    > >
    > > Well for starters, the existing interface is ugly as sin and will make
    > > kernel developers unhappy.
    > >
    > > There is a pretty strict one-value-per-file rule in sysfs files, and
    > > "multiple tagged values in one file" violates that a lot.

    > /sys/kernel/security/ is not sysfs but securityfs.
    > Does "one-value-per-file rule" also apply to securityfs?


    It should apply. It's not so much a matter of rules and regulations.
    One needs to look at the underlying _reasons_ why those rules came
    about. We got ourselves into a sticky mess with procfs with all sorts
    of ad-hoc data presentation and input formatting. It's inconsistent,
    complex, makes tool writing harder, etc.

    So we recognised our mistakes and when sysfs (otherwise known as procfs
    V2 ) came about we decided that sysfs files should not make the same
    mistakes.

    So, logically, that thinking should apply to all new pseudo-fs files.
    Even, in fact, ones which are in /proc!
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [TOMOYO #12 (2.6.28-rc2-mm1) 05/11] Memory and pathname management functions.

    Andrew Morton wrote:
    >> Are you saying "make the callers of tmy_alloc() tolerable with
    >> uninitialized memory"?

    >
    > Well. That would be a desirable objective. I can understand the
    > reasons for taking the easy way out. Given that Tomoyo doesn't seem to
    > ever free memory again, one hopes that this function doesn't get called
    > a lot, so the performance impact of zeroing out all that memory should
    > be negligible.
    >
    > I think. Maybe I misinterpreted tmy_alloc(), and perhaps it _is_
    > called frequently?

    It is called whenever open() / mkdir() / unlink() etc. are called,
    but not when read() / write() are called.
    Frequency of open() / mkdir() / unlink() etc. are much lower than frequency of
    read() / write().
    Main cost of pathname based access control is strcmp()ing (or even regexp()ing)
    over the list of strings, therefore zeroing buffer for pathname is relatively
    negligible.

    >>>> Creating pseudo files for each variables is fine, though I don't see
    >>>> advantage by changing from
    >>>> "echo Shared: 16777216 > /sys/kernel/security/tomoyo/meminfo" to
    >>>> "echo 16777216 > /sys/kernel/security/tomoyo/quota/shared_memory".
    >>> Well for starters, the existing interface is ugly as sin and will make
    >>> kernel developers unhappy.
    >>>
    >>> There is a pretty strict one-value-per-file rule in sysfs files, and
    >>> "multiple tagged values in one file" violates that a lot.

    >> /sys/kernel/security/ is not sysfs but securityfs.
    >> Does "one-value-per-file rule" also apply to securityfs?

    >
    > It should apply. It's not so much a matter of rules and regulations.
    > One needs to look at the underlying _reasons_ why those rules came
    > about. We got ourselves into a sticky mess with procfs with all sorts
    > of ad-hoc data presentation and input formatting. It's inconsistent,
    > complex, makes tool writing harder, etc.
    >
    > So we recognised our mistakes and when sysfs (otherwise known as procfs
    > V2 ) came about we decided that sysfs files should not make the same
    > mistakes.
    >
    > So, logically, that thinking should apply to all new pseudo-fs files.
    > Even, in fact, ones which are in /proc!

    Well, regarding memory usage, it is easy to follow "one-value-per-file rule".
    But regarding policy information (which is managed as lists),
    "one-value-per-file rule" is not suitable. I think none of SELinux, SMACK,
    AppArmor, TOMOYO create "one pseudo file for one value".
    This /sys/kernel/security/tomoyo/ interface is used by only TOMOYO's management
    programs, and not by generic programs.

    Regards,

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread
Page 2 of 2 FirstFirst 1 2