* Re: [PATCH v1 RFC 6/6] crypto: implement KFuzzTest targets for PKCS7 and RSA parsing [not found] ` <20250813133812.926145-7-ethan.w.s.graham@gmail.com> @ 2025-08-13 18:13 ` Marco Elver 2025-08-14 15:28 ` Ignat Korchagin 0 siblings, 1 reply; 8+ messages in thread From: Marco Elver @ 2025-08-13 18:13 UTC (permalink / raw) To: Ethan Graham Cc: ethangraham, glider, andreyknvl, brendan.higgins, davidgow, dvyukov, jannh, rmoar, shuah, tarasmadan, kasan-dev, kunit-dev, linux-kernel, linux-mm, David Howells, Lukas Wunner, Ignat Korchagin, Herbert Xu, David S. Miller, open list:HARDWARE RANDOM NUMBER GENERATOR CORE [+Cc crypto maintainers] On Wed, 13 Aug 2025 at 15:38, Ethan Graham <ethan.w.s.graham@gmail.com> wrote: > > From: Ethan Graham <ethangraham@google.com> Should also Cc crypto maintainers, as they'll be the ones giving feedback on how interesting this is to them. Use ./scripts/get_maintainer.pl for that in the next round, and either add the Cc list below your Signed-off-by so that git send-email picks it up only for this patch, or just for the whole series (normally preferred, so maintainers get context of the full series). > Add KFuzzTest targets for pkcs7_parse_message, rsa_parse_pub_key, and > rsa_parse_priv_key to serve as real-world examples of how the framework is used. > > These functions are ideal candidates for KFuzzTest as they perform complex > parsing of user-controlled data but are not directly exposed at the syscall > boundary. This makes them difficult to exercise with traditional fuzzing tools > and showcases the primary strength of the KFuzzTest framework: providing an > interface to fuzz internal, non-exported kernel functions. > > The targets are defined directly within the source files of the functions they > test, demonstrating how to colocate fuzz tests with the code under test. > > Signed-off-by: Ethan Graham <ethangraham@google.com> > --- > crypto/asymmetric_keys/pkcs7_parser.c | 15 ++++++++++++++ > crypto/rsa_helper.c | 29 +++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c > index 423d13c47545..e8477f8b0eaf 100644 > --- a/crypto/asymmetric_keys/pkcs7_parser.c > +++ b/crypto/asymmetric_keys/pkcs7_parser.c > @@ -13,6 +13,7 @@ > #include <linux/err.h> > #include <linux/oid_registry.h> > #include <crypto/public_key.h> > +#include <linux/kfuzztest.h> > #include "pkcs7_parser.h" > #include "pkcs7.asn1.h" > > @@ -169,6 +170,20 @@ struct pkcs7_message *pkcs7_parse_message(const void *data, size_t datalen) > } > EXPORT_SYMBOL_GPL(pkcs7_parse_message); > > +struct pkcs7_parse_message_arg { > + const void *data; > + size_t datalen; > +}; > + > +FUZZ_TEST(test_pkcs7_parse_message, struct pkcs7_parse_message_arg) > +{ > + KFUZZTEST_EXPECT_NOT_NULL(pkcs7_parse_message_arg, data); > + KFUZZTEST_ANNOTATE_LEN(pkcs7_parse_message_arg, datalen, data); > + KFUZZTEST_EXPECT_LE(pkcs7_parse_message_arg, datalen, 16 * PAGE_SIZE); > + > + pkcs7_parse_message(arg->data, arg->datalen); > +} > + > /** > * pkcs7_get_content_data - Get access to the PKCS#7 content > * @pkcs7: The preparsed PKCS#7 message to access > diff --git a/crypto/rsa_helper.c b/crypto/rsa_helper.c > index 94266f29049c..79b7ddc7c48d 100644 > --- a/crypto/rsa_helper.c > +++ b/crypto/rsa_helper.c > @@ -9,6 +9,7 @@ > #include <linux/export.h> > #include <linux/err.h> > #include <linux/fips.h> > +#include <linux/kfuzztest.h> > #include <crypto/internal/rsa.h> > #include "rsapubkey.asn1.h" > #include "rsaprivkey.asn1.h" > @@ -166,6 +167,20 @@ int rsa_parse_pub_key(struct rsa_key *rsa_key, const void *key, > } > EXPORT_SYMBOL_GPL(rsa_parse_pub_key); > > +struct rsa_parse_pub_key_arg { > + const void *key; > + size_t key_len; > +}; > + > +FUZZ_TEST(test_rsa_parse_pub_key, struct rsa_parse_pub_key_arg) > +{ > + KFUZZTEST_EXPECT_NOT_NULL(rsa_parse_pub_key_arg, key); > + KFUZZTEST_EXPECT_LE(rsa_parse_pub_key_arg, key_len, 16 * PAGE_SIZE); > + > + struct rsa_key out; > + rsa_parse_pub_key(&out, arg->key, arg->key_len); > +} > + > /** > * rsa_parse_priv_key() - decodes the BER encoded buffer and stores in the > * provided struct rsa_key, pointers to the raw key > @@ -184,3 +199,17 @@ int rsa_parse_priv_key(struct rsa_key *rsa_key, const void *key, > return asn1_ber_decoder(&rsaprivkey_decoder, rsa_key, key, key_len); > } > EXPORT_SYMBOL_GPL(rsa_parse_priv_key); > + > +struct rsa_parse_priv_key_arg { > + const void *key; > + size_t key_len; > +}; > + > +FUZZ_TEST(test_rsa_parse_priv_key, struct rsa_parse_priv_key_arg) > +{ > + KFUZZTEST_EXPECT_NOT_NULL(rsa_parse_priv_key_arg, key); > + KFUZZTEST_EXPECT_LE(rsa_parse_priv_key_arg, key_len, 16 * PAGE_SIZE); > + > + struct rsa_key out; > + rsa_parse_priv_key(&out, arg->key, arg->key_len); > +} > -- > 2.51.0.rc0.205.g4a044479a3-goog > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 RFC 6/6] crypto: implement KFuzzTest targets for PKCS7 and RSA parsing 2025-08-13 18:13 ` [PATCH v1 RFC 6/6] crypto: implement KFuzzTest targets for PKCS7 and RSA parsing Marco Elver @ 2025-08-14 15:28 ` Ignat Korchagin 2025-08-15 1:17 ` Eric Biggers 0 siblings, 1 reply; 8+ messages in thread From: Ignat Korchagin @ 2025-08-14 15:28 UTC (permalink / raw) To: Marco Elver, Ethan Graham, ethangraham Cc: glider, andreyknvl, brendan.higgins, davidgow, dvyukov, jannh, rmoar, shuah, tarasmadan, kasan-dev, kunit-dev, linux-kernel, linux-mm, David Howells, Lukas Wunner, Herbert Xu, David S. Miller, open list:HARDWARE RANDOM NUMBER GENERATOR CORE On Wed, Aug 13, 2025 at 7:14 PM Marco Elver <elver@google.com> wrote: > > [+Cc crypto maintainers] > > On Wed, 13 Aug 2025 at 15:38, Ethan Graham <ethan.w.s.graham@gmail.com> wrote: > > > > From: Ethan Graham <ethangraham@google.com> > > Should also Cc crypto maintainers, as they'll be the ones giving Thanks Marco! > feedback on how interesting this is to them. Use > ./scripts/get_maintainer.pl for that in the next round, and either add > the Cc list below your Signed-off-by so that git send-email picks it > up only for this patch, or just for the whole series (normally > preferred, so maintainers get context of the full series). > > > Add KFuzzTest targets for pkcs7_parse_message, rsa_parse_pub_key, and > > rsa_parse_priv_key to serve as real-world examples of how the framework is used. > > > > These functions are ideal candidates for KFuzzTest as they perform complex > > parsing of user-controlled data but are not directly exposed at the syscall > > boundary. This makes them difficult to exercise with traditional fuzzing tools > > and showcases the primary strength of the KFuzzTest framework: providing an > > interface to fuzz internal, non-exported kernel functions. > > > > The targets are defined directly within the source files of the functions they > > test, demonstrating how to colocate fuzz tests with the code under test. > > > > Signed-off-by: Ethan Graham <ethangraham@google.com> > > --- > > crypto/asymmetric_keys/pkcs7_parser.c | 15 ++++++++++++++ > > crypto/rsa_helper.c | 29 +++++++++++++++++++++++++++ > > 2 files changed, 44 insertions(+) > > > > diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c > > index 423d13c47545..e8477f8b0eaf 100644 > > --- a/crypto/asymmetric_keys/pkcs7_parser.c > > +++ b/crypto/asymmetric_keys/pkcs7_parser.c > > @@ -13,6 +13,7 @@ > > #include <linux/err.h> > > #include <linux/oid_registry.h> > > #include <crypto/public_key.h> > > +#include <linux/kfuzztest.h> > > #include "pkcs7_parser.h" > > #include "pkcs7.asn1.h" > > > > @@ -169,6 +170,20 @@ struct pkcs7_message *pkcs7_parse_message(const void *data, size_t datalen) > > } > > EXPORT_SYMBOL_GPL(pkcs7_parse_message); > > > > +struct pkcs7_parse_message_arg { > > + const void *data; > > + size_t datalen; > > +}; > > + > > +FUZZ_TEST(test_pkcs7_parse_message, struct pkcs7_parse_message_arg) Not sure if it has been mentioned elsewhere, but one thing I already don't like about it is that these definitions "pollute" the actual source files. Might not be such a big deal here, but kernel source files for core subsystems tend to become quite large and complex already, so not a great idea to make them even larger and harder to follow with fuzz definitions. As far as I'm aware, for the same reason KUnit [1] is not that popular (or at least less popular than other approaches, like selftests [2]). Is it possible to make it that these definitions live in separate files or even closer to selftests? Ignat > > +{ > > + KFUZZTEST_EXPECT_NOT_NULL(pkcs7_parse_message_arg, data); > > + KFUZZTEST_ANNOTATE_LEN(pkcs7_parse_message_arg, datalen, data); > > + KFUZZTEST_EXPECT_LE(pkcs7_parse_message_arg, datalen, 16 * PAGE_SIZE); > > + > > + pkcs7_parse_message(arg->data, arg->datalen); > > +} > > + > > /** > > * pkcs7_get_content_data - Get access to the PKCS#7 content > > * @pkcs7: The preparsed PKCS#7 message to access > > diff --git a/crypto/rsa_helper.c b/crypto/rsa_helper.c > > index 94266f29049c..79b7ddc7c48d 100644 > > --- a/crypto/rsa_helper.c > > +++ b/crypto/rsa_helper.c > > @@ -9,6 +9,7 @@ > > #include <linux/export.h> > > #include <linux/err.h> > > #include <linux/fips.h> > > +#include <linux/kfuzztest.h> > > #include <crypto/internal/rsa.h> > > #include "rsapubkey.asn1.h" > > #include "rsaprivkey.asn1.h" > > @@ -166,6 +167,20 @@ int rsa_parse_pub_key(struct rsa_key *rsa_key, const void *key, > > } > > EXPORT_SYMBOL_GPL(rsa_parse_pub_key); > > > > +struct rsa_parse_pub_key_arg { > > + const void *key; > > + size_t key_len; > > +}; > > + > > +FUZZ_TEST(test_rsa_parse_pub_key, struct rsa_parse_pub_key_arg) > > +{ > > + KFUZZTEST_EXPECT_NOT_NULL(rsa_parse_pub_key_arg, key); > > + KFUZZTEST_EXPECT_LE(rsa_parse_pub_key_arg, key_len, 16 * PAGE_SIZE); > > + > > + struct rsa_key out; > > + rsa_parse_pub_key(&out, arg->key, arg->key_len); > > +} > > + > > /** > > * rsa_parse_priv_key() - decodes the BER encoded buffer and stores in the > > * provided struct rsa_key, pointers to the raw key > > @@ -184,3 +199,17 @@ int rsa_parse_priv_key(struct rsa_key *rsa_key, const void *key, > > return asn1_ber_decoder(&rsaprivkey_decoder, rsa_key, key, key_len); > > } > > EXPORT_SYMBOL_GPL(rsa_parse_priv_key); > > + > > +struct rsa_parse_priv_key_arg { > > + const void *key; > > + size_t key_len; > > +}; > > + > > +FUZZ_TEST(test_rsa_parse_priv_key, struct rsa_parse_priv_key_arg) > > +{ > > + KFUZZTEST_EXPECT_NOT_NULL(rsa_parse_priv_key_arg, key); > > + KFUZZTEST_EXPECT_LE(rsa_parse_priv_key_arg, key_len, 16 * PAGE_SIZE); > > + > > + struct rsa_key out; > > + rsa_parse_priv_key(&out, arg->key, arg->key_len); > > +} > > -- > > 2.51.0.rc0.205.g4a044479a3-goog > > [1]: https://docs.kernel.org/dev-tools/kunit/index.html [2]: https://docs.kernel.org/dev-tools/kselftest.html ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 RFC 6/6] crypto: implement KFuzzTest targets for PKCS7 and RSA parsing 2025-08-14 15:28 ` Ignat Korchagin @ 2025-08-15 1:17 ` Eric Biggers 2025-08-15 13:00 ` Ignat Korchagin 0 siblings, 1 reply; 8+ messages in thread From: Eric Biggers @ 2025-08-15 1:17 UTC (permalink / raw) To: Ignat Korchagin Cc: Marco Elver, Ethan Graham, ethangraham, glider, andreyknvl, brendan.higgins, davidgow, dvyukov, jannh, rmoar, shuah, tarasmadan, kasan-dev, kunit-dev, linux-kernel, linux-mm, David Howells, Lukas Wunner, Herbert Xu, David S. Miller, open list:HARDWARE RANDOM NUMBER GENERATOR CORE On Thu, Aug 14, 2025 at 04:28:13PM +0100, Ignat Korchagin wrote: > Not sure if it has been mentioned elsewhere, but one thing I already > don't like about it is that these definitions "pollute" the actual > source files. Might not be such a big deal here, but kernel source > files for core subsystems tend to become quite large and complex > already, so not a great idea to make them even larger and harder to > follow with fuzz definitions. > > As far as I'm aware, for the same reason KUnit [1] is not that popular > (or at least less popular than other approaches, like selftests [2]). > Is it possible to make it that these definitions live in separate > files or even closer to selftests? That's not the impression I get. KUnit suites are normally defined in separate files, and KUnit seems to be increasing in popularity. KFuzzTest can use separate files too, it looks like? Would it make any sense for fuzz tests to be a special type of KUnit test, instead of a separate framework? - Eric ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 RFC 6/6] crypto: implement KFuzzTest targets for PKCS7 and RSA parsing 2025-08-15 1:17 ` Eric Biggers @ 2025-08-15 13:00 ` Ignat Korchagin 2025-08-19 10:08 ` Marco Elver 0 siblings, 1 reply; 8+ messages in thread From: Ignat Korchagin @ 2025-08-15 13:00 UTC (permalink / raw) To: Eric Biggers Cc: Marco Elver, Ethan Graham, ethangraham, glider, andreyknvl, brendan.higgins, davidgow, dvyukov, jannh, rmoar, shuah, tarasmadan, kasan-dev, kunit-dev, linux-kernel, linux-mm, David Howells, Lukas Wunner, Herbert Xu, David S. Miller, open list:HARDWARE RANDOM NUMBER GENERATOR CORE On Fri, Aug 15, 2025 at 2:18 AM Eric Biggers <ebiggers@kernel.org> wrote: > > On Thu, Aug 14, 2025 at 04:28:13PM +0100, Ignat Korchagin wrote: > > Not sure if it has been mentioned elsewhere, but one thing I already > > don't like about it is that these definitions "pollute" the actual > > source files. Might not be such a big deal here, but kernel source > > files for core subsystems tend to become quite large and complex > > already, so not a great idea to make them even larger and harder to > > follow with fuzz definitions. > > > > As far as I'm aware, for the same reason KUnit [1] is not that popular > > (or at least less popular than other approaches, like selftests [2]). > > Is it possible to make it that these definitions live in separate > > files or even closer to selftests? > > That's not the impression I get. KUnit suites are normally defined in > separate files, and KUnit seems to be increasing in popularity. Great! Either I was wrong from the start or it changed and I haven't looked there recently. > KFuzzTest can use separate files too, it looks like? > > Would it make any sense for fuzz tests to be a special type of KUnit > test, instead of a separate framework? I think so, if possible. There is always some hurdles adopting new framework, but if it would be a new feature of an existing one (either KUnit or selftests - whatever fits better semantically), the existing users of that framework are more likely to pick it up. > - Eric ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 RFC 6/6] crypto: implement KFuzzTest targets for PKCS7 and RSA parsing 2025-08-15 13:00 ` Ignat Korchagin @ 2025-08-19 10:08 ` Marco Elver 2025-08-19 11:41 ` Ignat Korchagin ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Marco Elver @ 2025-08-19 10:08 UTC (permalink / raw) To: Ignat Korchagin Cc: Eric Biggers, Ethan Graham, ethangraham, glider, andreyknvl, brendan.higgins, davidgow, dvyukov, jannh, rmoar, shuah, tarasmadan, kasan-dev, kunit-dev, linux-kernel, linux-mm, David Howells, Lukas Wunner, Herbert Xu, David S. Miller, open list:HARDWARE RANDOM NUMBER GENERATOR CORE On Fri, 15 Aug 2025 at 15:00, Ignat Korchagin <ignat@cloudflare.com> wrote: > > On Fri, Aug 15, 2025 at 2:18 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Thu, Aug 14, 2025 at 04:28:13PM +0100, Ignat Korchagin wrote: > > > Not sure if it has been mentioned elsewhere, but one thing I already > > > don't like about it is that these definitions "pollute" the actual > > > source files. Might not be such a big deal here, but kernel source > > > files for core subsystems tend to become quite large and complex > > > already, so not a great idea to make them even larger and harder to > > > follow with fuzz definitions. > > > > > > As far as I'm aware, for the same reason KUnit [1] is not that popular > > > (or at least less popular than other approaches, like selftests [2]). > > > Is it possible to make it that these definitions live in separate > > > files or even closer to selftests? > > > > That's not the impression I get. KUnit suites are normally defined in > > separate files, and KUnit seems to be increasing in popularity. > > Great! Either I was wrong from the start or it changed and I haven't > looked there recently. > > > KFuzzTest can use separate files too, it looks like? > > > > Would it make any sense for fuzz tests to be a special type of KUnit > > test, instead of a separate framework? > > I think so, if possible. There is always some hurdles adopting new > framework, but if it would be a new feature of an existing one (either > KUnit or selftests - whatever fits better semantically), the existing > users of that framework are more likely to pick it up. The dependency would be in name only (i.e. "branding"). Right now it's possible to use KFuzzTest without the KUnit dependency. So there is technical merit to decouple. Would sufficient documentation, and perhaps suggesting separate files to be the canonical way of defining KFuzzTests, improve the situation? For example something like: For subsystem foo.c, define a KFuzzTest in foo_kfuzz.c, and then in the Makfile add "obj-$(CONFIG_KFUZZTEST) += foo_kfuzz.o". Alternatively, to test internal static functions, place the KFuzzTest harness in a file foo_kfuzz.h, and include at the bottom of foo.c. Alex, Ethan, and KUnit folks: What's your preference? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 RFC 6/6] crypto: implement KFuzzTest targets for PKCS7 and RSA parsing 2025-08-19 10:08 ` Marco Elver @ 2025-08-19 11:41 ` Ignat Korchagin 2025-08-22 8:15 ` Ethan Graham 2025-08-22 8:57 ` David Gow 2 siblings, 0 replies; 8+ messages in thread From: Ignat Korchagin @ 2025-08-19 11:41 UTC (permalink / raw) To: Marco Elver Cc: Eric Biggers, Ethan Graham, ethangraham, glider, andreyknvl, brendan.higgins, davidgow, dvyukov, jannh, rmoar, shuah, tarasmadan, kasan-dev, kunit-dev, linux-kernel, linux-mm, David Howells, Lukas Wunner, Herbert Xu, David S. Miller, open list:HARDWARE RANDOM NUMBER GENERATOR CORE On Tue, Aug 19, 2025 at 11:08 AM Marco Elver <elver@google.com> wrote: > > On Fri, 15 Aug 2025 at 15:00, Ignat Korchagin <ignat@cloudflare.com> wrote: > > > > On Fri, Aug 15, 2025 at 2:18 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Thu, Aug 14, 2025 at 04:28:13PM +0100, Ignat Korchagin wrote: > > > > Not sure if it has been mentioned elsewhere, but one thing I already > > > > don't like about it is that these definitions "pollute" the actual > > > > source files. Might not be such a big deal here, but kernel source > > > > files for core subsystems tend to become quite large and complex > > > > already, so not a great idea to make them even larger and harder to > > > > follow with fuzz definitions. > > > > > > > > As far as I'm aware, for the same reason KUnit [1] is not that popular > > > > (or at least less popular than other approaches, like selftests [2]). > > > > Is it possible to make it that these definitions live in separate > > > > files or even closer to selftests? > > > > > > That's not the impression I get. KUnit suites are normally defined in > > > separate files, and KUnit seems to be increasing in popularity. > > > > Great! Either I was wrong from the start or it changed and I haven't > > looked there recently. > > > > > KFuzzTest can use separate files too, it looks like? > > > > > > Would it make any sense for fuzz tests to be a special type of KUnit > > > test, instead of a separate framework? > > > > I think so, if possible. There is always some hurdles adopting new > > framework, but if it would be a new feature of an existing one (either > > KUnit or selftests - whatever fits better semantically), the existing > > users of that framework are more likely to pick it up. > > The dependency would be in name only (i.e. "branding"). Right now it's > possible to use KFuzzTest without the KUnit dependency. So there is > technical merit to decouple. Probably strong (Kbuild) dependency is not what I was thinking about, rather just semantical similarity. That is, if I "learned" KUnit - KFuzzTest is easy to pick up for me. > Would sufficient documentation, and perhaps suggesting separate files > to be the canonical way of defining KFuzzTests, improve the situation? Probably. > For example something like: > For subsystem foo.c, define a KFuzzTest in foo_kfuzz.c, and then in > the Makfile add "obj-$(CONFIG_KFUZZTEST) += foo_kfuzz.o". > Alternatively, to test internal static functions, place the KFuzzTest > harness in a file foo_kfuzz.h, and include at the bottom of foo.c. Having includes at the bottom of the file feels weird and "leaks" kfuzz tests into the sources. Perhaps we can somehow rely on the fact that kernel is a flat address space and you can always get the address of a symbol (even if static - similar to how eBPF kprobes do it)? Or have a bit more complex Kbuild configuration: for example "foo_kfuzz.c" would include "foo.c" (although including .c files also feels weird). If CONFIG_KFUZZTEST is disabled, Kbuild just includes "foo.o", if enabled we include "foo_kfuzz.o" (which includes foo.c as a source). Ignat > Alex, Ethan, and KUnit folks: What's your preference? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 RFC 6/6] crypto: implement KFuzzTest targets for PKCS7 and RSA parsing 2025-08-19 10:08 ` Marco Elver 2025-08-19 11:41 ` Ignat Korchagin @ 2025-08-22 8:15 ` Ethan Graham 2025-08-22 8:57 ` David Gow 2 siblings, 0 replies; 8+ messages in thread From: Ethan Graham @ 2025-08-22 8:15 UTC (permalink / raw) To: Marco Elver Cc: Ignat Korchagin, Eric Biggers, ethangraham, glider, andreyknvl, brendan.higgins, davidgow, dvyukov, jannh, rmoar, shuah, tarasmadan, kasan-dev, kunit-dev, linux-kernel, linux-mm, David Howells, Lukas Wunner, Herbert Xu, David S. Miller, open list:HARDWARE RANDOM NUMBER GENERATOR CORE On Tue, Aug 19, 2025 at 12:08 PM Marco Elver <elver@google.com> wrote: > For example something like: > For subsystem foo.c, define a KFuzzTest in foo_kfuzz.c, and then in > the Makfile add "obj-$(CONFIG_KFUZZTEST) += foo_kfuzz.o". I agree that fuzz targets should only be built if CONFIG_KFUZZTEST is enabled. Building a separate foo_kfuzz.o is probably ideal, but will need to think about how to cleanly handle static functions. > Alternatively, to test internal static functions, place the KFuzzTest > harness in a file foo_kfuzz.h, and include at the bottom of foo.c. > > Alex, Ethan, and KUnit folks: What's your preference? I think placing fuzz targets in separate files is a step in the right direction. Including a foo_kfuzz.h file inside of the source does still pollute the file to some extent but certainly less than having one or more KFuzzTest targets defined alongside the code. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 RFC 6/6] crypto: implement KFuzzTest targets for PKCS7 and RSA parsing 2025-08-19 10:08 ` Marco Elver 2025-08-19 11:41 ` Ignat Korchagin 2025-08-22 8:15 ` Ethan Graham @ 2025-08-22 8:57 ` David Gow 2 siblings, 0 replies; 8+ messages in thread From: David Gow @ 2025-08-22 8:57 UTC (permalink / raw) To: Marco Elver Cc: Ignat Korchagin, Eric Biggers, Ethan Graham, ethangraham, glider, andreyknvl, brendan.higgins, dvyukov, jannh, rmoar, shuah, tarasmadan, kasan-dev, kunit-dev, linux-kernel, linux-mm, David Howells, Lukas Wunner, Herbert Xu, David S. Miller, open list:HARDWARE RANDOM NUMBER GENERATOR CORE [-- Attachment #1: Type: text/plain, Size: 5833 bytes --] On Tue, 19 Aug 2025 at 18:08, Marco Elver <elver@google.com> wrote: > > On Fri, 15 Aug 2025 at 15:00, Ignat Korchagin <ignat@cloudflare.com> wrote: > > > > On Fri, Aug 15, 2025 at 2:18 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Thu, Aug 14, 2025 at 04:28:13PM +0100, Ignat Korchagin wrote: > > > > Not sure if it has been mentioned elsewhere, but one thing I already > > > > don't like about it is that these definitions "pollute" the actual > > > > source files. Might not be such a big deal here, but kernel source > > > > files for core subsystems tend to become quite large and complex > > > > already, so not a great idea to make them even larger and harder to > > > > follow with fuzz definitions. > > > > > > > > As far as I'm aware, for the same reason KUnit [1] is not that popular > > > > (or at least less popular than other approaches, like selftests [2]). > > > > Is it possible to make it that these definitions live in separate > > > > files or even closer to selftests? > > > > > > That's not the impression I get. KUnit suites are normally defined in > > > separate files, and KUnit seems to be increasing in popularity. > > > > Great! Either I was wrong from the start or it changed and I haven't > > looked there recently. > > > > > KFuzzTest can use separate files too, it looks like? > > > > > > Would it make any sense for fuzz tests to be a special type of KUnit > > > test, instead of a separate framework? > > > > I think so, if possible. There is always some hurdles adopting new > > framework, but if it would be a new feature of an existing one (either > > KUnit or selftests - whatever fits better semantically), the existing > > users of that framework are more likely to pick it up. > > The dependency would be in name only (i.e. "branding"). Right now it's > possible to use KFuzzTest without the KUnit dependency. So there is > technical merit to decouple. > There's definitely some overlap between KFuzzTest and KUnit, from the relatively superficial API similarities: both having similar ASSERT/EXPECT macros; to the more specific: KUnit parameterised tests allow running the same 'test' code against several different pieces of input data. Then again, there are definitely some differences, too: KUnit doesn't have a way of describing complex binary data (though it's definitely a feature we'd like one day), and the purpose of KUnit tests, while having some overlap, is different than exposing fuzz targets. If there's a bit of KUnit functionality you can reasonably re-use or otherwise take advantage of, I'd not discourage you from doing so. There's a balance to be struck between taking the extra dependency and ending up with duplicate implementations of the same thing. I also think that what Ignat says below around simply ensuring that the API is familiar (i.e., not deviating from what KUnit or other frameworks do without a good reason) is a good middle ground here. So my gut feeling is that you could end up with one of three things: - Make KFuzzTests a special case of (parameterised) KUnit tests. This would probably involve adding a way to run the tests with a parameter from debugfs or a kernel command-line argument using the metadata format, and teaching the fuzzer to run KUnit tests. KUnit already has an attributes mechanism that could be used to note which tests are fuzz targets, and maybe even providing some of the annotation, but there'd be some work needed to improve it. The big advantage here is that you'd automatically gain the ability to use KUnit helpers to set up things like memory regions, fake devices, etc. - Share some of the implementation details between KUnit and KFuzzTest, but keep them as separate things. We already have a bunch of, e.g., work on assertions, logging, etc, which could possibly be helpful. This could be done by depending on CONFIG_KUNIT or by splitting those out into a shared test library. - Keep them separate, but be careful to make the APIs similar enough to be familiar. KFuzzTest already looks pretty similar to me, so I think we're already in a good place here. Personally, I'd quite like for there to be a bit more overlap/code sharing -- at least eventually -- as I could see some benefits to "borrowing" some KFuzzTest code to allow, e.g., providing custom inputs/outputs for tests. > Would sufficient documentation, and perhaps suggesting separate files > to be the canonical way of defining KFuzzTests, improve the situation? > > For example something like: > For subsystem foo.c, define a KFuzzTest in foo_kfuzz.c, and then in > the Makfile add "obj-$(CONFIG_KFUZZTEST) += foo_kfuzz.o". > Alternatively, to test internal static functions, place the KFuzzTest > harness in a file foo_kfuzz.h, and include at the bottom of foo.c. > > Alex, Ethan, and KUnit folks: What's your preference? I think that keeping tests in separate files by default is the right way to go, but obviously either #including them or using a whole bunch of conditional symbol exports (either with symbol namespaces or something like EXPORT_SYMBOL_IF_KUNIT) will be necessary in some cases to get coverage of internal functions. I'd suggest being a little careful with the naming scheme, as Linus was not happy with the foo_test.c names we were using as they make tab completion more annoying; we ended up putting tests in a 'tests/' subdirectory where appropriate: https://docs.kernel.org/dev-tools/kunit/style.html But ultimately, I think this is a style decision, not a critically important technical one: provide some good practices to follow -- and encourage people to be consistent -- but understand that occasionally a maintainer will override it (sometimes even for good reason). Cheers, -- David [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 5281 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-08-22 8:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250813133812.926145-1-ethan.w.s.graham@gmail.com>
[not found] ` <20250813133812.926145-7-ethan.w.s.graham@gmail.com>
2025-08-13 18:13 ` [PATCH v1 RFC 6/6] crypto: implement KFuzzTest targets for PKCS7 and RSA parsing Marco Elver
2025-08-14 15:28 ` Ignat Korchagin
2025-08-15 1:17 ` Eric Biggers
2025-08-15 13:00 ` Ignat Korchagin
2025-08-19 10:08 ` Marco Elver
2025-08-19 11:41 ` Ignat Korchagin
2025-08-22 8:15 ` Ethan Graham
2025-08-22 8:57 ` David Gow
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox