| Message ID | 20241123200527.7830-2-raag.jadav@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce devm_kmemdup_array() helper | expand |
On Sun, Nov 24, 2024 at 07:03:36AM +0000, Dmitry Torokhov wrote: > On Sun, Nov 24, 2024 at 01:35:23AM +0530, Raag Jadav wrote: > > Introduce '_array' variant of devm_kmemdup() for the users which lack > > multiplication overflow check. > > I am not sure that this new helper is needed. Unlike allocators for > brand new objects, such as kmalloc_array(), devm_kmemdup() makes a copy > of already existing object, which is supposed to be a valid object and > therefore will have a reasonable size. So there should be no chance for > hitting this overflow unless the caller is completely confused and calls > devm_kmemdup() with random arguments (in which case all bets are off). Don't we want to have a code more robust even if all what you say applies? Also this makes the call consistent with zillions of others from the alloc family of calls in the Linux kernel.
On Mon, Nov 25, 2024 at 09:49:22AM +0200, Andy Shevchenko wrote: > On Sun, Nov 24, 2024 at 07:03:36AM +0000, Dmitry Torokhov wrote: > > On Sun, Nov 24, 2024 at 01:35:23AM +0530, Raag Jadav wrote: > > > Introduce '_array' variant of devm_kmemdup() for the users which lack > > > multiplication overflow check. > > > > I am not sure that this new helper is needed. Unlike allocators for > > brand new objects, such as kmalloc_array(), devm_kmemdup() makes a copy > > of already existing object, which is supposed to be a valid object and > > therefore will have a reasonable size. So there should be no chance for > > hitting this overflow unless the caller is completely confused and calls > > devm_kmemdup() with random arguments (in which case all bets are off). > > Don't we want to have a code more robust even if all what you say applies? > Also this makes the call consistent with zillions of others from the alloc > family of calls in the Linux kernel. Agree. Although shooting in the foot is never the expectation, it is atleast better than having to debug such unexpected cases. Raag
On Mon, Nov 25, 2024 at 05:08:13PM +0200, Raag Jadav wrote: > On Mon, Nov 25, 2024 at 09:49:22AM +0200, Andy Shevchenko wrote: > > On Sun, Nov 24, 2024 at 07:03:36AM +0000, Dmitry Torokhov wrote: > > > On Sun, Nov 24, 2024 at 01:35:23AM +0530, Raag Jadav wrote: > > > > Introduce '_array' variant of devm_kmemdup() for the users which lack > > > > multiplication overflow check. > > > > > > I am not sure that this new helper is needed. Unlike allocators for > > > brand new objects, such as kmalloc_array(), devm_kmemdup() makes a copy > > > of already existing object, which is supposed to be a valid object and > > > therefore will have a reasonable size. So there should be no chance for > > > hitting this overflow unless the caller is completely confused and calls > > > devm_kmemdup() with random arguments (in which case all bets are off). > > > > Don't we want to have a code more robust even if all what you say applies? > > Also this makes the call consistent with zillions of others from the alloc > > family of calls in the Linux kernel. Having a clean API is fine, just do not bill it as something that is "safer". As I mentioned, unlike other allocators this one is supposed to operate with a valid source object and size passed to devm_kmemdup() should not exceed the size of the source object. There is no chance of overflowing. > > Agree. Although shooting in the foot is never the expectation, it is > atleast better than having to debug such unexpected cases. Then maybe have a BUG() there instead of returning NULL? I know BUG()s are frowned upon, but I think in this case overflow is really an indicator of a hard error by the caller which is passing garbage arguments to this function. Hm, I see we have kmemdup_array() already. Ok. How about making your devm_kmemdup_array() be similar to kmemdup_array()? static inline void *devm_kmemdup_array(struct device *dev, const void *src, size_t n, size_t size, gfp_t flags) { return devm_kmemdup(dev, src, size_mul(size, n), flags); } This will trigger a warning on a too large order of allocation in mm/page_alloc.c::__alloc_pages_noprof(). Thanks.
On Mon, Nov 25, 2024 at 08:29:10AM -0800, Dmitry Torokhov wrote: > On Mon, Nov 25, 2024 at 05:08:13PM +0200, Raag Jadav wrote: > > On Mon, Nov 25, 2024 at 09:49:22AM +0200, Andy Shevchenko wrote: > > > On Sun, Nov 24, 2024 at 07:03:36AM +0000, Dmitry Torokhov wrote: > > > > On Sun, Nov 24, 2024 at 01:35:23AM +0530, Raag Jadav wrote: ... > > > > > Introduce '_array' variant of devm_kmemdup() for the users which lack > > > > > multiplication overflow check. > > > > > > > > I am not sure that this new helper is needed. Unlike allocators for > > > > brand new objects, such as kmalloc_array(), devm_kmemdup() makes a copy > > > > of already existing object, which is supposed to be a valid object and > > > > therefore will have a reasonable size. So there should be no chance for > > > > hitting this overflow unless the caller is completely confused and calls > > > > devm_kmemdup() with random arguments (in which case all bets are off). > > > > > > Don't we want to have a code more robust even if all what you say applies? > > > Also this makes the call consistent with zillions of others from the alloc > > > family of calls in the Linux kernel. > > Having a clean API is fine, just do not bill it as something that is > "safer". As I mentioned, unlike other allocators this one is supposed to > operate with a valid source object and size passed to devm_kmemdup() > should not exceed the size of the source object. There is no chance of > overflowing. Agree. > > Agree. Although shooting in the foot is never the expectation, it is > > atleast better than having to debug such unexpected cases. > > Then maybe have a BUG() there instead of returning NULL? I know BUG()s > are frowned upon, but I think in this case overflow is really an > indicator of a hard error by the caller which is passing garbage > arguments to this function. > > Hm, I see we have kmemdup_array() already. Ok. How about making your > devm_kmemdup_array() be similar to kmemdup_array()? > > static inline void *devm_kmemdup_array(struct device *dev, const void *src, > size_t n, size_t size, gfp_t flags) > { > return devm_kmemdup(dev, src, size_mul(size, n), flags); > } > > This will trigger a warning on a too large order of allocation in > mm/page_alloc.c::__alloc_pages_noprof(). This is nice! I have overlooked that kmemdup_array() uses size_mul() instead of a check. Raag, can you rebuild your series on this?
diff --git a/include/linux/device.h b/include/linux/device.h index b4bde8d22697..c31f48d0dde0 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -358,6 +358,16 @@ char *devm_kstrdup(struct device *dev, const char *s, gfp_t gfp) __malloc; const char *devm_kstrdup_const(struct device *dev, const char *s, gfp_t gfp); void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp) __realloc_size(3); +static inline void *devm_kmemdup_array(struct device *dev, const void *src, + size_t n, size_t size, gfp_t flags) +{ + size_t bytes; + + if (unlikely(check_mul_overflow(n, size, &bytes))) + return NULL; + + return devm_kmemdup(dev, src, bytes, flags); +} unsigned long devm_get_free_pages(struct device *dev, gfp_t gfp_mask, unsigned int order);
Introduce '_array' variant of devm_kmemdup() for the users which lack multiplication overflow check. Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Raag Jadav <raag.jadav@intel.com> --- include/linux/device.h | 10 ++++++++++ 1 file changed, 10 insertions(+)