Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Fri, May 4, 2018 at 8:35 AM, Linus Torvalds wrote: > On Fri, May 4, 2018 at 3:14 AM Matthew Wilcox wrote: > >> > In fact, the conversion I saw was buggy. You can *not* convert a > GFP_ATOMIC >> > user of kmalloc() to use kvmalloc. > >> Not sure which conversion you're referring to; not one of mine, I hope? > > I was thinking of the coccinelle patch in this thread, but just realized > that that actually only did it for GFP_KERNEL, so I guess it would work, > apart from the "oops, now it doesn't enforce the kmalloc limits any more" > issue. Just to be clear: the Coccinelle scripts I'm building aren't doing a kmalloc -> kvmalloc conversion. I'm just removing all the 2-factor multiplication and replacing it with the appropriate calls to the allocator family's *calloc or *alloc_array(). This will get us to the place where we can do all the sanity-checking in the allocator functions (whatever that checking ends up being). As it turns out, though, we have kind of a lot of allocator families. Some are wrappers, like devm_*alloc(), etc. All that said, the overwhelming majority of *alloc() multiplications are just "count * sizeof()". It really feels like everything should just be using a new *alloc_struct() which can do the type checking, etc, etc, but we can get there. The remaining "count * size" are a minority and could be dealt with some other way. >> > - that divide is really really expensive on many architectures. > >> 'c' and 'size' are _supposed_ to be constant and get evaluated at >> compile-time. ie you should get something like this on x86: > > I guess that willalways be true of the 'kvzalloc_struct() version that > will always use a sizeof(). I was more thinking of any bare kvalloc_ab_c() > cases, but maybe we'd discourage that to ever be used as such? Yeah, bare *alloc_ab_c() is not great. Perhaps a leading "__" can hint to that? > Because we definitely have things like that, ie a quick grep finds > > f = kmalloc (sizeof (*f) + size*num, GFP_KERNEL); > > where 'size' is not obviously a constant. There may be others, but I didn't > really try to grep any further. > > Maybe they aren't common, and maybe the occasional divide doesn't matter, > but particularly if we use scripting to then catch and convert users, I > really hate the idea of "let's introduce something that is potentially much > more expensive than it needs to be". Yup: I'm not after that either. I just want to be able to get at the multiplication factors before they're multiplied. :) > (And the automated coccinelle scripting it's also something where we must > very much avoid then subtly lifting allocation size limits) Agreed. I think most cases are already getting lifted to size_t due to the sizeof(). It's the "two variables" cases I want to double-check. Another completely insane idea would be to have a macro that did type size checking and would DTRT, but with all the alloc families, it looks nasty. This is all RFC stage, as far as I'm concerned. Fun example: devm_kzalloc(dev, sizeof(...) * num, gfp...) $ git grep 'devm_kzalloc([^,]*, *sizeof([^,]*,' | egrep '\* *sizeof|\) *\*' | wc -l 88 some are constants: drivers/video/fbdev/au1100fb.c: devm_kzalloc(&dev->dev, sizeof(u32) * 16, GFP_KERNEL); but many aren't: sound/soc/generic/audio-graph-card.c: dai_link = devm_kzalloc(dev, sizeof(*dai_link) * num, GFP_KERNEL); While type-checking on the non-sizeof factor would let us know if it was safe, so would the division, and most of those could happen at compile time. It's the size_t variables that we want to catch. So, mainly I'm just trying to get the arguments reordered (for a compile-time division) into the correct helpers so the existing logic can do the right thing, and only for 2-factor products. After that, then I'm hoping to tackle the multi-factor products, of which the *alloc_struct() helper seems to cover the vast majority of the remaining cases. -Kees -- Kees Cook Pixel Security
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Fri, May 4, 2018 at 3:14 AM Matthew Wilcox wrote: > > In fact, the conversion I saw was buggy. You can *not* convert a GFP_ATOMIC > > user of kmalloc() to use kvmalloc. > Not sure which conversion you're referring to; not one of mine, I hope? I was thinking of the coccinelle patch in this thread, but just realized that that actually only did it for GFP_KERNEL, so I guess it would work, apart from the "oops, now it doesn't enforce the kmalloc limits any more" issue. > > - that divide is really really expensive on many architectures. > 'c' and 'size' are _supposed_ to be constant and get evaluated at > compile-time. ie you should get something like this on x86: I guess that willalways be true of the 'kvzalloc_struct() version that will always use a sizeof(). I was more thinking of any bare kvalloc_ab_c() cases, but maybe we'd discourage that to ever be used as such? Because we definitely have things like that, ie a quick grep finds f = kmalloc (sizeof (*f) + size*num, GFP_KERNEL); where 'size' is not obviously a constant. There may be others, but I didn't really try to grep any further. Maybe they aren't common, and maybe the occasional divide doesn't matter, but particularly if we use scripting to then catch and convert users, I really hate the idea of "let's introduce something that is potentially much more expensive than it needs to be". (And the automated coccinelle scripting it's also something where we must very much avoid then subtly lifting allocation size limits) > > Something like > > > > if (size > PAGE_SIZE) > >return NULL; > > if (elem > 65535) > >return NULL; > > if (offset > PAGE_SIZE) > >return NULL; > > return kzalloc(size*elem+offset); > > > > and now you (a) guarantee it can't overflow and (b) don't make people use > > crazy vmalloc() allocations when they damn well shouldn't. > I find your faith in the size of structs in the kernel touching ;-) I *really* hope some of those examples of yours aren't allocated with kmalloc using this pattern.. But yes, I may be naive on the sizes. > We really have two reasons for using vmalloc -- one is "fragmentation > currently makes it impossible to allocate enough contiguous memory > to satisfy your needs" and the other is "this request is for too much > memory to satisfy through the buddy allocator". kvmalloc is normally > (not always; see file descriptor example above) for the first kind of > problem, but I wonder if kvmalloc() shouldn't have the same limit as > kmalloc (2048 pages), then add a kvmalloc_large() which will not impose > that limit check. That would definitely solve at least one worry. We do potentially have users which use kmalloc optimistically and do not want to fall back to vmalloc (they'll fall back to smaller allocations entirely), but I guess if fwe make sure to not convert any __GFP_NOWARN/NORETRY users, that should all be ok. But honestly, I'd rather see just kmalloc users stay as kmalloc users. If instread of "kzvmalloc_ab_c()" you introduce the "struct size calculation" part as a "saturating non-overflow calculation", then it should be fairly easy to just do #define kzalloc_struct(p, member, n, gfp) \ kzalloc(struct_size(p, member, n, gfp) and you basically *know* that the only thing you changed was purely the overflow semantics. That also would take care of my diide worry, because now there wouldn't be any ab_c() calculations that might take a non-constant size. The "struct_size()" thing would always do the sizeof. So you'd have something like /* 'b' and 'c' are always constants */ static inline sizeof_t __ab_c_saturating(size_t a, size_t b, size_t c) { if (b && n > (SIZE_MAX -c) / size) return SIZE_MAX; return a*b+c; } #define struct_size(p, member, n) \ __ab_c_saturating(n, \ sizeof(*(p)->member) + __must_be_array((p)->member), \ offsetof(typeof(*(p)), member)) and then that kzalloc_struct() wrapper define above should just work. The above is entirely untested, but it *looks* sane, and avoids all semantic changes outside of the overflow protection. And nobody would use that __ab_c_saturating() by mistake, I hope. No? Linus
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Fri, May 04, 2018 at 07:42:52AM +, Linus Torvalds wrote: > On Wed, Feb 14, 2018 at 8:27 AM Matthew Wilcox wrote: > > +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); > > Ok, so some more bikeshedding: I'm putting up the bikeshed against your house ... the colour is your choice! > - I really don't want to encourage people to use kvmalloc(). > > In fact, the conversion I saw was buggy. You can *not* convert a GFP_ATOMIC > user of kmalloc() to use kvmalloc. Not sure which conversion you're referring to; not one of mine, I hope? > - that divide is really really expensive on many architectures. 'c' and 'size' are _supposed_ to be constant and get evaluated at compile-time. ie you should get something like this on x86: 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 Now, if someone's an idiot, then you'll get the divide done at runtime, and that'll be expensive. > Normal kernel allocations are *limited*. It's simply not ok to allocate > megabytes (or gigabytes) of mmory in general. We have serious limits, and > we *should* have serious limits. If people worry about the multiply > overflowing because a user is controlling the size valus, then dammit, such > a user should not be able to do a huge gigabyte vmalloc() that exhausts > memory and then spends time clearing it all! I agree. > So the whole notion that "overflows are dangerous, let's get rid of them" > somehow fixes a bug is BULLSHIT. You literally introduced a *new* bug by > removing the normal kmalloc() size limit because you thought that pverflows > are the only problem. Rather, I replaced one bug with another. The removed bug was one where we allocated 24 bytes and then indexed into the next slab object. The added bug was that someone can now persuade the driver to allocate gigabytes of memory. It's a less severe bug, but I take your point. We do have _some_ limits in vmalloc -- we fail if you're trying to allocate more memory than is in the machine, and we fail if there's insufficient contiguous space in the virtual address space. But, yes, this does allow people to allocate more memory than kmalloc would allow. > So stop doing things like this. We should not do a stupid divide, because > we damn well know that it is NOT VALID to allocate arrays that have > hundreds of fthousands of elements, or where the size of one element is > very big. > > So get rid of the stupid divide, and make the limits be much stricter. Like > saying "the array element size had better be smaller than one page" > (because honestly, bigger elements are not valid in the kernel), and "the > size of the array cannot be more than "pick-some-number-out-of-your-ass". > > So just make the divide go the hell away, a and check the size for validity. > > Something like > > if (size > PAGE_SIZE) >return NULL; > if (elem > 65535) >return NULL; > if (offset > PAGE_SIZE) >return NULL; > return kzalloc(size*elem+offset); > > and now you (a) guarantee it can't overflow and (b) don't make people use > crazy vmalloc() allocations when they damn well shouldn't. I find your faith in the size of structs in the kernel touching ;-) struct cmp_data { /* size: 290904, cachelines: 4546, members: 11 */ struct dec_data { /* size: 274520, cachelines: 4290, members: 10 */ struct cpu_entry_area { /* size: 180224, cachelines: 2816, members: 7 */ struct saved_cmdlines_buffer { /* size: 131104, cachelines: 2049, members: 5 */ struct debug_store_buffers { /* size: 131072, cachelines: 2048, members: 2 */ struct bunzip_data { /* size: 42648, cachelines: 667, members: 23 */ struct inflate_workspace { /* size: 42312, cachelines: 662, members: 2 */ struct xz_dec_lzma2 { /* size: 28496, cachelines: 446, members: 5 */ struct lzma_dec { /* size: 28304, cachelines: 443, members: 21 */ struct rcu_state { /* size: 17344, cachelines: 271, members: 34 */ struct pglist_data { /* size: 18304, cachelines: 286, members: 34 */ struct tss_struct { /* size: 12288, cachelines: 192, members: 2 */ struct bts_ctx { /* size: 12288, cachelines: 192, members: 3 */ Those are just the ones above 10kB. Sure, I can see some of them are boot time use only, or we allocate one per node, or whatever. But people do create arrays of these things. The biggest object we have in the slab_cache t
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Wed, Feb 14, 2018 at 8:27 AM Matthew Wilcox wrote: > +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); Ok, so some more bikeshedding: - I really don't want to encourage people to use kvmalloc(). In fact, the conversion I saw was buggy. You can *not* convert a GFP_ATOMIC user of kmalloc() to use kvmalloc. - that divide is really really expensive on many architectures. Note that these issues kind of go hand in hand. Normal kernel allocations are *limited*. It's simply not ok to allocate megabytes (or gigabytes) of mmory in general. We have serious limits, and we *should* have serious limits. If people worry about the multiply overflowing because a user is controlling the size valus, then dammit, such a user should not be able to do a huge gigabyte vmalloc() that exhausts memory and then spends time clearing it all! So the whole notion that "overflows are dangerous, let's get rid of them" somehow fixes a bug is BULLSHIT. You literally introduced a *new* bug by removing the normal kmalloc() size limit because you thought that pverflows are the only problem. So stop doing things like this. We should not do a stupid divide, because we damn well know that it is NOT VALID to allocate arrays that have hundreds of fthousands of elements, or where the size of one element is very big. So get rid of the stupid divide, and make the limits be much stricter. Like saying "the array element size had better be smaller than one page" (because honestly, bigger elements are not valid in the kernel), and "the size of the array cannot be more than "pick-some-number-out-of-your-ass". So just make the divide go the hell away, a and check the size for validity. Something like if (size > PAGE_SIZE) return NULL; if (elem > 65535) return NULL; if (offset > PAGE_SIZE) return NULL; return kzalloc(size*elem+offset); and now you (a) guarantee it can't overflow and (b) don't make people use crazy vmalloc() allocations when they damn well shouldn't. And yeah, if somebody has a page size bigger than 64k, then the above can still overflow. I'm sorry, that architecture s broken shit. Are there cases where vmalloc() is ok? Yes. But they should be rare, and they should have a good reason for them. And honestly, even then the above limits really really sound quite reasonable. There is no excuse for million-entry arrays in the kernel. You are doing something seriously wrong if you do those. Linus
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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 = &exec->shader_state[exec->shader_state_count -
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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, &c)) 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.
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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);
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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.
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Tue, Mar 13, 2018 at 11:32 AM, 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. Did this ever happen? 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...) -Kees -- Kees Cook Pixel Security
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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.
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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? >
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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?
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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.
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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 actually only pointers to that structure were to be stored in it. Note t
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Thu, Feb 15, 2018 at 9:06 AM, Christopher Lameter wrote: > On Thu, 15 Feb 2018, Matthew Wilcox wrote: > >> I dunno. Yes, there's macro trickery going on here, but it certainly >> resembles a function. It doesn't fail any of the rules laid out in that >> chapter of coding-style about unacceptable uses of macros. > > It sure looks like a function but does magic things with the struct > parameter. So its not working like a function and the capitalization makes > one aware of that. I think readability trumps that -- nearly everything else in the kernel that hides these kinds of details is lower case. -Kees -- Kees Cook Pixel Security
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Thu, 15 Feb 2018, Matthew Wilcox wrote: > I dunno. Yes, there's macro trickery going on here, but it certainly > resembles a function. It doesn't fail any of the rules laid out in that > chapter of coding-style about unacceptable uses of macros. It sure looks like a function but does magic things with the struct parameter. So its not working like a function and the capitalization makes one aware of that.
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Thu, Feb 15, 2018 at 09:55:11AM -0600, Christopher Lameter wrote: > On Wed, 14 Feb 2018, Matthew Wilcox wrote: > > > > Uppercase like the similar KMEM_CACHE related macros in > > > include/linux/slab.h?> > > > > Do you think that would look better in the users? Compare: > > Does looking matter? I thought we had the convention that macros are > uppercase. There are some tricks going on with the struct. Uppercase shows > that something special is going on. 12) Macros, Enums and RTL - Names of macros defining constants and labels in enums are capitalized. .. code-block:: c #define CONSTANT 0x12345 Enums are preferred when defining several related constants. CAPITALIZED macro names are appreciated but macros resembling functions may be named in lower case. I dunno. Yes, there's macro trickery going on here, but it certainly resembles a function. It doesn't fail any of the rules laid out in that chapter of coding-style about unacceptable uses of macros.
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Wed, 14 Feb 2018, Matthew Wilcox wrote: > > Uppercase like the similar KMEM_CACHE related macros in > > include/linux/slab.h?> > > Do you think that would look better in the users? Compare: Does looking matter? I thought we had the convention that macros are uppercase. There are some tricks going on with the struct. Uppercase shows that something special is going on. > Making it look like a function is more pleasing to my eye, but I'll > change it if that's the only thing keeping it from being merged. This should be consistent throughout the source.
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Wed, Feb 14, 2018 at 01:55:59PM -0600, Christopher Lameter wrote: > On Wed, 14 Feb 2018, Matthew Wilcox wrote: > > > +#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) > > + > > Uppercase like the similar KMEM_CACHE related macros in > include/linux/slab.h?> Do you think that would look better in the users? Compare: @@ -1284,7 +1284,7 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) return -EOPNOTSUPP; if (mem.nregions > max_mem_regions) return -E2BIG; - newmem = kvzalloc(size + mem.nregions * sizeof(*m->regions), GFP_KERNEL); + newmem = kvzalloc_struct(newmem, regions, mem.nregions, GFP_KERNEL); if (!newmem) return -ENOMEM; @@ -1284,7 +1284,7 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m) return -EOPNOTSUPP; if (mem.nregions > max_mem_regions) return -E2BIG; - newmem = kvzalloc(size + mem.nregions * sizeof(*m->regions), GFP_KERNEL); + newmem = KVZALLOC_STRUCT(newmem, regions, mem.nregions, GFP_KERNEL); if (!newmem) return -ENOMEM; Making it look like a function is more pleasing to my eye, but I'll change it if that's the only thing keeping it from being merged.
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
On Wed, 14 Feb 2018, Matthew Wilcox wrote: > +#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) > + Uppercase like the similar KMEM_CACHE related macros in include/linux/slab.h?>
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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.
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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
Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct
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