| Message ID | 20241125104011.36552-5-cgoettsche@seltendoof.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/11] coccinelle: Add script to reorder capable() calls | expand |
Hi Christian, On Mon, Nov 25, 2024 at 11:39:58AM +0100, Christian Göttsche wrote: > From: Christian Göttsche <cgzones@googlemail.com> > > capable() calls refer to enabled LSMs whether to permit or deny the > request. This is relevant in connection with SELinux, where a > capability check results in a policy decision and by default a denial > message on insufficient permission is issued. > It can lead to three undesired cases: > 1. A denial message is generated, even in case the operation was an > unprivileged one and thus the syscall succeeded, creating noise. > 2. To avoid the noise from 1. the policy writer adds a rule to ignore > those denial messages, hiding future syscalls, where the task > performs an actual privileged operation, leading to hidden limited > functionality of that task. > 3. To avoid the noise from 1. the policy writer adds a rule to permit > the task the requested capability, while it does not need it, > violating the principle of least privilege. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > drivers/gpu/drm/panthor/panthor_drv.c | 2 +- > fs/ubifs/budget.c | 5 +++-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index ac7e53f6e3f0..2de0c3627fbf 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -791,7 +791,7 @@ static int group_priority_permit(struct drm_file *file, > return 0; > > /* Higher priorities require CAP_SYS_NICE or DRM_MASTER */ > - if (capable(CAP_SYS_NICE) || drm_is_current_master(file)) > + if (drm_is_current_master(file) || capable(CAP_SYS_NICE)) > return 0; > > return -EACCES; Can the patch above be split into a separate one? It's for a different subsystem than ubifs. Otherwise, it looks good to me, so you can add my Reviewed-by to the new patch. Best regards, Liviu > diff --git a/fs/ubifs/budget.c b/fs/ubifs/budget.c > index d76eb7b39f56..6137aeadec3f 100644 > --- a/fs/ubifs/budget.c > +++ b/fs/ubifs/budget.c > @@ -256,8 +256,9 @@ long long ubifs_calc_available(const struct ubifs_info *c, int min_idx_lebs) > */ > static int can_use_rp(struct ubifs_info *c) > { > - if (uid_eq(current_fsuid(), c->rp_uid) || capable(CAP_SYS_RESOURCE) || > - (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid))) > + if (uid_eq(current_fsuid(), c->rp_uid) || > + (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid)) || > + capable(CAP_SYS_RESOURCE)) > return 1; > return 0; > } > -- > 2.45.2 >
----- Ursprüngliche Mail ----- > Von: "Christian Göttsche" <cgoettsche@seltendoof.de> > capable() calls refer to enabled LSMs whether to permit or deny the > request. This is relevant in connection with SELinux, where a > capability check results in a policy decision and by default a denial > message on insufficient permission is issued. > It can lead to three undesired cases: > 1. A denial message is generated, even in case the operation was an > unprivileged one and thus the syscall succeeded, creating noise. > 2. To avoid the noise from 1. the policy writer adds a rule to ignore > those denial messages, hiding future syscalls, where the task > performs an actual privileged operation, leading to hidden limited > functionality of that task. > 3. To avoid the noise from 1. the policy writer adds a rule to permit > the task the requested capability, while it does not need it, > violating the principle of least privilege. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > drivers/gpu/drm/panthor/panthor_drv.c | 2 +- This change is unrelated, please remove it. > fs/ubifs/budget.c | 5 +++-- > 2 files changed, 4 insertions(+), 3 deletions(-) [...] > diff --git a/fs/ubifs/budget.c b/fs/ubifs/budget.c > index d76eb7b39f56..6137aeadec3f 100644 > --- a/fs/ubifs/budget.c > +++ b/fs/ubifs/budget.c > @@ -256,8 +256,9 @@ long long ubifs_calc_available(const struct ubifs_info *c, > int min_idx_lebs) > */ > static int can_use_rp(struct ubifs_info *c) > { > - if (uid_eq(current_fsuid(), c->rp_uid) || capable(CAP_SYS_RESOURCE) || > - (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid))) > + if (uid_eq(current_fsuid(), c->rp_uid) || > + (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid)) || > + capable(CAP_SYS_RESOURCE)) > return 1; > return 0; > } The UBIFS part looks ok: Acked-by: Richard Weinberger <richard@nod.at> Since I was not CC'ed to the whole series, I miss a lot of context. Will this series merged as a whole? By whom? Thanks, //richard
On Mon, 25 Nov 2024 at 12:31, Richard Weinberger <richard@nod.at> wrote: > > ----- Ursprüngliche Mail ----- > > Von: "Christian Göttsche" <cgoettsche@seltendoof.de> > > capable() calls refer to enabled LSMs whether to permit or deny the > > request. This is relevant in connection with SELinux, where a > > capability check results in a policy decision and by default a denial > > message on insufficient permission is issued. > > It can lead to three undesired cases: > > 1. A denial message is generated, even in case the operation was an > > unprivileged one and thus the syscall succeeded, creating noise. > > 2. To avoid the noise from 1. the policy writer adds a rule to ignore > > those denial messages, hiding future syscalls, where the task > > performs an actual privileged operation, leading to hidden limited > > functionality of that task. > > 3. To avoid the noise from 1. the policy writer adds a rule to permit > > the task the requested capability, while it does not need it, > > violating the principle of least privilege. > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > drivers/gpu/drm/panthor/panthor_drv.c | 2 +- > > This change is unrelated, please remove it. Sorry, somehow these two changes got erroneously combined in a single patch. I'll send a v2 with them split into separate ones. > > > fs/ubifs/budget.c | 5 +++-- > > 2 files changed, 4 insertions(+), 3 deletions(-) > > [...] > > > diff --git a/fs/ubifs/budget.c b/fs/ubifs/budget.c > > index d76eb7b39f56..6137aeadec3f 100644 > > --- a/fs/ubifs/budget.c > > +++ b/fs/ubifs/budget.c > > @@ -256,8 +256,9 @@ long long ubifs_calc_available(const struct ubifs_info *c, > > int min_idx_lebs) > > */ > > static int can_use_rp(struct ubifs_info *c) > > { > > - if (uid_eq(current_fsuid(), c->rp_uid) || capable(CAP_SYS_RESOURCE) || > > - (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid))) > > + if (uid_eq(current_fsuid(), c->rp_uid) || > > + (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid)) || > > + capable(CAP_SYS_RESOURCE)) > > return 1; > > return 0; > > } > > The UBIFS part looks ok: > > Acked-by: Richard Weinberger <richard@nod.at> > > Since I was not CC'ed to the whole series, I miss a lot of context. The series consists of similar patches to other subsystems and a coccinelle script addition. See https://lore.kernel.org/linux-security-module/20241125104011.36552-11-cgoettsche@seltendoof.de/#t > Will this series merged as a whole? By whom? > > Thanks, > //richard
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c index ac7e53f6e3f0..2de0c3627fbf 100644 --- a/drivers/gpu/drm/panthor/panthor_drv.c +++ b/drivers/gpu/drm/panthor/panthor_drv.c @@ -791,7 +791,7 @@ static int group_priority_permit(struct drm_file *file, return 0; /* Higher priorities require CAP_SYS_NICE or DRM_MASTER */ - if (capable(CAP_SYS_NICE) || drm_is_current_master(file)) + if (drm_is_current_master(file) || capable(CAP_SYS_NICE)) return 0; return -EACCES; diff --git a/fs/ubifs/budget.c b/fs/ubifs/budget.c index d76eb7b39f56..6137aeadec3f 100644 --- a/fs/ubifs/budget.c +++ b/fs/ubifs/budget.c @@ -256,8 +256,9 @@ long long ubifs_calc_available(const struct ubifs_info *c, int min_idx_lebs) */ static int can_use_rp(struct ubifs_info *c) { - if (uid_eq(current_fsuid(), c->rp_uid) || capable(CAP_SYS_RESOURCE) || - (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid))) + if (uid_eq(current_fsuid(), c->rp_uid) || + (!gid_eq(c->rp_gid, GLOBAL_ROOT_GID) && in_group_p(c->rp_gid)) || + capable(CAP_SYS_RESOURCE)) return 1; return 0; }