| Message ID | 20240816045123.1934387-2-davidgow@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f2c6dbd220170c2396fb019ead67fbada1e23ebd |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [v2] kunit: Device wrappers should also manage driver name | expand |
On Fri, Aug 16, 2024 at 12:51:22PM +0800, David Gow wrote: > diff --git a/include/kunit/test.h b/include/kunit/test.h > index e2a1f0928e8b..5ac237c949a0 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -28,6 +28,7 @@ > #include <linux/types.h> > > #include <asm/rwonce.h> > +#include <asm/sections.h> > > /* Static key: true if any KUnit tests are currently running */ > DECLARE_STATIC_KEY_FALSE(kunit_running); > @@ -480,6 +481,53 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp > return kunit_kmalloc_array(test, n, size, gfp | __GFP_ZERO); > } > > + > +/** > + * kunit_kfree_const() - conditionally free test managed memory Hi David, Minor nit, but I believe the description of the 'test' parameter should be here as well (like it is done in kunit_kstrdup): ``` @test: The test context object ``` > + * @x: pointer to the memory > + * > + * Calls kunit_kfree() only if @x is not in .rodata section. > + * See kunit_kstrdup_const() for more information. > + */ > +void kunit_kfree_const(struct kunit *test, const void *x); > + > +/** > + * kunit_kstrdup() - Duplicates a string into a test managed allocation. > + * > + * @test: The test context object. > + * @str: The NULL-terminated string to duplicate. > + * @gfp: flags passed to underlying kmalloc(). > + * > + * See kstrdup() and kunit_kmalloc_array() for more information. > + */ > +static inline char *kunit_kstrdup(struct kunit *test, const char *str, gfp_t gfp) > +{ > + size_t len; > + char *buf; > + > + if (!str) > + return NULL; > + > + len = strlen(str) + 1; > + buf = kunit_kmalloc(test, len, gfp); > + if (buf) > + memcpy(buf, str, len); > + return buf; > +} > + > +/** > + * kunit_kstrdup_const() - Conditionally duplicates a string into a test managed allocation. > + * > + * @test: The test context object. > + * @str: The NULL-terminated string to duplicate. > + * @gfp: flags passed to underlying kmalloc(). > + * > + * Calls kunit_kstrdup() only if @str is not in the rodata section. Must be freed with > + * kunit_kfree_const() -- not kunit_kfree(). > + * See kstrdup_const() and kunit_kmalloc_array() for more information. > + */ > +const char *kunit_kstrdup_const(struct kunit *test, const char *str, gfp_t gfp); > + > /** > * kunit_vm_mmap() - Allocate KUnit-tracked vm_mmap() area > * @test: The test context object. > diff --git a/lib/kunit/device.c b/lib/kunit/device.c > index 25c81ed465fb..520c1fccee8a 100644 > --- a/lib/kunit/device.c > +++ b/lib/kunit/device.c > @@ -89,7 +89,7 @@ struct device_driver *kunit_driver_create(struct kunit *test, const char *name) > if (!driver) > return ERR_PTR(err); > > - driver->name = name; > + driver->name = kunit_kstrdup_const(test, name, GFP_KERNEL); > driver->bus = &kunit_bus_type; > driver->owner = THIS_MODULE; > > @@ -192,8 +192,11 @@ void kunit_device_unregister(struct kunit *test, struct device *dev) > const struct device_driver *driver = to_kunit_device(dev)->driver; > > kunit_release_action(test, device_unregister_wrapper, dev); > - if (driver) > + if (driver) { > + const char *driver_name = driver->name; Also a minor nit (and I haven't found anything in the kernel code style regarding it), but probably the declaration should be moved into beginning of the function (as it is done in the rest of the file) Thanks! -- Kind regards, Ivan Orlov
On Fri, Aug 16, 2024 at 12:51 AM David Gow <davidgow@google.com> wrote: > > kunit_driver_create() accepts a name for the driver, but does not copy > it, so if that name is either on the stack, or otherwise freed, we end > up with a use-after-free when the driver is cleaned up. > > Instead, strdup() the name, and manage it as another KUnit allocation. > As there was no existing kunit_kstrdup(), we add one. Further, add a > kunit_ variant of strdup_const() and kfree_const(), so we don't need to > allocate and manage the string in the majority of cases where it's a > constant. > > However, these are inline functions, and is_kernel_rodata() only works > for built-in code. This causes problems in two cases: > - If kunit is built as a module, __{start,end}_rodata is not defined. > - If a kunit test using these functions is built as a module, it will > suffer the same fate. > > This fixes a KASAN splat with overflow.overflow_allocation_test, when > built as a module. > > Restrict the is_kernel_rodata() case to when KUnit is built as a module, > which fixes the first case, at the cost of losing the optimisation. > > Also, make kunit_{kstrdup,kfree}_const non-inline, so that other modules > using them will not accidentally depend on is_kernel_rodata(). If KUnit > is built-in, they'll benefit from the optimisation, if KUnit is not, > they won't, but the string will be properly duplicated. > > Fixes: d03c720e03bd ("kunit: Add APIs for managing devices") > Reported-by: Nico Pache <npache@redhat.com> > Closes: https://groups.google.com/g/kunit-dev/c/81V9b9QYON0 > Reviewed-by: Kees Cook <kees@kernel.org> > Reviewed-by: Maxime Ripard <mripard@kernel.org> > Reviewed-by: Rae Moar <rmoar@google.com> > Signed-off-by: David Gow <davidgow@google.com> > --- > > This is a combination of the previous version of this patch with the > follow-up fix "kunit: Fix kunit_kstrdup_const() with modules". > > kunit_kstrdup_const() now falls back to kstrdup() if KUnit is built as a > module, and is no longer inlined. This should fix the issues we'd seen > before. > > I've not tried doing something fancy by looking at module rodata > sections: it might be a possible optimisation, but it seems like it'd > overcomplicate things for this initial change. If we hit a KUnit test > where this is a bottleneck (or if I have some more spare time), we can > look into it. > > The overflow_kunit test has been fixed independently to not rely on this > anyway, so there shouldn't be any current cases of this causing issues, > but it's worth making the API robust regardless. > > Changes since previous version: > https://lore.kernel.org/linux-kselftest/20240731070207.3918687-1-davidgow@google.com/ > - Fix module support by integrating: > https://lore.kernel.org/linux-kselftest/20240806020136.3481593-1-davidgow@google.com/ > Hello! I tested this new patch with modules, particularly the device tests and the overflow_kunit test. And it seems to be working well. Tested-by: Rae Moar <rmoar@google.com> Thanks! -Rae > --- > include/kunit/test.h | 48 ++++++++++++++++++++++++++++++++++++++++++++ > lib/kunit/device.c | 7 +++++-- > lib/kunit/test.c | 19 ++++++++++++++++++ > 3 files changed, 72 insertions(+), 2 deletions(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index e2a1f0928e8b..5ac237c949a0 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -28,6 +28,7 @@ > #include <linux/types.h> > > #include <asm/rwonce.h> > +#include <asm/sections.h> > > /* Static key: true if any KUnit tests are currently running */ > DECLARE_STATIC_KEY_FALSE(kunit_running); > @@ -480,6 +481,53 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp > return kunit_kmalloc_array(test, n, size, gfp | __GFP_ZERO); > } > > + > +/** > + * kunit_kfree_const() - conditionally free test managed memory > + * @x: pointer to the memory > + * > + * Calls kunit_kfree() only if @x is not in .rodata section. > + * See kunit_kstrdup_const() for more information. > + */ > +void kunit_kfree_const(struct kunit *test, const void *x); > + > +/** > + * kunit_kstrdup() - Duplicates a string into a test managed allocation. > + * > + * @test: The test context object. > + * @str: The NULL-terminated string to duplicate. > + * @gfp: flags passed to underlying kmalloc(). > + * > + * See kstrdup() and kunit_kmalloc_array() for more information. > + */ > +static inline char *kunit_kstrdup(struct kunit *test, const char *str, gfp_t gfp) > +{ > + size_t len; > + char *buf; > + > + if (!str) > + return NULL; > + > + len = strlen(str) + 1; > + buf = kunit_kmalloc(test, len, gfp); > + if (buf) > + memcpy(buf, str, len); > + return buf; > +} > + > +/** > + * kunit_kstrdup_const() - Conditionally duplicates a string into a test managed allocation. > + * > + * @test: The test context object. > + * @str: The NULL-terminated string to duplicate. > + * @gfp: flags passed to underlying kmalloc(). > + * > + * Calls kunit_kstrdup() only if @str is not in the rodata section. Must be freed with > + * kunit_kfree_const() -- not kunit_kfree(). > + * See kstrdup_const() and kunit_kmalloc_array() for more information. > + */ > +const char *kunit_kstrdup_const(struct kunit *test, const char *str, gfp_t gfp); > + > /** > * kunit_vm_mmap() - Allocate KUnit-tracked vm_mmap() area > * @test: The test context object. > diff --git a/lib/kunit/device.c b/lib/kunit/device.c > index 25c81ed465fb..520c1fccee8a 100644 > --- a/lib/kunit/device.c > +++ b/lib/kunit/device.c > @@ -89,7 +89,7 @@ struct device_driver *kunit_driver_create(struct kunit *test, const char *name) > if (!driver) > return ERR_PTR(err); > > - driver->name = name; > + driver->name = kunit_kstrdup_const(test, name, GFP_KERNEL); > driver->bus = &kunit_bus_type; > driver->owner = THIS_MODULE; > > @@ -192,8 +192,11 @@ void kunit_device_unregister(struct kunit *test, struct device *dev) > const struct device_driver *driver = to_kunit_device(dev)->driver; > > kunit_release_action(test, device_unregister_wrapper, dev); > - if (driver) > + if (driver) { > + const char *driver_name = driver->name; > kunit_release_action(test, driver_unregister_wrapper, (void *)driver); > + kunit_kfree_const(test, driver_name); > + } > } > EXPORT_SYMBOL_GPL(kunit_device_unregister); > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index e8b1b52a19ab..089c832e3cdb 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -874,6 +874,25 @@ void kunit_kfree(struct kunit *test, const void *ptr) > } > EXPORT_SYMBOL_GPL(kunit_kfree); > > +void kunit_kfree_const(struct kunit *test, const void *x) > +{ > +#if !IS_MODULE(CONFIG_KUNIT) > + if (!is_kernel_rodata((unsigned long)x)) > +#endif > + kunit_kfree(test, x); > +} > +EXPORT_SYMBOL_GPL(kunit_kfree_const); > + > +const char *kunit_kstrdup_const(struct kunit *test, const char *str, gfp_t gfp) > +{ > +#if !IS_MODULE(CONFIG_KUNIT) > + if (is_kernel_rodata((unsigned long)str)) > + return str; > +#endif > + return kunit_kstrdup(test, str, gfp); > +} > +EXPORT_SYMBOL_GPL(kunit_kstrdup_const); > + > void kunit_cleanup(struct kunit *test) > { > struct kunit_resource *res; > -- > 2.46.0.184.g6999bdac58-goog >
diff --git a/include/kunit/test.h b/include/kunit/test.h index e2a1f0928e8b..5ac237c949a0 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -28,6 +28,7 @@ #include <linux/types.h> #include <asm/rwonce.h> +#include <asm/sections.h> /* Static key: true if any KUnit tests are currently running */ DECLARE_STATIC_KEY_FALSE(kunit_running); @@ -480,6 +481,53 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp return kunit_kmalloc_array(test, n, size, gfp | __GFP_ZERO); } + +/** + * kunit_kfree_const() - conditionally free test managed memory + * @x: pointer to the memory + * + * Calls kunit_kfree() only if @x is not in .rodata section. + * See kunit_kstrdup_const() for more information. + */ +void kunit_kfree_const(struct kunit *test, const void *x); + +/** + * kunit_kstrdup() - Duplicates a string into a test managed allocation. + * + * @test: The test context object. + * @str: The NULL-terminated string to duplicate. + * @gfp: flags passed to underlying kmalloc(). + * + * See kstrdup() and kunit_kmalloc_array() for more information. + */ +static inline char *kunit_kstrdup(struct kunit *test, const char *str, gfp_t gfp) +{ + size_t len; + char *buf; + + if (!str) + return NULL; + + len = strlen(str) + 1; + buf = kunit_kmalloc(test, len, gfp); + if (buf) + memcpy(buf, str, len); + return buf; +} + +/** + * kunit_kstrdup_const() - Conditionally duplicates a string into a test managed allocation. + * + * @test: The test context object. + * @str: The NULL-terminated string to duplicate. + * @gfp: flags passed to underlying kmalloc(). + * + * Calls kunit_kstrdup() only if @str is not in the rodata section. Must be freed with + * kunit_kfree_const() -- not kunit_kfree(). + * See kstrdup_const() and kunit_kmalloc_array() for more information. + */ +const char *kunit_kstrdup_const(struct kunit *test, const char *str, gfp_t gfp); + /** * kunit_vm_mmap() - Allocate KUnit-tracked vm_mmap() area * @test: The test context object. diff --git a/lib/kunit/device.c b/lib/kunit/device.c index 25c81ed465fb..520c1fccee8a 100644 --- a/lib/kunit/device.c +++ b/lib/kunit/device.c @@ -89,7 +89,7 @@ struct device_driver *kunit_driver_create(struct kunit *test, const char *name) if (!driver) return ERR_PTR(err); - driver->name = name; + driver->name = kunit_kstrdup_const(test, name, GFP_KERNEL); driver->bus = &kunit_bus_type; driver->owner = THIS_MODULE; @@ -192,8 +192,11 @@ void kunit_device_unregister(struct kunit *test, struct device *dev) const struct device_driver *driver = to_kunit_device(dev)->driver; kunit_release_action(test, device_unregister_wrapper, dev); - if (driver) + if (driver) { + const char *driver_name = driver->name; kunit_release_action(test, driver_unregister_wrapper, (void *)driver); + kunit_kfree_const(test, driver_name); + } } EXPORT_SYMBOL_GPL(kunit_device_unregister); diff --git a/lib/kunit/test.c b/lib/kunit/test.c index e8b1b52a19ab..089c832e3cdb 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -874,6 +874,25 @@ void kunit_kfree(struct kunit *test, const void *ptr) } EXPORT_SYMBOL_GPL(kunit_kfree); +void kunit_kfree_const(struct kunit *test, const void *x) +{ +#if !IS_MODULE(CONFIG_KUNIT) + if (!is_kernel_rodata((unsigned long)x)) +#endif + kunit_kfree(test, x); +} +EXPORT_SYMBOL_GPL(kunit_kfree_const); + +const char *kunit_kstrdup_const(struct kunit *test, const char *str, gfp_t gfp) +{ +#if !IS_MODULE(CONFIG_KUNIT) + if (is_kernel_rodata((unsigned long)str)) + return str; +#endif + return kunit_kstrdup(test, str, gfp); +} +EXPORT_SYMBOL_GPL(kunit_kstrdup_const); + void kunit_cleanup(struct kunit *test) { struct kunit_resource *res;
kunit_driver_create() accepts a name for the driver, but does not copy it, so if that name is either on the stack, or otherwise freed, we end up with a use-after-free when the driver is cleaned up. Instead, strdup() the name, and manage it as another KUnit allocation. As there was no existing kunit_kstrdup(), we add one. Further, add a kunit_ variant of strdup_const() and kfree_const(), so we don't need to allocate and manage the string in the majority of cases where it's a constant. However, these are inline functions, and is_kernel_rodata() only works for built-in code. This causes problems in two cases: - If kunit is built as a module, __{start,end}_rodata is not defined. - If a kunit test using these functions is built as a module, it will suffer the same fate. This fixes a KASAN splat with overflow.overflow_allocation_test, when built as a module. Restrict the is_kernel_rodata() case to when KUnit is built as a module, which fixes the first case, at the cost of losing the optimisation. Also, make kunit_{kstrdup,kfree}_const non-inline, so that other modules using them will not accidentally depend on is_kernel_rodata(). If KUnit is built-in, they'll benefit from the optimisation, if KUnit is not, they won't, but the string will be properly duplicated. Fixes: d03c720e03bd ("kunit: Add APIs for managing devices") Reported-by: Nico Pache <npache@redhat.com> Closes: https://groups.google.com/g/kunit-dev/c/81V9b9QYON0 Reviewed-by: Kees Cook <kees@kernel.org> Reviewed-by: Maxime Ripard <mripard@kernel.org> Reviewed-by: Rae Moar <rmoar@google.com> Signed-off-by: David Gow <davidgow@google.com> --- This is a combination of the previous version of this patch with the follow-up fix "kunit: Fix kunit_kstrdup_const() with modules". kunit_kstrdup_const() now falls back to kstrdup() if KUnit is built as a module, and is no longer inlined. This should fix the issues we'd seen before. I've not tried doing something fancy by looking at module rodata sections: it might be a possible optimisation, but it seems like it'd overcomplicate things for this initial change. If we hit a KUnit test where this is a bottleneck (or if I have some more spare time), we can look into it. The overflow_kunit test has been fixed independently to not rely on this anyway, so there shouldn't be any current cases of this causing issues, but it's worth making the API robust regardless. Changes since previous version: https://lore.kernel.org/linux-kselftest/20240731070207.3918687-1-davidgow@google.com/ - Fix module support by integrating: https://lore.kernel.org/linux-kselftest/20240806020136.3481593-1-davidgow@google.com/ --- include/kunit/test.h | 48 ++++++++++++++++++++++++++++++++++++++++++++ lib/kunit/device.c | 7 +++++-- lib/kunit/test.c | 19 ++++++++++++++++++ 3 files changed, 72 insertions(+), 2 deletions(-)