[PATCH] sysctl: Avoid open coded arithmetic in memory allocator functions

[Len Baker <len.baker@gmx.com>]

As noted in the "Deprecated Interfaces, Language Features, Attributes,
and Conventions" documentation [1], size calculations (especially
multiplication) should not be performed in memory allocator (or similar)
function arguments due to the risk of them overflowing. This could lead
to values wrapping around and a smaller allocation being made than the
caller was expecting. Using those allocations could lead to linear
overflows of heap memory and other misbehaviors.

So, add some functions to calculate the size used in memory allocator
function arguments, saturating to SIZE_MAX on overflow. Here it is not
possible to use the struct_size() helper since the memory layouts used
when the memory is allocated are not simple ones.

However, for the kcalloc() case, don't define a new function and check
for overflow before its call.

This code was detected with the help of Coccinelle and audited and fixed
manually.

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments

Signed-off-by: Len Baker <len.baker@gmx.com>
---
Hi,

this patch is built against the linux-next tree (tag next-20211015).

Regards,
Len

 fs/proc/proc_sysctl.c | 114 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 100 insertions(+), 14 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 5d66faecd4ef..35734bc5e67e 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -942,6 +942,24 @@ static struct ctl_dir *find_subdir(struct ctl_dir *dir,
 	return container_of(head, struct ctl_dir, header);
 }

+static size_t new_dir_size(size_t namelen)
+{
+	size_t bytes;
+
+	if (check_add_overflow(sizeof(struct ctl_dir), sizeof(struct ctl_node),
+			       &bytes))
+		return SIZE_MAX;
+	if (check_add_overflow(bytes, array_size(sizeof(struct ctl_table), 2),
+			       &bytes))
+		return SIZE_MAX;
+	if (check_add_overflow(bytes, namelen, &bytes))
+		return SIZE_MAX;
+	if (check_add_overflow(bytes, (size_t)1, &bytes))
+		return SIZE_MAX;
+
+	return bytes;
+}
+
 static struct ctl_dir *new_dir(struct ctl_table_set *set,
 			       const char *name, int namelen)
 {
@@ -950,9 +968,15 @@ static struct ctl_dir *new_dir(struct ctl_table_set *set,
 	struct ctl_node *node;
 	char *new_name;

-	new = kzalloc(sizeof(*new) + sizeof(struct ctl_node) +
-		      sizeof(struct ctl_table)*2 +  namelen + 1,
-		      GFP_KERNEL);
+	/*
+	 * Allocation layout in bytes:
+	 *
+	 * sizeof(struct ctl_dir) +
+	 * sizeof(struct ctl_node) +
+	 * sizeof(struct ctl_table) * 2 +
+	 * namelen + 1
+	 */
+	new = kzalloc(new_dir_size(namelen), GFP_KERNEL);
 	if (!new)
 		return NULL;

@@ -1146,6 +1170,26 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
 	return err;
 }

+static size_t new_links_size(size_t nr_entries, size_t name_bytes)
+{
+	size_t bytes;
+
+	if (check_add_overflow(nr_entries, (size_t)1, &bytes))
+		return SIZE_MAX;
+	if (check_add_overflow(sizeof(struct ctl_table_header),
+			       array_size(sizeof(struct ctl_node), nr_entries),
+			       &bytes))
+		return SIZE_MAX;
+	if (check_add_overflow(bytes, array_size(sizeof(struct ctl_table),
+						 nr_entries + 1),
+			       &bytes))
+		return SIZE_MAX;
+	if (check_add_overflow(bytes, name_bytes, &bytes))
+		return SIZE_MAX;
+
+	return bytes;
+}
+
 static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table *table,
 	struct ctl_table_root *link_root)
 {
@@ -1162,11 +1206,15 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table
 		name_bytes += strlen(entry->procname) + 1;
 	}

-	links = kzalloc(sizeof(struct ctl_table_header) +
-			sizeof(struct ctl_node)*nr_entries +
-			sizeof(struct ctl_table)*(nr_entries + 1) +
-			name_bytes,
-			GFP_KERNEL);
+	/*
+	 * Allocation layout in bytes:
+	 *
+	 * sizeof(struct ctl_table_header) +
+	 * sizeof(struct ctl_node) * nr_entries +
+	 * sizeof(struct ctl_table) * (nr_entries + 1) +
+	 * name_bytes
+	 */
+	links = kzalloc(new_links_size(nr_entries, name_bytes), GFP_KERNEL);

 	if (!links)
 		return NULL;
@@ -1258,6 +1306,18 @@ static int insert_links(struct ctl_table_header *head)
 	return err;
 }

+static inline size_t sysctl_table_size(int nr_entries)
+{
+	size_t bytes;
+
+	if (check_add_overflow(sizeof(struct ctl_table_header),
+			       array_size(sizeof(struct ctl_node), nr_entries),
+			       &bytes))
+		return SIZE_MAX;
+
+	return bytes;
+}
+
 /**
  * __register_sysctl_table - register a leaf sysctl table
  * @set: Sysctl tree to register on
@@ -1315,8 +1375,13 @@ struct ctl_table_header *__register_sysctl_table(
 	for (entry = table; entry->procname; entry++)
 		nr_entries++;

-	header = kzalloc(sizeof(struct ctl_table_header) +
-			 sizeof(struct ctl_node)*nr_entries, GFP_KERNEL);
+	/*
+	 * Allocation layout in bytes:
+	 *
+	 * sizeof(struct ctl_table_header) +
+	 * sizeof(struct ctl_node) * nr_entries
+	 */
+	header = kzalloc(sysctl_table_size(nr_entries), GFP_KERNEL);
 	if (!header)
 		return NULL;

@@ -1437,8 +1502,11 @@ static int register_leaf_sysctl_tables(const char *path, char *pos,
 	/* If there are mixed files and directories we need a new table */
 	if (nr_dirs && nr_files) {
 		struct ctl_table *new;
-		files = kcalloc(nr_files + 1, sizeof(struct ctl_table),
-				GFP_KERNEL);
+		int n;
+
+		if (unlikely(check_add_overflow(nr_files, 1, &n)))
+			goto out;
+		files = kcalloc(n, sizeof(struct ctl_table), GFP_KERNEL);
 		if (!files)
 			goto out;

@@ -1490,6 +1558,19 @@ static int register_leaf_sysctl_tables(const char *path, char *pos,
 	return err;
 }

+static inline size_t sysctl_paths_size(int nr_subheaders)
+{
+	size_t bytes;
+
+	if (check_add_overflow(sizeof(struct ctl_table_header),
+			       array_size(sizeof(struct ctl_table_header *),
+					  nr_subheaders),
+			       &bytes))
+		return SIZE_MAX;
+
+	return bytes;
+}
+
 /**
  * __register_sysctl_paths - register a sysctl table hierarchy
  * @set: Sysctl tree to register on
@@ -1532,8 +1613,13 @@ struct ctl_table_header *__register_sysctl_paths(
 		if (header)
 			header->ctl_table_arg = ctl_table_arg;
 	} else {
-		header = kzalloc(sizeof(*header) +
-				 sizeof(*subheaders)*nr_subheaders, GFP_KERNEL);
+		/*
+		 * Allocation layout in bytes:
+		 *
+		 * sizeof(struct ctl_table_header) +
+		 * sizeof(struct ctl_table_header *) * nr_subheaders
+		 */
+		header = kzalloc(sysctl_paths_size(nr_subheaders), GFP_KERNEL);
 		if (!header)
 			goto out;

--
2.25.1