Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Igor Stoppa]

On 21/02/18 23:36, Dave Chinner wrote:
> On Wed, Feb 21, 2018 at 11:56:22AM +0200, Igor Stoppa wrote:

[...]

> It seems lots of people get confused when discussing concepts vs
> implementation... :)

IMHO, if possible, it's better to use unambiguous terms at every point.
__ro_after_init is already taken :-P

In this specific case, I wanted to be absolutely sure I understood
correctly what you need. I think I have now, thanks.

>> is this something that is readonly from the beginning and then shared
>> among mount points or is it specific to each mount point?
> 
> It's dynamically allocated for each mount point, made read-only
> before the mount completes and lives for the length of the mount
> point.

ok. And destroyed when the mount point is unmounted, I expect.

[...]

>> The "const" modifier is a nice way to catch errors through the compiler,
>> iff the ro data will not be initialized through this handle, when it's
>> still writable.
> 
> That's kinda implied by the const, isn't it? If we don't do it that
> way, then the compiler will throw errors


I might be splitting the hair, but since I'm advertising something I
worte, I don't want to look like a peddler of snake oil, in hindsight :-P

To clarify my previous comment:

* const can mean the world to the compiler, but that doesn't
automatically translate into write-protected memory, yet I do appreciate
the advantage of teaching the compiler what should not be altered.
And I have nothing against doing it.

* even if some handle will be const, it still needs to be aliased to
some other pointer that is not const, at the beginning, because it must
be initialized and it's anyway writable. So, this cannot be avoided.

--
igor

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Igor Stoppa]

On 21/02/18 23:36, Dave Chinner wrote:
> On Wed, Feb 21, 2018 at 11:56:22AM +0200, Igor Stoppa wrote:

[...]

> It seems lots of people get confused when discussing concepts vs
> implementation... :)

IMHO, if possible, it's better to use unambiguous terms at every point.
__ro_after_init is already taken :-P

In this specific case, I wanted to be absolutely sure I understood
correctly what you need. I think I have now, thanks.

>> is this something that is readonly from the beginning and then shared
>> among mount points or is it specific to each mount point?
> 
> It's dynamically allocated for each mount point, made read-only
> before the mount completes and lives for the length of the mount
> point.

ok. And destroyed when the mount point is unmounted, I expect.

[...]

>> The "const" modifier is a nice way to catch errors through the compiler,
>> iff the ro data will not be initialized through this handle, when it's
>> still writable.
> 
> That's kinda implied by the const, isn't it? If we don't do it that
> way, then the compiler will throw errors


I might be splitting the hair, but since I'm advertising something I
worte, I don't want to look like a peddler of snake oil, in hindsight :-P

To clarify my previous comment:

* const can mean the world to the compiler, but that doesn't
automatically translate into write-protected memory, yet I do appreciate
the advantage of teaching the compiler what should not be altered.
And I have nothing against doing it.

* even if some handle will be const, it still needs to be aliased to
some other pointer that is not const, at the beginning, because it must
be initialized and it's anyway writable. So, this cannot be avoided.

--
igor

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Dave Chinner]

On Wed, Feb 21, 2018 at 11:56:22AM +0200, Igor Stoppa wrote:
> On 21/02/18 03:36, Dave Chinner wrote:
> > On Tue, Feb 20, 2018 at 03:56:00PM -0800, Matthew Wilcox wrote:
> >> On Wed, Feb 21, 2018 at 08:36:04AM +1100, Dave Chinner wrote:
> >>> FWIW, I'm not wanting to use it to replace static variables. All the
> >>> structures are dynamically allocated right now, and get assigned to
> >>> other dynamically allocated pointers. I'd likely split the current
> >>> structures into a "ro after init" 
> 
> I would prefer to use a different terminology, because, if I have
> understood the use case, this is not exactly the same as __ro_after_init

I want a dynamically allocated "write once" structure.

A "write once" structure is, conceptually, is exactly the same as
"ro after init". Implementation wise, it may be different to
"__ro_after_init", especially when compared to static/global
variables.

It seems lots of people get confused when discussing concepts vs
implementation... :)

> >>>   ..
> >>
> >> No, you'd do:
> >>
> >> struct xfs_mount_ro {
> >>[...]
> >> };
> 
> is this something that is readonly from the beginning and then shared
> among mount points or is it specific to each mount point?

It's dynamically allocated for each mount point, made read-only
before the mount completes and lives for the length of the mount
point.

> >> struct xfs_mount {
> >>const struct xfs_mount_ro *ro;
> >>[...]
> >> };
> > 
> >  so that's pretty much the same thing :P
> 
> The "const" modifier is a nice way to catch errors through the compiler,
> iff the ro data will not be initialized through this handle, when it's
> still writable.

That's kinda implied by the const, isn't it? If we don't do it that
way, then the compiler will throw errors

Cheers,

Dave.
-- 
Dave Chinner
dchin...@redhat.com

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Dave Chinner]

On Wed, Feb 21, 2018 at 11:56:22AM +0200, Igor Stoppa wrote:
> On 21/02/18 03:36, Dave Chinner wrote:
> > On Tue, Feb 20, 2018 at 03:56:00PM -0800, Matthew Wilcox wrote:
> >> On Wed, Feb 21, 2018 at 08:36:04AM +1100, Dave Chinner wrote:
> >>> FWIW, I'm not wanting to use it to replace static variables. All the
> >>> structures are dynamically allocated right now, and get assigned to
> >>> other dynamically allocated pointers. I'd likely split the current
> >>> structures into a "ro after init" 
> 
> I would prefer to use a different terminology, because, if I have
> understood the use case, this is not exactly the same as __ro_after_init

I want a dynamically allocated "write once" structure.

A "write once" structure is, conceptually, is exactly the same as
"ro after init". Implementation wise, it may be different to
"__ro_after_init", especially when compared to static/global
variables.

It seems lots of people get confused when discussing concepts vs
implementation... :)

> >>>   ..
> >>
> >> No, you'd do:
> >>
> >> struct xfs_mount_ro {
> >>[...]
> >> };
> 
> is this something that is readonly from the beginning and then shared
> among mount points or is it specific to each mount point?

It's dynamically allocated for each mount point, made read-only
before the mount completes and lives for the length of the mount
point.

> >> struct xfs_mount {
> >>const struct xfs_mount_ro *ro;
> >>[...]
> >> };
> > 
> >  so that's pretty much the same thing :P
> 
> The "const" modifier is a nice way to catch errors through the compiler,
> iff the ro data will not be initialized through this handle, when it's
> still writable.

That's kinda implied by the const, isn't it? If we don't do it that
way, then the compiler will throw errors

Cheers,

Dave.
-- 
Dave Chinner
dchin...@redhat.com

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Igor Stoppa]

On 21/02/18 03:36, Dave Chinner wrote:
> On Tue, Feb 20, 2018 at 03:56:00PM -0800, Matthew Wilcox wrote:
>> On Wed, Feb 21, 2018 at 08:36:04AM +1100, Dave Chinner wrote:
>>> FWIW, I'm not wanting to use it to replace static variables. All the
>>> structures are dynamically allocated right now, and get assigned to
>>> other dynamically allocated pointers. I'd likely split the current
>>> structures into a "ro after init" 

I would prefer to use a different terminology, because, if I have
understood the use case, this is not exactly the same as __ro_after_init

So, this is my understanding:

* "const" needs to be known at link time - there might be some
adjustments later on, ex: patching of "const" pointers, after relocation
has taken place - I am assuming we are not planning to patch const data
The compiler can perform whatever optimization it feels like and it is
allowed to do, on this.

* __ro_after_init is almost the same as a const, from a r/w perspective,
but it will become effectively read_only after the completion of the
init phase. The compiler cannot use it in any way to detect errors,
AFAIK. The system will just generate a runtime error is someone tries to
alter some __ro_after_init data, when it's read-only.
The only trick available is to use, after the protection, a different
type of handle, const.

* pmalloc pools can be protected (hence the "p") at any time, but they
start as r/w. Also, they cannot be declared statically.

* data which is either const or __ro_after_init is placed into specific
sections (on arm64 it's actually the same) and its pages are then marked
as read-only.

>>> structure and rw structure, so
>>> how does the "__ro_after_init" attribute work in that case? Is it
>>> something like this?
>>>
>>> struct xfs_mount {
>>> struct xfs_mount_ro{
>>> ...
>>> } *ro __ro_after_init;
>
> 
> pointer, not embedded structure

I doubt this would work, because I think it's not possible to put a
field of a structure into a separate section, afaik.

__ro_after_init would refer to the ro field, not to the memory it refers to.

>>> ..
>>
>> No, you'd do:
>>
>> struct xfs_mount_ro {
>>  [...]
>> };

is this something that is readonly from the beginning and then shared
among mount points or is it specific to each mount point?

>> struct xfs_mount {
>>  const struct xfs_mount_ro *ro;
>>  [...]
>> };
> 
>  so that's pretty much the same thing :P

The "const" modifier is a nice way to catch errors through the compiler,
iff the ro data will not be initialized through this handle, when it's
still writable.

>>> Also, what compile time checks are in place to catch writes to
>>> ro structure members? Is sparse going to be able to check this sort
>>> of thing, like is does with endian-specific variables?
>>
>> Just labelling the pointer const should be enough for the compiler to
>> catch unintended writes.
> 
> Ok.

yes, anyway the first one trying to alter it at run time, is in for some
surprise.

 I'd be interested to have your review of the pmalloc API, if you think
 something is missing, once I send out the next revision.
>>>
>>> I'll look at it in more depth when it comes past again. :P
>>
>> I think the key question is whether you want a slab-style interface
>> or whether you want a kmalloc-style interface.  I'd been assuming
>> the former, but Igor has implemented the latter already.
> 
> Slabs are rally only useful when you have lots of a specific type of
> object. I'm concerned mostly about one-off per-mount point
> structures, of which there are relatively few. A heap-like pool per
> mount is fine for this.

That was my same sentiment.

Actually it would be even possible to simulate caches with pools: each
pool supports a granularity parameter, during creation. One could have
multiple pools, each with different granularity, but it would probably
lead to a proliferation of pools.

Instead, I preferred to have pmalloc as a drop-in replacement for the
variants of k/v/kv malloc.

The only real issue was the - previous - inability of tracking the size
of an allocation, given its address, but that is taken care of by the
patch for the genalloc bitmap.

If I could have a pointer to a good candidate for the pmalloc treatment,
I could come up with a patch, to show how it could be done.

Then it might be easier to discuss if the API needs to be modified
and/or extended somehow.

--
igor

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Igor Stoppa]

On 21/02/18 03:36, Dave Chinner wrote:
> On Tue, Feb 20, 2018 at 03:56:00PM -0800, Matthew Wilcox wrote:
>> On Wed, Feb 21, 2018 at 08:36:04AM +1100, Dave Chinner wrote:
>>> FWIW, I'm not wanting to use it to replace static variables. All the
>>> structures are dynamically allocated right now, and get assigned to
>>> other dynamically allocated pointers. I'd likely split the current
>>> structures into a "ro after init" 

I would prefer to use a different terminology, because, if I have
understood the use case, this is not exactly the same as __ro_after_init

So, this is my understanding:

* "const" needs to be known at link time - there might be some
adjustments later on, ex: patching of "const" pointers, after relocation
has taken place - I am assuming we are not planning to patch const data
The compiler can perform whatever optimization it feels like and it is
allowed to do, on this.

* __ro_after_init is almost the same as a const, from a r/w perspective,
but it will become effectively read_only after the completion of the
init phase. The compiler cannot use it in any way to detect errors,
AFAIK. The system will just generate a runtime error is someone tries to
alter some __ro_after_init data, when it's read-only.
The only trick available is to use, after the protection, a different
type of handle, const.

* pmalloc pools can be protected (hence the "p") at any time, but they
start as r/w. Also, they cannot be declared statically.

* data which is either const or __ro_after_init is placed into specific
sections (on arm64 it's actually the same) and its pages are then marked
as read-only.

>>> structure and rw structure, so
>>> how does the "__ro_after_init" attribute work in that case? Is it
>>> something like this?
>>>
>>> struct xfs_mount {
>>> struct xfs_mount_ro{
>>> ...
>>> } *ro __ro_after_init;
>
> 
> pointer, not embedded structure

I doubt this would work, because I think it's not possible to put a
field of a structure into a separate section, afaik.

__ro_after_init would refer to the ro field, not to the memory it refers to.

>>> ..
>>
>> No, you'd do:
>>
>> struct xfs_mount_ro {
>>  [...]
>> };

is this something that is readonly from the beginning and then shared
among mount points or is it specific to each mount point?

>> struct xfs_mount {
>>  const struct xfs_mount_ro *ro;
>>  [...]
>> };
> 
>  so that's pretty much the same thing :P

The "const" modifier is a nice way to catch errors through the compiler,
iff the ro data will not be initialized through this handle, when it's
still writable.

>>> Also, what compile time checks are in place to catch writes to
>>> ro structure members? Is sparse going to be able to check this sort
>>> of thing, like is does with endian-specific variables?
>>
>> Just labelling the pointer const should be enough for the compiler to
>> catch unintended writes.
> 
> Ok.

yes, anyway the first one trying to alter it at run time, is in for some
surprise.

 I'd be interested to have your review of the pmalloc API, if you think
 something is missing, once I send out the next revision.
>>>
>>> I'll look at it in more depth when it comes past again. :P
>>
>> I think the key question is whether you want a slab-style interface
>> or whether you want a kmalloc-style interface.  I'd been assuming
>> the former, but Igor has implemented the latter already.
> 
> Slabs are rally only useful when you have lots of a specific type of
> object. I'm concerned mostly about one-off per-mount point
> structures, of which there are relatively few. A heap-like pool per
> mount is fine for this.

That was my same sentiment.

Actually it would be even possible to simulate caches with pools: each
pool supports a granularity parameter, during creation. One could have
multiple pools, each with different granularity, but it would probably
lead to a proliferation of pools.

Instead, I preferred to have pmalloc as a drop-in replacement for the
variants of k/v/kv malloc.

The only real issue was the - previous - inability of tracking the size
of an allocation, given its address, but that is taken care of by the
patch for the genalloc bitmap.

If I could have a pointer to a good candidate for the pmalloc treatment,
I could come up with a patch, to show how it could be done.

Then it might be easier to discuss if the API needs to be modified
and/or extended somehow.

--
igor

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Dave Chinner]

On Tue, Feb 20, 2018 at 03:56:00PM -0800, Matthew Wilcox wrote:
> On Wed, Feb 21, 2018 at 08:36:04AM +1100, Dave Chinner wrote:
> > FWIW, I'm not wanting to use it to replace static variables. All the
> > structures are dynamically allocated right now, and get assigned to
> > other dynamically allocated pointers. I'd likely split the current
> > structures into a "ro after init" structure and rw structure, so
> > how does the "__ro_after_init" attribute work in that case? Is it
> > something like this?
> > 
> > struct xfs_mount {
> > struct xfs_mount_ro{
> > ...
> > } *ro __ro_after_init;
   

pointer, not embedded structure

> > ..
> 
> No, you'd do:
> 
> struct xfs_mount_ro {
>   [...]
> };
> 
> struct xfs_mount {
>   const struct xfs_mount_ro *ro;
>   [...]
> };

 so that's pretty much the same thing :P

> > Also, what compile time checks are in place to catch writes to
> > ro structure members? Is sparse going to be able to check this sort
> > of thing, like is does with endian-specific variables?
> 
> Just labelling the pointer const should be enough for the compiler to
> catch unintended writes.

Ok.

> > > I'd be interested to have your review of the pmalloc API, if you think
> > > something is missing, once I send out the next revision.
> > 
> > I'll look at it in more depth when it comes past again. :P
> 
> I think the key question is whether you want a slab-style interface
> or whether you want a kmalloc-style interface.  I'd been assuming
> the former, but Igor has implemented the latter already.

Slabs are rally only useful when you have lots of a specific type of
object. I'm concerned mostly about one-off per-mount point
structures, of which there are relatively few. A heap-like pool per
mount is fine for this.

Cheers,

Dave.
-- 
Dave Chinner
dchin...@redhat.com

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Dave Chinner]

On Tue, Feb 20, 2018 at 03:56:00PM -0800, Matthew Wilcox wrote:
> On Wed, Feb 21, 2018 at 08:36:04AM +1100, Dave Chinner wrote:
> > FWIW, I'm not wanting to use it to replace static variables. All the
> > structures are dynamically allocated right now, and get assigned to
> > other dynamically allocated pointers. I'd likely split the current
> > structures into a "ro after init" structure and rw structure, so
> > how does the "__ro_after_init" attribute work in that case? Is it
> > something like this?
> > 
> > struct xfs_mount {
> > struct xfs_mount_ro{
> > ...
> > } *ro __ro_after_init;
   

pointer, not embedded structure

> > ..
> 
> No, you'd do:
> 
> struct xfs_mount_ro {
>   [...]
> };
> 
> struct xfs_mount {
>   const struct xfs_mount_ro *ro;
>   [...]
> };

 so that's pretty much the same thing :P

> > Also, what compile time checks are in place to catch writes to
> > ro structure members? Is sparse going to be able to check this sort
> > of thing, like is does with endian-specific variables?
> 
> Just labelling the pointer const should be enough for the compiler to
> catch unintended writes.

Ok.

> > > I'd be interested to have your review of the pmalloc API, if you think
> > > something is missing, once I send out the next revision.
> > 
> > I'll look at it in more depth when it comes past again. :P
> 
> I think the key question is whether you want a slab-style interface
> or whether you want a kmalloc-style interface.  I'd been assuming
> the former, but Igor has implemented the latter already.

Slabs are rally only useful when you have lots of a specific type of
object. I'm concerned mostly about one-off per-mount point
structures, of which there are relatively few. A heap-like pool per
mount is fine for this.

Cheers,

Dave.
-- 
Dave Chinner
dchin...@redhat.com

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Matthew Wilcox]

On Wed, Feb 21, 2018 at 08:36:04AM +1100, Dave Chinner wrote:
> FWIW, I'm not wanting to use it to replace static variables. All the
> structures are dynamically allocated right now, and get assigned to
> other dynamically allocated pointers. I'd likely split the current
> structures into a "ro after init" structure and rw structure, so
> how does the "__ro_after_init" attribute work in that case? Is it
> something like this?
> 
> struct xfs_mount {
>   struct xfs_mount_ro{
>   ...
>   } *ro __ro_after_init;
>   ..

No, you'd do:

struct xfs_mount_ro {
[...]
};

struct xfs_mount {
const struct xfs_mount_ro *ro;
[...]
};

We can't do protection on less than a page boundary, so you can't embed
a ro struct inside a rw struct.

> Also, what compile time checks are in place to catch writes to
> ro structure members? Is sparse going to be able to check this sort
> of thing, like is does with endian-specific variables?

Just labelling the pointer const should be enough for the compiler to
catch unintended writes.

> > I'd be interested to have your review of the pmalloc API, if you think
> > something is missing, once I send out the next revision.
> 
> I'll look at it in more depth when it comes past again. :P

I think the key question is whether you want a slab-style interface
or whether you want a kmalloc-style interface.  I'd been assuming
the former, but Igor has implemented the latter already.

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Matthew Wilcox]

On Wed, Feb 21, 2018 at 08:36:04AM +1100, Dave Chinner wrote:
> FWIW, I'm not wanting to use it to replace static variables. All the
> structures are dynamically allocated right now, and get assigned to
> other dynamically allocated pointers. I'd likely split the current
> structures into a "ro after init" structure and rw structure, so
> how does the "__ro_after_init" attribute work in that case? Is it
> something like this?
> 
> struct xfs_mount {
>   struct xfs_mount_ro{
>   ...
>   } *ro __ro_after_init;
>   ..

No, you'd do:

struct xfs_mount_ro {
[...]
};

struct xfs_mount {
const struct xfs_mount_ro *ro;
[...]
};

We can't do protection on less than a page boundary, so you can't embed
a ro struct inside a rw struct.

> Also, what compile time checks are in place to catch writes to
> ro structure members? Is sparse going to be able to check this sort
> of thing, like is does with endian-specific variables?

Just labelling the pointer const should be enough for the compiler to
catch unintended writes.

> > I'd be interested to have your review of the pmalloc API, if you think
> > something is missing, once I send out the next revision.
> 
> I'll look at it in more depth when it comes past again. :P

I think the key question is whether you want a slab-style interface
or whether you want a kmalloc-style interface.  I'd been assuming
the former, but Igor has implemented the latter already.

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Dave Chinner]

On Tue, Feb 20, 2018 at 08:03:49PM +0200, Igor Stoppa wrote:
> 
> 
> On 20/02/18 03:21, Dave Chinner wrote:
> > On Mon, Feb 12, 2018 at 03:32:36PM -0800, Kees Cook wrote:
> >> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  
> >> wrote:
> >>> This patch-set introduces the possibility of protecting memory that has
> >>> been allocated dynamically.
> >>>
> >>> The memory is managed in pools: when a memory pool is turned into R/O,
> >>> all the memory that is part of it, will become R/O.
> >>>
> >>> A R/O pool can be destroyed, to recover its memory, but it cannot be
> >>> turned back into R/W mode.
> >>>
> >>> This is intentional. This feature is meant for data that doesn't need
> >>> further modifications after initialization.
> >>
> >> This series came up in discussions with Dave Chinner (and Matthew
> >> Wilcox, already part of the discussion, and others) at LCA. I wonder
> >> if XFS would make a good initial user of this, as it could allocate
> >> all the function pointers and other const information about a
> >> superblock in pmalloc(), keeping it separate from the R/W portions?
> >> Could other filesystems do similar things?
> > 
> > I wasn't cc'd on this patchset, (please use da...@fromorbit.com for
> > future postings) 
> 
> Apologies, somehow I didn't realize that I should have put you too in
> CC. It will be fixed at the next iteration.

No worries, If you weren't at LCA, you wouldn't have known :P

> > so I can't really say anything about it right
> > now. My interest for XFS was that we have a fair amount of static
> > data in XFS that we set up at mount time and it never gets modified
> > after that.
> 
> This is the typical use case I had in mind, although it requires a
> conversion.
> Ex:
> 
> before:
> 
> static int a;
> 
> 
> void set_a(void)
> {
>   a = 4;
> }
> 
> 
> 
> after:
> 
> static int *a __ro_after_init;
> struct gen_pool *pool;
> 
> void init_a(void)
> {
>   pool = pmalloc_create_pool("pool", 0);
>   a = (int *)pmalloc(pool, sizeof(int), GFP_KERNEL);
> }
> 
> void set_a(void)
> {
>   *a = 4;
>   pmalloc_protect_pool(pool);
> }

Yeah, that's kinda what I figured. I'm guessing that I treat a pool
just like a normal heap allocation, but then when all the objects I
need to allocate are fully initialised, then I write protect the
pool? i.e. the API isn't using an object-based protection scope?

FWIW, I'm not wanting to use it to replace static variables. All the
structures are dynamically allocated right now, and get assigned to
other dynamically allocated pointers. I'd likely split the current
structures into a "ro after init" structure and rw structure, so
how does the "__ro_after_init" attribute work in that case? Is it
something like this?

struct xfs_mount {
struct xfs_mount_ro{
...
} *ro __ro_after_init;
..

Also, what compile time checks are in place to catch writes to
ro structure members? Is sparse going to be able to check this sort
of thing, like is does with endian-specific variables?

> I'd be interested to have your review of the pmalloc API, if you think
> something is missing, once I send out the next revision.

I'll look at it in more depth when it comes past again. :P

Cheers,

Dave.
-- 
Dave Chinner
dchin...@redhat.com

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Dave Chinner]

On Tue, Feb 20, 2018 at 08:03:49PM +0200, Igor Stoppa wrote:
> 
> 
> On 20/02/18 03:21, Dave Chinner wrote:
> > On Mon, Feb 12, 2018 at 03:32:36PM -0800, Kees Cook wrote:
> >> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  
> >> wrote:
> >>> This patch-set introduces the possibility of protecting memory that has
> >>> been allocated dynamically.
> >>>
> >>> The memory is managed in pools: when a memory pool is turned into R/O,
> >>> all the memory that is part of it, will become R/O.
> >>>
> >>> A R/O pool can be destroyed, to recover its memory, but it cannot be
> >>> turned back into R/W mode.
> >>>
> >>> This is intentional. This feature is meant for data that doesn't need
> >>> further modifications after initialization.
> >>
> >> This series came up in discussions with Dave Chinner (and Matthew
> >> Wilcox, already part of the discussion, and others) at LCA. I wonder
> >> if XFS would make a good initial user of this, as it could allocate
> >> all the function pointers and other const information about a
> >> superblock in pmalloc(), keeping it separate from the R/W portions?
> >> Could other filesystems do similar things?
> > 
> > I wasn't cc'd on this patchset, (please use da...@fromorbit.com for
> > future postings) 
> 
> Apologies, somehow I didn't realize that I should have put you too in
> CC. It will be fixed at the next iteration.

No worries, If you weren't at LCA, you wouldn't have known :P

> > so I can't really say anything about it right
> > now. My interest for XFS was that we have a fair amount of static
> > data in XFS that we set up at mount time and it never gets modified
> > after that.
> 
> This is the typical use case I had in mind, although it requires a
> conversion.
> Ex:
> 
> before:
> 
> static int a;
> 
> 
> void set_a(void)
> {
>   a = 4;
> }
> 
> 
> 
> after:
> 
> static int *a __ro_after_init;
> struct gen_pool *pool;
> 
> void init_a(void)
> {
>   pool = pmalloc_create_pool("pool", 0);
>   a = (int *)pmalloc(pool, sizeof(int), GFP_KERNEL);
> }
> 
> void set_a(void)
> {
>   *a = 4;
>   pmalloc_protect_pool(pool);
> }

Yeah, that's kinda what I figured. I'm guessing that I treat a pool
just like a normal heap allocation, but then when all the objects I
need to allocate are fully initialised, then I write protect the
pool? i.e. the API isn't using an object-based protection scope?

FWIW, I'm not wanting to use it to replace static variables. All the
structures are dynamically allocated right now, and get assigned to
other dynamically allocated pointers. I'd likely split the current
structures into a "ro after init" structure and rw structure, so
how does the "__ro_after_init" attribute work in that case? Is it
something like this?

struct xfs_mount {
struct xfs_mount_ro{
...
} *ro __ro_after_init;
..

Also, what compile time checks are in place to catch writes to
ro structure members? Is sparse going to be able to check this sort
of thing, like is does with endian-specific variables?

> I'd be interested to have your review of the pmalloc API, if you think
> something is missing, once I send out the next revision.

I'll look at it in more depth when it comes past again. :P

Cheers,

Dave.
-- 
Dave Chinner
dchin...@redhat.com

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Igor Stoppa]

On 20/02/18 03:21, Dave Chinner wrote:
> On Mon, Feb 12, 2018 at 03:32:36PM -0800, Kees Cook wrote:
>> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:
>>> This patch-set introduces the possibility of protecting memory that has
>>> been allocated dynamically.
>>>
>>> The memory is managed in pools: when a memory pool is turned into R/O,
>>> all the memory that is part of it, will become R/O.
>>>
>>> A R/O pool can be destroyed, to recover its memory, but it cannot be
>>> turned back into R/W mode.
>>>
>>> This is intentional. This feature is meant for data that doesn't need
>>> further modifications after initialization.
>>
>> This series came up in discussions with Dave Chinner (and Matthew
>> Wilcox, already part of the discussion, and others) at LCA. I wonder
>> if XFS would make a good initial user of this, as it could allocate
>> all the function pointers and other const information about a
>> superblock in pmalloc(), keeping it separate from the R/W portions?
>> Could other filesystems do similar things?
> 
> I wasn't cc'd on this patchset, (please use da...@fromorbit.com for
> future postings) 

Apologies, somehow I didn't realize that I should have put you too in
CC. It will be fixed at the next iteration.

> so I can't really say anything about it right
> now. My interest for XFS was that we have a fair amount of static
> data in XFS that we set up at mount time and it never gets modified
> after that.

This is the typical use case I had in mind, although it requires a
conversion.
Ex:

before:

static int a;


void set_a(void)
{
a = 4;
}



after:

static int *a __ro_after_init;
struct gen_pool *pool;

void init_a(void)
{
pool = pmalloc_create_pool("pool", 0);
a = (int *)pmalloc(pool, sizeof(int), GFP_KERNEL);
}

void set_a(void)
{
*a = 4;
pmalloc_protect_pool(pool);
}

> I'm not so worried about VFS level objects (that's a
> much more complex issue) but there is a lot of low hanging fruit in
> the XFS structures we could convert to write-once structures.

I'd be interested to have your review of the pmalloc API, if you think
something is missing, once I send out the next revision.

--
igor

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Igor Stoppa]

On 20/02/18 03:21, Dave Chinner wrote:
> On Mon, Feb 12, 2018 at 03:32:36PM -0800, Kees Cook wrote:
>> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:
>>> This patch-set introduces the possibility of protecting memory that has
>>> been allocated dynamically.
>>>
>>> The memory is managed in pools: when a memory pool is turned into R/O,
>>> all the memory that is part of it, will become R/O.
>>>
>>> A R/O pool can be destroyed, to recover its memory, but it cannot be
>>> turned back into R/W mode.
>>>
>>> This is intentional. This feature is meant for data that doesn't need
>>> further modifications after initialization.
>>
>> This series came up in discussions with Dave Chinner (and Matthew
>> Wilcox, already part of the discussion, and others) at LCA. I wonder
>> if XFS would make a good initial user of this, as it could allocate
>> all the function pointers and other const information about a
>> superblock in pmalloc(), keeping it separate from the R/W portions?
>> Could other filesystems do similar things?
> 
> I wasn't cc'd on this patchset, (please use da...@fromorbit.com for
> future postings) 

Apologies, somehow I didn't realize that I should have put you too in
CC. It will be fixed at the next iteration.

> so I can't really say anything about it right
> now. My interest for XFS was that we have a fair amount of static
> data in XFS that we set up at mount time and it never gets modified
> after that.

This is the typical use case I had in mind, although it requires a
conversion.
Ex:

before:

static int a;


void set_a(void)
{
a = 4;
}



after:

static int *a __ro_after_init;
struct gen_pool *pool;

void init_a(void)
{
pool = pmalloc_create_pool("pool", 0);
a = (int *)pmalloc(pool, sizeof(int), GFP_KERNEL);
}

void set_a(void)
{
*a = 4;
pmalloc_protect_pool(pool);
}

> I'm not so worried about VFS level objects (that's a
> much more complex issue) but there is a lot of low hanging fruit in
> the XFS structures we could convert to write-once structures.

I'd be interested to have your review of the pmalloc API, if you think
something is missing, once I send out the next revision.

--
igor

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Dave Chinner]

On Mon, Feb 12, 2018 at 03:32:36PM -0800, Kees Cook wrote:
> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:
> > This patch-set introduces the possibility of protecting memory that has
> > been allocated dynamically.
> >
> > The memory is managed in pools: when a memory pool is turned into R/O,
> > all the memory that is part of it, will become R/O.
> >
> > A R/O pool can be destroyed, to recover its memory, but it cannot be
> > turned back into R/W mode.
> >
> > This is intentional. This feature is meant for data that doesn't need
> > further modifications after initialization.
> 
> This series came up in discussions with Dave Chinner (and Matthew
> Wilcox, already part of the discussion, and others) at LCA. I wonder
> if XFS would make a good initial user of this, as it could allocate
> all the function pointers and other const information about a
> superblock in pmalloc(), keeping it separate from the R/W portions?
> Could other filesystems do similar things?

I wasn't cc'd on this patchset, (please use da...@fromorbit.com for
future postings) so I can't really say anything about it right
now. My interest for XFS was that we have a fair amount of static
data in XFS that we set up at mount time and it never gets modified
after that. I'm not so worried about VFS level objects (that's a
much more complex issue) but there is a lot of low hanging fruit in
the XFS structures we could convert to write-once structures.

Cheers,

Dave.
-- 
Dave Chinner
dchin...@redhat.com

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Dave Chinner]

On Mon, Feb 12, 2018 at 03:32:36PM -0800, Kees Cook wrote:
> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:
> > This patch-set introduces the possibility of protecting memory that has
> > been allocated dynamically.
> >
> > The memory is managed in pools: when a memory pool is turned into R/O,
> > all the memory that is part of it, will become R/O.
> >
> > A R/O pool can be destroyed, to recover its memory, but it cannot be
> > turned back into R/W mode.
> >
> > This is intentional. This feature is meant for data that doesn't need
> > further modifications after initialization.
> 
> This series came up in discussions with Dave Chinner (and Matthew
> Wilcox, already part of the discussion, and others) at LCA. I wonder
> if XFS would make a good initial user of this, as it could allocate
> all the function pointers and other const information about a
> superblock in pmalloc(), keeping it separate from the R/W portions?
> Could other filesystems do similar things?

I wasn't cc'd on this patchset, (please use da...@fromorbit.com for
future postings) so I can't really say anything about it right
now. My interest for XFS was that we have a fair amount of static
data in XFS that we set up at mount time and it never gets modified
after that. I'm not so worried about VFS level objects (that's a
much more complex issue) but there is a lot of low hanging fruit in
the XFS structures we could convert to write-once structures.

Cheers,

Dave.
-- 
Dave Chinner
dchin...@redhat.com

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Kees Cook]

On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:
> This patch-set introduces the possibility of protecting memory that has
> been allocated dynamically.
>
> The memory is managed in pools: when a memory pool is turned into R/O,
> all the memory that is part of it, will become R/O.
>
> A R/O pool can be destroyed, to recover its memory, but it cannot be
> turned back into R/W mode.
>
> This is intentional. This feature is meant for data that doesn't need
> further modifications after initialization.

This series came up in discussions with Dave Chinner (and Matthew
Wilcox, already part of the discussion, and others) at LCA. I wonder
if XFS would make a good initial user of this, as it could allocate
all the function pointers and other const information about a
superblock in pmalloc(), keeping it separate from the R/W portions?
Could other filesystems do similar things?

Do you have other immediate users in mind for pmalloc? Adding those to
the series could really help both define the API and clarify the
usage.

-Kees

>
> However the data might need to be released, for example as part of module
> unloading.
> To do this, the memory must first be freed, then the pool can be destroyed.
>
> An example is provided, in the form of self-testing.
>
> Changes since v15:
> [http://www.openwall.com/lists/kernel-hardening/2018/02/11/4]
>
> - Fixed remaining broken comments
> - Fixed remaining broken "Returns" instead of "Return:" in kernel-doc
> - Converted "Return:" values to lists
> - Fixed SPDX license statements
>
> Igor Stoppa (6):
>   genalloc: track beginning of allocations
>   genalloc: selftest
>   struct page: add field for vm_struct
>   Protectable Memory
>   Pmalloc: self-test
>   Documentation for Pmalloc
>
>  Documentation/core-api/index.rst   |   1 +
>  Documentation/core-api/pmalloc.rst | 114 +++
>  include/linux/genalloc-selftest.h  |  26 ++
>  include/linux/genalloc.h   |   7 +-
>  include/linux/mm_types.h   |   1 +
>  include/linux/pmalloc.h| 242 ++
>  include/linux/vmalloc.h|   1 +
>  init/main.c|   2 +
>  lib/Kconfig|  15 +
>  lib/Makefile   |   1 +
>  lib/genalloc-selftest.c| 400 ++
>  lib/genalloc.c | 658 
> +++--
>  mm/Kconfig |  15 +
>  mm/Makefile|   2 +
>  mm/pmalloc-selftest.c  |  64 
>  mm/pmalloc-selftest.h  |  24 ++
>  mm/pmalloc.c   | 501 
>  mm/usercopy.c  |  33 ++
>  mm/vmalloc.c   |  18 +-
>  19 files changed, 1950 insertions(+), 175 deletions(-)
>  create mode 100644 Documentation/core-api/pmalloc.rst
>  create mode 100644 include/linux/genalloc-selftest.h
>  create mode 100644 include/linux/pmalloc.h
>  create mode 100644 lib/genalloc-selftest.c
>  create mode 100644 mm/pmalloc-selftest.c
>  create mode 100644 mm/pmalloc-selftest.h
>  create mode 100644 mm/pmalloc.c
>
> --
> 2.14.1
>



-- 
Kees Cook
Pixel Security

Re: [RFC PATCH v16 0/6] mm: security: ro protection for dynamic data

[Kees Cook]

On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:
> This patch-set introduces the possibility of protecting memory that has
> been allocated dynamically.
>
> The memory is managed in pools: when a memory pool is turned into R/O,
> all the memory that is part of it, will become R/O.
>
> A R/O pool can be destroyed, to recover its memory, but it cannot be
> turned back into R/W mode.
>
> This is intentional. This feature is meant for data that doesn't need
> further modifications after initialization.

This series came up in discussions with Dave Chinner (and Matthew
Wilcox, already part of the discussion, and others) at LCA. I wonder
if XFS would make a good initial user of this, as it could allocate
all the function pointers and other const information about a
superblock in pmalloc(), keeping it separate from the R/W portions?
Could other filesystems do similar things?

Do you have other immediate users in mind for pmalloc? Adding those to
the series could really help both define the API and clarify the
usage.

-Kees

>
> However the data might need to be released, for example as part of module
> unloading.
> To do this, the memory must first be freed, then the pool can be destroyed.
>
> An example is provided, in the form of self-testing.
>
> Changes since v15:
> [http://www.openwall.com/lists/kernel-hardening/2018/02/11/4]
>
> - Fixed remaining broken comments
> - Fixed remaining broken "Returns" instead of "Return:" in kernel-doc
> - Converted "Return:" values to lists
> - Fixed SPDX license statements
>
> Igor Stoppa (6):
>   genalloc: track beginning of allocations
>   genalloc: selftest
>   struct page: add field for vm_struct
>   Protectable Memory
>   Pmalloc: self-test
>   Documentation for Pmalloc
>
>  Documentation/core-api/index.rst   |   1 +
>  Documentation/core-api/pmalloc.rst | 114 +++
>  include/linux/genalloc-selftest.h  |  26 ++
>  include/linux/genalloc.h   |   7 +-
>  include/linux/mm_types.h   |   1 +
>  include/linux/pmalloc.h| 242 ++
>  include/linux/vmalloc.h|   1 +
>  init/main.c|   2 +
>  lib/Kconfig|  15 +
>  lib/Makefile   |   1 +
>  lib/genalloc-selftest.c| 400 ++
>  lib/genalloc.c | 658 
> +++--
>  mm/Kconfig |  15 +
>  mm/Makefile|   2 +
>  mm/pmalloc-selftest.c  |  64 
>  mm/pmalloc-selftest.h  |  24 ++
>  mm/pmalloc.c   | 501 
>  mm/usercopy.c  |  33 ++
>  mm/vmalloc.c   |  18 +-
>  19 files changed, 1950 insertions(+), 175 deletions(-)
>  create mode 100644 Documentation/core-api/pmalloc.rst
>  create mode 100644 include/linux/genalloc-selftest.h
>  create mode 100644 include/linux/pmalloc.h
>  create mode 100644 lib/genalloc-selftest.c
>  create mode 100644 mm/pmalloc-selftest.c
>  create mode 100644 mm/pmalloc-selftest.h
>  create mode 100644 mm/pmalloc.c
>
> --
> 2.14.1
>



-- 
Kees Cook
Pixel Security