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 <[email protected]>
---
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
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 <[email protected]> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]> wrote:
>
>> On Mar 11, 2014, at 21:06, Jeffrey Layton <[email protected]> wrote:
>>
>>> On Tue, 11 Mar 2014 20:38:18 -0400
>>> Eric Paris <[email protected]> wrote:
>>>
>>>> On Tue, 2014-03-11 at 18:00 -0400, Trond Myklebust wrote:
>>>>> On Mar 11, 2014, at 17:27, Trond Myklebust
>>>>> <[email protected]> wrote:
>>>>>
>>>>>> On Mar 11, 2014, at 17:11, Anna Schumaker
>>>>>> <[email protected]> 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 <[email protected]>
>>>>>>> ---
>>>>>>> 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.
>
On Wed, 12 Mar 2014 05:21:13 -0400
Trond Myklebust <[email protected]> wrote:
>
> On Mar 11, 2014, at 21:06, Jeffrey Layton <[email protected]> wrote:
>
> > On Tue, 11 Mar 2014 20:38:18 -0400
> > Eric Paris <[email protected]> wrote:
> >
> >> On Tue, 2014-03-11 at 18:00 -0400, Trond Myklebust wrote:
> >>> On Mar 11, 2014, at 17:27, Trond Myklebust
> >>> <[email protected]> wrote:
> >>>
> >>>>
> >>>> On Mar 11, 2014, at 17:11, Anna Schumaker
> >>>> <[email protected]> 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 <[email protected]>
> >>>>> ---
> >>>>> 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.
--
Jeff Layton <[email protected]>
On Mar 11, 2014, at 21:06, Jeffrey Layton <[email protected]> wrote:
> On Tue, 11 Mar 2014 20:38:18 -0400
> Eric Paris <[email protected]> wrote:
>
>> On Tue, 2014-03-11 at 18:00 -0400, Trond Myklebust wrote:
>>> On Mar 11, 2014, at 17:27, Trond Myklebust
>>> <[email protected]> wrote:
>>>
>>>>
>>>> On Mar 11, 2014, at 17:11, Anna Schumaker
>>>> <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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?
_________________________________
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
On Mar 11, 2014, at 17:11, Anna Schumaker <[email protected]> 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 <[email protected]>
> ---
> 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?
_________________________________
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
On Tue, 11 Mar 2014 20:38:18 -0400
Eric Paris <[email protected]> wrote:
> On Tue, 2014-03-11 at 18:00 -0400, Trond Myklebust wrote:
> > On Mar 11, 2014, at 17:27, Trond Myklebust
> > <[email protected]> wrote:
> >
> > >
> > > On Mar 11, 2014, at 17:11, Anna Schumaker
> > > <[email protected]> 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 <[email protected]>
> > >> ---
> > >> 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.
--
Jeff Layton <[email protected]>
On Mar 11, 2014, at 17:27, Trond Myklebust <[email protected]> wrote:
>
> On Mar 11, 2014, at 17:11, Anna Schumaker <[email protected]> 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 <[email protected]>
>> ---
>> 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?
_________________________________
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
On Tue, 2014-03-11 at 18:00 -0400, Trond Myklebust wrote:
> On Mar 11, 2014, at 17:27, Trond Myklebust <[email protected]> wrote:
>
> >
> > On Mar 11, 2014, at 17:11, Anna Schumaker <[email protected]> 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 <[email protected]>
> >> ---
> >> 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