Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1543897 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] staging: lustre: ldlm: use designated initializers Kees Cook <keescook@chromium.org> - 2016-12-17 02:10 +0100
    Re: [PATCH] staging: lustre: ldlm: use designated initializers James Simmons <jsimmons@infradead.org> - 2016-12-19 17:30 +0100
      Re: [PATCH] staging: lustre: ldlm: use designated initializers Bruce Korb <bruce.korb@gmail.com> - 2016-12-19 17:50 +0100
        Re: [PATCH] staging: lustre: ldlm: use designated initializers James Simmons <jsimmons@infradead.org> - 2016-12-19 18:20 +0100
        Re: [PATCH] staging: lustre: ldlm: use designated initializers Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-20 08:20 +0100
          RE: [PATCH] staging: lustre: ldlm: use designated initializers "Hammond, John" <john.hammond@intel.com> - 2016-12-20 16:00 +0100
            Re: [PATCH] staging: lustre: ldlm: use designated initializers Bruce Korb <bruce.korb@gmail.com> - 2016-12-20 17:50 +0100
              Re: [PATCH] staging: lustre: ldlm: use designated initializers Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-20 20:00 +0100
            Re: [PATCH] staging: lustre: ldlm: use designated initializers Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-20 20:20 +0100
              Re: [PATCH] staging: lustre: ldlm: use designated initializers Kees Cook <keescook@chromium.org> - 2016-12-20 20:50 +0100
      Re: [PATCH] staging: lustre: ldlm: use designated initializers Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-20 11:50 +0100
    Designated initializers, struct randomization and addressing? Joe Perches <joe@perches.com> - 2016-12-20 18:40 +0100

#1543897 — [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",
-- 
2.7.4


-- 
Kees Cook
Nexus Security

[toc] | [next] | [standalone]

#1544652

> 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.

[toc] | [prev] | [next] | [standalone]

#1544668

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.

[toc] | [prev] | [next] | [standalone]

#1544688

> 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.
 

[toc] | [prev] | [next] | [standalone]

#1544957

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

[toc] | [prev] | [next] | [standalone]

#1545183

> 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.

[toc] | [prev] | [next] | [standalone]

#1545258

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

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

[toc] | [prev] | [next] | [standalone]

#1545370

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

[toc] | [prev] | [next] | [standalone]

#1545396

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

[toc] | [prev] | [next] | [standalone]

#1545445

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

[toc] | [prev] | [next] | [standalone]

#1545041

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

[toc] | [prev] | [next] | [standalone]

#1545318 — Designated initializers, struct randomization and addressing?

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?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web