diff mbox series

[v2,next] sysctl: Avoid open coded arithmetic in memory allocator functions

Message ID 20211023105414.7316-1-len.baker@gmx.com (mailing list archive)
State Rejected
Delegated to: Gustavo A. R. Silva
Headers show
Series [v2,next] sysctl: Avoid open coded arithmetic in memory allocator functions | expand

Commit Message

Len Baker Oct. 23, 2021, 10:54 a.m. UTC 
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>
---
Changelog v1 -> v2
- Remove the new_dir_size function and its use (Matthew Wilcox).

The previous version can be found here [1]

[1] https://lore.kernel.org/linux-hardening/20211016152829.9836-1-len.baker@gmx.com/

 fs/proc/proc_sysctl.c | 84 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 73 insertions(+), 11 deletions(-)

--
2.25.1

Comments

Matthew Wilcox Oct. 23, 2021, 2:27 p.m. UTC | #1 
On Sat, Oct 23, 2021 at 12:54:14PM +0200, Len Baker wrote:
> Changelog v1 -> v2
> - Remove the new_dir_size function and its use (Matthew Wilcox).

Why do you think the other functions are any different?  Please
provide reasoning.
Len Baker Oct. 24, 2021, 9:13 a.m. UTC | #2 
Hi Matthew,

thanks for looking at this. More below.

On Sat, Oct 23, 2021 at 03:27:17PM +0100, Matthew Wilcox wrote:
> On Sat, Oct 23, 2021 at 12:54:14PM +0200, Len Baker wrote:
> > Changelog v1 -> v2
> > - Remove the new_dir_size function and its use (Matthew Wilcox).
>
> Why do you think the other functions are any different?  Please
> provide reasoning.

I think it is better to be defensive. IMHO I believe that if the
struct_size() helper could be used in this patch, it would be more
easy to ACK. But it is not possible due to the complex memory
layouts. However, there are a lot of code in the kernel that uses the
struct_size() helper for memory allocator arguments where we know
that it don't overflow. For example:

1.- Function imx8mm_tmu_probe()
    Uses: struct_size(tmu, sensors, data->num_sensors)
    Where: tmu has a sizeof(struct imx8mm_tmu) -> Not very big
           data->num_sensors -> A little number

    So, almost certainly it doesn't overflow.

2.- Function igb_alloc_q_vector()
    Uses: struct_size(q_vector, ring, ring_count)
    Where: q_vector has a sizeof(struct igb_q_vector) -> Not very big
           ring_count -> At most two.

    So, almost certainly it doesn't overflow.

3.- And so on...

So, I think that these new functions for the size calculation are
helpers like struct_size (but specific due to the memory layouts).
I don't see any difference here. Also, I think that to be defensive
in memory allocation arguments it is better than a possible heap
overflow ;)

Also, under the KSPP [1][2][3] there is an effort to keep out of
code all the open-coded arithmetic (To avoid unwanted overflows).

[1] https://github.com/KSPP/linux/issues/83
[2] https://github.com/KSPP/linux/issues/92
[3] https://github.com/KSPP/linux/issues/160

Moreover, after writing these reasons and thinking for a while, I
think that the v1 it is correct patch to apply. This is my opinion
but I'm open minded. Any other solution that makes the code more
secure is welcome.

As a last point I would like to know the opinion of Kees and
Gustavo since they are also working on this task.

Kees and Gustavo, what do you think?

Regards,
Len
Len Baker Oct. 24, 2021, 4:58 p.m. UTC | #3 
Hi all, some additions below :)

On Sun, Oct 24, 2021 at 11:13:28AM +0200, Len Baker wrote:
> Hi Matthew,
>
> thanks for looking at this. More below.
>
> On Sat, Oct 23, 2021 at 03:27:17PM +0100, Matthew Wilcox wrote:
> > On Sat, Oct 23, 2021 at 12:54:14PM +0200, Len Baker wrote:
> > > Changelog v1 -> v2
> > > - Remove the new_dir_size function and its use (Matthew Wilcox).
> >
> > Why do you think the other functions are any different?  Please
> > provide reasoning.
>
> I think it is better to be defensive. IMHO I believe that if the
> struct_size() helper could be used in this patch, it would be more
> easy to ACK. But it is not possible due to the complex memory
> layouts. However, there are a lot of code in the kernel that uses the
> struct_size() helper for memory allocator arguments where we know
> that it don't overflow. For example:
>
> 1.- Function imx8mm_tmu_probe()
>     Uses: struct_size(tmu, sensors, data->num_sensors)
>     Where: tmu has a sizeof(struct imx8mm_tmu) -> Not very big
             sensors is an array of struct tmu_sensor and the
	     sizeof(struct tmu_sensor) is small enough
>            data->num_sensors -> A little number
>
>     So, almost certainly it doesn't overflow.
>
> 2.- Function igb_alloc_q_vector()
>     Uses: struct_size(q_vector, ring, ring_count)
>     Where: q_vector has a sizeof(struct igb_q_vector) -> Not very big
             ring is an array of struct igb_ring and the
	     sizeof(struct igb_ring) is not small but also no very big.
>            ring_count -> At most two.
>
>     So, almost certainly it doesn't overflow.
>
> 3.- And so on...
>
> So, I think that these new functions for the size calculation are
> helpers like struct_size (but specific due to the memory layouts).
> I don't see any difference here. Also, I think that to be defensive
> in memory allocation arguments it is better than a possible heap
> overflow ;)
>
> Also, under the KSPP [1][2][3] there is an effort to keep out of
> code all the open-coded arithmetic (To avoid unwanted overflows).
>
> [1] https://github.com/KSPP/linux/issues/83
> [2] https://github.com/KSPP/linux/issues/92
> [3] https://github.com/KSPP/linux/issues/160
>
> Moreover, after writing these reasons and thinking for a while, I
> think that the v1 it is correct patch to apply. This is my opinion
> but I'm open minded. Any other solution that makes the code more
> secure is welcome.
>
> As a last point I would like to know the opinion of Kees and
> Gustavo since they are also working on this task.
>
> Kees and Gustavo, what do you think?
>
> Regards,
> Len
Matthew Wilcox Oct. 24, 2021, 5:55 p.m. UTC | #4 
On Sun, Oct 24, 2021 at 11:13:28AM +0200, Len Baker wrote:
> I think it is better to be defensive. IMHO I believe that if the
> struct_size() helper could be used in this patch, it would be more
> easy to ACK. But it is not possible due to the complex memory
> layouts.

I think it's better for code to be understandable.  Your patch makes
the code less readable in the name of "security", which is a poor
justification.

> However, there are a lot of code in the kernel that uses the
> struct_size() helper for memory allocator arguments where we know
> that it don't overflow. For example:

Well, yes.  That's because struct_size() actually makes code more
readable as well as more secure.

> As a last point I would like to know the opinion of Kees and
> Gustavo since they are also working on this task.
> 
> Kees and Gustavo, what do you think?

You might want to check who was co-author on 610b15c50e86 before
discarding my opinion.
Gustavo A. R. Silva Oct. 24, 2021, 6:54 p.m. UTC | #5 
On Sun, Oct 24, 2021 at 06:55:13PM +0100, Matthew Wilcox wrote:
> On Sun, Oct 24, 2021 at 11:13:28AM +0200, Len Baker wrote:
> 
> I think it's better for code to be understandable.  Your patch makes
> the code less readable in the name of "security", which is a poor
> justification.

I agree with Matthew. Those functions seem to be a bit too much, for
now.

Let's keep it simple and start by replacing the open-coded instances
when possible, first. Then we can dig much deeper depending on each
particular case, taking into consideration readability, which is
certainly important.

Thanks
--
Gustavo
Len Baker Oct. 29, 2021, 4:57 p.m. UTC | #6 
Hi Matthew,

On Sun, Oct 24, 2021 at 06:55:13PM +0100, Matthew Wilcox wrote:
> On Sun, Oct 24, 2021 at 11:13:28AM +0200, Len Baker wrote:
>
> > As a last point I would like to know the opinion of Kees and
> > Gustavo since they are also working on this task.
> >
> > Kees and Gustavo, what do you think?
>
> You might want to check who was co-author on 610b15c50e86 before
> discarding my opinion.

That was not my intention. I apologize if you have been offended. My
intention was to involve Kees and Gustavo (who are working actively
on this task) to add new points of view (if possible). I'm so sorry.

Again apologies,
Len
diff mbox series

Patch

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 5d66faecd4ef..0b3b3f11ca11 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1146,6 +1146,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 +1182,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 +1282,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 +1351,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 +1478,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 +1534,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 +1589,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;