diff mbox series

driver core: Add __alloc_size hint to devm allocators

Message ID 20221018073430.never.551-kees@kernel.org
State New
Headers
Series driver core: Add __alloc_size hint to devm allocators |

Commit Message

Kees Cook Oct. 18, 2022, 7:34 a.m. UTC 
  Mark the devm_*alloc()-family of allocations with appropriate
__alloc_size() hints so the compiler can attempt to reason about buffer
lengths from allocations.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Nishanth Menon <nm@ti.com>
Cc: Michael Kelley <mikelley@microsoft.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Won Chung <wonchung@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/device.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Rasmus Villemoes Oct. 18, 2022, 10:09 a.m. UTC | #1 
On 18/10/2022 09.34, Kees Cook wrote:
> Mark the devm_*alloc()-family of allocations with appropriate
> __alloc_size() hints so the compiler can attempt to reason about buffer
> lengths from allocations.
> 

> @@ -226,7 +226,8 @@ static inline void *devm_kcalloc(struct device *dev,
>  void devm_kfree(struct device *dev, const void *p);
>  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);
> +void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp)
> +	__alloc_size(3);

I think it's wrong to apply the __malloc attribute to kmemdup() and
variants.

'malloc'
     This tells the compiler that a function is 'malloc'-like, i.e.,
     that the pointer P returned by the function cannot alias any other
     pointer valid when the function returns, and moreover no pointers
     to valid objects occur in any storage addressed by P.

See also commit d64e85d3e1c5, introducing __malloc in the first place.
Maybe worth lifting some of that to a comment somewhere.

Rasmus
Kees Cook Oct. 18, 2022, 10:15 a.m. UTC | #2 
On Tue, Oct 18, 2022 at 12:09:30PM +0200, Rasmus Villemoes wrote:
> On 18/10/2022 09.34, Kees Cook wrote:
> > Mark the devm_*alloc()-family of allocations with appropriate
> > __alloc_size() hints so the compiler can attempt to reason about buffer
> > lengths from allocations.
> > 
> 
> > @@ -226,7 +226,8 @@ static inline void *devm_kcalloc(struct device *dev,
> >  void devm_kfree(struct device *dev, const void *p);
> >  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);
> > +void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp)
> > +	__alloc_size(3);
> 
> I think it's wrong to apply the __malloc attribute to kmemdup() and
> variants.
>
> 'malloc'
>      This tells the compiler that a function is 'malloc'-like, i.e.,
>      that the pointer P returned by the function cannot alias any other
>      pointer valid when the function returns, and moreover no pointers
>      to valid objects occur in any storage addressed by P.

Oh, ew, it defines rules about _contents_ as well. Thank you for
pointing that out!

I suppose we can use __realloc_size for these cases then?
Rasmus Villemoes Oct. 18, 2022, 10:43 a.m. UTC | #3 
On 18/10/2022 12.15, Kees Cook wrote:
> On Tue, Oct 18, 2022 at 12:09:30PM +0200, Rasmus Villemoes wrote:
>> On 18/10/2022 09.34, Kees Cook wrote:
>>> Mark the devm_*alloc()-family of allocations with appropriate
>>> __alloc_size() hints so the compiler can attempt to reason about buffer
>>> lengths from allocations.
>>>
>>
>>> @@ -226,7 +226,8 @@ static inline void *devm_kcalloc(struct device *dev,
>>>  void devm_kfree(struct device *dev, const void *p);
>>>  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);
>>> +void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp)
>>> +	__alloc_size(3);
>>
>> I think it's wrong to apply the __malloc attribute to kmemdup() and
>> variants.
>>
>> 'malloc'
>>      This tells the compiler that a function is 'malloc'-like, i.e.,
>>      that the pointer P returned by the function cannot alias any other
>>      pointer valid when the function returns, and moreover no pointers
>>      to valid objects occur in any storage addressed by P.
> 
> Oh, ew, it defines rules about _contents_ as well. Thank you for
> pointing that out!
> 
> I suppose we can use __realloc_size for these cases then?

Probably, but it's gonna be mighty confusing for people reading the code.

I was never really a fan of including __malloc in __alloc_size in the
first place, this is the kind of confusion that comes from having one
attribute include another without having the developer forced to think
about whether both actually apply in a given situation.

And that malloc documentation (both the old and the fixed) even came up
in what I assume is the thread that led up to that

    Since anything marked with __alloc_size would also qualify for marking
    with __malloc, just include __malloc along with it to avoid redundant
    markings.  (Suggested by Linus Torvalds.)

in commit 86cffecd, namely
https://lore.kernel.org/mm-commits/202109101138.53FCADF5C@keescook/ .

Rasmus
diff mbox series

Patch

diff --git a/include/linux/device.h b/include/linux/device.h
index 424b55df0272..a1cbbab9a57a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -197,9 +197,9 @@  void devres_remove_group(struct device *dev, void *id);
 int devres_release_group(struct device *dev, void *id);
 
 /* managed devm_k.alloc/kfree for device drivers */
-void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __malloc;
+void *devm_kmalloc(struct device *dev, size_t size, gfp_t gfp) __alloc_size(2);
 void *devm_krealloc(struct device *dev, void *ptr, size_t size,
-		    gfp_t gfp) __must_check;
+		    gfp_t gfp) __must_check __realloc_size(3);
 __printf(3, 0) char *devm_kvasprintf(struct device *dev, gfp_t gfp,
 				     const char *fmt, va_list ap) __malloc;
 __printf(3, 4) char *devm_kasprintf(struct device *dev, gfp_t gfp,
@@ -226,7 +226,8 @@  static inline void *devm_kcalloc(struct device *dev,
 void devm_kfree(struct device *dev, const void *p);
 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);
+void *devm_kmemdup(struct device *dev, const void *src, size_t len, gfp_t gfp)
+	__alloc_size(3);
 
 unsigned long devm_get_free_pages(struct device *dev,
 				  gfp_t gfp_mask, unsigned int order);