On Tue, Mar 05, 2013 at 03:24:27PM -0800, Nathaniel Chen 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, and attempts to try /smack in case it fails.
> You will need to create the /smack mountpoint manually if needed.
> 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
Hi,
looks nice and clean in general. Some comments below.
> ---
> Makefile.am | 2 +
> src/core/main.c | 3 ++
> src/core/mount-setup.c | 4 +-
> src/core/smack-setup.c | 100
> +++++++++++++++++++++++++++++++++++++++++++++++++
> src/core/smack-setup.h | 26 +++++++++++++
> 5 files changed, 134 insertions(+), 1 deletion(-)
> create mode 100644 src/core/smack-setup.c
> create mode 100644 src/core/smack-setup.h
>
> diff --git a/Makefile.am b/Makefile.am
> index 2108abe..b8bac5f 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -832,6 +832,8 @@ libsystemd_core_la_SOURCES = \
> src/core/selinux-access.h \
> src/core/selinux-setup.c \
> src/core/selinux-setup.h \
> + src/core/smack-setup.c \
> + src/core/smack-setup.h \
> src/core/ima-setup.c \
> src/core/ima-setup.h \
> src/core/locale-setup.h \
> diff --git a/src/core/main.c b/src/core/main.c
> index 71e0a6c..71e7124 100644
> --- a/src/core/main.c
> +++ b/src/core/main.c
> @@ -67,6 +67,7 @@
> #include "selinux-setup.h"
> #include "ima-setup.h"
> #include "fileio.h"
> +#include "smack-setup.h"
>
> static enum {
> ACTION_RUN,
> @@ -1350,6 +1351,8 @@ int main(int argc, char *argv[]) {
> goto finish;
> if (ima_setup() < 0)
> goto finish;
> + if (smack_setup() < 0)
> + goto finish;
As you can see from indentation here, something is askew: and indeed,
we only use spaces for indenation, while you add tabs.
> }
>
> if (label_init(NULL) < 0)
> diff --git a/src/core/mount-setup.c b/src/core/mount-setup.c
> index dab3601..0917fbe 100644
> --- a/src/core/mount-setup.c
> +++ b/src/core/mount-setup.c
> @@ -66,7 +66,7 @@ typedef struct MountPoint {
> /* The first three entries we might need before SELinux is up. The
> * fourth (securityfs) is needed by IMA to load a custom policy. The
> * other ones we can delay until SELinux and IMA are loaded. */
> -#define N_EARLY_MOUNT 4
> +#define N_EARLY_MOUNT 5
>
> static const MountPoint mount_table[] = {
> { "proc", "/proc", "proc", NULL,
> MS_NOSUID|MS_NOEXEC|MS_NODEV,
> @@ -77,6 +77,8 @@ static const MountPoint mount_table[] = {
> NULL, MNT_FATAL|MNT_IN_CONTAINER },
> { "securityfs", "/sys/kernel/security", "securityfs", NULL,
> MS_NOSUID|MS_NOEXEC|MS_NODEV,
> NULL, MNT_NONE },
> + { "smackfs", "/sys/fs/smackfs", "smackfs",
> "smackfsdef=*", MS_NOSUID|MS_NOEXEC|MS_NODEV|MS_STRICTATIME,
> + NULL, MNT_NONE},
> { "tmpfs", "/dev/shm", "tmpfs",
> "mode=1777", MS_NOSUID|MS_NODEV|MS_STRICTATIME,
> NULL, MNT_FATAL|MNT_IN_CONTAINER },
> { "devpts", "/dev/pts", "devpts",
> "mode=620,gid=" STRINGIFY(TTY_GID), MS_NOSUID|MS_NOEXEC,
> diff --git a/src/core/smack-setup.c b/src/core/smack-setup.c
> new file mode 100644
> index 0000000..38ce471
> --- /dev/null
> +++ b/src/core/smack-setup.c
> @@ -0,0 +1,100 @@
> +/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
> +
> +/***
> + This file is part of systemd.
> +
> + Copyright (C) 2013 Intel Corporation
> + Authors:
> + Nathaniel Chen <nathaniel.c...@intel.com>
> +
> + systemd is free software; you can redistribute it and/or modify it
> + under the terms of the GNU Lesser General Public License as published
> + by the Free Software Foundation; either version 2.1 of the License,
> + or (at your option) any later version.
> +
> + systemd is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public License
> + along with systemd; If not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +#include <stdio.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <sys/vfs.h>
> +#include <fcntl.h>
> +#include <sys/types.h>
> +#include <dirent.h>
> +#include <sys/mount.h>
> +#include <stdint.h>
> +
> +#include "smack-setup.h"
> +#include "mount-setup.h"
> +#include "util.h"
> +#include "log.h"
> +#include "label.h"
> +#include "macro.h"
> +
> +#define ACCESSES_D_PATH "/etc/smack/accesses.d/"
> +
> +int smack_setup(void) {
> +
> + FILE *smack;
> + FILE *policy;
We have _cleanup_close_ which closes the fd's automatically when exiting
from scope.
Please spell this as:
#include <macro.h>
FILE _cleanup_close_ *smack = NULL;
DIR _cleanup_closedir_ *dir = NULL;
and in the for-loop below
FILE _cleanup_close *policy = NULL;
and remove all the fclose() and closedir() calls below.
> + int m = -1;
> + DIR *dir;
> + struct dirent *entry;
> + char pol[PATH_MAX];
> + char buf[NAME_MAX];
> +
> + /* assure that we've attempted to mount smackfs */
> + mount_setup_early();
I compared this function to ima_setup, and the difference is that over there
mount_setup_early() is guarded by #ifndef SELINUX. Would it be possible
to extract this part into the main function, to ensure that mount_setup_early
is done before setting security modules up, so that there's no need to repeat
mount_setup_early() invocations?
> + smack = fopen("/sys/fs/smackfs/load2", "w");
Please use "we".
> + if (smack == NULL) {
> + m = mount("smackfs", "/smack", "smackfs",
> MS_NOSUID|MS_NOEXEC|MS_NODEV|MS_STRICTATIME, "smackfsdef=*");
> + if (m == -1) {
> + log_error("Smack is not enabled in the kernel. error:
> %s", strerror(errno));
> + return 0;
> + }
> + smack = fopen("/smack/load2", "w");
No error check here?
> + }
> +
> + /* write rules to load2 from every file in the directory */
> + dir = opendir(ACCESSES_D_PATH);
> + if (dir == NULL) {
> + log_error("failed to open Smack policy directory %s, error:
> %s", ACCESSES_D_PATH, strerror(errno));
> + fclose(smack);
> + return -errno;
> + }
If smack is not available, then this function should return 0. Right now it'll
fail,
which will interrupt the rest of the boot.
> + while ((entry = readdir(dir))) {
> + if (entry->d_name[0] == '.')
> + continue;
> +
> + snprintf(pol, PATH_MAX, ACCESSES_D_PATH"%s", entry->d_name);
You could use openat and completely avoid the snprintfs into a fixed buffer,
which _are_ a bit ugly.
> + policy = fopen(pol, "r");
"re"
> + if (policy == NULL) {
> + log_error("Failed to open Smack policy file %s, error:
> %s", pol, strerror(errno));
> + fclose(smack);
> + closedir(dir);
> + return -errno;
> + }
> + /* load2 write rules in the kernel require a line buffered
> stream */
> + while ((fgets(buf, NAME_MAX, policy))) {
> + fprintf(smack, "%s", buf);
fputs?
> + fflush(smack);
> + log_info("Smack rule written: %s", buf);
How many of those are there usually? Wouldn't log_debug be more appropriate?
> + }
> + log_info("Successfully loaded custom Smack policy %s", pol);
How many of those are there usually? Wouldn't log_debug be more appropriate?
> + fclose(policy);
> + }
> + closedir(dir);
> + fclose(smack);
> +
> +return 0;
> +}
> diff --git a/src/core/smack-setup.h b/src/core/smack-setup.h
> new file mode 100644
> index 0000000..2397646
> --- /dev/null
> +++ b/src/core/smack-setup.h
> @@ -0,0 +1,26 @@
> +/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
> +
> +#pragma once
> +
> +/***
> + This file is part of systemd.
> +
> + Copyright (C) 2013 Intel Corporation
> + Authors:
> + Nathaniel Chen <nathaniel.c...@intel.com>
> +
> + systemd is free software; you can redistribute it and/or modify it
> + under the terms of the GNU Lesser General Public License as published
> + by the Free Software Foundation; either version 2.1 of the License,
> + or (at your option) any later version.
> +
> + systemd is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public License
> + along with systemd; If not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +int smack_setup(void);
Zbyszek
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
