[PATCH 0/3] pull: add swapon control structure

[PATCH 0/3] pull: add swapon control structure

[Sami Kerola]

Hello,

The first patch moves all global variables to new struct swapon_ctl, and the
second does the same for few variables introduced during execution.  This
should make reading what happens in swapon.c a little bit easier.

The last change will forbid executing mkswap when swapon has setuid bit, and
it has taken effect.  This should make simple PATH preference execvp()
hijack vulnerability go away, among other similar setuid execution issues. 
I do not think this change is CVE worty because swapon(8) is _not_ expected
to have setuid.  The point of this change is to add at least some safety
mechanism for users are aiming to shoot their own feets.

Sami Kerola (3):
  swapon: add control struct
  swapon: move function arguments to control structure
  swapon: do not run execvp() calls when swapon is setuid binary

 sys-utils/swapon.c | 381 +++++++++++++++++++++++++++--------------------------
 1 file changed, 193 insertions(+), 188 deletions(-)

-- 
2.7.0

[PATCH 1/3] swapon: add control struct

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/swapon.c | 253 +++++++++++++++++++++++++++--------------------------
 1 file changed, 129 insertions(+), 124 deletions(-)

diff --git a/sys-utils/swapon.c b/sys-utils/swapon.c
index c4d1544..b677f50 100644
--- a/sys-utils/swapon.c
+++ b/sys-utils/swapon.c
@@ -65,7 +65,6 @@
 # define swapon(path, flags) syscall(SYS_swapon, path, flags)
 #endif
 
-#define QUIET	1
 #define CANONIC	1
 
 #define MAX_PAGESIZE	(64 * 1024)
@@ -75,15 +74,6 @@ enum {
 	SIG_SWSUSPEND
 };
 
-static int all;
-static int priority = -1;	/* non-prioritized swap by default */
-static int discard;		/* don't send swap discards by default */
-
-/* If true, don't complain if the device/file doesn't exist */
-static int ifexists;
-static int fixpgsz;
-static int verbose;
-
 /* column names */
 struct colinfo {
         const char *name; /* header */
@@ -92,10 +82,6 @@ struct colinfo {
         const char *help;
 };
 
-/* basic output flags */
-static int no_headings;
-static int raw;
-
 enum {
 	COL_PATH,
 	COL_TYPE,
@@ -115,8 +101,25 @@ struct colinfo infos[] = {
 	[COL_LABEL]    = { "LABEL",	0.20, 0, N_("swap label")},
 };
 
-static int columns[ARRAY_SIZE(infos) * 2];
-static int ncolumns;
+/* control struct */
+struct swapon_ctl {
+	char *options;			/* fstab-compatible option string */
+	const char *label;		/* swap label */
+	const char *uuid;		/* unique identifier */
+	int discard;			/* discard policy */
+	int columns[ARRAY_SIZE(infos) * 2];	/* --show columns */
+	int ncolumns;			/* number of columns */
+	int priority;			/* non-prioritized swap by default */
+	unsigned int
+		all:1,			/* turn on all swap devices */
+		bytes:1,		/* display --show in bytes */
+		fix_page_size:1,	/* reinitialize page size */
+		no_fail:1,		/* skip devices that do not exist */
+		no_heading:1,		/* toggle --show headers */
+		raw:1,			/* toggle --show alignment */
+		show:1,			/* display --show information */
+		verbose:1;		/* be chatty */
+};
 
 static int column_name_to_id(const char *name, size_t namesz)
 {
@@ -134,20 +137,20 @@ static int column_name_to_id(const char *name, size_t namesz)
 	return -1;
 }
 
-static inline int get_column_id(int num)
+static inline int get_column_id(const struct swapon_ctl *ctl, int num)
 {
-	assert(num < ncolumns);
-	assert(columns[num] < (int) ARRAY_SIZE(infos));
+	assert(num < ctl->ncolumns);
+	assert(ctl->columns[num] < (int) ARRAY_SIZE(infos));
 
-	return columns[num];
+	return ctl->columns[num];
 }
 
-static inline struct colinfo *get_column_info(unsigned num)
+static inline struct colinfo *get_column_info(const struct swapon_ctl *ctl, unsigned num)
 {
-	return &infos[get_column_id(num)];
+	return &infos[get_column_id(ctl, num)];
 }
 
-static void add_scols_line(struct libscols_table *table, struct libmnt_fs *fs, int bytes)
+static void add_scols_line(const struct swapon_ctl *ctl, struct libscols_table *table, struct libmnt_fs *fs)
 {
 	int i;
 	struct libscols_line *line;
@@ -163,11 +166,11 @@ static void add_scols_line(struct libscols_table *table, struct libmnt_fs *fs, i
 	data = mnt_fs_get_source(fs);
 	if (access(data, R_OK) == 0)
 		pr = get_swap_prober(data);
-	for (i = 0; i < ncolumns; i++) {
+	for (i = 0; i < ctl->ncolumns; i++) {
 		char *str = NULL;
 		off_t size;
 
-		switch (get_column_id(i)) {
+		switch (get_column_id(ctl, i)) {
 		case COL_PATH:
 			xasprintf(&str, "%s", mnt_fs_get_source(fs));
 			break;
@@ -177,7 +180,7 @@ static void add_scols_line(struct libscols_table *table, struct libmnt_fs *fs, i
 		case COL_SIZE:
 			size = mnt_fs_get_size(fs);
 			size *= 1024;	/* convert to bytes */
-			if (bytes)
+			if (ctl->bytes)
 				xasprintf(&str, "%jd", size);
 			else
 				str = size_to_human_string(SIZE_SUFFIX_1LETTER, size);
@@ -185,7 +188,7 @@ static void add_scols_line(struct libscols_table *table, struct libmnt_fs *fs, i
 		case COL_USED:
 			size = mnt_fs_get_usedsize(fs);
 			size *= 1024;	/* convert to bytes */
-			if (bytes)
+			if (ctl->bytes)
 				xasprintf(&str, "%jd", size);
 			else
 				str = size_to_human_string(SIZE_SUFFIX_1LETTER, size);
@@ -244,7 +247,7 @@ static int display_summary(void)
 	return 0;
 }
 
-static int show_table(int bytes)
+static int show_table(struct swapon_ctl *ctl)
 {
 	struct libmnt_table *st = get_swaps();
 	struct libmnt_iter *itr = NULL;
@@ -265,18 +268,18 @@ static int show_table(int bytes)
 	if (!table)
 		err(EXIT_FAILURE, _("failed to initialize output table"));
 
-	scols_table_enable_raw(table, raw);
-	scols_table_enable_noheadings(table, no_headings);
+	scols_table_enable_raw(table, ctl->raw);
+	scols_table_enable_noheadings(table, ctl->no_heading);
 
-	for (i = 0; i < ncolumns; i++) {
-		struct colinfo *col = get_column_info(i);
+	for (i = 0; i < ctl->ncolumns; i++) {
+		struct colinfo *col = get_column_info(ctl, i);
 
 		if (!scols_table_new_column(table, col->name, col->whint, col->flags))
 			err(EXIT_FAILURE, _("failed to initialize output column"));
 	}
 
 	while (mnt_table_next_fs(st, itr, &fs) == 0)
-		add_scols_line(table, fs, bytes);
+		add_scols_line(ctl, table, fs);
 
 	scols_print_table(table);
 	scols_unref_table(table);
@@ -418,7 +421,8 @@ err:
 }
 
 /* returns real size of swap space */
-static unsigned long long swap_get_size(const char *hdr, const char *devname,
+static unsigned long long swap_get_size(const struct swapon_ctl *ctl,
+					const char *hdr, const char *devname,
 					unsigned int pagesize)
 {
 	unsigned int last_page = 0;
@@ -433,7 +437,7 @@ static unsigned long long swap_get_size(const char *hdr, const char *devname,
 		flip = 1;
 		last_page = swab32(s->last_page);
 	}
-	if (verbose)
+	if (ctl->verbose)
 		warnx(_("%s: found swap signature: version %ud, "
 			"page-size %d, %s byte order"),
 			devname,
@@ -444,14 +448,14 @@ static unsigned long long swap_get_size(const char *hdr, const char *devname,
 	return ((unsigned long long) last_page + 1) * pagesize;
 }
 
-static void swap_get_info(const char *hdr, char **label, char **uuid)
+static void swap_get_info(struct swapon_ctl *ctl, const char *hdr)
 {
 	struct swap_header_v1_2 *s = (struct swap_header_v1_2 *) hdr;
 
-	if (s && *s->volume_name && label)
-		*label = xstrdup(s->volume_name);
+	if (s && *s->volume_name && ctl->label)
+		ctl->label = xstrdup(s->volume_name);
 
-	if (s && *s->uuid && uuid) {
+	if (s && *s->uuid && ctl->uuid) {
 		const unsigned char *u = s->uuid;
 		char str[37];
 
@@ -462,11 +466,11 @@ static void swap_get_info(const char *hdr, char **label, char **uuid)
 			u[0], u[1], u[2], u[3],
 			u[4], u[5], u[6], u[7],
 			u[8], u[9], u[10], u[11], u[12], u[13], u[14], u[15]);
-		*uuid = xstrdup(str);
+		ctl->uuid = xstrdup(str);
 	}
 }
 
-static int swapon_checks(const char *special)
+static int swapon_checks(struct swapon_ctl *ctl, const char *special)
 {
 	struct stat st;
 	int fd = -1, sig;
@@ -519,24 +523,24 @@ static int swapon_checks(const char *special)
 
 	if (sig == SIG_SWAPSPACE && pagesize) {
 		unsigned long long swapsize =
-				swap_get_size(hdr, special, pagesize);
+				swap_get_size(ctl, hdr, special, pagesize);
 		int syspg = getpagesize();
 
-		if (verbose)
+		if (ctl->verbose)
 			warnx(_("%s: pagesize=%d, swapsize=%llu, devsize=%llu"),
 				special, pagesize, swapsize, devsize);
 
 		if (swapsize > devsize) {
-			if (verbose)
+			if (ctl->verbose)
 				warnx(_("%s: last_page 0x%08llx is larger"
 					" than actual size of swapspace"),
 					special, swapsize);
 		} else if (syspg < 0 || (unsigned) syspg != pagesize) {
-			if (fixpgsz) {
+			if (ctl->fix_page_size) {
 				char *label = NULL, *uuid = NULL;
 				int rc;
 
-				swap_get_info(hdr, &label, &uuid);
+				swap_get_info(ctl, hdr);
 
 				warnx(_("%s: swap format pagesize does not match."),
 					special);
@@ -572,14 +576,14 @@ err:
 	return -1;
 }
 
-static int do_swapon(const char *orig_special, int prio,
-		     int fl_discard, int canonic)
+static int do_swapon(struct swapon_ctl *ctl, const char *orig_special,
+		     int canonic)
 {
 	int status;
 	const char *special = orig_special;
 	int flags = 0;
 
-	if (verbose)
+	if (ctl->verbose)
 		printf(_("swapon %s\n"), orig_special);
 
 	if (!canonic) {
@@ -588,15 +592,15 @@ static int do_swapon(const char *orig_special, int prio,
 			return cannot_find(orig_special);
 	}
 
-	if (swapon_checks(special))
+	if (swapon_checks(ctl, special))
 		return -1;
 
 #ifdef SWAP_FLAG_PREFER
-	if (prio >= 0) {
-		if (prio > SWAP_FLAG_PRIO_MASK)
-			prio = SWAP_FLAG_PRIO_MASK;
+	if (ctl->priority >= 0) {
+		if (ctl->priority > SWAP_FLAG_PRIO_MASK)
+			ctl->priority = SWAP_FLAG_PRIO_MASK;
 		flags = SWAP_FLAG_PREFER
-			| ((prio & SWAP_FLAG_PRIO_MASK)
+			| ((ctl->priority & SWAP_FLAG_PRIO_MASK)
 			   << SWAP_FLAG_PRIO_SHIFT);
 	}
 #endif
@@ -604,17 +608,17 @@ static int do_swapon(const char *orig_special, int prio,
 	 * Validate the discard flags passed and set them
 	 * accordingly before calling sys_swapon.
 	 */
-	if (fl_discard && !(fl_discard & ~SWAP_FLAGS_DISCARD_VALID)) {
+	if (ctl->discard && !(ctl->discard & ~SWAP_FLAGS_DISCARD_VALID)) {
 		/*
 		 * If we get here with both discard policy flags set,
 		 * we just need to tell the kernel to enable discards
 		 * and it will do correctly, just as we expect.
 		 */
-		if ((fl_discard & SWAP_FLAG_DISCARD_ONCE) &&
-		    (fl_discard & SWAP_FLAG_DISCARD_PAGES))
+		if ((ctl->discard & SWAP_FLAG_DISCARD_ONCE) &&
+		    (ctl->discard & SWAP_FLAG_DISCARD_PAGES))
 			flags |= SWAP_FLAG_DISCARD;
 		else
-			flags |= fl_discard;
+			flags |= ctl->discard;
 	}
 
 	status = swapon(special, flags);
@@ -624,57 +628,56 @@ static int do_swapon(const char *orig_special, int prio,
 	return status;
 }
 
-static int swapon_by_label(const char *label, int prio, int dsc)
+static int swapon_by_label(struct swapon_ctl *ctl)
 {
-	const char *special = mnt_resolve_tag("LABEL", label, mntcache);
-	return special ? do_swapon(special, prio, dsc, CANONIC) :
-			 cannot_find(label);
+	const char *special = mnt_resolve_tag("LABEL", ctl->label, mntcache);
+	return special ? do_swapon(ctl, special, CANONIC) :
+			 cannot_find(ctl->label);
 }
 
-static int swapon_by_uuid(const char *uuid, int prio, int dsc)
+static int swapon_by_uuid(struct swapon_ctl *ctl)
 {
-	const char *special = mnt_resolve_tag("UUID", uuid, mntcache);
-	return special ? do_swapon(special, prio, dsc, CANONIC) :
-			 cannot_find(uuid);
+	const char *special = mnt_resolve_tag("UUID", ctl->uuid, mntcache);
+	return special ? do_swapon(ctl, special, CANONIC) :
+			 cannot_find(ctl->uuid);
 }
 
 /* -o <options> or fstab */
-static int parse_options(const char *optstr,
-			 int *prio, int *disc, int *nofail)
+static int parse_options(struct swapon_ctl *ctl)
 {
 	char *arg = NULL;
 
-	assert(optstr);
-	assert(prio);
-	assert(disc);
-	assert(nofail);
+	assert(ctl->options);
+	assert(ctl->priority);
+	assert(ctl->discard);
+	assert(ctl->no_fail);
 
-	if (mnt_optstr_get_option(optstr, "nofail", NULL, 0) == 0)
-		*nofail = 1;
+	if (mnt_optstr_get_option(ctl->options, "nofail", NULL, 0) == 0)
+		ctl->no_fail = 1;
 
-	if (mnt_optstr_get_option(optstr, "discard", &arg, NULL) == 0) {
-		*disc |= SWAP_FLAG_DISCARD;
+	if (mnt_optstr_get_option(ctl->options, "discard", &arg, NULL) == 0) {
+		ctl->discard |= SWAP_FLAG_DISCARD;
 
 		if (arg) {
 			/* only single-time discards are wanted */
 			if (strcmp(arg, "once") == 0)
-				*disc |= SWAP_FLAG_DISCARD_ONCE;
+				ctl->discard |= SWAP_FLAG_DISCARD_ONCE;
 
 			/* do discard for every released swap page */
 			if (strcmp(arg, "pages") == 0)
-				*disc |= SWAP_FLAG_DISCARD_PAGES;
+				ctl->discard |= SWAP_FLAG_DISCARD_PAGES;
 			}
 	}
 
 	arg = NULL;
-	if (mnt_optstr_get_option(optstr, "pri", &arg, NULL) == 0 && arg)
-		*prio = atoi(arg);
+	if (mnt_optstr_get_option(ctl->options, "pri", &arg, NULL) == 0 && arg)
+		ctl->priority = atoi(arg);
 
 	return 0;
 }
 
 
-static int swapon_all(void)
+static int swapon_all(struct swapon_ctl *ctl)
 {
 	struct libmnt_table *tb = get_fstab();
 	struct libmnt_iter *itr;
@@ -690,7 +693,6 @@ static int swapon_all(void)
 
 	while (mnt_table_find_next_fs(tb, itr, match_swap, NULL, &fs) == 0) {
 		/* defaults */
-		int pri = priority, dsc = discard, nofail = ifexists;
 		const char *opts, *src;
 
 		if (mnt_fs_get_option(fs, "noauto", NULL, NULL) == 0)
@@ -698,18 +700,18 @@ static int swapon_all(void)
 
 		opts = mnt_fs_get_options(fs);
 		if (opts)
-			parse_options(opts, &pri, &dsc, &nofail);
+			parse_options(ctl);
 
 		src = mnt_resolve_spec(mnt_fs_get_source(fs), mntcache);
 		if (!src) {
-			if (!nofail)
+			if (!ctl->no_fail)
 				status |= cannot_find(mnt_fs_get_source(fs));
 			continue;
 		}
 
 		if (!is_active_swap(src) &&
-		    (!nofail || !access(src, R_OK)))
-			status |= do_swapon(src, pri, dsc, CANONIC);
+		    (!ctl->no_fail || !access(src, R_OK)))
+			status |= do_swapon(ctl, src, CANONIC);
 	}
 
 	mnt_free_iter(itr);
@@ -769,10 +771,7 @@ static void __attribute__ ((__noreturn__)) usage(FILE * out)
 
 int main(int argc, char *argv[])
 {
-	char *options = NULL;
 	int status = 0, c;
-	int show = 0;
-	int bytes = 0;
 	size_t i;
 
 	enum {
@@ -809,6 +808,8 @@ int main(int argc, char *argv[])
 	};
 	int excl_st[ARRAY_SIZE(excl)] = UL_EXCL_STATUS_INIT;
 
+	struct swapon_ctl ctl = { .priority = -1, 0 };
+
 	setlocale(LC_ALL, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE);
@@ -824,16 +825,16 @@ int main(int argc, char *argv[])
 
 		switch (c) {
 		case 'a':		/* all */
-			++all;
+			ctl.all = 1;
 			break;
 		case 'h':		/* help */
 			usage(stdout);
 			break;
 		case 'o':
-			options = optarg;
+			ctl.options = optarg;
 			break;
 		case 'p':		/* priority */
-			priority = strtos16_or_err(optarg,
+			ctl.priority = strtos16_or_err(optarg,
 					   _("failed to parse priority"));
 			break;
 		case 'L':
@@ -843,50 +844,50 @@ int main(int argc, char *argv[])
 			add_uuid(optarg);
 			break;
 		case 'd':
-			discard |= SWAP_FLAG_DISCARD;
+			ctl.discard |= SWAP_FLAG_DISCARD;
 			if (optarg) {
 				if (*optarg == '=')
 					optarg++;
 
 				if (strcmp(optarg, "once") == 0)
-					discard |= SWAP_FLAG_DISCARD_ONCE;
+					ctl.discard |= SWAP_FLAG_DISCARD_ONCE;
 				else if (strcmp(optarg, "pages") == 0)
-					discard |= SWAP_FLAG_DISCARD_PAGES;
+					ctl.discard |= SWAP_FLAG_DISCARD_PAGES;
 				else
 					errx(EXIT_FAILURE, _("unsupported discard policy: %s"), optarg);
 			}
 			break;
 		case 'e':               /* ifexists */
-		        ifexists = 1;
+			ctl.no_fail = 1;
 			break;
 		case 'f':
-			fixpgsz = 1;
+			ctl.fix_page_size = 1;
 			break;
 		case 's':		/* status report */
 			status = display_summary();
 			return status;
 		case 'v':		/* be chatty */
-			++verbose;
+			ctl.verbose = 1;
 			break;
 		case SHOW_OPTION:
 			if (optarg) {
-				ncolumns = string_to_idarray(optarg,
-							     columns,
-							     ARRAY_SIZE(columns),
+				ctl.ncolumns = string_to_idarray(optarg,
+							     ctl.columns,
+							     ARRAY_SIZE(ctl.columns),
 							     column_name_to_id);
-				if (ncolumns < 0)
+				if (ctl.ncolumns < 0)
 					return EXIT_FAILURE;
 			}
-			show = 1;
+			ctl.show = 1;
 			break;
 		case NOHEADINGS_OPTION:
-			no_headings = 1;
+			ctl.no_heading = 1;
 			break;
 		case RAW_OPTION:
-			raw = 1;
+			ctl.raw = 1;
 			break;
 		case BYTES_OPTION:
-			bytes = 1;
+			ctl.bytes = 1;
 			break;
 		case 'V':		/* version */
 			printf(UTIL_LINUX_VERSION);
@@ -900,36 +901,40 @@ int main(int argc, char *argv[])
 	}
 	argv += optind;
 
-	if (show || (!all && !numof_labels() && !numof_uuids() && *argv == NULL)) {
-		if (!ncolumns) {
+	if (ctl.show || (!ctl.all && !numof_labels() && !numof_uuids() && *argv == NULL)) {
+		if (!ctl.ncolumns) {
 			/* default columns */
-			columns[ncolumns++] = COL_PATH;
-			columns[ncolumns++] = COL_TYPE;
-			columns[ncolumns++] = COL_SIZE;
-			columns[ncolumns++] = COL_USED;
-			columns[ncolumns++] = COL_PRIO;
+			ctl.columns[ctl.ncolumns++] = COL_PATH;
+			ctl.columns[ctl.ncolumns++] = COL_TYPE;
+			ctl.columns[ctl.ncolumns++] = COL_SIZE;
+			ctl.columns[ctl.ncolumns++] = COL_USED;
+			ctl.columns[ctl.ncolumns++] = COL_PRIO;
 		}
-		status = show_table(bytes);
+		status = show_table(&ctl);
 		return status;
 	}
 
-	if (ifexists && !all)
+	if (ctl.no_fail && !ctl.all)
 		usage(stderr);
 
-	if (all)
-		status |= swapon_all();
+	if (ctl.all)
+		status |= swapon_all(&ctl);
 
-	if (options)
-		parse_options(options, &priority, &discard, &ifexists);
+	if (ctl.options)
+		parse_options(&ctl);
 
-	for (i = 0; i < numof_labels(); i++)
-		status |= swapon_by_label(get_label(i), priority, discard);
+	for (i = 0; i < numof_labels(); i++) {
+		ctl.label = get_label(i);
+		status |= swapon_by_label(&ctl);
+	}
 
-	for (i = 0; i < numof_uuids(); i++)
-		status |= swapon_by_uuid(get_uuid(i), priority, discard);
+	for (i = 0; i < numof_uuids(); i++) {
+		ctl.uuid = get_uuid(i);
+		status |= swapon_by_uuid(&ctl);
+	}
 
 	while (*argv != NULL)
-		status |= do_swapon(*argv++, priority, discard, !CANONIC);
+		status |= do_swapon(&ctl, *argv++, !CANONIC);
 
 	free_tables();
 	mnt_unref_cache(mntcache);
-- 
2.7.0

[PATCH 2/3] swapon: move function arguments to control structure

[Sami Kerola]

This makes reading what the code does easier.  This change also makes error
messages to prefer none-canonical device path, e.g., the one user supplied
rather than the canonical path needed internally.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/swapon.c | 158 ++++++++++++++++++++++++++---------------------------
 1 file changed, 77 insertions(+), 81 deletions(-)

diff --git a/sys-utils/swapon.c b/sys-utils/swapon.c
index b677f50..5ad5cd1 100644
--- a/sys-utils/swapon.c
+++ b/sys-utils/swapon.c
@@ -65,8 +65,6 @@
 # define swapon(path, flags) syscall(SYS_swapon, path, flags)
 #endif
 
-#define CANONIC	1
-
 #define MAX_PAGESIZE	(64 * 1024)
 
 enum {
@@ -103,6 +101,8 @@ struct colinfo infos[] = {
 
 /* control struct */
 struct swapon_ctl {
+	char *device;			/* device or file to be turned on */
+	char *dev_canonical;		/* canonical path to device */
 	char *options;			/* fstab-compatible option string */
 	const char *label;		/* swap label */
 	const char *uuid;		/* unique identifier */
@@ -110,9 +110,11 @@ struct swapon_ctl {
 	int columns[ARRAY_SIZE(infos) * 2];	/* --show columns */
 	int ncolumns;			/* number of columns */
 	int priority;			/* non-prioritized swap by default */
+	unsigned int pagesize;		/* swap page size */
 	unsigned int
 		all:1,			/* turn on all swap devices */
 		bytes:1,		/* display --show in bytes */
+		canonic:1,		/* is device path canonical */
 		fix_page_size:1,	/* reinitialize page size */
 		no_fail:1,		/* skip devices that do not exist */
 		no_heading:1,		/* toggle --show headers */
@@ -288,15 +290,14 @@ static int show_table(struct swapon_ctl *ctl)
 }
 
 /* calls mkswap */
-static int swap_reinitialize(const char *device,
-			     const char *label, const char *uuid)
+static int swap_reinitialize(struct swapon_ctl *ctl)
 {
 	pid_t pid;
 	int status, ret;
-	char *cmd[7];
+	char const *cmd[7];
 	int idx=0;
 
-	warnx(_("%s: reinitializing the swap."), device);
+	warnx(_("%s: reinitializing the swap."), ctl->device);
 
 	switch((pid=fork())) {
 	case -1: /* fork error */
@@ -305,17 +306,17 @@ static int swap_reinitialize(const char *device,
 
 	case 0:	/* child */
 		cmd[idx++] = "mkswap";
-		if (label && *label) {
+		if (ctl->label) {
 			cmd[idx++] = "-L";
-			cmd[idx++] = (char *) label;
+			cmd[idx++] = ctl->label;
 		}
-		if (uuid && *uuid) {
+		if (ctl->uuid) {
 			cmd[idx++] = "-U";
-			cmd[idx++] = (char *) uuid;
+			cmd[idx++] = ctl->uuid;
 		}
-		cmd[idx++] = (char *) device;
+		cmd[idx++] = ctl->device;
 		cmd[idx++] = NULL;
-		execvp(cmd[0], cmd);
+		execvp(cmd[0], (char * const *) cmd);
 		err(EXIT_FAILURE, _("failed to execute %s"), cmd[0]);
 
 	default: /* parent */
@@ -336,31 +337,31 @@ static int swap_reinitialize(const char *device,
 	return -1; /* error */
 }
 
-static int swap_rewrite_signature(const char *devname, unsigned int pagesize)
+static int swap_rewrite_signature(const struct swapon_ctl *ctl)
 {
 	int fd, rc = -1;
 
-	fd = open(devname, O_WRONLY);
+	fd = open(ctl->dev_canonical, O_WRONLY);
 	if (fd == -1) {
-		warn(_("cannot open %s"), devname);
+		warn(_("cannot open %s"), ctl->device);
 		return -1;
 	}
 
-	if (lseek(fd, pagesize - SWAP_SIGNATURE_SZ, SEEK_SET) < 0) {
-		warn(_("%s: lseek failed"), devname);
+	if (lseek(fd, ctl->pagesize - SWAP_SIGNATURE_SZ, SEEK_SET) < 0) {
+		warn(_("%s: lseek failed"), ctl->device);
 		goto err;
 	}
 
 	if (write(fd, (void *) SWAP_SIGNATURE,
 			SWAP_SIGNATURE_SZ) != SWAP_SIGNATURE_SZ) {
-		warn(_("%s: write signature failed"), devname);
+		warn(_("%s: write signature failed"), ctl->device);
 		goto err;
 	}
 
 	rc  = 0;
 err:
 	if (close_fd(fd) != 0) {
-		warn(_("write failed: %s"), devname);
+		warn(_("write failed: %s"), ctl->device);
 		rc = -1;
 	}
 	return rc;
@@ -383,13 +384,13 @@ static int swap_detect_signature(const char *buf, int *sig)
 	return 1;
 }
 
-static char *swap_get_header(int fd, int *sig, unsigned int *pagesize)
+static char *swap_get_header(struct swapon_ctl *ctl, int fd, int *sig)
 {
 	char *buf;
 	ssize_t datasz;
 	unsigned int page;
 
-	*pagesize = 0;
+	ctl->pagesize = 0;
 	*sig = 0;
 
 	buf = xmalloc(MAX_PAGESIZE);
@@ -408,12 +409,12 @@ static char *swap_get_header(int fd, int *sig, unsigned int *pagesize)
 		if (datasz < 0 || (size_t) datasz < (page - SWAP_SIGNATURE_SZ))
 			break;
 		if (swap_detect_signature(buf + page - SWAP_SIGNATURE_SZ, sig)) {
-			*pagesize = page;
+			ctl->pagesize = page;
 			break;
 		}
 	}
 
-	if (*pagesize)
+	if (ctl->pagesize)
 		return buf;
 err:
 	free(buf);
@@ -421,9 +422,7 @@ err:
 }
 
 /* returns real size of swap space */
-static unsigned long long swap_get_size(const struct swapon_ctl *ctl,
-					const char *hdr, const char *devname,
-					unsigned int pagesize)
+static unsigned long long swap_get_size(const struct swapon_ctl *ctl, const char *hdr)
 {
 	unsigned int last_page = 0;
 	const unsigned int swap_version = SWAP_VERSION;
@@ -440,12 +439,12 @@ static unsigned long long swap_get_size(const struct swapon_ctl *ctl,
 	if (ctl->verbose)
 		warnx(_("%s: found swap signature: version %ud, "
 			"page-size %d, %s byte order"),
-			devname,
+			ctl->device,
 			swap_version,
-			pagesize / 1024,
+			ctl->pagesize / 1024,
 			flip ? _("different") : _("same"));
 
-	return ((unsigned long long) last_page + 1) * pagesize;
+	return ((unsigned long long) last_page + 1) * ctl->pagesize;
 }
 
 static void swap_get_info(struct swapon_ctl *ctl, const char *hdr)
@@ -470,89 +469,85 @@ static void swap_get_info(struct swapon_ctl *ctl, const char *hdr)
 	}
 }
 
-static int swapon_checks(struct swapon_ctl *ctl, const char *special)
+static int swapon_checks(struct swapon_ctl *ctl)
 {
 	struct stat st;
 	int fd = -1, sig;
 	char *hdr = NULL;
-	unsigned int pagesize;
 	unsigned long long devsize = 0;
 	int permMask;
 
-	fd = open(special, O_RDONLY);
+	fd = open(ctl->dev_canonical, O_RDONLY);
 	if (fd == -1) {
-		warn(_("cannot open %s"), special);
+		warn(_("cannot open %s"), ctl->device);
 		goto err;
 	}
 
 	if (fstat(fd, &st) < 0) {
-		warn(_("stat of %s failed"), special);
+		warn(_("stat of %s failed"), ctl->device);
 		goto err;
 	}
 
 	permMask = S_ISBLK(st.st_mode) ? 07007 : 07077;
 	if ((st.st_mode & permMask) != 0)
 		warnx(_("%s: insecure permissions %04o, %04o suggested."),
-				special, st.st_mode & 07777,
+				ctl->device, st.st_mode & 07777,
 				~permMask & 0666);
 
 	if (S_ISREG(st.st_mode) && st.st_uid != 0)
 		warnx(_("%s: insecure file owner %d, 0 (root) suggested."),
-				special, st.st_uid);
+				ctl->device, st.st_uid);
 
 	/* test for holes by LBT */
 	if (S_ISREG(st.st_mode)) {
 		if (st.st_blocks * 512 < st.st_size) {
 			warnx(_("%s: skipping - it appears to have holes."),
-				special);
+				ctl->device);
 			goto err;
 		}
 		devsize = st.st_size;
 	}
 
 	if (S_ISBLK(st.st_mode) && blkdev_get_size(fd, &devsize)) {
-		warnx(_("%s: get size failed"), special);
+		warnx(_("%s: get size failed"), ctl->device);
 		goto err;
 	}
 
-	hdr = swap_get_header(fd, &sig, &pagesize);
+	hdr = swap_get_header(ctl, fd, &sig);
 	if (!hdr) {
-		warnx(_("%s: read swap header failed"), special);
+		warnx(_("%s: read swap header failed"), ctl->device);
 		goto err;
 	}
 
-	if (sig == SIG_SWAPSPACE && pagesize) {
+	if (sig == SIG_SWAPSPACE && ctl->pagesize) {
 		unsigned long long swapsize =
-				swap_get_size(ctl, hdr, special, pagesize);
+				swap_get_size(ctl, hdr);
 		int syspg = getpagesize();
 
 		if (ctl->verbose)
 			warnx(_("%s: pagesize=%d, swapsize=%llu, devsize=%llu"),
-				special, pagesize, swapsize, devsize);
+				ctl->device, ctl->pagesize, swapsize, devsize);
 
 		if (swapsize > devsize) {
 			if (ctl->verbose)
 				warnx(_("%s: last_page 0x%08llx is larger"
 					" than actual size of swapspace"),
-					special, swapsize);
-		} else if (syspg < 0 || (unsigned) syspg != pagesize) {
+					ctl->device, swapsize);
+		} else if (syspg < 0 || (unsigned int) syspg != ctl->pagesize) {
 			if (ctl->fix_page_size) {
-				char *label = NULL, *uuid = NULL;
 				int rc;
 
 				swap_get_info(ctl, hdr);
 
 				warnx(_("%s: swap format pagesize does not match."),
-					special);
-				rc = swap_reinitialize(special, label, uuid);
-				free(label);
-				free(uuid);
+					ctl->device);
+				rc = swap_reinitialize(ctl);
 				if (rc < 0)
 					goto err;
 			} else
 				warnx(_("%s: swap format pagesize does not match. "
 					"(Use --fixpgsz to reinitialize it.)"),
-					special);
+					ctl->device);
 		}
 	} else if (sig == SIG_SWSUSPEND) {
 		/* We have to reinitialize swap with old (=useless) software suspend
@@ -561,8 +556,8 @@ static int swapon_checks(struct swapon_ctl *ctl, const char *special)
 		 */
 		warnx(_("%s: software suspend data detected. "
 				"Rewriting the swap signature."),
-			special);
-		if (swap_rewrite_signature(special, pagesize) < 0)
+			ctl->device);
+		if (swap_rewrite_signature(ctl) < 0)
 			goto err;
 	}
 
@@ -576,23 +571,22 @@ err:
 	return -1;
 }
 
-static int do_swapon(struct swapon_ctl *ctl, const char *orig_special,
-		     int canonic)
+static int do_swapon(struct swapon_ctl *ctl)
 {
 	int status;
-	const char *special = orig_special;
 	int flags = 0;
 
 	if (ctl->verbose)
-		printf(_("swapon %s\n"), orig_special);
+		printf(_("swapon %s\n"), ctl->device);
 
-	if (!canonic) {
-		special = mnt_resolve_spec(orig_special, mntcache);
-		if (!special)
-			return cannot_find(orig_special);
-	}
+	if (!ctl->canonic) {
+		ctl->dev_canonical = mnt_resolve_spec(ctl->device, mntcache);
+		if (!ctl->dev_canonical)
+			return cannot_find(ctl->device);
+	} else
+		ctl->dev_canonical = ctl->device;
 
-	if (swapon_checks(ctl, special))
+	if (swapon_checks(ctl))
 		return -1;
 
 #ifdef SWAP_FLAG_PREFER
@@ -621,25 +615,23 @@ static int do_swapon(struct swapon_ctl *ctl, const char *orig_special,
 			flags |= ctl->discard;
 	}
 
-	status = swapon(special, flags);
+	status = swapon(ctl->dev_canonical, flags);
 	if (status < 0)
-		warn(_("%s: swapon failed"), orig_special);
+		warn(_("%s: swapon failed"), ctl->device);
 
 	return status;
 }
 
 static int swapon_by_label(struct swapon_ctl *ctl)
 {
-	const char *special = mnt_resolve_tag("LABEL", ctl->label, mntcache);
-	return special ? do_swapon(ctl, special, CANONIC) :
-			 cannot_find(ctl->label);
+	ctl->device = mnt_resolve_tag("LABEL", ctl->label, mntcache);
+	return ctl->device ? do_swapon(ctl) :  cannot_find(ctl->label);
 }
 
 static int swapon_by_uuid(struct swapon_ctl *ctl)
 {
-	const char *special = mnt_resolve_tag("UUID", ctl->uuid, mntcache);
-	return special ? do_swapon(ctl, special, CANONIC) :
-			 cannot_find(ctl->uuid);
+	ctl->device = mnt_resolve_tag("UUID", ctl->uuid, mntcache);
+	return ctl->device ? do_swapon(ctl) : cannot_find(ctl->uuid);
 }
 
 /* -o <options> or fstab */
@@ -693,7 +685,7 @@ static int swapon_all(struct swapon_ctl *ctl)
 
 	while (mnt_table_find_next_fs(tb, itr, match_swap, NULL, &fs) == 0) {
 		/* defaults */
-		const char *opts, *src;
+		const char *opts;
 
 		if (mnt_fs_get_option(fs, "noauto", NULL, NULL) == 0)
 			continue;
@@ -702,16 +694,16 @@ static int swapon_all(struct swapon_ctl *ctl)
 		if (opts)
 			parse_options(ctl);
 
-		src = mnt_resolve_spec(mnt_fs_get_source(fs), mntcache);
-		if (!src) {
+		ctl->device = mnt_resolve_spec(mnt_fs_get_source(fs), mntcache);
+		if (!ctl->device) {
 			if (!ctl->no_fail)
 				status |= cannot_find(mnt_fs_get_source(fs));
 			continue;
 		}
 
-		if (!is_active_swap(src) &&
-		    (!ctl->no_fail || !access(src, R_OK)))
-			status |= do_swapon(ctl, src, CANONIC);
+		if (!is_active_swap(ctl->device) &&
+		    (!ctl->no_fail || !access(ctl->device, R_OK)))
+			status |= do_swapon(ctl);
 	}
 
 	mnt_free_iter(itr);
@@ -808,7 +800,7 @@ int main(int argc, char *argv[])
 	};
 	int excl_st[ARRAY_SIZE(excl)] = UL_EXCL_STATUS_INIT;
 
-	struct swapon_ctl ctl = { .priority = -1, 0 };
+	struct swapon_ctl ctl = { .priority = -1, .canonic = 1, 0 };
 
 	setlocale(LC_ALL, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
@@ -932,9 +924,13 @@ int main(int argc, char *argv[])
 		ctl.uuid = get_uuid(i);
 		status |= swapon_by_uuid(&ctl);
 	}
-
-	while (*argv != NULL)
-		status |= do_swapon(&ctl, *argv++, !CANONIC);
+	if (*argv != NULL) {
+		ctl.canonic = 0;
+		while (*argv != NULL) {
+			ctl.device = *argv++;
+			status |= do_swapon(&ctl);
+		}
+	}
 
 	free_tables();
 	mnt_unref_cache(mntcache);
-- 
2.7.0

[PATCH 3/3] swapon: do not run execvp() calls when swapon is setuid binary

[Sami Kerola]

swapon(8) is not expected to be setuid binary, but if it is try to avoid
obvious security vulnerability of executing user preferred mkswap file as
someone else, such as root.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/swapon.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sys-utils/swapon.c b/sys-utils/swapon.c
index 5ad5cd1..be8f771 100644
--- a/sys-utils/swapon.c
+++ b/sys-utils/swapon.c
@@ -297,6 +297,10 @@ static int swap_reinitialize(struct swapon_ctl *ctl)
 	char const *cmd[7];
 	int idx=0;
 
+	if (geteuid() != getuid()) {
+		warnx(_("will not execute mkswap when swapon is setuid binary"));
+		return -1;
+	}
 	warnx(_("%s: reinitializing the swap."), ctl->device);
 
 	switch((pid=fork())) {
-- 
2.7.0

Re: [PATCH 1/3] swapon: add control struct

[Karel Zak]

On Tue, Feb 02, 2016 at 01:40:08PM +0000, Sami Kerola wrote:
> -static void swap_get_info(const char *hdr, char **label, char **uuid)
> +static void swap_get_info(struct swapon_ctl *ctl, const char *hdr)
>  {
>  	struct swap_header_v1_2 *s = (struct swap_header_v1_2 *) hdr;
>  
> -	if (s && *s->volume_name && label)
> -		*label = xstrdup(s->volume_name);
> +	if (s && *s->volume_name && ctl->label)
> +		ctl->label = xstrdup(s->volume_name);

Your semantic for "label" is different that in the original version
where label was pointer to pointer, so "&& label" had sense. In your
code "&& ctl->label" is always true.

I'll fix it.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/3] swapon: move function arguments to control structure

[Karel Zak]

On Tue, Feb 02, 2016 at 01:40:09PM +0000, Sami Kerola wrote:
> This makes reading what the code does easier. 

I'm not sure in all cases, for example ->dev_canonical and ->canonic make
things more messy. I'll revert part of the patch.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 3/3] swapon: do not run execvp() calls when swapon is setuid binary

[Karel Zak]

On Tue, Feb 02, 2016 at 01:40:10PM +0000, Sami Kerola wrote:
> swapon(8) is not expected to be setuid binary, but if it is try to avoid
> obvious security vulnerability of executing user preferred mkswap file as
> someone else, such as root.

I have replaced this with code to drop permissions, so it executes
mkswap, but as non-root and all depends on mkswap and file/device
perms.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/3] pull: add swapon control structure

[Karel Zak]

On Tue, Feb 02, 2016 at 01:40:07PM +0000, Sami Kerola wrote:
> Sami Kerola (3):
>   swapon: add control struct
>   swapon: move function arguments to control structure
>   swapon: do not run execvp() calls when swapon is setuid binary

Merged with changes.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com