[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[Kees Cook]

Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
index 722160784f83..f815827532dc 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
@@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags,
 	int added = (mode == LCK_NL);
 	int overlaps = 0;
 	int splitted = 0;
-	const struct ldlm_callback_suite null_cbs = { NULL };
+	const struct ldlm_callback_suite null_cbs = { };
 
 	CDEBUG(D_DLMTRACE,
 	       "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n",
-- 
2.7.4


-- 
Kees Cook
Nexus Security

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[James Simmons]

> Prepare to mark sensitive kernel structures for randomization by making
> sure they're using designated initializers. These were identified during
> allyesconfig builds of x86, arm, and arm64, with most initializer fixes
> extracted from grsecurity.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> index 722160784f83..f815827532dc 100644
> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags,
>  	int added = (mode == LCK_NL);
>  	int overlaps = 0;
>  	int splitted = 0;
> -	const struct ldlm_callback_suite null_cbs = { NULL };
> +	const struct ldlm_callback_suite null_cbs = { };
>  
>  	CDEBUG(D_DLMTRACE,
>  	       "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n",

Nak. Filling null_cbs with random data is a bad idea. If you look at 
ldlm_lock_create() where this is used you have

if (cbs) {
	lock->l_blocking_ast = cbs->lcs_blocking;
	lock->l_completion_ast = cbs->lcs_completion;
	lock->l_glimpse_ast = cbs->lcs_glimpse;
}

Having lock->l_* point to random addresses is a bad idea.
What really needs to be done is proper initialization of that
structure. A bunch of patches will be coming to address this.

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[Bruce Korb]

On Mon, Dec 19, 2016 at 8:22 AM, James Simmons
>> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
>> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
>> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags,
>>       int added = (mode == LCK_NL);
>>       int overlaps = 0;
>>       int splitted = 0;
>> -     const struct ldlm_callback_suite null_cbs = { NULL };
>> +     const struct ldlm_callback_suite null_cbs = { };
>>
>>       CDEBUG(D_DLMTRACE,
>>              "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n",
>
> Nak. Filling null_cbs with random data is a bad idea. If you look at
> ldlm_lock_create() where this is used you have
>
> if (cbs) {
>         lock->l_blocking_ast = cbs->lcs_blocking;
>         lock->l_completion_ast = cbs->lcs_completion;
>         lock->l_glimpse_ast = cbs->lcs_glimpse;
> }
>
> Having lock->l_* point to random addresses is a bad idea.
> What really needs to be done is proper initialization of that
> structure. A bunch of patches will be coming to address this.

I'm not understanding the effect of the original difference.  If you
specify any initializer, then all fields not specified are filled with
zero bits. Any pointers are, perforce, NULL.  That should make both "{
NULL }" and "{}" equivalent.  Maybe a worthwhile change would be to:

    static const struct ldlm_callback_suite null_cbs;

then it is not even necessary to specify an initializer.

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[Patrick Farrell]

James,


This should be a purely syntactical change, to help out tools - for GCC, I'm pretty sure the meaning of { } and { NULL } are the same.  Also, I don't think struct randomization does what you're thinking.


Kees,


Is there anything written up on kernel struct randomization?  I was trying to find a talk/post from you or something from LWN, but I couldn't find something about this specifically.  (Probably because I can't find it among the other stuff that's been written up)


- Patrick

________________________________
From: lustre-devel <lustre-devel-bounces@lists.lustre.org> on behalf of James Simmons <jsimmons@infradead.org>
Sent: Monday, December 19, 2016 10:22:58 AM
To: Kees Cook
Cc: devel at driverdev.osuosl.org; Greg Kroah-Hartman; linux-kernel at vger.kernel.org; Oleg Drokin; Vitaly Fertman; Bruce Korb; Emoly Liu; lustre-devel at lists.lustre.org
Subject: Re: [lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers


> Prepare to mark sensitive kernel structures for randomization by making
> sure they're using designated initializers. These were identified during
> allyesconfig builds of x86, arm, and arm64, with most initializer fixes
> extracted from grsecurity.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> index 722160784f83..f815827532dc 100644
> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags,
>        int added = (mode == LCK_NL);
>        int overlaps = 0;
>        int splitted = 0;
> -     const struct ldlm_callback_suite null_cbs = { NULL };
> +     const struct ldlm_callback_suite null_cbs = { };
>
>        CDEBUG(D_DLMTRACE,
>               "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n",

Nak. Filling null_cbs with random data is a bad idea. If you look at
ldlm_lock_create() where this is used you have

if (cbs) {
        lock->l_blocking_ast = cbs->lcs_blocking;
        lock->l_completion_ast = cbs->lcs_completion;
        lock->l_glimpse_ast = cbs->lcs_glimpse;
}

Having lock->l_* point to random addresses is a bad idea.
What really needs to be done is proper initialization of that
structure. A bunch of patches will be coming to address this.
_______________________________________________
lustre-devel mailing list
lustre-devel at lists.lustre.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20161219/bd5785da/attachment.htm>

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[James Simmons]

> James,
> 
> 
> This should be a purely syntactical change, to help out tools - for GCC, I'm pretty sure the meaning of {
> } and { NULL } are the same.? Also, I don't think struct randomization does what you're thinking.

Yep. I misread his commit message. That is why it didn't make sense to 
me.

> Is there anything written up on kernel struct randomization?? I was trying to find a talk/post from you or
> something from LWN, but I couldn't find something about this specifically.? (Probably because I can't find
> it among the other stuff that's been written up)
> 
> 
> - Patrick
> 
> __________________________________________________________________________________________________________
> From: lustre-devel <lustre-devel-bounces@lists.lustre.org> on behalf of James Simmons
> <jsimmons@infradead.org>
> Sent: Monday, December 19, 2016 10:22:58 AM
> To: Kees Cook
> Cc: devel at driverdev.osuosl.org; Greg Kroah-Hartman; linux-kernel at vger.kernel.org; Oleg Drokin; Vitaly
> Fertman; Bruce Korb; Emoly Liu; lustre-devel at lists.lustre.org
> Subject: Re: [lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers ?
> 
> > Prepare to mark sensitive kernel structures for randomization by making
> > sure they're using designated initializers. These were identified during
> > allyesconfig builds of x86, arm, and arm64, with most initializer fixes
> > extracted from grsecurity.
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >? drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +-
> >? 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> > index 722160784f83..f815827532dc 100644
> > --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> > +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> > @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags,
> >??????? int added = (mode == LCK_NL);
> >??????? int overlaps = 0;
> >??????? int splitted = 0;
> > -???? const struct ldlm_callback_suite null_cbs = { NULL };
> > +???? const struct ldlm_callback_suite null_cbs = { };
> >?
> >??????? CDEBUG(D_DLMTRACE,
> >?????????????? "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n",
> 
> Nak. Filling null_cbs with random data is a bad idea. If you look at
> ldlm_lock_create() where this is used you have
> 
> if (cbs) {
> ??????? lock->l_blocking_ast = cbs->lcs_blocking;
> ??????? lock->l_completion_ast = cbs->lcs_completion;
> ??????? lock->l_glimpse_ast = cbs->lcs_glimpse;
> }
> 
> Having lock->l_* point to random addresses is a bad idea.
> What really needs to be done is proper initialization of that
> structure. A bunch of patches will be coming to address this.
> _______________________________________________
> lustre-devel mailing list
> lustre-devel at lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
> 
> 

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[James Simmons]

> On Mon, Dec 19, 2016 at 8:22 AM, James Simmons
> >> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> >> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> >> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags,
> >>       int added = (mode == LCK_NL);
> >>       int overlaps = 0;
> >>       int splitted = 0;
> >> -     const struct ldlm_callback_suite null_cbs = { NULL };
> >> +     const struct ldlm_callback_suite null_cbs = { };
> >>
> >>       CDEBUG(D_DLMTRACE,
> >>              "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n",
> >
> > Nak. Filling null_cbs with random data is a bad idea. If you look at
> > ldlm_lock_create() where this is used you have
> >
> > if (cbs) {
> >         lock->l_blocking_ast = cbs->lcs_blocking;
> >         lock->l_completion_ast = cbs->lcs_completion;
> >         lock->l_glimpse_ast = cbs->lcs_glimpse;
> > }
> >
> > Having lock->l_* point to random addresses is a bad idea.
> > What really needs to be done is proper initialization of that
> > structure. A bunch of patches will be coming to address this.
> 
> I'm not understanding the effect of the original difference.  If you
> specify any initializer, then all fields not specified are filled with
> zero bits. Any pointers are, perforce, NULL.  That should make both "{
> NULL }" and "{}" equivalent.  Maybe a worthwhile change would be to:
> 
>     static const struct ldlm_callback_suite null_cbs;

I perfer this as well.
 
> then it is not even necessary to specify an initializer.
 

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[Dan Carpenter]

On Mon, Dec 19, 2016 at 08:47:50AM -0800, Bruce Korb wrote:
> On Mon, Dec 19, 2016 at 8:22 AM, James Simmons
> >> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> >> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> >> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags,
> >>       int added = (mode == LCK_NL);
> >>       int overlaps = 0;
> >>       int splitted = 0;
> >> -     const struct ldlm_callback_suite null_cbs = { NULL };
> >> +     const struct ldlm_callback_suite null_cbs = { };
> >>
> >>       CDEBUG(D_DLMTRACE,
> >>              "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n",
> >
> > Nak. Filling null_cbs with random data is a bad idea. If you look at
> > ldlm_lock_create() where this is used you have
> >
> > if (cbs) {
> >         lock->l_blocking_ast = cbs->lcs_blocking;
> >         lock->l_completion_ast = cbs->lcs_completion;
> >         lock->l_glimpse_ast = cbs->lcs_glimpse;
> > }
> >
> > Having lock->l_* point to random addresses is a bad idea.
> > What really needs to be done is proper initialization of that
> > structure. A bunch of patches will be coming to address this.
> 
> I'm not understanding the effect of the original difference.  If you
> specify any initializer, then all fields not specified are filled with
> zero bits. Any pointers are, perforce, NULL.  That should make both "{
> NULL }" and "{}" equivalent.

They are equivalent, yes, but people want to use a GCC plugin that
randomizes struct layouts for internal structures and the plugin doesn't
work when you use struct ordering to initialize the struct.  The plugin
requires that you use designated intializers.

regards,
dan carpenter

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[Dan Carpenter]

On Mon, Dec 19, 2016 at 04:22:58PM +0000, James Simmons wrote:
> 
> > Prepare to mark sensitive kernel structures for randomization by making
> > sure they're using designated initializers. These were identified during
> > allyesconfig builds of x86, arm, and arm64, with most initializer fixes
> > extracted from grsecurity.
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  drivers/staging/lustre/lustre/ldlm/ldlm_flock.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> > index 722160784f83..f815827532dc 100644
> > --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> > +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> > @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock *req, __u64 *flags,
> >  	int added = (mode == LCK_NL);
> >  	int overlaps = 0;
> >  	int splitted = 0;
> > -	const struct ldlm_callback_suite null_cbs = { NULL };
> > +	const struct ldlm_callback_suite null_cbs = { };
> >  
> >  	CDEBUG(D_DLMTRACE,
> >  	       "flags %#llx owner %llu pid %u mode %u start %llu end %llu\n",
> 
> Nak. Filling null_cbs with random data is a bad idea.

You've misunderstood.  The plugin just changes how the struct is laid
out, it doesn't put data into the struct.  So this is fine.

The places where it's not fine are when the layout is required because
it's shared with userspace or set by the hardware.

regards,
dan carpenter

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[Hammond, John]

> On Mon, Dec 19, 2016 at 08:47:50AM -0800, Bruce Korb wrote:
> > On Mon, Dec 19, 2016 at 8:22 AM, James Simmons
> > >> --- a/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> > >> +++ b/drivers/staging/lustre/lustre/ldlm/ldlm_flock.c
> > >> @@ -143,7 +143,7 @@ static int ldlm_process_flock_lock(struct ldlm_lock
> *req, __u64 *flags,
> > >>       int added = (mode == LCK_NL);
> > >>       int overlaps = 0;
> > >>       int splitted = 0;
> > >> -     const struct ldlm_callback_suite null_cbs = { NULL };
> > >> +     const struct ldlm_callback_suite null_cbs = { };
> > >>
> > >>       CDEBUG(D_DLMTRACE,
> > >>              "flags %#llx owner %llu pid %u mode %u start %llu end
> > >> %llu\n",
> > >
> > > Nak. Filling null_cbs with random data is a bad idea. If you look at
> > > ldlm_lock_create() where this is used you have
> > >
> > > if (cbs) {
> > >         lock->l_blocking_ast = cbs->lcs_blocking;
> > >         lock->l_completion_ast = cbs->lcs_completion;
> > >         lock->l_glimpse_ast = cbs->lcs_glimpse; }
> > >
> > > Having lock->l_* point to random addresses is a bad idea.
> > > What really needs to be done is proper initialization of that
> > > structure. A bunch of patches will be coming to address this.
> >
> > I'm not understanding the effect of the original difference.  If you
> > specify any initializer, then all fields not specified are filled with
> > zero bits. Any pointers are, perforce, NULL.  That should make both "{
> > NULL }" and "{}" equivalent.
> 
> They are equivalent, yes, but people want to use a GCC plugin that randomizes
> struct layouts for internal structures and the plugin doesn't work when you use
> struct ordering to initialize the struct.  The plugin requires that you use
> designated intializers.

"{ NULL }" is valid ISO C, but unfortunately "{}" is not.

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[Bruce Korb]

>
> "{ NULL }" is valid ISO C, but unfortunately "{}" is not.

Just make the thing "static const" and don't use an initializer.

[lustre-devel] Designated initializers, struct randomization and addressing?

[Joe Perches]

On Fri, 2016-12-16 at 17:00 -0800, Kees Cook wrote:
> Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers.

About the designated initializer patches,
which by themselves are fine of course,
and the fundamental randomization plugin,
c guarantees that struct member ordering
is as specified.

how is the code to be verified so that
any use of things like offsetof and any
address/indexing is not impacted?

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[Dan Carpenter]

On Tue, Dec 20, 2016 at 08:47:51AM -0800, Bruce Korb wrote:
> >
> > "{ NULL }" is valid ISO C, but unfortunately "{}" is not.
> 
> Just make the thing "static const" and don't use an initializer.

That also works, of course.

regards,
dan carpenter

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[Dan Carpenter]

On Tue, Dec 20, 2016 at 02:57:17PM +0000, Hammond, John wrote:
> "{ NULL }" is valid ISO C, but unfortunately "{}" is not.

In the kernel we don't care.  We use lots of GCC extensions.

regards,
dan carpenter

[lustre-devel] [PATCH] staging: lustre: ldlm: use designated initializers

[Kees Cook]

On Tue, Dec 20, 2016 at 11:07 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Tue, Dec 20, 2016 at 02:57:17PM +0000, Hammond, John wrote:
>> "{ NULL }" is valid ISO C, but unfortunately "{}" is not.
>
> In the kernel we don't care.  We use lots of GCC extensions.

We depend on the compiler to do "incomplete zero-initialization" of
structures that are not mentioned in an initializer. The reason { NULL
} works is because the first field in the structure can take a NULL
value, and then the rest are zero-initialized by the compiler. { } is
the same thing, but doesn't use ordered initialization. If this style
is truly unacceptable to you, then { .somefield = NULL } can work, or
as you point out, if it's being initialized later, the static
initializer can be dropped entirely.

-Kees

-- 
Kees Cook
Nexus Security