Received-SPF: pass (google.com: domain of linux-kernel+bounces-62181-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 12 Feb 2024 10:30:17 -0800
From: Kees Cook <keescook@chromium.org>
To: Erick Archer <erick.archer@gmx.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>, Leon Romanovsky <leon@kernel.org>,
	Edward Srouji <edwards@nvidia.com>,
	Patrisious Haddad <phaddad@nvidia.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH] RDMA/uverbs: Remove flexible arrays from struct *_filter
Message-ID: <202402121026.0AF90DBA@keescook>
References: <20240211115856.9788-1-erick.archer@gmx.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240211115856.9788-1-erick.archer@gmx.com>

On Sun, Feb 11, 2024 at 12:58:56PM +0100, Erick Archer wrote:
> When a struct containing a flexible array is included in another struct,
> and there is a member after the struct-with-flex-array, there is a
> possibility of memory overlap. These cases must be audited [1]. See:
> 
> struct inner {
> 	...
> 	int flex[];
> };
> 
> struct outer {
> 	...
> 	struct inner header;
> 	int overlap;
> 	...
> };
> 
> This is the scenario for all the "struct *_filter" structures that are
> included in the following "struct ib_flow_spec_*" structures:
> 
> struct ib_flow_spec_eth
> struct ib_flow_spec_ib
> struct ib_flow_spec_ipv4
> struct ib_flow_spec_ipv6
> struct ib_flow_spec_tcp_udp
> struct ib_flow_spec_tunnel
> struct ib_flow_spec_esp
> struct ib_flow_spec_gre
> struct ib_flow_spec_mpls
> 
> The pattern is like the one shown below:
> 
> struct *_filter {
> 	...
> 	u8 real_sz[];
> };
> 
> struct ib_flow_spec_mpls {
> 	...
> 	struct *_filter val;
> 	struct *_filter mask;
> };
> 
> In this case, the trailing flexible array "real_sz" is never allocated
> and is only used to calculate the size of the structures. Here the use
> of the "offsetof" helper can be changed by the "sizeof" operator because
> the goal is to get the size of these structures. Therefore, the trailing
> flexible arrays can also be removed.
> 
> Link: https://github.com/KSPP/linux/issues/202 [1]
> Signed-off-by: Erick Archer <erick.archer@gmx.com>
> ---
> Hi everyone,
> 
> This patch has not been tested. This has only been built-tested.

I might suggest doing a binary difference comparison[1], as it's possible
that "real_sz" is being used to try to avoid trailing padding on
structs. I wasn't able to trivially construct an example, so maybe I'm
not understanding its purpose correctly.

If, however, there are cases where offsetof(..., real_sz) !=
sizeof(...), then I would check two alternatives:

	struct { } real_sz;

but that may induce padding still, or:

	u8 real_sz[0];

which would be a literally zero-sized array, used only for addressing.

Or, these can be left as-is, and the "flex array not at end of struct"
warnings can be disabled for these targets.

-Kees

-- 
Kees Cook