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

From: Len Baker <len.baker@gmx.com>
To: Matthew Wilcox <willy@infradead.org>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Kees Cook <keescook@chromium.org>
Cc: Len Baker <len.baker@gmx.com>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Iurii Zaikin <yzaikin@google.com>,
	linux-hardening@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2][next] sysctl: Avoid open coded arithmetic in memory allocator functions
Date: Sun, 24 Oct 2021 11:13:28 +0200	[thread overview]
Message-ID: <20211024091328.GA2912@titan> (raw)
In-Reply-To: <YXQbxSSw9qan87cm@casper.infradead.org>

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