Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Kees Cook]

On Thu, May 3, 2018 at 5:36 PM, Kees Cook  wrote:
> On Thu, May 3, 2018 at 4:00 PM, Rasmus Villemoes
>  wrote:
>> On 2018-05-01 19:00, Kees Cook wrote:
>>> On Mon, Apr 30, 2018 at 2:29 PM, Rasmus Villemoes
>>>  wrote:

 gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should
 generate reasonable code. Too bad there's no completely generic
 check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's
 hard to define what they should be checked against - probably would
 require all subexpressions (including the variables themselves) to have
 the same type.

 plug: https://lkml.org/lkml/2015/7/19/358
>>>
>>> That's a very nice series. Why did it never get taken?
>>
>> Well, nobody seemed particularly interested, and then
>> https://lkml.org/lkml/2015/10/28/215 happened... but he did later seem
>> to admit that it could be useful for the multiplication checking, and
>> that "the gcc interface for multiplication overflow is fine".
>
> Oh, excellent. Thank you for that pointer! That conversation covered a
> lot of ground. I need to think a little more about how to apply the
> thoughts there with the kmalloc() needs and the GPU driver needs...
>
>> I still think even for unsigned types overflow checking can be subtle. E.g.
>>
>> u32 somevar;
>>
>> if (somevar + sizeof(foo) < somevar)
>>   return -EOVERFLOW;
>> somevar += sizeof(this);
>>
>> is broken, because the LHS is promoted to unsigned long/size_t, then so
>> is the RHS for the comparison, and the comparison is thus always false
>> (on 64bit). It gets worse if the two types are more "opaque", and in any
>> case it's not always easy to verify at a glance that the types are the
>> same, or at least that the expression of the widest type is on the RHS.
>
> That's an excellent example, yes. (And likely worth including in the
> commit log somewhere.)
>
>>
>>> It seems to do the right things quite correctly.
>>
>> Yes, I wouldn't suggest it without the test module verifying corner
>> cases, and checking it has the same semantics whether used with old or
>> new gcc.
>>
>> Would you shepherd it through if I updated the patches and resent?
>
> Yes, though we may need reworking if we actually want to do the
> try/catch style (since that was talked about with GPU stuff too...)
>
> Either way, yes, a refresh would be lovely! :)

Whatever the case, I think we need to clean up all the kmalloc() math
anyway. As mentioned earlier, there are a handful of more complex
cases, but the vast majority are just A * B. I've put up a series here
now, and I'll send it out soon. I want to think more about 3-factor
products, addition, etc:

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/kmalloc/2-factor-products

The commit logs need more details (i.e. about making constants the
second argument for optimal compiler results, etc), but there's a
Coccinelle-generated first pass.

-Kees

-- 
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Kees Cook]

On Thu, May 3, 2018 at 4:00 PM, Rasmus Villemoes
 wrote:
> On 2018-05-01 19:00, Kees Cook wrote:
>> On Mon, Apr 30, 2018 at 2:29 PM, Rasmus Villemoes
>>  wrote:
>>>
>>> gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should
>>> generate reasonable code. Too bad there's no completely generic
>>> check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's
>>> hard to define what they should be checked against - probably would
>>> require all subexpressions (including the variables themselves) to have
>>> the same type.
>>>
>>> plug: https://lkml.org/lkml/2015/7/19/358
>>
>> That's a very nice series. Why did it never get taken?
>
> Well, nobody seemed particularly interested, and then
> https://lkml.org/lkml/2015/10/28/215 happened... but he did later seem
> to admit that it could be useful for the multiplication checking, and
> that "the gcc interface for multiplication overflow is fine".

Oh, excellent. Thank you for that pointer! That conversation covered a
lot of ground. I need to think a little more about how to apply the
thoughts there with the kmalloc() needs and the GPU driver needs...

> I still think even for unsigned types overflow checking can be subtle. E.g.
>
> u32 somevar;
>
> if (somevar + sizeof(foo) < somevar)
>   return -EOVERFLOW;
> somevar += sizeof(this);
>
> is broken, because the LHS is promoted to unsigned long/size_t, then so
> is the RHS for the comparison, and the comparison is thus always false
> (on 64bit). It gets worse if the two types are more "opaque", and in any
> case it's not always easy to verify at a glance that the types are the
> same, or at least that the expression of the widest type is on the RHS.

That's an excellent example, yes. (And likely worth including in the
commit log somewhere.)

>
>> It seems to do the right things quite correctly.
>
> Yes, I wouldn't suggest it without the test module verifying corner
> cases, and checking it has the same semantics whether used with old or
> new gcc.
>
> Would you shepherd it through if I updated the patches and resent?

Yes, though we may need reworking if we actually want to do the
try/catch style (since that was talked about with GPU stuff too...)

Either way, yes, a refresh would be lovely! :)

-Kees

-- 
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Rasmus Villemoes]

On 2018-05-01 19:00, Kees Cook wrote:
> On Mon, Apr 30, 2018 at 2:29 PM, Rasmus Villemoes
>  wrote:
>>
>> gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should
>> generate reasonable code. Too bad there's no completely generic
>> check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's
>> hard to define what they should be checked against - probably would
>> require all subexpressions (including the variables themselves) to have
>> the same type.
>>
>> plug: https://lkml.org/lkml/2015/7/19/358
> 
> That's a very nice series. Why did it never get taken?

Well, nobody seemed particularly interested, and then
https://lkml.org/lkml/2015/10/28/215 happened... but he did later seem
to admit that it could be useful for the multiplication checking, and
that "the gcc interface for multiplication overflow is fine".

I still think even for unsigned types overflow checking can be subtle. E.g.

u32 somevar;

if (somevar + sizeof(foo) < somevar)
  return -EOVERFLOW;
somevar += sizeof(this);

is broken, because the LHS is promoted to unsigned long/size_t, then so
is the RHS for the comparison, and the comparison is thus always false
(on 64bit). It gets worse if the two types are more "opaque", and in any
case it's not always easy to verify at a glance that the types are the
same, or at least that the expression of the widest type is on the RHS.

> It seems to do the right things quite correctly.

Yes, I wouldn't suggest it without the test module verifying corner
cases, and checking it has the same semantics whether used with old or
new gcc.

Would you shepherd it through if I updated the patches and resent?

Rasmus
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Julia Lawall]

On Tue, 1 May 2018, Kees Cook wrote:

> On Mon, Apr 30, 2018 at 2:29 PM, Rasmus Villemoes
>  wrote:
> > On 2018-04-30 22:16, Matthew Wilcox wrote:
> >> On Mon, Apr 30, 2018 at 12:02:14PM -0700, Kees Cook wrote:
> >>>
> >>> Getting the constant ordering right could be part of the macro
> >>> definition, maybe? i.e.:
> >>>
> >>> static inline void *kmalloc_ab(size_t a, size_t b, gfp_t flags)
> >>> {
> >>> if (__builtin_constant_p(a) && a != 0 && \
> >>> b > SIZE_MAX / a)
> >>> return NULL;
> >>> else if (__builtin_constant_p(b) && b != 0 && \
> >>>a > SIZE_MAX / b)
> >>> return NULL;
> >>>
> >>> return kmalloc(a * b, flags);
> >>> }
> >>
> >> Ooh, if neither a nor b is constant, it just didn't do a check ;-(  This
> >> stuff is hard.
> >>
> >>> (I just wish C had a sensible way to catch overflow...)
> >>
> >> Every CPU I ever worked with had an "overflow" bit ... do we have a
> >> friend on the C standards ctte who might figure out a way to let us
> >> write code that checks it?
> >
> > gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should
> > generate reasonable code. Too bad there's no completely generic
> > check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's
> > hard to define what they should be checked against - probably would
> > require all subexpressions (including the variables themselves) to have
> > the same type.
> >
> > plug: https://lkml.org/lkml/2015/7/19/358
>
> That's a very nice series. Why did it never get taken? It seems to do
> the right things quite correctly.
>
> Daniel, while this isn't a perfect solution, is this something you'd
> use in graphics-land?

Opportunities for this, found with the following are shown below:

@@
expression a,b;
@@

*if (a + b < a)
 { ... return ...; }

- at the beginning of a line indicates an opportunity, not a suggestion
for removal.

I haven't checked the results carefully, but most look relevant.

julia



diff -u -p /var/linuxes/linux-next/lib/zstd/decompress.c 
/tmp/nothing/lib/zstd/decompress.c
--- /var/linuxes/linux-next/lib/zstd/decompress.c
+++ /tmp/nothing/lib/zstd/decompress.c
@@ -343,7 +343,6 @@ unsigned long long ZSTD_findDecompressed
return ret;

/* check for overflow */
-   if (totalDstSize + ret < totalDstSize)
return ZSTD_CONTENTSIZE_ERROR;
totalDstSize += ret;
}
diff -u -p /var/linuxes/linux-next/lib/scatterlist.c 
/tmp/nothing/lib/scatterlist.c
--- /var/linuxes/linux-next/lib/scatterlist.c
+++ /tmp/nothing/lib/scatterlist.c
@@ -503,7 +503,6 @@ struct scatterlist *sgl_alloc_order(unsi
nalloc = nent;
if (chainable) {
/* Check for integer overflow */
-   if (nalloc + 1 < nalloc)
return NULL;
nalloc++;
}
diff -u -p /var/linuxes/linux-next/drivers/dma-buf/dma-buf.c 
/tmp/nothing/drivers/dma-buf/dma-buf.c
--- /var/linuxes/linux-next/drivers/dma-buf/dma-buf.c
+++ /tmp/nothing/drivers/dma-buf/dma-buf.c
@@ -954,7 +954,6 @@ int dma_buf_mmap(struct dma_buf *dmabuf,
return -EINVAL;

/* check for offset overflow */
-   if (pgoff + vma_pages(vma) < pgoff)
return -EOVERFLOW;

/* check for overflowing the buffer's size */
diff -u -p /var/linuxes/linux-next/drivers/md/dm-verity-target.c 
/tmp/nothing/drivers/md/dm-verity-target.c
--- /var/linuxes/linux-next/drivers/md/dm-verity-target.c
+++ /tmp/nothing/drivers/md/dm-verity-target.c
@@ -1043,7 +1043,6 @@ static int verity_ctr(struct dm_target *
v->hash_level_block[i] = hash_position;
s = (v->data_blocks + ((sector_t)1 << ((i + 1) * 
v->hash_per_block_bits)) - 1)
>> ((i + 1) * v->hash_per_block_bits);
-   if (hash_position + s < hash_position) {
ti->error = "Hash device offset overflow";
r = -E2BIG;
goto bad;
diff -u -p /var/linuxes/linux-next/drivers/md/dm-flakey.c 
/tmp/nothing/drivers/md/dm-flakey.c
--- /var/linuxes/linux-next/drivers/md/dm-flakey.c
+++ /tmp/nothing/drivers/md/dm-flakey.c
@@ -233,7 +233,6 @@ static int flakey_ctr(struct dm_target *
goto bad;
}

-   if (fc->up_interval + fc->down_interval < fc->up_interval) {
ti->error = "Interval overflow";
r = -EINVAL;
goto bad;
diff -u -p /var/linuxes/linux-next/drivers/gpu/drm/vc4/vc4_validate.c 
/tmp/nothing/drivers/gpu/drm/vc4/vc4_validate.c
--- /var/linuxes/linux-next/drivers/gpu/drm/vc4/vc4_validate.c
+++ /tmp/nothing/drivers/gpu/drm/vc4/vc4_validate.c
@@ -306,7 +306,6 @@ validate_gl_array_primitive(VALIDATE_ARG
}
shader_state = 

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Kees Cook]

On Mon, Apr 30, 2018 at 2:29 PM, Rasmus Villemoes
 wrote:
> On 2018-04-30 22:16, Matthew Wilcox wrote:
>> On Mon, Apr 30, 2018 at 12:02:14PM -0700, Kees Cook wrote:
>>>
>>> Getting the constant ordering right could be part of the macro
>>> definition, maybe? i.e.:
>>>
>>> static inline void *kmalloc_ab(size_t a, size_t b, gfp_t flags)
>>> {
>>> if (__builtin_constant_p(a) && a != 0 && \
>>> b > SIZE_MAX / a)
>>> return NULL;
>>> else if (__builtin_constant_p(b) && b != 0 && \
>>>a > SIZE_MAX / b)
>>> return NULL;
>>>
>>> return kmalloc(a * b, flags);
>>> }
>>
>> Ooh, if neither a nor b is constant, it just didn't do a check ;-(  This
>> stuff is hard.
>>
>>> (I just wish C had a sensible way to catch overflow...)
>>
>> Every CPU I ever worked with had an "overflow" bit ... do we have a
>> friend on the C standards ctte who might figure out a way to let us
>> write code that checks it?
>
> gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should
> generate reasonable code. Too bad there's no completely generic
> check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's
> hard to define what they should be checked against - probably would
> require all subexpressions (including the variables themselves) to have
> the same type.
>
> plug: https://lkml.org/lkml/2015/7/19/358

That's a very nice series. Why did it never get taken? It seems to do
the right things quite correctly.

Daniel, while this isn't a perfect solution, is this something you'd
use in graphics-land?

-Kees

-- 
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Mon, Apr 30, 2018 at 11:29:04PM +0200, Rasmus Villemoes wrote:
> On 2018-04-30 22:16, Matthew Wilcox wrote:
> > On Mon, Apr 30, 2018 at 12:02:14PM -0700, Kees Cook wrote:
> >> (I just wish C had a sensible way to catch overflow...)
> > 
> > Every CPU I ever worked with had an "overflow" bit ... do we have a
> > friend on the C standards ctte who might figure out a way to let us
> > write code that checks it?
> 
> gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should
> generate reasonable code. Too bad there's no completely generic
> check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's
> hard to define what they should be checked against - probably would
> require all subexpressions (including the variables themselves) to have
> the same type.

Nevertheless these generate much better code than our current safeguards!

extern void *malloc(unsigned long);

#define ULONG_MAX (~0UL)
#define SZ  8UL

void *a(unsigned long a)
{
if ((ULONG_MAX / SZ) > a)
return 0;
return malloc(a * SZ);
}

void *b(unsigned long a)
{
unsigned long c;
if (__builtin_mul_overflow(a, SZ, ))
return 0;
return malloc(c);
}

(a lot of code uses a constant '8' as sizeof(void *)).  Here's the
difference with gcc 7.3:

   0:   48 b8 fe ff ff ff ffmovabs $0x1ffe,%rax
   7:   ff ff 1f 
   a:   48 39 c7cmp%rax,%rdi
   d:   76 09   jbe18 
   f:   48 c1 e7 03 shl$0x3,%rdi
  13:   e9 00 00 00 00  jmpq   18 
14: R_X86_64_PLT32  malloc-0x4
  18:   31 c0   xor%eax,%eax
  1a:   c3  retq   

vs

  20:   48 89 f8mov%rdi,%rax
  23:   ba 08 00 00 00  mov$0x8,%edx
  28:   48 f7 e2mul%rdx
  2b:   48 89 c7mov%rax,%rdi
  2e:   70 05   jo 35 
  30:   e9 00 00 00 00  jmpq   35 
31: R_X86_64_PLT32  malloc-0x4
  35:   31 c0   xor%eax,%eax
  37:   c3  retq   

We've traded a shl for a mul (because shl doesn't set Overflow, only
Carry, and that's only bit 65, not an OR of bits 35-n), but we lose the
movabs and cmp.  I'd rather run the second code fragment than the first.
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Kees Cook]

On Mon, Apr 30, 2018 at 1:16 PM, Matthew Wilcox  wrote:
> On Mon, Apr 30, 2018 at 12:02:14PM -0700, Kees Cook wrote:
>> For any longer multiplications, I've only found[1]:
>>
>> drivers/staging/rtl8188eu/os_dep/osdep_service.c:   void **a =
>> kzalloc(h * sizeof(void *) + h * w * size, GFP_KERNEL);
>
> That's pretty good, although it's just an atrocious vendor driver and
> it turns out all of those things are constants, and it'd be far better
> off with just declaring an array.  I bet they used to declare one on
> the stack ...

Yeah, it was just a quick hack to look for stuff.

>
>> At the end of the day, though, I don't really like having all these
>> different names...
>>
>> kmalloc(), kmalloc_array(), kmalloc_ab_c(), kmalloc_array_3d()
>>
>> with their "matching" zeroing function:
>>
>> kzalloc(), kcalloc(), kzalloc_ab_c(), kmalloc_array_3d(..., gfp | __GFP_ZERO)
>
> Yes, it's not very regular.
>
>> For the multiplication cases, I wonder if we could just have:
>>
>> kmalloc_multN(gfp, a, b, c, ...)
>> kzalloc_multN(gfp, a, b, c, ...)
>>
>> and we can replace all kcalloc() users with kzalloc_mult2(), all
>> kmalloc_array() users with kmalloc_mult2(), the abc uses with
>> kmalloc_mult3().
>
> I'm reluctant to do away with kcalloc() as it has the obvious heritage
> from user-space calloc() with the addition of GFP flags.

But it encourages misuse with calloc(N * M, gfp) ... if we removed
calloc and kept k[mz]alloc_something(gfp, a, b, c...) I think we'd
have better adoption.

>> That said, I *do* like kmalloc_struct() as it's a very common pattern...
>
> Thanks!  And way harder to misuse than kmalloc_ab_c().

Yes, quite so. It's really why I went with kmalloc_array_3d(), but now
I'm thinking better of it...

>> Or maybe, just leave the pattern in the name? kmalloc_ab(),
>> kmalloc_abc(), kmalloc_ab_c(), kmalloc_ab_cd() ?
>>
>> Getting the constant ordering right could be part of the macro
>> definition, maybe? i.e.:
>>
>> static inline void *kmalloc_ab(size_t a, size_t b, gfp_t flags)
>> {
>> if (__builtin_constant_p(a) && a != 0 && \
>> b > SIZE_MAX / a)
>> return NULL;
>> else if (__builtin_constant_p(b) && b != 0 && \
>>a > SIZE_MAX / b)
>> return NULL;
>>
>> return kmalloc(a * b, flags);
>> }
>
> Ooh, if neither a nor b is constant, it just didn't do a check ;-(  This
> stuff is hard.

Yup, quite true. Obviously not the final form. ;) I meant to
illustrate that we could do compile-time tricks to reorder the
division in an efficient manner.

>> (I just wish C had a sensible way to catch overflow...)
>
> Every CPU I ever worked with had an "overflow" bit ... do we have a
> friend on the C standards ctte who might figure out a way to let us
> write code that checks it?

On the CPU it's not retained across multiple calculations. And the
type matters too. This came up recently in a separate thread too:
http://openwall.com/lists/kernel-hardening/2018/03/26/4

>> [1] git grep -E 'alloc\([^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+,'
>
> I'm impressed, but it's not going to catch
>
> veryLongPointerNameThatsMeaningfulToMe = kmalloc(initialSize +
> numberOfEntries * entrySize + someOtherThing * yourMum,
> GFP_KERNEL);

Right, it wasn't meant to be exhaustive. I just included it in case
anyone wanted to go grepping around for themselves.

-Kees

-- 
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Rasmus Villemoes]

On 2018-04-30 22:16, Matthew Wilcox wrote:
> On Mon, Apr 30, 2018 at 12:02:14PM -0700, Kees Cook wrote:
>>
>> Getting the constant ordering right could be part of the macro
>> definition, maybe? i.e.:
>>
>> static inline void *kmalloc_ab(size_t a, size_t b, gfp_t flags)
>> {
>> if (__builtin_constant_p(a) && a != 0 && \
>> b > SIZE_MAX / a)
>> return NULL;
>> else if (__builtin_constant_p(b) && b != 0 && \
>>a > SIZE_MAX / b)
>> return NULL;
>>
>> return kmalloc(a * b, flags);
>> }
> 
> Ooh, if neither a nor b is constant, it just didn't do a check ;-(  This
> stuff is hard.
> 
>> (I just wish C had a sensible way to catch overflow...)
> 
> Every CPU I ever worked with had an "overflow" bit ... do we have a
> friend on the C standards ctte who might figure out a way to let us
> write code that checks it?

gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should
generate reasonable code. Too bad there's no completely generic
check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's
hard to define what they should be checked against - probably would
require all subexpressions (including the variables themselves) to have
the same type.

plug: https://lkml.org/lkml/2015/7/19/358

Rasmus

___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Mon, Apr 30, 2018 at 12:02:14PM -0700, Kees Cook wrote:
> On Sun, Apr 29, 2018 at 1:30 PM, Matthew Wilcox  wrote:
> > On Sun, Apr 29, 2018 at 09:59:27AM -0700, Kees Cook wrote:
> >> Did this ever happen?
> >
> > Not yet.  I brought it up at LSFMM, and I'll repost the patches soon.
> >
> >> I'd also like to see kmalloc_array_3d() or
> >> something that takes three size arguments. We have a lot of this
> >> pattern too:
> >>
> >> kmalloc(sizeof(foo) * A * B, gfp...)
> >>
> >> And we could turn that into:
> >>
> >> kmalloc_array_3d(sizeof(foo), A, B, gfp...)
> >
> > Are either of A or B constant?  Because if so, we could just use
> > kmalloc_array.  If not, then kmalloc_array_3d becomes a little more
> > expensive than kmalloc_array because we have to do a divide at runtime
> > instead of compile-time.  that's still better than allocating too few
> > bytes, of course.
> 
> Yeah, getting the order of the division is nice. Some thoughts below...
> 
> >
> > I'm wondering how far down the abc + ab + ac + bc + d rabbit-hole we're
> > going to end up going.  As far as we have to, I guess.
> 
> Well, the common patterns I've seen so far are:
> 
> a
> ab
> abc
> a + bc
> ab + cd
> 
> For any longer multiplications, I've only found[1]:
> 
> drivers/staging/rtl8188eu/os_dep/osdep_service.c:   void **a =
> kzalloc(h * sizeof(void *) + h * w * size, GFP_KERNEL);

That's pretty good, although it's just an atrocious vendor driver and
it turns out all of those things are constants, and it'd be far better
off with just declaring an array.  I bet they used to declare one on
the stack ...

> At the end of the day, though, I don't really like having all these
> different names...
> 
> kmalloc(), kmalloc_array(), kmalloc_ab_c(), kmalloc_array_3d()
> 
> with their "matching" zeroing function:
> 
> kzalloc(), kcalloc(), kzalloc_ab_c(), kmalloc_array_3d(..., gfp | __GFP_ZERO)

Yes, it's not very regular.

> For the multiplication cases, I wonder if we could just have:
> 
> kmalloc_multN(gfp, a, b, c, ...)
> kzalloc_multN(gfp, a, b, c, ...)
> 
> and we can replace all kcalloc() users with kzalloc_mult2(), all
> kmalloc_array() users with kmalloc_mult2(), the abc uses with
> kmalloc_mult3().

I'm reluctant to do away with kcalloc() as it has the obvious heritage
from user-space calloc() with the addition of GFP flags.

> That said, I *do* like kmalloc_struct() as it's a very common pattern...

Thanks!  And way harder to misuse than kmalloc_ab_c().

> Or maybe, just leave the pattern in the name? kmalloc_ab(),
> kmalloc_abc(), kmalloc_ab_c(), kmalloc_ab_cd() ?
> 
> Getting the constant ordering right could be part of the macro
> definition, maybe? i.e.:
> 
> static inline void *kmalloc_ab(size_t a, size_t b, gfp_t flags)
> {
> if (__builtin_constant_p(a) && a != 0 && \
> b > SIZE_MAX / a)
> return NULL;
> else if (__builtin_constant_p(b) && b != 0 && \
>a > SIZE_MAX / b)
> return NULL;
> 
> return kmalloc(a * b, flags);
> }

Ooh, if neither a nor b is constant, it just didn't do a check ;-(  This
stuff is hard.

> (I just wish C had a sensible way to catch overflow...)

Every CPU I ever worked with had an "overflow" bit ... do we have a
friend on the C standards ctte who might figure out a way to let us
write code that checks it?

> -Kees
> 
> [1] git grep -E 'alloc\([^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+,'

I'm impressed, but it's not going to catch

veryLongPointerNameThatsMeaningfulToMe = kmalloc(initialSize +
numberOfEntries * entrySize + someOtherThing * yourMum,
GFP_KERNEL);

___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Kees Cook]

On Sun, Apr 29, 2018 at 1:30 PM, Matthew Wilcox  wrote:
> On Sun, Apr 29, 2018 at 09:59:27AM -0700, Kees Cook wrote:
>> Did this ever happen?
>
> Not yet.  I brought it up at LSFMM, and I'll repost the patches soon.
>
>> I'd also like to see kmalloc_array_3d() or
>> something that takes three size arguments. We have a lot of this
>> pattern too:
>>
>> kmalloc(sizeof(foo) * A * B, gfp...)
>>
>> And we could turn that into:
>>
>> kmalloc_array_3d(sizeof(foo), A, B, gfp...)
>
> Are either of A or B constant?  Because if so, we could just use
> kmalloc_array.  If not, then kmalloc_array_3d becomes a little more
> expensive than kmalloc_array because we have to do a divide at runtime
> instead of compile-time.  that's still better than allocating too few
> bytes, of course.

Yeah, getting the order of the division is nice. Some thoughts below...

>
> I'm wondering how far down the abc + ab + ac + bc + d rabbit-hole we're
> going to end up going.  As far as we have to, I guess.

Well, the common patterns I've seen so far are:

a
ab
abc
a + bc
ab + cd

For any longer multiplications, I've only found[1]:

drivers/staging/rtl8188eu/os_dep/osdep_service.c:   void **a =
kzalloc(h * sizeof(void *) + h * w * size, GFP_KERNEL);


At the end of the day, though, I don't really like having all these
different names...

kmalloc(), kmalloc_array(), kmalloc_ab_c(), kmalloc_array_3d()

with their "matching" zeroing function:

kzalloc(), kcalloc(), kzalloc_ab_c(), kmalloc_array_3d(..., gfp | __GFP_ZERO)

For the multiplication cases, I wonder if we could just have:

kmalloc_multN(gfp, a, b, c, ...)
kzalloc_multN(gfp, a, b, c, ...)

and we can replace all kcalloc() users with kzalloc_mult2(), all
kmalloc_array() users with kmalloc_mult2(), the abc uses with
kmalloc_mult3().

That said, I *do* like kmalloc_struct() as it's a very common pattern...

Or maybe, just leave the pattern in the name? kmalloc_ab(),
kmalloc_abc(), kmalloc_ab_c(), kmalloc_ab_cd() ?

Getting the constant ordering right could be part of the macro
definition, maybe? i.e.:

static inline void *kmalloc_ab(size_t a, size_t b, gfp_t flags)
{
if (__builtin_constant_p(a) && a != 0 && \
b > SIZE_MAX / a)
return NULL;
else if (__builtin_constant_p(b) && b != 0 && \
   a > SIZE_MAX / b)
return NULL;

return kmalloc(a * b, flags);
}

(I just wish C had a sensible way to catch overflow...)

-Kees

[1] git grep -E 'alloc\([^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+,'

-- 
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Sun, Apr 29, 2018 at 09:59:27AM -0700, Kees Cook wrote:
> Did this ever happen?

Not yet.  I brought it up at LSFMM, and I'll repost the patches soon.

> I'd also like to see kmalloc_array_3d() or
> something that takes three size arguments. We have a lot of this
> pattern too:
> 
> kmalloc(sizeof(foo) * A * B, gfp...)
> 
> And we could turn that into:
> 
> kmalloc_array_3d(sizeof(foo), A, B, gfp...)

Are either of A or B constant?  Because if so, we could just use
kmalloc_array.  If not, then kmalloc_array_3d becomes a little more
expensive than kmalloc_array because we have to do a divide at runtime
instead of compile-time.  that's still better than allocating too few
bytes, of course.

I'm wondering how far down the abc + ab + ac + bc + d rabbit-hole we're
going to end up going.  As far as we have to, I guess.
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Tue, Mar 13, 2018 at 06:19:51PM +0100, Julia Lawall wrote:
> On Thu, 8 Mar 2018, Matthew Wilcox wrote:
> > On Thu, Mar 08, 2018 at 07:24:47AM +0100, Julia Lawall wrote:
> > > Thanks.  So it's OK to replace kmalloc and kzalloc, even though they
> > > didn't previously consider vmalloc and even though kmalloc doesn't zero?
> >
> > We'll also need to replace the corresponding places where those structs
> > are freed with kvfree().  Can coccinelle handle that too?
> 
> Is the use of vmalloc a necessary part of the design?  Or could there be a
> non vmalloc versions for call sites that are already ok with that?

We can also add kmalloc_struct() along with kmalloc_ab_c that won't fall
back to vmalloc but just return NULL.

___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Julia Lawall]

On Tue, 13 Mar 2018, Matthew Wilcox wrote:

> On Tue, Mar 13, 2018 at 06:19:51PM +0100, Julia Lawall wrote:
> > On Thu, 8 Mar 2018, Matthew Wilcox wrote:
> > > On Thu, Mar 08, 2018 at 07:24:47AM +0100, Julia Lawall wrote:
> > > > Thanks.  So it's OK to replace kmalloc and kzalloc, even though they
> > > > didn't previously consider vmalloc and even though kmalloc doesn't zero?
> > >
> > > We'll also need to replace the corresponding places where those structs
> > > are freed with kvfree().  Can coccinelle handle that too?
> >
> > Is the use of vmalloc a necessary part of the design?  Or could there be a
> > non vmalloc versions for call sites that are already ok with that?
>
> We can also add kmalloc_struct() along with kmalloc_ab_c that won't fall
> back to vmalloc but just return NULL.

It could be safer than being sure to find all of the relevant kfrees.

julia
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Julia Lawall]

On Thu, 8 Mar 2018, Matthew Wilcox wrote:

> On Thu, Mar 08, 2018 at 07:24:47AM +0100, Julia Lawall wrote:
> > On Wed, 7 Mar 2018, Matthew Wilcox wrote:
> > > On Wed, Mar 07, 2018 at 10:18:21PM +0100, Julia Lawall wrote:
> > > > > Otherwise, yes, please. We could build a coccinelle rule for
> > > > > additional replacements...
> > > >
> > > > A potential semantic patch and the changes it generates are attached
> > > > below.  Himanshu Jha helped with its development.  Working on this
> > > > uncovered one bug, where the allocated array is too large, because the
> > > > size provided for it was a structure size, but actually only pointers to
> > > > that structure were to be stored in it.
> > >
> > > This is cool!  Thanks for doing the coccinelle patch!  Diffstat:
> > >
> > >  50 files changed, 81 insertions(+), 124 deletions(-)
> > >
> > > I find that pretty compelling.  I'll repost the kvmalloc_struct patch
> > > imminently.
> >
> > Thanks.  So it's OK to replace kmalloc and kzalloc, even though they
> > didn't previously consider vmalloc and even though kmalloc doesn't zero?
>
> We'll also need to replace the corresponding places where those structs
> are freed with kvfree().  Can coccinelle handle that too?

Is the use of vmalloc a necessary part of the design?  Or could there be a
non vmalloc versions for call sites that are already ok with that?

julia

> > There are a few other cases that use GFP_NOFS and GFP_NOWAIT, but I didn't
> > transform those because the comment says that the flags should be
> > GFP_KERNEL based.  Should those be transformed too?
>
> The problem with non-GFP_KERNEL allocations is that vmalloc may have to
> allocate page tables, which is always done with an implicit GFP_KERNEL
> allocation.  There's an intent to get rid of GFP_NOFS, but that's not
> been realised yet (and I'm not sure of our strategy to eliminate it ...
> I'll send a separate email about that).  I'm not sure why anything's
> trying to allocate with GFP_NOWAIT; can you send a list of those places?
>
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Julia Lawall]

On Thu, 8 Mar 2018, Matthew Wilcox wrote:

> On Thu, Mar 08, 2018 at 07:24:47AM +0100, Julia Lawall wrote:
> > On Wed, 7 Mar 2018, Matthew Wilcox wrote:
> > > On Wed, Mar 07, 2018 at 10:18:21PM +0100, Julia Lawall wrote:
> > > > > Otherwise, yes, please. We could build a coccinelle rule for
> > > > > additional replacements...
> > > >
> > > > A potential semantic patch and the changes it generates are attached
> > > > below.  Himanshu Jha helped with its development.  Working on this
> > > > uncovered one bug, where the allocated array is too large, because the
> > > > size provided for it was a structure size, but actually only pointers to
> > > > that structure were to be stored in it.
> > >
> > > This is cool!  Thanks for doing the coccinelle patch!  Diffstat:
> > >
> > >  50 files changed, 81 insertions(+), 124 deletions(-)
> > >
> > > I find that pretty compelling.  I'll repost the kvmalloc_struct patch
> > > imminently.
> >
> > Thanks.  So it's OK to replace kmalloc and kzalloc, even though they
> > didn't previously consider vmalloc and even though kmalloc doesn't zero?
>
> We'll also need to replace the corresponding places where those structs
> are freed with kvfree().  Can coccinelle handle that too?

This would be harder to do 100% reliably.  Coccinelle would have to rely
on the structure name or the structure type, if the free is in a different
function.  But I guess that the type should be mostly reliable, since all
instances of allocations of the same type should be transformed in the
same way.

>
> > There are a few other cases that use GFP_NOFS and GFP_NOWAIT, but I didn't
> > transform those because the comment says that the flags should be
> > GFP_KERNEL based.  Should those be transformed too?
>
> The problem with non-GFP_KERNEL allocations is that vmalloc may have to
> allocate page tables, which is always done with an implicit GFP_KERNEL
> allocation.  There's an intent to get rid of GFP_NOFS, but that's not
> been realised yet (and I'm not sure of our strategy to eliminate it ...
> I'll send a separate email about that).  I'm not sure why anything's
> trying to allocate with GFP_NOWAIT; can you send a list of those places?

drivers/dma/fsl-edma.c:

fsl_desc = kzalloc(sizeof(*fsl_desc) + sizeof(struct fsl_edma_sw_tcd) * sg_len, 
GFP_NOWAIT);

drivers/dma/st_fdma.c:

fdesc = kzalloc(sizeof(*fdesc) + sizeof(struct st_fdma_sw_node) * sg_len,
GFP_NOWAIT);

drivers/dma/pxa_dma.c:

sw_desc = kzalloc(sizeof(*sw_desc) + nb_hw_desc * sizeof(struct
pxad_desc_hw *), GFP_NOWAIT);

julia
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Thu, Mar 08, 2018 at 07:24:47AM +0100, Julia Lawall wrote:
> On Wed, 7 Mar 2018, Matthew Wilcox wrote:
> > On Wed, Mar 07, 2018 at 10:18:21PM +0100, Julia Lawall wrote:
> > > > Otherwise, yes, please. We could build a coccinelle rule for
> > > > additional replacements...
> > >
> > > A potential semantic patch and the changes it generates are attached
> > > below.  Himanshu Jha helped with its development.  Working on this
> > > uncovered one bug, where the allocated array is too large, because the
> > > size provided for it was a structure size, but actually only pointers to
> > > that structure were to be stored in it.
> >
> > This is cool!  Thanks for doing the coccinelle patch!  Diffstat:
> >
> >  50 files changed, 81 insertions(+), 124 deletions(-)
> >
> > I find that pretty compelling.  I'll repost the kvmalloc_struct patch
> > imminently.
> 
> Thanks.  So it's OK to replace kmalloc and kzalloc, even though they
> didn't previously consider vmalloc and even though kmalloc doesn't zero?

We'll also need to replace the corresponding places where those structs
are freed with kvfree().  Can coccinelle handle that too?

> There are a few other cases that use GFP_NOFS and GFP_NOWAIT, but I didn't
> transform those because the comment says that the flags should be
> GFP_KERNEL based.  Should those be transformed too?

The problem with non-GFP_KERNEL allocations is that vmalloc may have to
allocate page tables, which is always done with an implicit GFP_KERNEL
allocation.  There's an intent to get rid of GFP_NOFS, but that's not
been realised yet (and I'm not sure of our strategy to eliminate it ...
I'll send a separate email about that).  I'm not sure why anything's
trying to allocate with GFP_NOWAIT; can you send a list of those places?
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Wed, Mar 07, 2018 at 10:18:21PM +0100, Julia Lawall wrote:
> > Otherwise, yes, please. We could build a coccinelle rule for
> > additional replacements...
> 
> A potential semantic patch and the changes it generates are attached
> below.  Himanshu Jha helped with its development.  Working on this
> uncovered one bug, where the allocated array is too large, because the
> size provided for it was a structure size, but actually only pointers to
> that structure were to be stored in it.

This is cool!  Thanks for doing the coccinelle patch!  Diffstat:

 50 files changed, 81 insertions(+), 124 deletions(-)

I find that pretty compelling.  I'll repost the kvmalloc_struct patch
imminently.
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Julia Lawall]

On Wed, 7 Mar 2018, Matthew Wilcox wrote:

> On Wed, Mar 07, 2018 at 10:18:21PM +0100, Julia Lawall wrote:
> > > Otherwise, yes, please. We could build a coccinelle rule for
> > > additional replacements...
> >
> > A potential semantic patch and the changes it generates are attached
> > below.  Himanshu Jha helped with its development.  Working on this
> > uncovered one bug, where the allocated array is too large, because the
> > size provided for it was a structure size, but actually only pointers to
> > that structure were to be stored in it.
>
> This is cool!  Thanks for doing the coccinelle patch!  Diffstat:
>
>  50 files changed, 81 insertions(+), 124 deletions(-)
>
> I find that pretty compelling.  I'll repost the kvmalloc_struct patch
> imminently.

Thanks.  So it's OK to replace kmalloc and kzalloc, even though they
didn't previously consider vmalloc and even though kmalloc doesn't zero?

There are a few other cases that use GFP_NOFS and GFP_NOWAIT, but I didn't
transform those because the comment says that the flags should be
GFP_KERNEL based.  Should those be transformed too?

julia
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Julia Lawall]

On Wed, 14 Feb 2018, Kees Cook wrote:

> On Wed, Feb 14, 2018 at 10:26 AM, Matthew Wilcox  wrote:
> > From: Matthew Wilcox 
> >
> > We have kvmalloc_array in order to safely allocate an array with a
> > number of elements specified by userspace (avoiding arithmetic overflow
> > leading to a buffer overrun).  But it's fairly common to have a header
> > in front of that array (eg specifying the length of the array), so we
> > need a helper function for that situation.
> >
> > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> > of our best efforts to name the arguments, it's really hard to remember
> > which order to put the arguments in.  kvzalloc_struct() eliminates that
> > effort; you tell it about the struct you're allocating, and it puts the
> > arguments in the right order for you (and checks that the arguments
> > you've given are at least plausible).
> >
> > For comparison between the three schemes:
> >
> > sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> > GFP_KERNEL);
> > sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> > GFP_KERNEL);
> > sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
> >
> > Signed-off-by: Matthew Wilcox 
> > ---
> >  include/linux/mm.h | 51 +++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 81bd7f0be286..ddf929c5aaee 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -557,6 +557,57 @@ static inline void *kvmalloc_array(size_t n, size_t 
> > size, gfp_t flags)
> > return kvmalloc(n * size, flags);
> >  }
> >
> > +/**
> > + * kvmalloc_ab_c() - Allocate memory.
>
> Longer description, maybe? "Allocate a *b + c bytes of memory"?
>
> > + * @n: Number of elements.
> > + * @size: Size of each element (should be constant).
> > + * @c: Size of header (should be constant).
>
> If these should be constant, should we mark them as "const"? Or WARN
> if __builtin_constant_p() isn't true?
>
> > + * @gfp: Memory allocation flags.
> > + *
> > + * Use this function to allocate @n * @size + @c bytes of memory.  This
> > + * function is safe to use when @n is controlled from userspace; it will
> > + * return %NULL if the required amount of memory cannot be allocated.
> > + * Use kvfree() to free the allocated memory.
> > + *
> > + * The kvzalloc_hdr_arr() function is easier to use as it has typechecking
>
> renaming typo? Should this be "kvzalloc_struct()"?
>
> > + * and you do not need to remember which of the arguments should be 
> > constants.
> > + *
> > + * Context: Process context.  May sleep; the @gfp flags should be based on
> > + * %GFP_KERNEL.
> > + * Return: A pointer to the allocated memory or %NULL.
> > + */
> > +static inline __must_check
> > +void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
> > +{
> > +   if (size != 0 && n > (SIZE_MAX - c) / size)
> > +   return NULL;
> > +
> > +   return kvmalloc(n * size + c, gfp);
> > +}
> > +#define kvzalloc_ab_c(a, b, c, gfp)kvmalloc_ab_c(a, b, c, gfp | 
> > __GFP_ZERO)
>
> Nit: "(gfp) | __GFP_ZERO" just in case of insane usage.
>
> > +
> > +/**
> > + * kvzalloc_struct() - Allocate and zero-fill a structure containing a
> > + *variable length array.
> > + * @p: Pointer to the structure.
> > + * @member: Name of the array member.
> > + * @n: Number of elements in the array.
> > + * @gfp: Memory allocation flags.
> > + *
> > + * Allocate (and zero-fill) enough memory for a structure with an array
> > + * of @n elements.  This function is safe to use when @n is specified by
> > + * userspace as the arithmetic will not overflow.
> > + * Use kvfree() to free the allocated memory.
> > + *
> > + * Context: Process context.  May sleep; the @gfp flags should be based on
> > + * %GFP_KERNEL.
> > + * Return: Zero-filled memory or a NULL pointer.
> > + */
> > +#define kvzalloc_struct(p, member, n, gfp) \
> > +   (typeof(p))kvzalloc_ab_c(n, \
> > +   sizeof(*(p)->member) + __must_be_array((p)->member),\
> > +   offsetof(typeof(*(p)), member), gfp)
> > +
> >  extern void kvfree(const void *addr);
> >
> >  static inline atomic_t *compound_mapcount_ptr(struct page *page)
>
> It might be nice to include another patch that replaces some of the
> existing/common uses of a*b+c with the new function...
>
> Otherwise, yes, please. We could build a coccinelle rule for
> additional replacements...

A potential semantic patch and the changes it generates are attached
below.  Himanshu Jha helped with its development.  Working on this
uncovered one bug, where the allocated array is too large, because the
size provided for it was a structure size, but 

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Matthew Wilcox]

On Wed, Feb 14, 2018 at 11:22:38AM -0800, Kees Cook wrote:
> > +/**
> > + * kvmalloc_ab_c() - Allocate memory.
> 
> Longer description, maybe? "Allocate a *b + c bytes of memory"?

Done!

> > + * @n: Number of elements.
> > + * @size: Size of each element (should be constant).
> > + * @c: Size of header (should be constant).
> 
> If these should be constant, should we mark them as "const"? Or WARN
> if __builtin_constant_p() isn't true?

It's only less efficient if they're not const.  Theoretically they could be
variable ... and I've been bitten by __builtin_constant_p() recently
(gcc bug 83653 which I still don't really understand).

> > + * @gfp: Memory allocation flags.
> > + *
> > + * Use this function to allocate @n * @size + @c bytes of memory.  This
> > + * function is safe to use when @n is controlled from userspace; it will
> > + * return %NULL if the required amount of memory cannot be allocated.
> > + * Use kvfree() to free the allocated memory.
> > + *
> > + * The kvzalloc_hdr_arr() function is easier to use as it has typechecking
> 
> renaming typo? Should this be "kvzalloc_struct()"?

Urgh, yes.  I swear I searched for it ... must've typoed my search string.
Anyway, fixed, because kvzalloc_hdr_arr() wasn't a good name.

> > +#define kvzalloc_ab_c(a, b, c, gfp)kvmalloc_ab_c(a, b, c, gfp | 
> > __GFP_ZERO)
> 
> Nit: "(gfp) | __GFP_ZERO" just in case of insane usage.

Fixed!

> It might be nice to include another patch that replaces some of the
> existing/common uses of a*b+c with the new function...

Sure!  I have a few examples in my tree, I just didn't want to complicate
things by sending a patch that crossed dozens of maintainer trees.

___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Julia Lawall]

On Wed, 14 Feb 2018, Kees Cook wrote:

> On Wed, Feb 14, 2018 at 10:26 AM, Matthew Wilcox  wrote:
> > From: Matthew Wilcox 
> >
> > We have kvmalloc_array in order to safely allocate an array with a
> > number of elements specified by userspace (avoiding arithmetic overflow
> > leading to a buffer overrun).  But it's fairly common to have a header
> > in front of that array (eg specifying the length of the array), so we
> > need a helper function for that situation.
> >
> > kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> > of our best efforts to name the arguments, it's really hard to remember
> > which order to put the arguments in.  kvzalloc_struct() eliminates that
> > effort; you tell it about the struct you're allocating, and it puts the
> > arguments in the right order for you (and checks that the arguments
> > you've given are at least plausible).
> >
> > For comparison between the three schemes:
> >
> > sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> > GFP_KERNEL);
> > sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> > GFP_KERNEL);
> > sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
> >
> > Signed-off-by: Matthew Wilcox 
> > ---
> >  include/linux/mm.h | 51 +++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 81bd7f0be286..ddf929c5aaee 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -557,6 +557,57 @@ static inline void *kvmalloc_array(size_t n, size_t 
> > size, gfp_t flags)
> > return kvmalloc(n * size, flags);
> >  }
> >
> > +/**
> > + * kvmalloc_ab_c() - Allocate memory.
>
> Longer description, maybe? "Allocate a *b + c bytes of memory"?
>
> > + * @n: Number of elements.
> > + * @size: Size of each element (should be constant).
> > + * @c: Size of header (should be constant).
>
> If these should be constant, should we mark them as "const"? Or WARN
> if __builtin_constant_p() isn't true?
>
> > + * @gfp: Memory allocation flags.
> > + *
> > + * Use this function to allocate @n * @size + @c bytes of memory.  This
> > + * function is safe to use when @n is controlled from userspace; it will
> > + * return %NULL if the required amount of memory cannot be allocated.
> > + * Use kvfree() to free the allocated memory.
> > + *
> > + * The kvzalloc_hdr_arr() function is easier to use as it has typechecking
>
> renaming typo? Should this be "kvzalloc_struct()"?
>
> > + * and you do not need to remember which of the arguments should be 
> > constants.
> > + *
> > + * Context: Process context.  May sleep; the @gfp flags should be based on
> > + * %GFP_KERNEL.
> > + * Return: A pointer to the allocated memory or %NULL.
> > + */
> > +static inline __must_check
> > +void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
> > +{
> > +   if (size != 0 && n > (SIZE_MAX - c) / size)
> > +   return NULL;
> > +
> > +   return kvmalloc(n * size + c, gfp);
> > +}
> > +#define kvzalloc_ab_c(a, b, c, gfp)kvmalloc_ab_c(a, b, c, gfp | 
> > __GFP_ZERO)
>
> Nit: "(gfp) | __GFP_ZERO" just in case of insane usage.
>
> > +
> > +/**
> > + * kvzalloc_struct() - Allocate and zero-fill a structure containing a
> > + *variable length array.
> > + * @p: Pointer to the structure.
> > + * @member: Name of the array member.
> > + * @n: Number of elements in the array.
> > + * @gfp: Memory allocation flags.
> > + *
> > + * Allocate (and zero-fill) enough memory for a structure with an array
> > + * of @n elements.  This function is safe to use when @n is specified by
> > + * userspace as the arithmetic will not overflow.
> > + * Use kvfree() to free the allocated memory.
> > + *
> > + * Context: Process context.  May sleep; the @gfp flags should be based on
> > + * %GFP_KERNEL.
> > + * Return: Zero-filled memory or a NULL pointer.
> > + */
> > +#define kvzalloc_struct(p, member, n, gfp) \
> > +   (typeof(p))kvzalloc_ab_c(n, \
> > +   sizeof(*(p)->member) + __must_be_array((p)->member),\
> > +   offsetof(typeof(*(p)), member), gfp)
> > +
> >  extern void kvfree(const void *addr);
> >
> >  static inline atomic_t *compound_mapcount_ptr(struct page *page)
>
> It might be nice to include another patch that replaces some of the
> existing/common uses of a*b+c with the new function...
>
> Otherwise, yes, please. We could build a coccinelle rule for
> additional replacements...

Thanks for the suggestion.  I will look into it.

julia
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct

[Kees Cook]

On Wed, Feb 14, 2018 at 10:26 AM, Matthew Wilcox  wrote:
> From: Matthew Wilcox 
>
> We have kvmalloc_array in order to safely allocate an array with a
> number of elements specified by userspace (avoiding arithmetic overflow
> leading to a buffer overrun).  But it's fairly common to have a header
> in front of that array (eg specifying the length of the array), so we
> need a helper function for that situation.
>
> kvmalloc_ab_c() is the workhorse that does the calculation, but in spite
> of our best efforts to name the arguments, it's really hard to remember
> which order to put the arguments in.  kvzalloc_struct() eliminates that
> effort; you tell it about the struct you're allocating, and it puts the
> arguments in the right order for you (and checks that the arguments
> you've given are at least plausible).
>
> For comparison between the three schemes:
>
> sev = kvzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems,
> GFP_KERNEL);
> sev = kvzalloc_ab_c(elems, sizeof(struct v4l2_kevent), sizeof(*sev),
> GFP_KERNEL);
> sev = kvzalloc_struct(sev, events, elems, GFP_KERNEL);
>
> Signed-off-by: Matthew Wilcox 
> ---
>  include/linux/mm.h | 51 +++
>  1 file changed, 51 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 81bd7f0be286..ddf929c5aaee 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -557,6 +557,57 @@ static inline void *kvmalloc_array(size_t n, size_t 
> size, gfp_t flags)
> return kvmalloc(n * size, flags);
>  }
>
> +/**
> + * kvmalloc_ab_c() - Allocate memory.

Longer description, maybe? "Allocate a *b + c bytes of memory"?

> + * @n: Number of elements.
> + * @size: Size of each element (should be constant).
> + * @c: Size of header (should be constant).

If these should be constant, should we mark them as "const"? Or WARN
if __builtin_constant_p() isn't true?

> + * @gfp: Memory allocation flags.
> + *
> + * Use this function to allocate @n * @size + @c bytes of memory.  This
> + * function is safe to use when @n is controlled from userspace; it will
> + * return %NULL if the required amount of memory cannot be allocated.
> + * Use kvfree() to free the allocated memory.
> + *
> + * The kvzalloc_hdr_arr() function is easier to use as it has typechecking

renaming typo? Should this be "kvzalloc_struct()"?

> + * and you do not need to remember which of the arguments should be 
> constants.
> + *
> + * Context: Process context.  May sleep; the @gfp flags should be based on
> + * %GFP_KERNEL.
> + * Return: A pointer to the allocated memory or %NULL.
> + */
> +static inline __must_check
> +void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
> +{
> +   if (size != 0 && n > (SIZE_MAX - c) / size)
> +   return NULL;
> +
> +   return kvmalloc(n * size + c, gfp);
> +}
> +#define kvzalloc_ab_c(a, b, c, gfp)kvmalloc_ab_c(a, b, c, gfp | 
> __GFP_ZERO)

Nit: "(gfp) | __GFP_ZERO" just in case of insane usage.

> +
> +/**
> + * kvzalloc_struct() - Allocate and zero-fill a structure containing a
> + *variable length array.
> + * @p: Pointer to the structure.
> + * @member: Name of the array member.
> + * @n: Number of elements in the array.
> + * @gfp: Memory allocation flags.
> + *
> + * Allocate (and zero-fill) enough memory for a structure with an array
> + * of @n elements.  This function is safe to use when @n is specified by
> + * userspace as the arithmetic will not overflow.
> + * Use kvfree() to free the allocated memory.
> + *
> + * Context: Process context.  May sleep; the @gfp flags should be based on
> + * %GFP_KERNEL.
> + * Return: Zero-filled memory or a NULL pointer.
> + */
> +#define kvzalloc_struct(p, member, n, gfp) \
> +   (typeof(p))kvzalloc_ab_c(n, \
> +   sizeof(*(p)->member) + __must_be_array((p)->member),\
> +   offsetof(typeof(*(p)), member), gfp)
> +
>  extern void kvfree(const void *addr);
>
>  static inline atomic_t *compound_mapcount_ptr(struct page *page)

It might be nice to include another patch that replaces some of the
existing/common uses of a*b+c with the new function...

Otherwise, yes, please. We could build a coccinelle rule for
additional replacements...

-Kees

-- 
Kees Cook
Pixel Security
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci