netdev - Re: [PATCH net-next v3 3/9] lib: packing: add pack_fields() and unpack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241108183104.wfzvav42zgslqofy@skbuf>
Date: Fri, 8 Nov 2024 20:31:04 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Masahiro Yamada <masahiroy@...nel.org>,
	netdev <netdev@...r.kernel.org>, linux-kbuild@...r.kernel.org,
	Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next v3 3/9] lib: packing: add pack_fields() and
 unpack_fields()

On Fri, Nov 08, 2024 at 01:24:07PM +0200, Vladimir Oltean wrote:
> On Thu, Nov 07, 2024 at 11:50:34AM -0800, Jacob Keller wrote:
> > +#define DECLARE_PACKED_FIELDS_S(name, buffer_sz) \
> > +	const size_t __ ## name ## _buffer_sz __pf_section_s = buffer_sz; \
> > +	const struct packed_field_s name[] __pf_section_s
> 
> This will need sorting out - how to make this declaration macro
> compatible with the "static" keyword. The obvious way (to group the
> field array and the buffer size into a structure) doesn't work. It loses
> the ARRAY_SIZE() of the fields, which is important to the pack_fields()
> and unpack_fields() wrappers.
> 
> Maybe a different tool is needed for checking that no packed fields
> exceed the buffer size? Forcing the buffer size be a symbol just because
> that's what modpost works with seems unnatural.
> 
> If we knew the position of the largest field array element in C, and if
> we enforced that all pack_fields() use a strong type (size included) for
> the packed buffer, we'd have all information inside the pack_fields()
> macro, because we only need to compare the largest field against the
> buffer size. We could just have that part as a BUILD_BUG_ON() wrapped
> inside the pack_fields() macro itself. And have the compile-time checks
> spill over between C and modpost...
> 
> Not to mention, pack_fields() would have one argument less: pbuflen.

I was thinking something like this (attached). I still don't like
modpost more than the assertions in C code, because it imposes more
constraints upon the library user which didn't exist before. Though
without the extra restrictions added in this patch (just ascending order
for packed fields + strong type for packed buffer), I don't think the
modpost variant is going to work, or is going to become extremely complex.

View attachment "0001-pack_fields-changes.patch" of type "text/x-diff" (13963 bytes)