| 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
| ||
|
Message-ID: <20241204171215.hb5v74kebekwhca4@skbuf>
Date: Wed, 4 Dec 2024 19:12:15 +0200
From: Vladimir Oltean <vladimir.oltean@....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>
Subject: Re: [PATCH net-next v8 03/10] lib: packing: add pack_fields() and
unpack_fields()
On Tue, Dec 03, 2024 at 03:53:49PM -0800, Jacob Keller wrote:
> +#define CHECK_PACKED_FIELD(field) ({ \
> + typeof(field) __f = (field); \
> + BUILD_BUG_ON(__f.startbit < __f.endbit); \
> + BUILD_BUG_ON(__f.startbit - __f.endbit >= BITS_PER_BYTE * __f.size); \
> + BUILD_BUG_ON(__f.size != 1 && __f.size != 2 && \
> + __f.size != 4 && __f.size != 8); \
> +})
> +
> +
> +#define CHECK_PACKED_FIELD_OVERLAP(ascending, field1, field2) ({ \
> + typeof(field1) _f1 = (field1); typeof(field2) _f2 = (field2); \
> + const bool _a = (ascending); \
> + BUILD_BUG_ON(_a && _f1.startbit >= _f2.startbit); \
> + BUILD_BUG_ON(!_a && _f1.startbit <= _f2.startbit); \
> + BUILD_BUG_ON(max(_f1.endbit, _f2.endbit) <= \
> + min(_f1.startbit, _f2.startbit)); \
> +})
> +
> +#define CHECK_PACKED_FIELDS_SIZE(fields, pbuflen) ({ \
> + typeof(&(fields)[0]) _f = (fields); \
> + typeof(pbuflen) _len = (pbuflen); \
> + const size_t num_fields = ARRAY_SIZE(fields); \
> + BUILD_BUG_ON(!__builtin_constant_p(_len)); \
> + BUILD_BUG_ON(_f[0].startbit >= BITS_PER_BYTE * _len); \
Please add a comment here stating that we check both the first and last
element to cover the ascending as well as descending ordering scenarios.
It took me a while to realize this, I thought the _f[0] check was unnecessary.
> + BUILD_BUG_ON(_f[num_fields - 1].startbit >= BITS_PER_BYTE * _len); \
> +})
> +
> #define QUIRK_MSB_ON_THE_RIGHT BIT(0)
> #define QUIRK_LITTLE_ENDIAN BIT(1)
> #define QUIRK_LSW32_IS_FIRST BIT(2)
I spent some time today to play around with this version, and it seems
to work, but I took some liberty and made the following changes:
- Tail-call CHECK_PACKED_FIELD_OVERLAP() from CHECK_PACKED_FIELD(). This
reduces the size of the generated code from 2753 lines to 1478 lines,
which already brings it a little bit more into the realm of "tolerable" IMO.
- Remove the BUILD_BUG_ON(ARRAY_SIZE(fields) == N), since I think
that's just wasteful (in terms of space and compiler CPU cycles) and
ultra-defensive, when the auto-generated __builtin_choose_expr() is
the only caller. It was justified when the consumer had to explicitly
select the right checking macro.
- Add some prettier error messages. Compare (for an error injected by me):
../drivers/net/ethernet/intel/ice/ice_common.c:1419:2: error: call to '__compiletime_assert_3302' declared with 'error' attribute: BUILD_BUG_ON failed: max(_f1.endbit, _f2.endbit) <= min(_f1.startbit, _f2.startbit)
pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
^
with:
../drivers/net/ethernet/intel/ice/ice_common.c:1419:2: error: call to '__compiletime_assert_3414' declared with 'error' attribute: ice_rlan_ctx_fields field 3 overlaps with previous field
pack_fields(buf, sizeof(*buf), ctx, ice_rlan_ctx_fields,
^
That incremental improvement is below, if you'd be interested in including it
(the auto-generated code is not part of the diff):
diff --git a/include/linux/packing.h b/include/linux/packing.h
index c4fc76ae64a5..1c89a5129b06 100644
--- a/include/linux/packing.h
+++ b/include/linux/packing.h
@@ -36,22 +36,38 @@ struct packed_field_m {
sizeof_field(struct_name, struct_field), \
}
-#define CHECK_PACKED_FIELD(field) ({ \
- typeof(field) __f = (field); \
- BUILD_BUG_ON(__f.startbit < __f.endbit); \
- BUILD_BUG_ON(__f.startbit - __f.endbit >= BITS_PER_BYTE * __f.size); \
- BUILD_BUG_ON(__f.size != 1 && __f.size != 2 && \
- __f.size != 4 && __f.size != 8); \
+#define CHECK_PACKED_FIELD_OVERLAP(fields, index1, index2) ({ \
+ typeof(&(fields)[0]) __f = (fields); \
+ typeof(__f[0]) _f1 = __f[index1]; typeof(__f[0]) _f2 = __f[index2]; \
+ const bool _ascending = __f[0].startbit < __f[1].startbit; \
+ BUILD_BUG_ON_MSG(_ascending && _f1.startbit >= _f2.startbit, \
+ __stringify(fields) " field " __stringify(index2) \
+ " breaks ascending order"); \
+ BUILD_BUG_ON_MSG(!_ascending && _f1.startbit <= _f2.startbit, \
+ __stringify(fields) " field " __stringify(index2) \
+ " breaks descending order"); \
+ BUILD_BUG_ON_MSG(max(_f1.endbit, _f2.endbit) <= \
+ min(_f1.startbit, _f2.startbit), \
+ __stringify(fields) " field " __stringify(index2) \
+ " overlaps with previous field"); \
})
-
-#define CHECK_PACKED_FIELD_OVERLAP(ascending, field1, field2) ({ \
- typeof(field1) _f1 = (field1); typeof(field2) _f2 = (field2); \
- const bool _a = (ascending); \
- BUILD_BUG_ON(_a && _f1.startbit >= _f2.startbit); \
- BUILD_BUG_ON(!_a && _f1.startbit <= _f2.startbit); \
- BUILD_BUG_ON(max(_f1.endbit, _f2.endbit) <= \
- min(_f1.startbit, _f2.startbit)); \
+#define CHECK_PACKED_FIELD(fields, index) ({ \
+ typeof(&(fields)[0]) _f = (fields); \
+ typeof(_f[0]) __f = _f[index]; \
+ BUILD_BUG_ON_MSG(__f.startbit < __f.endbit, \
+ __stringify(fields) " field " __stringify(index) \
+ " start bit must not be smaller than end bit"); \
+ BUILD_BUG_ON_MSG(__f.size != 1 && __f.size != 2 && \
+ __f.size != 4 && __f.size != 8, \
+ __stringify(fields) " field " __stringify(index) \
+ " has unsupported unpacked storage size"); \
+ BUILD_BUG_ON_MSG(__f.startbit - __f.endbit >= BITS_PER_BYTE * __f.size, \
+ __stringify(fields) " field " __stringify(index) \
+ " exceeds unpacked storage size"); \
+ __builtin_choose_expr(index != 0, \
+ CHECK_PACKED_FIELD_OVERLAP(fields, index - 1, index), \
+ 1); \
})
#define CHECK_PACKED_FIELDS_SIZE(fields, pbuflen) ({ \
diff --git a/scripts/gen_packed_field_checks.c b/scripts/gen_packed_field_checks.c
index 09a21afd640b..fabbb741c9a8 100644
--- a/scripts/gen_packed_field_checks.c
+++ b/scripts/gen_packed_field_checks.c
@@ -9,15 +9,9 @@ int main(int argc, char **argv)
{
for (int i = 1; i <= MAX_PACKED_FIELD_SIZE; i++) {
printf("#define CHECK_PACKED_FIELDS_%d(fields) ({ \\\n", i);
- printf("\ttypeof(&(fields)[0]) _f = (fields); \\\n");
- printf("\tBUILD_BUG_ON(ARRAY_SIZE(fields) != %d); \\\n", i);
for (int j = 0; j < i; j++)
- printf("\tCHECK_PACKED_FIELD(_f[%d]); \\\n", j);
-
- for (int j = 1; j < i; j++)
- printf("\tCHECK_PACKED_FIELD_OVERLAP(_f[0].startbit < _f[1].startbit, _f[%d], _f[%d]); \\\n",
- j - 1, j);
+ printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", j);
printf("})\n\n");
}
And there's one more thing I tried, which mostly worked. That was to
express CHECK_PACKED_FIELDS_N in terms of CHECK_PACKED_FIELDS_N-1.
This further reduced the auto-generated code size from 1478 lines to 302
lines, which I think is appealing.
diff --git a/scripts/gen_packed_field_checks.c b/scripts/gen_packed_field_checks.c
index fabbb741c9a8..bac85c04ef20 100644
--- a/scripts/gen_packed_field_checks.c
+++ b/scripts/gen_packed_field_checks.c
@@ -10,9 +10,10 @@ int main(int argc, char **argv)
for (int i = 1; i <= MAX_PACKED_FIELD_SIZE; i++) {
printf("#define CHECK_PACKED_FIELDS_%d(fields) ({ \\\n", i);
- for (int j = 0; j < i; j++)
- printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", j);
+ if (i != 1)
+ printf("\tCHECK_PACKED_FIELDS_%d(fields); \\\n", i - 1);
+ printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", i);
printf("})\n\n");
}
The problem is that, for some reason, it introduces this sparse warning:
../lib/packing_test.c:436:9: warning: invalid access past the end of 'test_fields' (24 24)
../lib/packing_test.c:448:9: warning: invalid access past the end of 'test_fields' (24 24)
Nobody accesses past element 6 (ARRAY_SIZE) of test_fields[]. I ran the
KUnit with kasan and I saw no warning. The strace warning comes from
check_access() in flow.c, but I don't have any energy left today to go
further into this.
I'm suspecting either a strace bug/false positive, or some sort of
variable name aliasing issue which I haven't identified yet.
Powered by blists - more mailing lists