Received-SPF: pass (google.com: domain of linux-kernel+bounces-94752-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 6 Mar 2024 15:52:13 -0800
From: Kees Cook <keescook@chromium.org>
To: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Cc: "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
	Jesse Brandeburg <jesse.brandeburg@intel.com>,
	Tony Nguyen <anthony.l.nguyen@intel.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
	linux-hardening@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] overflow: Change DEFINE_FLEX to take __counted_by member
Message-ID: <202403061551.00DAFE8B39@keescook>
References: <20240306010746.work.678-kees@kernel.org>
 <9c2990f0-7407-49c6-9e3a-b92de82ea437@embeddedor.com>
 <bd21f7dc-9f89-40ee-895e-601c80165225@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <bd21f7dc-9f89-40ee-895e-601c80165225@intel.com>

On Wed, Mar 06, 2024 at 08:06:29AM +0100, Przemek Kitszel wrote:
> On 3/6/24 04:25, Gustavo A. R. Silva wrote:
> > 
> > 
> > On 05/03/24 19:07, Kees Cook wrote:
> > > The norm should be flexible array structures with __counted_by
> > > annotations, so DEFINE_FLEX() is updated to expect that. Rename
> > > the non-annotated version to DEFINE_RAW_FLEX(), and update the few
> > > existing users. Additionally add self-tests to validate syntax and
> > > size calculations.
> > > 
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > 
> > [..]
> 
> Just a note that ice changes are purely mechanical, so this seems ok
> to go via linux-hardening tree. And changes per-se are fine too :)

Thanks!

> 
> > 
> > > +/**
> > > + * DEFINE_FLEX() - Define an on-stack instance of structure with a
> > > trailing
> > > + * flexible array member.
> > > + *
> > > + * @TYPE: structure type name, including "struct" keyword.
> > > + * @NAME: Name for a variable to define.
> > > + * @COUNTER: Name of the __counted_by member.
> > > + * @MEMBER: Name of the array member.
> > > + * @COUNT: Number of elements in the array; must be compile-time const.
> > > + *
> > > + * Define a zeroed, on-stack, instance of @TYPE structure with a
> > > trailing
> > > + * flexible array member.
> > > + * Use __struct_size(@NAME) to get compile-time size of it afterwards.
> > > + */
> > > +#define DEFINE_FLEX(TYPE, NAME, COUNTER, MEMBER, COUNT)??? \
> > 
> > Probably, swapping COUNTER and MEMBER is better?
> 
> right now we have usage scenario (from Kunits):
> 	DEFINE_FLEX(struct foo, eight, counter, array, 8);
> 
> > 
> >  ????DEFINE_FLEX(TYPE, NAME, MEMBER, COUNTER, COUNT)
> 
> usage would become:
> 	DEFINE_FLEX(struct foo, eight, array, counter, 8);
> 
> which reads a bit better indeed, with the added benefit that we
> go from broader to more specific:
> whole struct -> array -> array size variable -> given array size
> 
> so +1 from me for the params swap

Sounds good. You and Gustavo have convinced me. :) I've sent a v2 now.

-- 
Kees Cook