| Message ID | 1394572274-16474-1-git-send-email-Anna.Schumaker@netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mar 11, 2014, at 17:11, Anna Schumaker <Anna.Schumaker@netapp.com> wrote: > If the i_security field isn't set then security_dentry_init_security() > won't initialize some of the values used by the security label. This > causes my client to hit a BUG_ON() while encoding a label of size > -2128927414. > > I hit this bug while testing on a client without SELinux installed. > > Signed-off-by: Anna Schumaker <anna.schumaker@netapp.com> > --- > fs/nfs/nfs4proc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index b8cd560..994ccc2 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -105,6 +105,9 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL) == 0) > return NULL; > > + if (!dir->i_security) > + return NULL; > + > err = security_dentry_init_security(dentry, sattr->ia_mode, > &dentry->d_name, (void **)&label->label, &label->len); > if (err == 0) Hi Anna, This looks like a check that needs to be done by selinux_dentry_init_security() itself. The dir->i_security field is not something that NFS knows about. David, what needs to happen there when dentry->d_parent->i_security (a.k.a. dsec) is NULL?
Could it be bad to do this when selinux is enabled? Could you do something based on selinux_is_enabled()? -dros On Mar 11, 2014, at 5:11 PM, Anna Schumaker <Anna.Schumaker@netapp.com> wrote: > If the i_security field isn't set then security_dentry_init_security() > won't initialize some of the values used by the security label. This > causes my client to hit a BUG_ON() while encoding a label of size > -2128927414. > > I hit this bug while testing on a client without SELinux installed. > > Signed-off-by: Anna Schumaker <anna.schumaker@netapp.com> > --- > fs/nfs/nfs4proc.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > index b8cd560..994ccc2 100644 > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -105,6 +105,9 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL) == 0) > return NULL; > > + if (!dir->i_security) > + return NULL; > + > err = security_dentry_init_security(dentry, sattr->ia_mode, > &dentry->d_name, (void **)&label->label, &label->len); > if (err == 0) > -- > 1.9.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mar 11, 2014, at 17:27, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > > On Mar 11, 2014, at 17:11, Anna Schumaker <Anna.Schumaker@netapp.com> wrote: > >> If the i_security field isn't set then security_dentry_init_security() >> won't initialize some of the values used by the security label. This >> causes my client to hit a BUG_ON() while encoding a label of size >> -2128927414. >> >> I hit this bug while testing on a client without SELinux installed. >> >> Signed-off-by: Anna Schumaker <anna.schumaker@netapp.com> >> --- >> fs/nfs/nfs4proc.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >> index b8cd560..994ccc2 100644 >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -105,6 +105,9 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, >> if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL) == 0) >> return NULL; >> >> + if (!dir->i_security) >> + return NULL; >> + >> err = security_dentry_init_security(dentry, sattr->ia_mode, >> &dentry->d_name, (void **)&label->label, &label->len); >> if (err == 0) > > Hi Anna, > > This looks like a check that needs to be done by selinux_dentry_init_security() itself. The dir->i_security field is not something that NFS knows about. > David, what needs to happen there when dentry->d_parent->i_security (a.k.a. dsec) is NULL? > Oh, wait. I missed the bit about ‘without SELinux installed’. So the problem here is that you have a NFS client that does not have SELinux set up, but running against a server that is advertising NFSv4.2 with labeled NFS. Is that correct? It looks to me as if cap_dentry_init_security() will indeed trigger this behaviour since it returns ‘0’ without doing anything to the label. As far as I can see, the right thing to do there is to return -EOPNOTSUPP, no?
On Tue, 2014-03-11 at 18:00 -0400, Trond Myklebust wrote: > On Mar 11, 2014, at 17:27, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > > > > > On Mar 11, 2014, at 17:11, Anna Schumaker <Anna.Schumaker@netapp.com> wrote: > > > >> If the i_security field isn't set then security_dentry_init_security() > >> won't initialize some of the values used by the security label. This > >> causes my client to hit a BUG_ON() while encoding a label of size > >> -2128927414. > >> > >> I hit this bug while testing on a client without SELinux installed. > >> > >> Signed-off-by: Anna Schumaker <anna.schumaker@netapp.com> > >> --- > >> fs/nfs/nfs4proc.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > >> index b8cd560..994ccc2 100644 > >> --- a/fs/nfs/nfs4proc.c > >> +++ b/fs/nfs/nfs4proc.c > >> @@ -105,6 +105,9 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, > >> if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL) == 0) > >> return NULL; > >> > >> + if (!dir->i_security) > >> + return NULL; > >> + > >> err = security_dentry_init_security(dentry, sattr->ia_mode, > >> &dentry->d_name, (void **)&label->label, &label->len); > >> if (err == 0) > > > > Hi Anna, > > > > This looks like a check that needs to be done by selinux_dentry_init_security() itself. The dir->i_security field is not something that NFS knows about. > > David, what needs to happen there when dentry->d_parent->i_security (a.k.a. dsec) is NULL? > > > > Oh, wait. I missed the bit about ‘without SELinux installed’. So the problem here is that you have a NFS client that does not have SELinux set up, but running against a server that is advertising NFSv4.2 with labeled NFS. Is that correct? > > It looks to me as if cap_dentry_init_security() will indeed trigger this behaviour since it returns ‘0’ without doing anything to the label. As far as I can see, the right thing to do there is to return -EOPNOTSUPP, no? I feel like Jeff Layton was looking at the same thing, and came to the same conclusion... Jeff? -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 11 Mar 2014 20:38:18 -0400 Eric Paris <eparis@redhat.com> wrote: > On Tue, 2014-03-11 at 18:00 -0400, Trond Myklebust wrote: > > On Mar 11, 2014, at 17:27, Trond Myklebust > > <trond.myklebust@primarydata.com> wrote: > > > > > > > > On Mar 11, 2014, at 17:11, Anna Schumaker > > > <Anna.Schumaker@netapp.com> wrote: > > > > > >> If the i_security field isn't set then > > >> security_dentry_init_security() won't initialize some of the > > >> values used by the security label. This causes my client to hit > > >> a BUG_ON() while encoding a label of size -2128927414. > > >> > > >> I hit this bug while testing on a client without SELinux > > >> installed. > > >> > > >> Signed-off-by: Anna Schumaker <anna.schumaker@netapp.com> > > >> --- > > >> fs/nfs/nfs4proc.c | 3 +++ > > >> 1 file changed, 3 insertions(+) > > >> > > >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > > >> index b8cd560..994ccc2 100644 > > >> --- a/fs/nfs/nfs4proc.c > > >> +++ b/fs/nfs/nfs4proc.c > > >> @@ -105,6 +105,9 @@ nfs4_label_init_security(struct inode *dir, > > >> struct dentry *dentry, if (nfs_server_capable(dir, > > >> NFS_CAP_SECURITY_LABEL) == 0) return NULL; > > >> > > >> + if (!dir->i_security) > > >> + return NULL; > > >> + > > >> err = security_dentry_init_security(dentry, > > >> sattr->ia_mode, &dentry->d_name, (void **)&label->label, > > >> &label->len); if (err == 0) > > > > > > Hi Anna, > > > > > > This looks like a check that needs to be done by > > > selinux_dentry_init_security() itself. The dir->i_security field > > > is not something that NFS knows about. David, what needs to > > > happen there when dentry->d_parent->i_security (a.k.a. dsec) is > > > NULL? > > > > > > > Oh, wait. I missed the bit about ‘without SELinux installed’. So > > the problem here is that you have a NFS client that does not have > > SELinux set up, but running against a server that is advertising > > NFSv4.2 with labeled NFS. Is that correct? > > > > It looks to me as if cap_dentry_init_security() will indeed trigger > > this behaviour since it returns ‘0’ without doing anything to the > > label. As far as I can see, the right thing to do there is to > > return -EOPNOTSUPP, no? > > I feel like Jeff Layton was looking at the same thing, and came to the > same conclusion... > > Jeff? > I posted a patch for this last week and James has merged it: [PATCH] security: have cap_dentry_init_security return error I didn't note it in the patch description but it fixes 4.2 when SELinux is compiled in but disabled.
On Mar 11, 2014, at 21:06, Jeffrey Layton <jlayton@redhat.com> wrote: > On Tue, 11 Mar 2014 20:38:18 -0400 > Eric Paris <eparis@redhat.com> wrote: > >> On Tue, 2014-03-11 at 18:00 -0400, Trond Myklebust wrote: >>> On Mar 11, 2014, at 17:27, Trond Myklebust >>> <trond.myklebust@primarydata.com> wrote: >>> >>>> >>>> On Mar 11, 2014, at 17:11, Anna Schumaker >>>> <Anna.Schumaker@netapp.com> wrote: >>>> >>>>> If the i_security field isn't set then >>>>> security_dentry_init_security() won't initialize some of the >>>>> values used by the security label. This causes my client to hit >>>>> a BUG_ON() while encoding a label of size -2128927414. >>>>> >>>>> I hit this bug while testing on a client without SELinux >>>>> installed. >>>>> >>>>> Signed-off-by: Anna Schumaker <anna.schumaker@netapp.com> >>>>> --- >>>>> fs/nfs/nfs4proc.c | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>>>> index b8cd560..994ccc2 100644 >>>>> --- a/fs/nfs/nfs4proc.c >>>>> +++ b/fs/nfs/nfs4proc.c >>>>> @@ -105,6 +105,9 @@ nfs4_label_init_security(struct inode *dir, >>>>> struct dentry *dentry, if (nfs_server_capable(dir, >>>>> NFS_CAP_SECURITY_LABEL) == 0) return NULL; >>>>> >>>>> + if (!dir->i_security) >>>>> + return NULL; >>>>> + >>>>> err = security_dentry_init_security(dentry, >>>>> sattr->ia_mode, &dentry->d_name, (void **)&label->label, >>>>> &label->len); if (err == 0) >>>> >>>> Hi Anna, >>>> >>>> This looks like a check that needs to be done by >>>> selinux_dentry_init_security() itself. The dir->i_security field >>>> is not something that NFS knows about. David, what needs to >>>> happen there when dentry->d_parent->i_security (a.k.a. dsec) is >>>> NULL? >>>> >>> >>> Oh, wait. I missed the bit about ‘without SELinux installed’. So >>> the problem here is that you have a NFS client that does not have >>> SELinux set up, but running against a server that is advertising >>> NFSv4.2 with labeled NFS. Is that correct? >>> >>> It looks to me as if cap_dentry_init_security() will indeed trigger >>> this behaviour since it returns ‘0’ without doing anything to the >>> label. As far as I can see, the right thing to do there is to >>> return -EOPNOTSUPP, no? >> >> I feel like Jeff Layton was looking at the same thing, and came to the >> same conclusion... >> >> Jeff? >> > > I posted a patch for this last week and James has merged it: > > [PATCH] security: have cap_dentry_init_security return error > > I didn't note it in the patch description but it fixes 4.2 when SELinux > is compiled in but disabled. Thanks! Then I expect no further action is needed on our part, and that the fix will come through the security tree?
On Wed, 12 Mar 2014 05:21:13 -0400 Trond Myklebust <trond.myklebust@primarydata.com> wrote: > > On Mar 11, 2014, at 21:06, Jeffrey Layton <jlayton@redhat.com> wrote: > > > On Tue, 11 Mar 2014 20:38:18 -0400 > > Eric Paris <eparis@redhat.com> wrote: > > > >> On Tue, 2014-03-11 at 18:00 -0400, Trond Myklebust wrote: > >>> On Mar 11, 2014, at 17:27, Trond Myklebust > >>> <trond.myklebust@primarydata.com> wrote: > >>> > >>>> > >>>> On Mar 11, 2014, at 17:11, Anna Schumaker > >>>> <Anna.Schumaker@netapp.com> wrote: > >>>> > >>>>> If the i_security field isn't set then > >>>>> security_dentry_init_security() won't initialize some of the > >>>>> values used by the security label. This causes my client to hit > >>>>> a BUG_ON() while encoding a label of size -2128927414. > >>>>> > >>>>> I hit this bug while testing on a client without SELinux > >>>>> installed. > >>>>> > >>>>> Signed-off-by: Anna Schumaker <anna.schumaker@netapp.com> > >>>>> --- > >>>>> fs/nfs/nfs4proc.c | 3 +++ > >>>>> 1 file changed, 3 insertions(+) > >>>>> > >>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c > >>>>> index b8cd560..994ccc2 100644 > >>>>> --- a/fs/nfs/nfs4proc.c > >>>>> +++ b/fs/nfs/nfs4proc.c > >>>>> @@ -105,6 +105,9 @@ nfs4_label_init_security(struct inode *dir, > >>>>> struct dentry *dentry, if (nfs_server_capable(dir, > >>>>> NFS_CAP_SECURITY_LABEL) == 0) return NULL; > >>>>> > >>>>> + if (!dir->i_security) > >>>>> + return NULL; > >>>>> + > >>>>> err = security_dentry_init_security(dentry, > >>>>> sattr->ia_mode, &dentry->d_name, (void **)&label->label, > >>>>> &label->len); if (err == 0) > >>>> > >>>> Hi Anna, > >>>> > >>>> This looks like a check that needs to be done by > >>>> selinux_dentry_init_security() itself. The dir->i_security field > >>>> is not something that NFS knows about. David, what needs to > >>>> happen there when dentry->d_parent->i_security (a.k.a. dsec) is > >>>> NULL? > >>>> > >>> > >>> Oh, wait. I missed the bit about ‘without SELinux installed’. So > >>> the problem here is that you have a NFS client that does not have > >>> SELinux set up, but running against a server that is advertising > >>> NFSv4.2 with labeled NFS. Is that correct? > >>> > >>> It looks to me as if cap_dentry_init_security() will indeed trigger > >>> this behaviour since it returns ‘0’ without doing anything to the > >>> label. As far as I can see, the right thing to do there is to > >>> return -EOPNOTSUPP, no? > >> > >> I feel like Jeff Layton was looking at the same thing, and came to the > >> same conclusion... > >> > >> Jeff? > >> > > > > I posted a patch for this last week and James has merged it: > > > > [PATCH] security: have cap_dentry_init_security return error > > > > I didn't note it in the patch description but it fixes 4.2 when SELinux > > is compiled in but disabled. > > Thanks! Then I expect no further action is needed on our part, and that the fix will come through the security tree? > FWIW, this is the bug that was causing the NFS server to spew messages like this to the ring buffer when Anna was testing against my server at Connectathon: [ 243.479524] SELinux: Context \xffffffdf is not valid (left unmapped). We may want to do a follow-on patch to clean up the struct nfs4_label initializers since they're not really needed. But that should probably wait until James merges this up to Linus.
I just tried Jeff's patch and it does fix my problem. I wish I had remembered it earlier in the day yesterday! On 03/12/2014 06:22 AM, Jeff Layton wrote: > On Wed, 12 Mar 2014 05:21:13 -0400 > Trond Myklebust <trond.myklebust@primarydata.com> wrote: > >> On Mar 11, 2014, at 21:06, Jeffrey Layton <jlayton@redhat.com> wrote: >> >>> On Tue, 11 Mar 2014 20:38:18 -0400 >>> Eric Paris <eparis@redhat.com> wrote: >>> >>>> On Tue, 2014-03-11 at 18:00 -0400, Trond Myklebust wrote: >>>>> On Mar 11, 2014, at 17:27, Trond Myklebust >>>>> <trond.myklebust@primarydata.com> wrote: >>>>> >>>>>> On Mar 11, 2014, at 17:11, Anna Schumaker >>>>>> <Anna.Schumaker@netapp.com> wrote: >>>>>> >>>>>>> If the i_security field isn't set then >>>>>>> security_dentry_init_security() won't initialize some of the >>>>>>> values used by the security label. This causes my client to hit >>>>>>> a BUG_ON() while encoding a label of size -2128927414. >>>>>>> >>>>>>> I hit this bug while testing on a client without SELinux >>>>>>> installed. >>>>>>> >>>>>>> Signed-off-by: Anna Schumaker <anna.schumaker@netapp.com> >>>>>>> --- >>>>>>> fs/nfs/nfs4proc.c | 3 +++ >>>>>>> 1 file changed, 3 insertions(+) >>>>>>> >>>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c >>>>>>> index b8cd560..994ccc2 100644 >>>>>>> --- a/fs/nfs/nfs4proc.c >>>>>>> +++ b/fs/nfs/nfs4proc.c >>>>>>> @@ -105,6 +105,9 @@ nfs4_label_init_security(struct inode *dir, >>>>>>> struct dentry *dentry, if (nfs_server_capable(dir, >>>>>>> NFS_CAP_SECURITY_LABEL) == 0) return NULL; >>>>>>> >>>>>>> + if (!dir->i_security) >>>>>>> + return NULL; >>>>>>> + >>>>>>> err = security_dentry_init_security(dentry, >>>>>>> sattr->ia_mode, &dentry->d_name, (void **)&label->label, >>>>>>> &label->len); if (err == 0) >>>>>> Hi Anna, >>>>>> >>>>>> This looks like a check that needs to be done by >>>>>> selinux_dentry_init_security() itself. The dir->i_security field >>>>>> is not something that NFS knows about. David, what needs to >>>>>> happen there when dentry->d_parent->i_security (a.k.a. dsec) is >>>>>> NULL? >>>>>> >>>>> Oh, wait. I missed the bit about ‘without SELinux installed’. So >>>>> the problem here is that you have a NFS client that does not have >>>>> SELinux set up, but running against a server that is advertising >>>>> NFSv4.2 with labeled NFS. Is that correct? >>>>> >>>>> It looks to me as if cap_dentry_init_security() will indeed trigger >>>>> this behaviour since it returns ‘0’ without doing anything to the >>>>> label. As far as I can see, the right thing to do there is to >>>>> return -EOPNOTSUPP, no? >>>> I feel like Jeff Layton was looking at the same thing, and came to the >>>> same conclusion... >>>> >>>> Jeff? >>>> >>> I posted a patch for this last week and James has merged it: >>> >>> [PATCH] security: have cap_dentry_init_security return error >>> >>> I didn't note it in the patch description but it fixes 4.2 when SELinux >>> is compiled in but disabled. >> Thanks! Then I expect no further action is needed on our part, and that the fix will come through the security tree? >> > FWIW, this is the bug that was causing the NFS server to spew messages > like this to the ring buffer when Anna was testing against my server at > Connectathon: > > [ 243.479524] SELinux: Context \xffffffdf is not valid (left unmapped). > > We may want to do a follow-on patch to clean up the struct nfs4_label > initializers since they're not really needed. But that should probably > wait until James merges this up to Linus. > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index b8cd560..994ccc2 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -105,6 +105,9 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry, if (nfs_server_capable(dir, NFS_CAP_SECURITY_LABEL) == 0) return NULL; + if (!dir->i_security) + return NULL; + err = security_dentry_init_security(dentry, sattr->ia_mode, &dentry->d_name, (void **)&label->label, &label->len); if (err == 0)
If the i_security field isn't set then security_dentry_init_security() won't initialize some of the values used by the security label. This causes my client to hit a BUG_ON() while encoding a label of size -2128927414. I hit this bug while testing on a client without SELinux installed. Signed-off-by: Anna Schumaker <anna.schumaker@netapp.com> --- fs/nfs/nfs4proc.c | 3 +++ 1 file changed, 3 insertions(+)