Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 24 Oct 2021 18:58:38 +0200
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
Message-ID: <20211024165838.GA2347@titan>
References: <20211023105414.7316-1-len.baker@gmx.com>
 <YXQbxSSw9qan87cm@casper.infradead.org>
 <20211024091328.GA2912@titan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211024091328.GA2912@titan>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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