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

Zbigniew Jędrzejewski-Szmek zbyszek at in.waw.pl
Tue Mar 5 15:56:02 PST 2013


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.chen at 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.chen at 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


More information about the systemd-devel mailing list