[systemd-devel] [PATCH] mount and initialize Smack

[Lennart Poettering]

On Thu, 07.03.13 11:06, Nathaniel Chen (nathaniel.chen at intel.com) wrote:

> SMACK is the Simple Mandatory Access Control Kernel, a minimal
> approach to Access Control implemented as a kernel LSM.
> 
> The kernel exposes the smackfs filesystem API through which access
> rules can be loaded. At boot time, we want to load the access rules
> as early as possible to ensure all early boot steps are checked by Smack.
> 
> This patch mounts smackfs at the new location at /sys/fs/smackfs for
> kernels 3.8 and above. The /smack mountpoint is not supported.
> After mounting smackfs, rules are loaded from the usual location.
> 
> For more information about Smack see:
>   http://www.kernel.org/doc/Documentation/security/Smack.txt

Merged all three, but made some changes. Just to point this out:

> +int smack_setup(void) {
> +
> +        FILE _cleanup_fclose_ *smack = NULL;
> +        FILE _cleanup_fclose_ *policy = NULL;
> +        int _cleanup_close_ dfd = -1;
> +        int _cleanup_close_ pol = -1;

The _cleanup_xyz_ stuff works on the C block scope. i.e. if you use this
like this these the four variables will be freed once when the function
exits. However, you allocate into them a couple of times inside the
inner loop, so the earlier instances are lost. Moving this into the loop
instead fixes it, since then the per-scope logic makes sure the
variables are freed on each loop iteration.

Also, the diretcory fd is closed as part of the DIR* object anyway, and
similar the pol fd is closed as part of the policy FILE* object. So you
closed these twice.

> +        DIR *dir = NULL;
> +        struct dirent *entry;
> +        char buf[NAME_MAX];
> +
> +        smack = fopen("/sys/fs/smackfs/load2", "we");
> +        if (smack == NULL)  {
> +                log_info("Smack is not enabled in the kernel, not loading access rules.");
> +                return 0;
> +        }
> +
> +        /* write rules to load2 from every file in the directory */
> +        dir = opendir(ACCESSES_D_PATH);
> +        if (dir == NULL) {
> +                log_info("Smack access rules directory not found: %s", ACCESSES_D_PATH);
> +                return 0;
> +        }
> +        dfd = dirfd(dir);
> +        if (dfd == -1) {
> +                log_error("Smack access rules directory %s not opened: %m.", ACCESSES_D_PATH);
> +                return 0;
> +        }
> +        FOREACH_DIRENT(entry, dir, return 0;) {
> +                if (entry->d_name[0] == '.')
> +                        continue;

This is actually redundant, as the macro does it anyway...

> +                pol = openat(dfd, entry->d_name, O_RDONLY);
> +                if (pol == -1) {
> +                        log_error("Smack access rule file %s not opened: %m", entry->d_name);
> +                        continue;
> +                }
> +                policy = fdopen(pol, "re");
> +                if (policy == NULL) {
> +                        log_error("Smack access rule file %s not opened: %m", entry->d_name);
> +                        continue;
> +                }
> +                /* load2 write rules in the kernel require a line buffered stream */
> +                FOREACH_LINE(buf, policy, log_error("Failed to read
> from Smack access rule file %s: %m", policy); continue;) {

You certainly don't want to continue reading file after we hit an IO
error... That would result in a busy loop.

> +                        fputs(buf, smack);
> +                        fflush(smack);
> +                }
> +        }
> +        log_info("Successfully loaded Smack policies.");

Anyway, all commited, but not tested after my changes, since I don't
have SMACK. Please test.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.