Re: [PATCH 0/3 v2] linux-next: mm: Track genalloc allocations

[Igor Stoppa]

On 03/05/18 01:50, Andrew Morton wrote:

On Wed,  2 May 2018 05:05:19 +0400 Igor Stoppa  wrote:


This patchset was created as part of an older version of pmalloc, however
it has value per-se, as it hardens the memory management for the generic
allocator genalloc.

Genalloc does not currently track the size of the allocations it hands
out.

Either by mistake, or due to an attack, it is possible that more memory
than what was initially allocated is freed, leaving behind dangling
pointers, ready for an use-after-free attack.

With this patch, genalloc becomes capable of tracking the size of each
allocation it has handed out, when it's time to free it.

It can either verify that the size received is correct, when free is
invoked, or it can decide autonomously how much memory to free, if the
value received for the size parameter is 0.

These patches are proposed for beign merged into linux-next, to verify
that they do not introduce regressions, by comparing the value received
from the callers of the free function with the internal tracking.

For this reason, the patchset does not contain the removal of the size
parameter from users of the free() function.

Later on, the "size" parameter can be dropped, and each caller can be
adjusted accordingly.

However, I do not have access to most of the HW required for confirming
that all of its users are not negatively affected.
This is where I believe having the patches in linux-next would help to
coordinate with the maintaiers of the code that uses gen_alloc.

Since there were comments about the (lack-of) efficiency introduced by
this patchset, I have added some more explanations and calculations to the
description of the first patch, the one adding the bitmap.
My conclusion is that this patch should not cause any major perfomance
problem.

Regarding the possibility of completely changing genalloc into some other
type of allocator, I think it should not be a showstopper for this
patchset, which aims to plug a security hole in genalloc, without
introducing any major regression.

The security flaw is clear and present, while the benefit of introducing a
new allocator is not clear, at least for the current users of genalloc.

And anyway the users of genalloc should be fixed to not pass any size
parameter, which can be done after this patch is merged.

A newer, more efficient allocator will still benefit from not receiving a
spurious parameter (size), when freeing memory.

...

  Documentation/core-api/genalloc.rst |   4 +
  include/linux/genalloc.h| 112 +++---
  lib/Kconfig.debug   |  23 ++
  lib/Makefile|   1 +
  lib/genalloc.c  | 742 ++--
  lib/test_genalloc.c | 419 


That's a big patch,


True, but I am afraid I do not see how to split it further without 
braking bisection.


 and I'm having trouble believing that it's

justified?  We're trying to reduce the harm in bugs (none of which are
known to exist) in a small number of drivers to avoid exploits, none of
which are known to exist and which may not even be possible.


Should I create one, to justify the patch?
Maybe, what we are really discussing if security should be reactive or 
preventive. And what amount of extra complexity is acceptable, without a 
current, present threat that has already materialized.


My personal take is, if I see something that I think I could exploit, 
most likely those who do write exploits for a (really well paid) living 
can do much more harm than I can even think of.



Or something like that.  Perhaps all this is taking defensiveness a bit
too far?


My main goal was to remove the "size" parameter from the free() call, 
without introducing noticeable performance regression.


Is that a reasonable endeavor?

After all, we have IOMMUs also for preventing similar types of attack.

The current users of genalloc are primarily:
* SRAM memory managers, which are attractive because they are used for 
example to store system wide state inbetween transitions to off, when 
some components (like the MMU) might not be even active.


* DMA page allocators, another nice side channel, where a DMA controller 
could be used to completely side-step the type of protection enforced by 
the MMU



And a bitmap is a pretty crappy way of managing memory anyway, surely?


I did not put it there :-P
It also depends what one needs it for and if it's good enough.
Or if something better is justified.


If this code is indeed performance-sensitive then perhaps a
reimplementation with some standard textbook allocator(?) is warranted?


But, is it really performance sensitive?

I might be wrong, but I think this change that I am proposing is not 
really affecting performance.


I did get a question/comment about performance implications.

I have explained why I think my patches are not adding any real 
performance problem, in the comment of the patch 

Re: [PATCH 0/3 v2] linux-next: mm: Track genalloc allocations

[Igor Stoppa]

On 03/05/18 01:50, Andrew Morton wrote:

On Wed,  2 May 2018 05:05:19 +0400 Igor Stoppa  wrote:


This patchset was created as part of an older version of pmalloc, however
it has value per-se, as it hardens the memory management for the generic
allocator genalloc.

Genalloc does not currently track the size of the allocations it hands
out.

Either by mistake, or due to an attack, it is possible that more memory
than what was initially allocated is freed, leaving behind dangling
pointers, ready for an use-after-free attack.

With this patch, genalloc becomes capable of tracking the size of each
allocation it has handed out, when it's time to free it.

It can either verify that the size received is correct, when free is
invoked, or it can decide autonomously how much memory to free, if the
value received for the size parameter is 0.

These patches are proposed for beign merged into linux-next, to verify
that they do not introduce regressions, by comparing the value received
from the callers of the free function with the internal tracking.

For this reason, the patchset does not contain the removal of the size
parameter from users of the free() function.

Later on, the "size" parameter can be dropped, and each caller can be
adjusted accordingly.

However, I do not have access to most of the HW required for confirming
that all of its users are not negatively affected.
This is where I believe having the patches in linux-next would help to
coordinate with the maintaiers of the code that uses gen_alloc.

Since there were comments about the (lack-of) efficiency introduced by
this patchset, I have added some more explanations and calculations to the
description of the first patch, the one adding the bitmap.
My conclusion is that this patch should not cause any major perfomance
problem.

Regarding the possibility of completely changing genalloc into some other
type of allocator, I think it should not be a showstopper for this
patchset, which aims to plug a security hole in genalloc, without
introducing any major regression.

The security flaw is clear and present, while the benefit of introducing a
new allocator is not clear, at least for the current users of genalloc.

And anyway the users of genalloc should be fixed to not pass any size
parameter, which can be done after this patch is merged.

A newer, more efficient allocator will still benefit from not receiving a
spurious parameter (size), when freeing memory.

...

  Documentation/core-api/genalloc.rst |   4 +
  include/linux/genalloc.h| 112 +++---
  lib/Kconfig.debug   |  23 ++
  lib/Makefile|   1 +
  lib/genalloc.c  | 742 ++--
  lib/test_genalloc.c | 419 


That's a big patch,


True, but I am afraid I do not see how to split it further without 
braking bisection.


 and I'm having trouble believing that it's

justified?  We're trying to reduce the harm in bugs (none of which are
known to exist) in a small number of drivers to avoid exploits, none of
which are known to exist and which may not even be possible.


Should I create one, to justify the patch?
Maybe, what we are really discussing if security should be reactive or 
preventive. And what amount of extra complexity is acceptable, without a 
current, present threat that has already materialized.


My personal take is, if I see something that I think I could exploit, 
most likely those who do write exploits for a (really well paid) living 
can do much more harm than I can even think of.



Or something like that.  Perhaps all this is taking defensiveness a bit
too far?


My main goal was to remove the "size" parameter from the free() call, 
without introducing noticeable performance regression.


Is that a reasonable endeavor?

After all, we have IOMMUs also for preventing similar types of attack.

The current users of genalloc are primarily:
* SRAM memory managers, which are attractive because they are used for 
example to store system wide state inbetween transitions to off, when 
some components (like the MMU) might not be even active.


* DMA page allocators, another nice side channel, where a DMA controller 
could be used to completely side-step the type of protection enforced by 
the MMU



And a bitmap is a pretty crappy way of managing memory anyway, surely?


I did not put it there :-P
It also depends what one needs it for and if it's good enough.
Or if something better is justified.


If this code is indeed performance-sensitive then perhaps a
reimplementation with some standard textbook allocator(?) is warranted?


But, is it really performance sensitive?

I might be wrong, but I think this change that I am proposing is not 
really affecting performance.


I did get a question/comment about performance implications.

I have explained why I think my patches are not adding any real 
performance problem, in the comment of the patch that does the actual 

Re: [PATCH 0/3 v2] linux-next: mm: Track genalloc allocations

[Andrew Morton]

On Wed,  2 May 2018 05:05:19 +0400 Igor Stoppa  wrote:

> This patchset was created as part of an older version of pmalloc, however
> it has value per-se, as it hardens the memory management for the generic
> allocator genalloc.
> 
> Genalloc does not currently track the size of the allocations it hands
> out.
> 
> Either by mistake, or due to an attack, it is possible that more memory
> than what was initially allocated is freed, leaving behind dangling
> pointers, ready for an use-after-free attack.
> 
> With this patch, genalloc becomes capable of tracking the size of each
> allocation it has handed out, when it's time to free it.
> 
> It can either verify that the size received is correct, when free is
> invoked, or it can decide autonomously how much memory to free, if the
> value received for the size parameter is 0.
> 
> These patches are proposed for beign merged into linux-next, to verify
> that they do not introduce regressions, by comparing the value received
> from the callers of the free function with the internal tracking.
> 
> For this reason, the patchset does not contain the removal of the size
> parameter from users of the free() function.
> 
> Later on, the "size" parameter can be dropped, and each caller can be
> adjusted accordingly.
> 
> However, I do not have access to most of the HW required for confirming
> that all of its users are not negatively affected.
> This is where I believe having the patches in linux-next would help to
> coordinate with the maintaiers of the code that uses gen_alloc.
> 
> Since there were comments about the (lack-of) efficiency introduced by
> this patchset, I have added some more explanations and calculations to the
> description of the first patch, the one adding the bitmap.
> My conclusion is that this patch should not cause any major perfomance
> problem.
> 
> Regarding the possibility of completely changing genalloc into some other
> type of allocator, I think it should not be a showstopper for this
> patchset, which aims to plug a security hole in genalloc, without
> introducing any major regression.
> 
> The security flaw is clear and present, while the benefit of introducing a
> new allocator is not clear, at least for the current users of genalloc.
> 
> And anyway the users of genalloc should be fixed to not pass any size
> parameter, which can be done after this patch is merged.
> 
> A newer, more efficient allocator will still benefit from not receiving a
> spurious parameter (size), when freeing memory.
> 
> ...
> 
>  Documentation/core-api/genalloc.rst |   4 +
>  include/linux/genalloc.h| 112 +++---
>  lib/Kconfig.debug   |  23 ++
>  lib/Makefile|   1 +
>  lib/genalloc.c  | 742 
> ++--
>  lib/test_genalloc.c | 419 

That's a big patch, and I'm having trouble believing that it's
justified?  We're trying to reduce the harm in bugs (none of which are
known to exist) in a small number of drivers to avoid exploits, none of
which are known to exist and which may not even be possible.

Or something like that.  Perhaps all this is taking defensiveness a bit
too far?

And a bitmap is a pretty crappy way of managing memory anyway, surely? 
If this code is indeed performance-sensitive then perhaps a
reimplementation with some standard textbook allocator(?) is warranted?

Re: [PATCH 0/3 v2] linux-next: mm: Track genalloc allocations

[Andrew Morton]

On Wed,  2 May 2018 05:05:19 +0400 Igor Stoppa  wrote:

> This patchset was created as part of an older version of pmalloc, however
> it has value per-se, as it hardens the memory management for the generic
> allocator genalloc.
> 
> Genalloc does not currently track the size of the allocations it hands
> out.
> 
> Either by mistake, or due to an attack, it is possible that more memory
> than what was initially allocated is freed, leaving behind dangling
> pointers, ready for an use-after-free attack.
> 
> With this patch, genalloc becomes capable of tracking the size of each
> allocation it has handed out, when it's time to free it.
> 
> It can either verify that the size received is correct, when free is
> invoked, or it can decide autonomously how much memory to free, if the
> value received for the size parameter is 0.
> 
> These patches are proposed for beign merged into linux-next, to verify
> that they do not introduce regressions, by comparing the value received
> from the callers of the free function with the internal tracking.
> 
> For this reason, the patchset does not contain the removal of the size
> parameter from users of the free() function.
> 
> Later on, the "size" parameter can be dropped, and each caller can be
> adjusted accordingly.
> 
> However, I do not have access to most of the HW required for confirming
> that all of its users are not negatively affected.
> This is where I believe having the patches in linux-next would help to
> coordinate with the maintaiers of the code that uses gen_alloc.
> 
> Since there were comments about the (lack-of) efficiency introduced by
> this patchset, I have added some more explanations and calculations to the
> description of the first patch, the one adding the bitmap.
> My conclusion is that this patch should not cause any major perfomance
> problem.
> 
> Regarding the possibility of completely changing genalloc into some other
> type of allocator, I think it should not be a showstopper for this
> patchset, which aims to plug a security hole in genalloc, without
> introducing any major regression.
> 
> The security flaw is clear and present, while the benefit of introducing a
> new allocator is not clear, at least for the current users of genalloc.
> 
> And anyway the users of genalloc should be fixed to not pass any size
> parameter, which can be done after this patch is merged.
> 
> A newer, more efficient allocator will still benefit from not receiving a
> spurious parameter (size), when freeing memory.
> 
> ...
> 
>  Documentation/core-api/genalloc.rst |   4 +
>  include/linux/genalloc.h| 112 +++---
>  lib/Kconfig.debug   |  23 ++
>  lib/Makefile|   1 +
>  lib/genalloc.c  | 742 
> ++--
>  lib/test_genalloc.c | 419 

That's a big patch, and I'm having trouble believing that it's
justified?  We're trying to reduce the harm in bugs (none of which are
known to exist) in a small number of drivers to avoid exploits, none of
which are known to exist and which may not even be possible.

Or something like that.  Perhaps all this is taking defensiveness a bit
too far?

And a bitmap is a pretty crappy way of managing memory anyway, surely? 
If this code is indeed performance-sensitive then perhaps a
reimplementation with some standard textbook allocator(?) is warranted?

[PATCH 0/3 v2] linux-next: mm: Track genalloc allocations

[Igor Stoppa]

This patchset was created as part of an older version of pmalloc, however
it has value per-se, as it hardens the memory management for the generic
allocator genalloc.

Genalloc does not currently track the size of the allocations it hands
out.

Either by mistake, or due to an attack, it is possible that more memory
than what was initially allocated is freed, leaving behind dangling
pointers, ready for an use-after-free attack.

With this patch, genalloc becomes capable of tracking the size of each
allocation it has handed out, when it's time to free it.

It can either verify that the size received is correct, when free is
invoked, or it can decide autonomously how much memory to free, if the
value received for the size parameter is 0.

These patches are proposed for beign merged into linux-next, to verify
that they do not introduce regressions, by comparing the value received
from the callers of the free function with the internal tracking.

For this reason, the patchset does not contain the removal of the size
parameter from users of the free() function.

Later on, the "size" parameter can be dropped, and each caller can be
adjusted accordingly.

However, I do not have access to most of the HW required for confirming
that all of its users are not negatively affected.
This is where I believe having the patches in linux-next would help to
coordinate with the maintaiers of the code that uses gen_alloc.

Since there were comments about the (lack-of) efficiency introduced by
this patchset, I have added some more explanations and calculations to the
description of the first patch, the one adding the bitmap.
My conclusion is that this patch should not cause any major perfomance
problem.

Regarding the possibility of completely changing genalloc into some other
type of allocator, I think it should not be a showstopper for this
patchset, which aims to plug a security hole in genalloc, without
introducing any major regression.

The security flaw is clear and present, while the benefit of introducing a
new allocator is not clear, at least for the current users of genalloc.

And anyway the users of genalloc should be fixed to not pass any size
parameter, which can be done after this patch is merged.

A newer, more efficient allocator will still benefit from not receiving a
spurious parameter (size), when freeing memory.

Changes since v1:

[http://www.openwall.com/lists/kernel-hardening/2018/04/29/1]

* make the tester code a kernel module
* turn selftest BUG() error exit paths into WARN()
* add analysis of impact on current users of genalloc


Igor Stoppa (3):
  genalloc: track beginning of allocations
  Add label and license to genalloc.rst
  genalloc: selftest

 Documentation/core-api/genalloc.rst |   4 +
 include/linux/genalloc.h| 112 +++---
 lib/Kconfig.debug   |  23 ++
 lib/Makefile|   1 +
 lib/genalloc.c  | 742 ++--
 lib/test_genalloc.c | 419 
 6 files changed, 1046 insertions(+), 255 deletions(-)
 create mode 100644 lib/test_genalloc.c

-- 
2.14.1

[PATCH 0/3 v2] linux-next: mm: Track genalloc allocations

[Igor Stoppa]

This patchset was created as part of an older version of pmalloc, however
it has value per-se, as it hardens the memory management for the generic
allocator genalloc.

Genalloc does not currently track the size of the allocations it hands
out.

Either by mistake, or due to an attack, it is possible that more memory
than what was initially allocated is freed, leaving behind dangling
pointers, ready for an use-after-free attack.

With this patch, genalloc becomes capable of tracking the size of each
allocation it has handed out, when it's time to free it.

It can either verify that the size received is correct, when free is
invoked, or it can decide autonomously how much memory to free, if the
value received for the size parameter is 0.

These patches are proposed for beign merged into linux-next, to verify
that they do not introduce regressions, by comparing the value received
from the callers of the free function with the internal tracking.

For this reason, the patchset does not contain the removal of the size
parameter from users of the free() function.

Later on, the "size" parameter can be dropped, and each caller can be
adjusted accordingly.

However, I do not have access to most of the HW required for confirming
that all of its users are not negatively affected.
This is where I believe having the patches in linux-next would help to
coordinate with the maintaiers of the code that uses gen_alloc.

Since there were comments about the (lack-of) efficiency introduced by
this patchset, I have added some more explanations and calculations to the
description of the first patch, the one adding the bitmap.
My conclusion is that this patch should not cause any major perfomance
problem.

Regarding the possibility of completely changing genalloc into some other
type of allocator, I think it should not be a showstopper for this
patchset, which aims to plug a security hole in genalloc, without
introducing any major regression.

The security flaw is clear and present, while the benefit of introducing a
new allocator is not clear, at least for the current users of genalloc.

And anyway the users of genalloc should be fixed to not pass any size
parameter, which can be done after this patch is merged.

A newer, more efficient allocator will still benefit from not receiving a
spurious parameter (size), when freeing memory.

Changes since v1:

[http://www.openwall.com/lists/kernel-hardening/2018/04/29/1]

* make the tester code a kernel module
* turn selftest BUG() error exit paths into WARN()
* add analysis of impact on current users of genalloc


Igor Stoppa (3):
  genalloc: track beginning of allocations
  Add label and license to genalloc.rst
  genalloc: selftest

 Documentation/core-api/genalloc.rst |   4 +
 include/linux/genalloc.h| 112 +++---
 lib/Kconfig.debug   |  23 ++
 lib/Makefile|   1 +
 lib/genalloc.c  | 742 ++--
 lib/test_genalloc.c | 419 
 6 files changed, 1046 insertions(+), 255 deletions(-)
 create mode 100644 lib/test_genalloc.c

-- 
2.14.1