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(+)

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);
-- 
2.35.3

Hi Raag,

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).

> 
> 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(+)
> 
> 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);
> -- 
> 2.35.3
> 

Thanks.

-- 
Dmitry

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.

-- 
With Best Regards,
Andy Shevchenko

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.

-- 
Dmitry

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?

-- 
With Best Regards,
Andy Shevchenko

On Mon, Nov 25, 2024 at 07:13:06PM +0200, Andy Shevchenko wrote:
> 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?

Sure.

Raag