[012/200] mm: slab: clarify krealloc()'s behavior with __GFP_ZERO

Commit Message

Andrew Morton Dec. 15, 2020, 3:03 a.m. UTC

From: Bartosz Golaszewski <[e-mail]>
Subject: mm: slab: clarify krealloc()'s behavior with __GFP_ZERO

Patch series "slab: provide and use krealloc_array()", v3.

Andy brought to my attention the fact that users allocating an array of
equally sized elements should check if the size multiplication doesn't
overflow.  This is why we have helpers like kmalloc_array().

However we don't have krealloc_array() equivalent and there are many users
who do their own multiplication when calling krealloc() for arrays.

This series provides krealloc_array() and uses it in a couple places.

A separate series will follow adding devm_krealloc_array() which is needed
in the xilinx adc driver.


This patch (of 9):

__GFP_ZERO is ignored by krealloc() (unless we fall-back to kmalloc()
path, in which case it's honored).  Point that out in the kerneldoc.

Link: https://lkml.kernel.org/r/[e-mail]
Link: https://lkml.kernel.org/r/[e-mail]
Signed-off-by: Bartosz Golaszewski <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Sumit Semwal <[e-mail]>
Cc: Gustavo Padovan <[e-mail]>
Cc: Christian Knig <[e-mail]>
Cc: Mauro Carvalho Chehab <[e-mail]>
Cc: Borislav Petkov <[e-mail]>
Cc: Tony Luck <[e-mail]>
Cc: James Morse <[e-mail]>
Cc: Robert Richter <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Alexander Shishkin <[e-mail]>
Cc: Linus Walleij <[e-mail]>
Cc: "Michael S . Tsirkin" <[e-mail]>
Cc: Jason Wang <[e-mail]>
Cc: Christoph Lameter <[e-mail]>
Cc: Pekka Enberg <[e-mail]>
Cc: David Rientjes <[e-mail]>
Cc: Joonsoo Kim <[e-mail]>
Cc: Jaroslav Kysela <[e-mail]>
Cc: Takashi Iwai <[e-mail]>
Cc: Borislav Petkov <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Takashi Iwai <[e-mail]>
Cc: Vlastimil Babka <[e-mail]>
Signed-off-by: Andrew Morton <[e-mail]>
---

 mm/slab_common.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Christian König Dec. 15, 2020, 2:30 p.m. UTC | #1

Am 15.12.20 um 04:03 schrieb Andrew Morton:
> From: Bartosz Golaszewski <[e-mail]>
> Subject: mm: slab: clarify krealloc()'s behavior with __GFP_ZERO
>
> Patch series "slab: provide and use krealloc_array()", v3.
>
> Andy brought to my attention the fact that users allocating an array of
> equally sized elements should check if the size multiplication doesn't
> overflow.  This is why we have helpers like kmalloc_array().
>
> However we don't have krealloc_array() equivalent and there are many users
> who do their own multiplication when calling krealloc() for arrays.
>
> This series provides krealloc_array() and uses it in a couple places.
>
> A separate series will follow adding devm_krealloc_array() which is needed
> in the xilinx adc driver.
>
>
> This patch (of 9):
>
> __GFP_ZERO is ignored by krealloc() (unless we fall-back to kmalloc()
> path, in which case it's honored).  Point that out in the kerneldoc.
>
> Link: https://lkml.kernel.org/r/[e-mail]
> Link: https://lkml.kernel.org/r/[e-mail]
> Signed-off-by: Bartosz Golaszewski <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>
> Cc: Sumit Semwal <[e-mail]>
> Cc: Gustavo Padovan <[e-mail]>
> Cc: Christian Knig <[e-mail]>
> Cc: Mauro Carvalho Chehab <[e-mail]>
> Cc: Borislav Petkov <[e-mail]>
> Cc: Tony Luck <[e-mail]>
> Cc: James Morse <[e-mail]>
> Cc: Robert Richter <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Alexander Shishkin <[e-mail]>
> Cc: Linus Walleij <[e-mail]>
> Cc: "Michael S . Tsirkin" <[e-mail]>
> Cc: Jason Wang <[e-mail]>
> Cc: Christoph Lameter <[e-mail]>
> Cc: Pekka Enberg <[e-mail]>
> Cc: David Rientjes <[e-mail]>
> Cc: Joonsoo Kim <[e-mail]>
> Cc: Jaroslav Kysela <[e-mail]>
> Cc: Takashi Iwai <[e-mail]>
> Cc: Borislav Petkov <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Takashi Iwai <[e-mail]>
> Cc: Vlastimil Babka <[e-mail]>
> Signed-off-by: Andrew Morton <[e-mail]>
> ---
>
>   mm/slab_common.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> --- a/mm/slab_common.c~mm-slab-clarify-kreallocs-behavior-with-__gfp_zero
> +++ a/mm/slab_common.c
> @@ -1091,9 +1091,9 @@ static __always_inline void *__do_kreall
>    * @flags: the type of memory to allocate.
>    *
>    * The contents of the object pointed to are preserved up to the
> - * lesser of the new and old sizes.  If @p is %NULL, krealloc()
> - * behaves exactly like kmalloc().  If @new_size is 0 and @p is not a
> - * %NULL pointer, the object pointed to is freed.
> + * lesser of the new and old sizes (__GFP_ZERO flag is effectively ignored).
> + * If @p is %NULL, krealloc() behaves exactly like kmalloc().  If @new_size
> + * is 0 and @p is not a %NULL pointer, the object pointed to is freed.

Question: Can the fact that __GFP_ZERO is effectively ignored cause an 
information leak if new size is larger than old size and the array is 
somehow copied to user space?

I think the answer is no, but just wanted to double check. Maybe we 
should note that here.

Thanks,
Christian.

>    *
>    * Return: pointer to the allocated memory or %NULL in case of error
>    */
> _

Andy Shevchenko Dec. 15, 2020, 7:08 p.m. UTC | #2

On Tue, Dec 15, 2020 at 03:30:44PM +0100, Christian König wrote:
> Am 15.12.20 um 04:03 schrieb Andrew Morton:

...

> Question: Can the fact that __GFP_ZERO is effectively ignored cause an
> information leak if new size is larger than old size and the array is
> somehow copied to user space?
> 
> I think the answer is no, but just wanted to double check. Maybe we should
> note that here.

kmalloc()/kmalloc_array()/etc has the same. Should it be mentioned there as well?

Christian König Dec. 16, 2020, 7:47 a.m. UTC | #3

Am 15.12.20 um 20:08 schrieb Andy Shevchenko:
> On Tue, Dec 15, 2020 at 03:30:44PM +0100, Christian König wrote:
>> Am 15.12.20 um 04:03 schrieb Andrew Morton:
> ...
>
>> Question: Can the fact that __GFP_ZERO is effectively ignored cause an
>> information leak if new size is larger than old size and the array is
>> somehow copied to user space?
>>
>> I think the answer is no, but just wanted to double check. Maybe we should
>> note that here.
> kmalloc()/kmalloc_array()/etc has the same. Should it be mentioned there as well?

No, they don't. If kmalloc()/kmalloc_array() would ignore __GFP_ZERO we 
would have quite a problem.

It is only krealloc()/krealloc_array() which ignore __GFP_ZERO when they 
don't reallocate memory because newsize is smaller than oldsize. In 
other words the freed up space is not cleared in any way.

Christian.

Andy Shevchenko Dec. 16, 2020, 11:23 a.m. UTC | #4

On Wed, Dec 16, 2020 at 08:47:25AM +0100, Christian König wrote:
> Am 15.12.20 um 20:08 schrieb Andy Shevchenko:
> > On Tue, Dec 15, 2020 at 03:30:44PM +0100, Christian König wrote:
> > > Am 15.12.20 um 04:03 schrieb Andrew Morton:
> > ...
> > 
> > > Question: Can the fact that __GFP_ZERO is effectively ignored cause an
> > > information leak if new size is larger than old size and the array is
> > > somehow copied to user space?
> > > 
> > > I think the answer is no, but just wanted to double check. Maybe we should
> > > note that here.
> > kmalloc()/kmalloc_array()/etc has the same. Should it be mentioned there as well?
> 
> No, they don't. If kmalloc()/kmalloc_array() would ignore __GFP_ZERO we
> would have quite a problem.
> 
> It is only krealloc()/krealloc_array() which ignore __GFP_ZERO when they
> don't reallocate memory because newsize is smaller than oldsize. In other
> words the freed up space is not cleared in any way.

Yes, true. So, you meant that comment now a bit misleading. I agree.

Patch

--- a/mm/slab_common.c~mm-slab-clarify-kreallocs-behavior-with-__gfp_zero
+++ a/mm/slab_common.c
@@ -1091,9 +1091,9 @@  static __always_inline void *__do_kreall
  * @flags: the type of memory to allocate.
  *
  * The contents of the object pointed to are preserved up to the
- * lesser of the new and old sizes.  If @p is %NULL, krealloc()
- * behaves exactly like kmalloc().  If @new_size is 0 and @p is not a
- * %NULL pointer, the object pointed to is freed.
+ * lesser of the new and old sizes (__GFP_ZERO flag is effectively ignored).
+ * If @p is %NULL, krealloc() behaves exactly like kmalloc().  If @new_size
+ * is 0 and @p is not a %NULL pointer, the object pointed to is freed.
  *
  * Return: pointer to the allocated memory or %NULL in case of error
  */