| Message ID | 20241205010919.1419288-1-tweek@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2] selinux: ignore unknown extended permissions | expand |
On Thu, Dec 5, 2024 at 6:42 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > Dec 5, 2024 02:09:39 Thiébaud Weksteen <tweek@google.com>: > > > When evaluating extended permissions, ignore unknown permissions instead > > of calling BUG(). This commit ensures that future permissions can be > > added without interfering with older kernels. > > > > Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls") > > Cc: stable@vger.kernel.org > > Signed-off-by: Thiébaud Weksteen <tweek@google.com> > > - BUG(); > > + pr_warn_once( > > + "SELinux: unknown extended permission (%u) will be ignored\n", > > + node->datum.u.xperms->specified); > > + return; > > } > > What about instead of logging once per boot at access decision time logging once per policyload at parse time, like suggested for patch https://patchwork.kernel.org/project/selinux/patch/20241115133619.114393-11-cgoettsche@seltendoof.de/ ? > I agree, warning when the policy is loaded makes more sense. For this particular bug, I am trying to keep the patch to a bare minimum as I intend to backport it to stable kernels (on Android, this is preventing us from deploying a policy compatible with both older and newer kernels). Maybe we could land the first version of this patch (without any warning message), with the understanding that your patch will land soon after?
On Dec 4, 2024 "=?UTF-8?q?Thi=C3=A9baud=20Weksteen?=" <tweek@google.com> wrote: > > When evaluating extended permissions, ignore unknown permissions instead > of calling BUG(). This commit ensures that future permissions can be > added without interfering with older kernels. > > Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls") > Cc: stable@vger.kernel.org > Signed-off-by: Thiébaud Weksteen <tweek@google.com> > --- > v2: Add pr_warn_once, remove other BUG() call for key.specified > > security/selinux/ss/services.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) Merged into selinux/stable-6.13, thanks! -- paul-moore.com
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 971c45d576ba..3d5c563cfc4c 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -979,7 +979,10 @@ void services_compute_xperms_decision(struct extended_perms_decision *xpermd, return; break; default: - BUG(); + pr_warn_once( + "SELinux: unknown extended permission (%u) will be ignored\n", + node->datum.u.xperms->specified); + return; } if (node->key.specified == AVTAB_XPERMS_ALLOWED) { @@ -998,7 +1001,8 @@ void services_compute_xperms_decision(struct extended_perms_decision *xpermd, &node->datum.u.xperms->perms, xpermd->dontaudit); } else { - BUG(); + pr_warn_once("SELinux: unknown specified key (%u)\n", + node->key.specified); } }
When evaluating extended permissions, ignore unknown permissions instead of calling BUG(). This commit ensures that future permissions can be added without interfering with older kernels. Fixes: fa1aa143ac4a ("selinux: extended permissions for ioctls") Cc: stable@vger.kernel.org Signed-off-by: Thiébaud Weksteen <tweek@google.com> --- v2: Add pr_warn_once, remove other BUG() call for key.specified security/selinux/ss/services.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)