| Message ID | 1481045078-15000-2-git-send-email-william.c.roberts@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 12/06/2016 12:24 PM, william.c.roberts@intel.com wrote: > From: Yongqin Liu <yongqin.liu@linaro.org> > > Here is just the quick fix for tracefs with selinux. > just add tracefs to the list of whitelisted filesystem > types in selinux_is_sblabel_mnt(), but the right fix would be to > generalize this logic as described in the last item on the todo list, > https://bitbucket.org/seandroid/wiki/wiki/ToDo > > Change-Id: I2aa803ccffbcd2802a7287514da7648e6b364157 Please rewrite the subject line and patch description per the kernel's submission guidelines, drop the Change-Id and the link to the SEAndroid todo list, and don't say that this is a quick fix but the right fix is something else if you want this to actually be merged. Because in that case, you ought to just implement the right fix. There is now an upstream kernel issue for the right fix: https://github.com/SELinuxProject/selinux-kernel/issues/2 > Signed-off-by: Yongqin Liu <yongqin.liu@linaro.org> > --- > security/selinux/hooks.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 09fd610..24bd84d 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -491,6 +491,7 @@ static int selinux_is_sblabel_mnt(struct super_block *sb) > !strcmp(sb->s_type->name, "sysfs") || > !strcmp(sb->s_type->name, "pstore") || > !strcmp(sb->s_type->name, "debugfs") || > + !strcmp(sb->s_type->name, "tracefs") || > !strcmp(sb->s_type->name, "rootfs"); > } > >
> -----Original Message----- > From: Stephen Smalley [mailto:sds@tycho.nsa.gov] > Sent: Tuesday, December 6, 2016 9:41 AM > To: Roberts, William C <william.c.roberts@intel.com>; nnk@google.com; > selinux@tycho.nsa.gov > Cc: Yongqin Liu <yongqin.liu@linaro.org>; Paul Moore <paul@paul-moore.com> > Subject: Re: [PATCH] quick selinux support for tracefs > > On 12/06/2016 12:24 PM, william.c.roberts@intel.com wrote: > > From: Yongqin Liu <yongqin.liu@linaro.org> > > > > Here is just the quick fix for tracefs with selinux. > > just add tracefs to the list of whitelisted filesystem types in > > selinux_is_sblabel_mnt(), but the right fix would be to generalize > > this logic as described in the last item on the todo list, > > https://bitbucket.org/seandroid/wiki/wiki/ToDo > > > > Change-Id: I2aa803ccffbcd2802a7287514da7648e6b364157 > > Please rewrite the subject line and patch description per the kernel's submission > guidelines, drop the Change-Id and the link to the SEAndroid todo list, and don't > say that this is a quick fix but Why would anyone do that for this patch when below you say it won’t be merged unless we fix issue #2? the right fix is something else if you want this to > actually be merged. Because in that case, you ought to just implement the right > fix. There is now an upstream kernel issue for the right fix: > https://github.com/SELinuxProject/selinux-kernel/issues/2 The other question here is tracefs safe to label in this fashion, I would assume yes. Looking through I didn't see any eviction code. > > > Signed-off-by: Yongqin Liu <yongqin.liu@linaro.org> > > --- > > security/selinux/hooks.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index > > 09fd610..24bd84d 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -491,6 +491,7 @@ static int selinux_is_sblabel_mnt(struct super_block > *sb) > > !strcmp(sb->s_type->name, "sysfs") || > > !strcmp(sb->s_type->name, "pstore") || > > !strcmp(sb->s_type->name, "debugfs") || > > + !strcmp(sb->s_type->name, "tracefs") || > > !strcmp(sb->s_type->name, "rootfs"); } > > > >
On 12/06/2016 12:50 PM, Roberts, William C wrote: > > >> -----Original Message----- >> From: Stephen Smalley [mailto:sds@tycho.nsa.gov] >> Sent: Tuesday, December 6, 2016 9:41 AM >> To: Roberts, William C <william.c.roberts@intel.com>; nnk@google.com; >> selinux@tycho.nsa.gov >> Cc: Yongqin Liu <yongqin.liu@linaro.org>; Paul Moore <paul@paul-moore.com> >> Subject: Re: [PATCH] quick selinux support for tracefs >> >> On 12/06/2016 12:24 PM, william.c.roberts@intel.com wrote: >>> From: Yongqin Liu <yongqin.liu@linaro.org> >>> >>> Here is just the quick fix for tracefs with selinux. >>> just add tracefs to the list of whitelisted filesystem types in >>> selinux_is_sblabel_mnt(), but the right fix would be to generalize >>> this logic as described in the last item on the todo list, >>> https://bitbucket.org/seandroid/wiki/wiki/ToDo >>> >>> Change-Id: I2aa803ccffbcd2802a7287514da7648e6b364157 >> >> Please rewrite the subject line and patch description per the kernel's submission >> guidelines, drop the Change-Id and the link to the SEAndroid todo list, and don't >> say that this is a quick fix but > > Why would anyone do that for this patch when below you say it won’t be merged unless > we fix issue #2? I didn't say it couldn't be merged; I said it isn't a good idea to say "this is a quick fix but the right fix is X" in an upstream patch submission if you want it to be merged, unless it is for a serious security or stability bug that needs to be fixed right away. > > the right fix is something else if you want this to >> actually be merged. Because in that case, you ought to just implement the right >> fix. There is now an upstream kernel issue for the right fix: >> https://github.com/SELinuxProject/selinux-kernel/issues/2 > > The other question here is tracefs safe to label in this fashion, I would assume yes. > Looking through I didn't see any eviction code. Yes, the inodes are pinned.
> -----Original Message----- > From: Stephen Smalley [mailto:sds@tycho.nsa.gov] > Sent: Tuesday, December 6, 2016 10:01 AM > To: Roberts, William C <william.c.roberts@intel.com>; nnk@google.com; > selinux@tycho.nsa.gov > Cc: Yongqin Liu <yongqin.liu@linaro.org>; Paul Moore <paul@paul-moore.com> > Subject: Re: [PATCH] quick selinux support for tracefs > > On 12/06/2016 12:50 PM, Roberts, William C wrote: > > > > > >> -----Original Message----- > >> From: Stephen Smalley [mailto:sds@tycho.nsa.gov] > >> Sent: Tuesday, December 6, 2016 9:41 AM > >> To: Roberts, William C <william.c.roberts@intel.com>; nnk@google.com; > >> selinux@tycho.nsa.gov > >> Cc: Yongqin Liu <yongqin.liu@linaro.org>; Paul Moore > >> <paul@paul-moore.com> > >> Subject: Re: [PATCH] quick selinux support for tracefs > >> > >> On 12/06/2016 12:24 PM, william.c.roberts@intel.com wrote: > >>> From: Yongqin Liu <yongqin.liu@linaro.org> > >>> > >>> Here is just the quick fix for tracefs with selinux. > >>> just add tracefs to the list of whitelisted filesystem types in > >>> selinux_is_sblabel_mnt(), but the right fix would be to generalize > >>> this logic as described in the last item on the todo list, > >>> https://bitbucket.org/seandroid/wiki/wiki/ToDo > >>> > >>> Change-Id: I2aa803ccffbcd2802a7287514da7648e6b364157 > >> > >> Please rewrite the subject line and patch description per the > >> kernel's submission guidelines, drop the Change-Id and the link to > >> the SEAndroid todo list, and don't say that this is a quick fix but > > > > Why would anyone do that for this patch when below you say it won’t be > > merged unless we fix issue #2? > > I didn't say it couldn't be merged; I said it isn't a good idea to say "this is a quick fix > but the right fix is X" in an upstream patch submission if you want it to be > merged, unless it is for a serious security or stability bug that needs to be fixed > right away. Oh sure, but I didn't right that patch message, Liu Yonggin is tha author, ill fix up the message and resubmit preserving him as the author. > > > > > the right fix is something else if you want this to > >> actually be merged. Because in that case, you ought to just > >> implement the right fix. There is now an upstream kernel issue for the right > fix: > >> https://github.com/SELinuxProject/selinux-kernel/issues/2 > > > > The other question here is tracefs safe to label in this fashion, I would assume > yes. > > Looking through I didn't see any eviction code. > > Yes, the inodes are pinned.
Hi, Roberts On 7 December 2016 at 02:05, Roberts, William C <william.c.roberts@intel.com > wrote: > > > > -----Original Message----- > > From: Stephen Smalley [mailto:sds@tycho.nsa.gov] > > Sent: Tuesday, December 6, 2016 10:01 AM > > To: Roberts, William C <william.c.roberts@intel.com>; nnk@google.com; > > selinux@tycho.nsa.gov > > Cc: Yongqin Liu <yongqin.liu@linaro.org>; Paul Moore < > paul@paul-moore.com> > > Subject: Re: [PATCH] quick selinux support for tracefs > > > > On 12/06/2016 12:50 PM, Roberts, William C wrote: > > > > > > > > >> -----Original Message----- > > >> From: Stephen Smalley [mailto:sds@tycho.nsa.gov] > > >> Sent: Tuesday, December 6, 2016 9:41 AM > > >> To: Roberts, William C <william.c.roberts@intel.com>; nnk@google.com; > > >> selinux@tycho.nsa.gov > > >> Cc: Yongqin Liu <yongqin.liu@linaro.org>; Paul Moore > > >> <paul@paul-moore.com> > > >> Subject: Re: [PATCH] quick selinux support for tracefs > > >> > > >> On 12/06/2016 12:24 PM, william.c.roberts@intel.com wrote: > > >>> From: Yongqin Liu <yongqin.liu@linaro.org> > > >>> > > >>> Here is just the quick fix for tracefs with selinux. > > >>> just add tracefs to the list of whitelisted filesystem types in > > >>> selinux_is_sblabel_mnt(), but the right fix would be to generalize > > >>> this logic as described in the last item on the todo list, > > >>> https://bitbucket.org/seandroid/wiki/wiki/ToDo > > >>> > > >>> Change-Id: I2aa803ccffbcd2802a7287514da7648e6b364157 > > >> > > >> Please rewrite the subject line and patch description per the > > >> kernel's submission guidelines, drop the Change-Id and the link to > > >> the SEAndroid todo list, and don't say that this is a quick fix but > > > > > > Why would anyone do that for this patch when below you say it won’t be > > > merged unless we fix issue #2? > > > > I didn't say it couldn't be merged; I said it isn't a good idea to say > "this is a quick fix > > but the right fix is X" in an upstream patch submission if you want it > to be > > merged, unless it is for a serious security or stability bug that needs > to be fixed > > right away. > > Oh sure, but I didn't right that patch message, Liu Yonggin is tha author, > ill fix up the > message and resubmit preserving him as the author. > This change was submitted to http://android-review.googlesource.com/ via gerrit instructions before, and now seems that it is going to be submitted via the instructions for kernel changes which I am not familiar. If you like, please help to update the patch message and submit the patch. Or you could share me the instructions on how to do that, and then I will submit the changes with new message following your instructions. Thanks, Yongqin Liu > > > > > > > > > the right fix is something else if you want this to > > >> actually be merged. Because in that case, you ought to just > > >> implement the right fix. There is now an upstream kernel issue for > the right > > fix: > > >> https://github.com/SELinuxProject/selinux-kernel/issues/2 > > > > > > The other question here is tracefs safe to label in this fashion, I > would assume > > yes. > > > Looking through I didn't see any eviction code. > > > > Yes, the inodes are pinned. > >
Yes it was submitted to Android’s Gerrit Here by you: https://android-review.googlesource.com/#/c/220895/ I already modified the commit message and got an Ack on V3 from Stephen: http://marc.info/?l=selinux&m=148105682211911&w=2 From: YongQin Liu [mailto:yongqin.liu@linaro.org] Sent: Tuesday, December 6, 2016 11:06 PM To: Roberts, William C <william.c.roberts@intel.com> Cc: Stephen Smalley <sds@tycho.nsa.gov>; nnk@google.com; selinux@tycho.nsa.gov; Paul Moore <paul@paul-moore.com> Subject: Re: [PATCH] quick selinux support for tracefs Hi, Roberts On 7 December 2016 at 02:05, Roberts, William C <william.c.roberts@intel.com<mailto:william.c.roberts@intel.com>> wrote: > -----Original Message----- > From: Stephen Smalley [mailto:sds@tycho.nsa.gov<mailto:sds@tycho.nsa.gov>] > Sent: Tuesday, December 6, 2016 10:01 AM > To: Roberts, William C <william.c.roberts@intel.com<mailto:william.c.roberts@intel.com>>; nnk@google.com<mailto:nnk@google.com>; > selinux@tycho.nsa.gov<mailto:selinux@tycho.nsa.gov> > Cc: Yongqin Liu <yongqin.liu@linaro.org<mailto:yongqin.liu@linaro.org>>; Paul Moore <paul@paul-moore.com<mailto:paul@paul-moore.com>> > Subject: Re: [PATCH] quick selinux support for tracefs > > On 12/06/2016 12:50 PM, Roberts, William C wrote: > > > > > >> -----Original Message----- > >> From: Stephen Smalley [mailto:sds@tycho.nsa.gov<mailto:sds@tycho.nsa.gov>] > >> Sent: Tuesday, December 6, 2016 9:41 AM > >> To: Roberts, William C <william.c.roberts@intel.com<mailto:william.c.roberts@intel.com>>; nnk@google.com<mailto:nnk@google.com>; > >> selinux@tycho.nsa.gov<mailto:selinux@tycho.nsa.gov> > >> Cc: Yongqin Liu <yongqin.liu@linaro.org<mailto:yongqin.liu@linaro.org>>; Paul Moore > >> <paul@paul-moore.com<mailto:paul@paul-moore.com>> > >> Subject: Re: [PATCH] quick selinux support for tracefs > >> > >> On 12/06/2016 12:24 PM, william.c.roberts@intel.com<mailto:william.c.roberts@intel.com> wrote: > >>> From: Yongqin Liu <yongqin.liu@linaro.org<mailto:yongqin.liu@linaro.org>> > >>> > >>> Here is just the quick fix for tracefs with selinux. > >>> just add tracefs to the list of whitelisted filesystem types in > >>> selinux_is_sblabel_mnt(), but the right fix would be to generalize > >>> this logic as described in the last item on the todo list, > >>> https://bitbucket.org/seandroid/wiki/wiki/ToDo > >>> > >>> Change-Id: I2aa803ccffbcd2802a7287514da7648e6b364157 > >> > >> Please rewrite the subject line and patch description per the > >> kernel's submission guidelines, drop the Change-Id and the link to > >> the SEAndroid todo list, and don't say that this is a quick fix but > > > > Why would anyone do that for this patch when below you say it won’t be > > merged unless we fix issue #2? > > I didn't say it couldn't be merged; I said it isn't a good idea to say "this is a quick fix > but the right fix is X" in an upstream patch submission if you want it to be > merged, unless it is for a serious security or stability bug that needs to be fixed > right away. Oh sure, but I didn't right that patch message, Liu Yonggin is tha author, ill fix up the message and resubmit preserving him as the author. This change was submitted to http://android-review.googlesource.com/ via gerrit instructions before, and now seems that it is going to be submitted via the instructions for kernel changes which I am not familiar. If you like, please help to update the patch message and submit the patch. Or you could share me the instructions on how to do that, and then I will submit the changes with new message following your instructions. Thanks, Yongqin Liu > > > > > the right fix is something else if you want this to > >> actually be merged. Because in that case, you ought to just > >> implement the right fix. There is now an upstream kernel issue for the right > fix: > >> https://github.com/SELinuxProject/selinux-kernel/issues/2 > > > > The other question here is tracefs safe to label in this fashion, I would assume > yes. > > Looking through I didn't see any eviction code. > > Yes, the inodes are pinned. -- Best Regards, Yongqin Liu --------------------------------------------------------------- #mailing list linaro-android@lists.linaro.org<mailto:linaro-dev@lists.linaro.org> http://lists.linaro.org/mailman/listinfo/linaro-android
On 7 December 2016 at 18:59, Roberts, William C <william.c.roberts@intel.com > wrote: > Yes it was submitted to Android’s Gerrit Here by you: > > https://android-review.googlesource.com/#/c/220895/ > > > > I already modified the commit message and got an Ack on V3 from Stephen: > > http://marc.info/?l=selinux&m=148105682211911&w=2 > > > Thanks, Roberts! > > > *From:* YongQin Liu [mailto:yongqin.liu@linaro.org] > *Sent:* Tuesday, December 6, 2016 11:06 PM > *To:* Roberts, William C <william.c.roberts@intel.com> > *Cc:* Stephen Smalley <sds@tycho.nsa.gov>; nnk@google.com; > selinux@tycho.nsa.gov; Paul Moore <paul@paul-moore.com> > > *Subject:* Re: [PATCH] quick selinux support for tracefs > > > > Hi, Roberts > > > > On 7 December 2016 at 02:05, Roberts, William C < > william.c.roberts@intel.com> wrote: > > > > > -----Original Message----- > > From: Stephen Smalley [mailto:sds@tycho.nsa.gov] > > > Sent: Tuesday, December 6, 2016 10:01 AM > > To: Roberts, William C <william.c.roberts@intel.com>; nnk@google.com; > > selinux@tycho.nsa.gov > > Cc: Yongqin Liu <yongqin.liu@linaro.org>; Paul Moore < > paul@paul-moore.com> > > Subject: Re: [PATCH] quick selinux support for tracefs > > > > On 12/06/2016 12:50 PM, Roberts, William C wrote: > > > > > > > > >> -----Original Message----- > > >> From: Stephen Smalley [mailto:sds@tycho.nsa.gov] > > >> Sent: Tuesday, December 6, 2016 9:41 AM > > >> To: Roberts, William C <william.c.roberts@intel.com>; nnk@google.com; > > >> selinux@tycho.nsa.gov > > >> Cc: Yongqin Liu <yongqin.liu@linaro.org>; Paul Moore > > >> <paul@paul-moore.com> > > >> Subject: Re: [PATCH] quick selinux support for tracefs > > >> > > >> On 12/06/2016 12:24 PM, william.c.roberts@intel.com wrote: > > >>> From: Yongqin Liu <yongqin.liu@linaro.org> > > >>> > > >>> Here is just the quick fix for tracefs with selinux. > > >>> just add tracefs to the list of whitelisted filesystem types in > > >>> selinux_is_sblabel_mnt(), but the right fix would be to generalize > > >>> this logic as described in the last item on the todo list, > > >>> https://bitbucket.org/seandroid/wiki/wiki/ToDo > > >>> > > >>> Change-Id: I2aa803ccffbcd2802a7287514da7648e6b364157 > > >> > > >> Please rewrite the subject line and patch description per the > > >> kernel's submission guidelines, drop the Change-Id and the link to > > >> the SEAndroid todo list, and don't say that this is a quick fix but > > > > > > Why would anyone do that for this patch when below you say it won’t be > > > merged unless we fix issue #2? > > > > I didn't say it couldn't be merged; I said it isn't a good idea to say > "this is a quick fix > > but the right fix is X" in an upstream patch submission if you want it > to be > > merged, unless it is for a serious security or stability bug that needs > to be fixed > > right away. > > Oh sure, but I didn't right that patch message, Liu Yonggin is tha author, > ill fix up the > message and resubmit preserving him as the author. > > > > This change was submitted to http://android-review.googlesource.com/ via > gerrit instructions before, > > and now seems that it is going to be submitted via the instructions for > kernel changes which I am not familiar. > > > > If you like, please help to update the patch message and submit the patch. > > > > Or you could share me the instructions on how to do that, and then I will > submit the changes with new message following your instructions. > > > > Thanks, > > Yongqin Liu > > > > > > > > > > the right fix is something else if you want this to > > >> actually be merged. Because in that case, you ought to just > > >> implement the right fix. There is now an upstream kernel issue for > the right > > fix: > > >> https://github.com/SELinuxProject/selinux-kernel/issues/2 > > > > > > The other question here is tracefs safe to label in this fashion, I > would assume > > yes. > > > Looking through I didn't see any eviction code. > > > > Yes, the inodes are pinned. > > > > > > -- > > Best Regards, > > Yongqin Liu > > --------------------------------------------------------------- > > #mailing list > > linaro-android@lists.linaro.org <linaro-dev@lists.linaro.org> > > http://lists.linaro.org/mailman/listinfo/linaro-android >
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 09fd610..24bd84d 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -491,6 +491,7 @@ static int selinux_is_sblabel_mnt(struct super_block *sb) !strcmp(sb->s_type->name, "sysfs") || !strcmp(sb->s_type->name, "pstore") || !strcmp(sb->s_type->name, "debugfs") || + !strcmp(sb->s_type->name, "tracefs") || !strcmp(sb->s_type->name, "rootfs"); }