diff mbox series

[v2] LSM: SafeSetID: fix UID printed instead of GID

Message ID 20230503064344.45825-1-aleksandr.mikhalitsyn@canonical.com
State New
Headers
Series [v2] LSM: SafeSetID: fix UID printed instead of GID |

Commit Message

Aleksandr Mikhalitsyn May 3, 2023, 6:43 a.m. UTC 
  pr_warn message clearly says that GID should be printed,
but we have UID there. Let's fix that.

Found accidentaly during the work on isolated user namespaces.

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
---
v2: __kuid_val -> __kgid_val
---
 security/safesetid/lsm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul Moore May 18, 2023, 6:59 p.m. UTC | #1 
On Wed, May 3, 2023 at 2:44 AM Alexander Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:
>
> pr_warn message clearly says that GID should be printed,
> but we have UID there. Let's fix that.
>
> Found accidentaly during the work on isolated user namespaces.
>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> ---
> v2: __kuid_val -> __kgid_val
> ---
>  security/safesetid/lsm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I'm assuming you're going to pick this up Micah?

Reviewed-by: Paul Moore <paul@paul-moore.com>

> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index e806739f7868..5be5894aa0ea 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -131,7 +131,7 @@ static int safesetid_security_capable(const struct cred *cred,
>                  * set*gid() (e.g. setting up userns gid mappings).
>                  */
>                 pr_warn("Operation requires CAP_SETGID, which is not available to GID %u for operations besides approved set*gid transitions\n",
> -                       __kuid_val(cred->uid));
> +                       __kgid_val(cred->gid));
>                 return -EPERM;
>         default:
>                 /* Error, the only capabilities were checking for is CAP_SETUID/GID */
> --
> 2.34.1
Aleksandr Mikhalitsyn June 6, 2023, 6:50 p.m. UTC | #2 
On Thu, May 18, 2023 at 8:59 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, May 3, 2023 at 2:44 AM Alexander Mikhalitsyn
> <aleksandr.mikhalitsyn@canonical.com> wrote:
> >
> > pr_warn message clearly says that GID should be printed,
> > but we have UID there. Let's fix that.
> >
> > Found accidentaly during the work on isolated user namespaces.
> >
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > ---
> > v2: __kuid_val -> __kgid_val
> > ---
> >  security/safesetid/lsm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I'm assuming you're going to pick this up Micah?
>
> Reviewed-by: Paul Moore <paul@paul-moore.com>

Dear Paul!

Thanks for your review!

Gentle ping to Micah Morton :-)

Kind regards,
Alex

>
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index e806739f7868..5be5894aa0ea 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -131,7 +131,7 @@ static int safesetid_security_capable(const struct cred *cred,
> >                  * set*gid() (e.g. setting up userns gid mappings).
> >                  */
> >                 pr_warn("Operation requires CAP_SETGID, which is not available to GID %u for operations besides approved set*gid transitions\n",
> > -                       __kuid_val(cred->uid));
> > +                       __kgid_val(cred->gid));
> >                 return -EPERM;
> >         default:
> >                 /* Error, the only capabilities were checking for is CAP_SETUID/GID */
> > --
> > 2.34.1
>
> --
> paul-moore.com
Paul Moore June 6, 2023, 9:13 p.m. UTC | #3 
On Tue, Jun 6, 2023 at 2:50 PM Aleksandr Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:
> On Thu, May 18, 2023 at 8:59 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, May 3, 2023 at 2:44 AM Alexander Mikhalitsyn
> > <aleksandr.mikhalitsyn@canonical.com> wrote:
> > >
> > > pr_warn message clearly says that GID should be printed,
> > > but we have UID there. Let's fix that.
> > >
> > > Found accidentaly during the work on isolated user namespaces.
> > >
> > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > ---
> > > v2: __kuid_val -> __kgid_val
> > > ---
> > >  security/safesetid/lsm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > I'm assuming you're going to pick this up Micah?
> >
> > Reviewed-by: Paul Moore <paul@paul-moore.com>
>
> Dear Paul!
>
> Thanks for your review!
>
> Gentle ping to Micah Morton :-)

Micah?

The right thing would be for Micah to merge this via the SafeSetID
tree, however, considering that it's been over a month with no
response, and this patch looks trivially correct, I can pick this up
via the LSM tree if we don't see anything from Micah this week.
Paul Moore June 8, 2023, 6:34 p.m. UTC | #4 
On Tue, Jun 6, 2023 at 5:13 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jun 6, 2023 at 2:50 PM Aleksandr Mikhalitsyn
> <aleksandr.mikhalitsyn@canonical.com> wrote:
> > On Thu, May 18, 2023 at 8:59 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, May 3, 2023 at 2:44 AM Alexander Mikhalitsyn
> > > <aleksandr.mikhalitsyn@canonical.com> wrote:
> > > >
> > > > pr_warn message clearly says that GID should be printed,
> > > > but we have UID there. Let's fix that.
> > > >
> > > > Found accidentaly during the work on isolated user namespaces.
> > > >
> > > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > > ---
> > > > v2: __kuid_val -> __kgid_val
> > > > ---
> > > >  security/safesetid/lsm.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > I'm assuming you're going to pick this up Micah?
> > >
> > > Reviewed-by: Paul Moore <paul@paul-moore.com>
> >
> > Dear Paul!
> >
> > Thanks for your review!
> >
> > Gentle ping to Micah Morton :-)
>
> Micah?
>
> The right thing would be for Micah to merge this via the SafeSetID
> tree, however, considering that it's been over a month with no
> response, and this patch looks trivially correct, I can pick this up
> via the LSM tree if we don't see anything from Micah this week.

Searching through all of the archives on lore I don't see any email
from Micah past August of 2022.  I'll still stick to the plan of
merging this via the LSM tree next week if we don't see any response
from Micah, but beyond this patch we may need to consider the
possibility that Micah has moved on from SafeSetID.

 * https://lore.kernel.org/all/?q=f%3Amortonm%40chromium.org
Paul Moore June 21, 2023, 12:30 a.m. UTC | #5 
On Thu, Jun 8, 2023 at 2:34 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jun 6, 2023 at 5:13 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Jun 6, 2023 at 2:50 PM Aleksandr Mikhalitsyn
> > <aleksandr.mikhalitsyn@canonical.com> wrote:
> > > On Thu, May 18, 2023 at 8:59 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Wed, May 3, 2023 at 2:44 AM Alexander Mikhalitsyn
> > > > <aleksandr.mikhalitsyn@canonical.com> wrote:
> > > > >
> > > > > pr_warn message clearly says that GID should be printed,
> > > > > but we have UID there. Let's fix that.
> > > > >
> > > > > Found accidentaly during the work on isolated user namespaces.
> > > > >
> > > > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > > > ---
> > > > > v2: __kuid_val -> __kgid_val
> > > > > ---
> > > > >  security/safesetid/lsm.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > I'm assuming you're going to pick this up Micah?
> > > >
> > > > Reviewed-by: Paul Moore <paul@paul-moore.com>
> > >
> > > Dear Paul!
> > >
> > > Thanks for your review!
> > >
> > > Gentle ping to Micah Morton :-)
> >
> > Micah?
> >
> > The right thing would be for Micah to merge this via the SafeSetID
> > tree, however, considering that it's been over a month with no
> > response, and this patch looks trivially correct, I can pick this up
> > via the LSM tree if we don't see anything from Micah this week.
>
> Searching through all of the archives on lore I don't see any email
> from Micah past August of 2022.  I'll still stick to the plan of
> merging this via the LSM tree next week if we don't see any response
> from Micah, but beyond this patch we may need to consider the
> possibility that Micah has moved on from SafeSetID.
>
>  * https://lore.kernel.org/all/?q=f%3Amortonm%40chromium.org

This fell through the cracks in my inbox last week, but I just went
ahead and merged this into lsm/next.

After the upcoming merge window closes we'll have to revisit
SafeSetID's status as "supported", we might need to demote it to
"maintained" or "odd fixes".
Aleksandr Mikhalitsyn June 21, 2023, 7:37 a.m. UTC | #6 
On Wed, Jun 21, 2023 at 2:30 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Jun 8, 2023 at 2:34 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Jun 6, 2023 at 5:13 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Tue, Jun 6, 2023 at 2:50 PM Aleksandr Mikhalitsyn
> > > <aleksandr.mikhalitsyn@canonical.com> wrote:
> > > > On Thu, May 18, 2023 at 8:59 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Wed, May 3, 2023 at 2:44 AM Alexander Mikhalitsyn
> > > > > <aleksandr.mikhalitsyn@canonical.com> wrote:
> > > > > >
> > > > > > pr_warn message clearly says that GID should be printed,
> > > > > > but we have UID there. Let's fix that.
> > > > > >
> > > > > > Found accidentaly during the work on isolated user namespaces.
> > > > > >
> > > > > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > > > > ---
> > > > > > v2: __kuid_val -> __kgid_val
> > > > > > ---
> > > > > >  security/safesetid/lsm.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > I'm assuming you're going to pick this up Micah?
> > > > >
> > > > > Reviewed-by: Paul Moore <paul@paul-moore.com>
> > > >
> > > > Dear Paul!
> > > >
> > > > Thanks for your review!
> > > >
> > > > Gentle ping to Micah Morton :-)
> > >
> > > Micah?
> > >
> > > The right thing would be for Micah to merge this via the SafeSetID
> > > tree, however, considering that it's been over a month with no
> > > response, and this patch looks trivially correct, I can pick this up
> > > via the LSM tree if we don't see anything from Micah this week.
> >
> > Searching through all of the archives on lore I don't see any email
> > from Micah past August of 2022.  I'll still stick to the plan of
> > merging this via the LSM tree next week if we don't see any response
> > from Micah, but beyond this patch we may need to consider the
> > possibility that Micah has moved on from SafeSetID.
> >
> >  * https://lore.kernel.org/all/?q=f%3Amortonm%40chromium.org

Hi Paul,

>
> This fell through the cracks in my inbox last week, but I just went
> ahead and merged this into lsm/next.

Thanks!

Kind regards,
Alex

>
> After the upcoming merge window closes we'll have to revisit
> SafeSetID's status as "supported", we might need to demote it to
> "maintained" or "odd fixes".
>
> --
> paul-moore.com
Micah Morton July 4, 2023, 6:24 p.m. UTC | #7 
On Wed, Jun 21, 2023 at 12:37 AM Aleksandr Mikhalitsyn
<aleksandr.mikhalitsyn@canonical.com> wrote:
>
> On Wed, Jun 21, 2023 at 2:30 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Thu, Jun 8, 2023 at 2:34 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Tue, Jun 6, 2023 at 5:13 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Tue, Jun 6, 2023 at 2:50 PM Aleksandr Mikhalitsyn
> > > > <aleksandr.mikhalitsyn@canonical.com> wrote:
> > > > > On Thu, May 18, 2023 at 8:59 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > On Wed, May 3, 2023 at 2:44 AM Alexander Mikhalitsyn
> > > > > > <aleksandr.mikhalitsyn@canonical.com> wrote:
> > > > > > >
> > > > > > > pr_warn message clearly says that GID should be printed,
> > > > > > > but we have UID there. Let's fix that.
> > > > > > >
> > > > > > > Found accidentaly during the work on isolated user namespaces.
> > > > > > >
> > > > > > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
> > > > > > > ---
> > > > > > > v2: __kuid_val -> __kgid_val
> > > > > > > ---
> > > > > > >  security/safesetid/lsm.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > I'm assuming you're going to pick this up Micah?
> > > > > >
> > > > > > Reviewed-by: Paul Moore <paul@paul-moore.com>
> > > > >
> > > > > Dear Paul!
> > > > >
> > > > > Thanks for your review!
> > > > >
> > > > > Gentle ping to Micah Morton :-)
> > > >
> > > > Micah?
> > > >
> > > > The right thing would be for Micah to merge this via the SafeSetID
> > > > tree, however, considering that it's been over a month with no
> > > > response, and this patch looks trivially correct, I can pick this up
> > > > via the LSM tree if we don't see anything from Micah this week.
> > >
> > > Searching through all of the archives on lore I don't see any email
> > > from Micah past August of 2022.  I'll still stick to the plan of
> > > merging this via the LSM tree next week if we don't see any response
> > > from Micah, but beyond this patch we may need to consider the
> > > possibility that Micah has moved on from SafeSetID.

Sorry guys, this is my first time checking my @chromium.org email in a
couple months. I have indeed moved on from being regularly plugged in
to the goings on of the linux-security-module mailing list. @Paul
Moore whatever you think is the best way forward here is good for me,
I can't really make any promises that I'll be checking this mailing
list on a regular basis.

> > >
> > >  * https://lore.kernel.org/all/?q=f%3Amortonm%40chromium.org
>
> Hi Paul,
>
> >
> > This fell through the cracks in my inbox last week, but I just went
> > ahead and merged this into lsm/next.
>
> Thanks!
>
> Kind regards,
> Alex
>
> >
> > After the upcoming merge window closes we'll have to revisit
> > SafeSetID's status as "supported", we might need to demote it to
> > "maintained" or "odd fixes".
> >
> > --
> > paul-moore.com
diff mbox series

Patch

diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index e806739f7868..5be5894aa0ea 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -131,7 +131,7 @@  static int safesetid_security_capable(const struct cred *cred,
 		 * set*gid() (e.g. setting up userns gid mappings).
 		 */
 		pr_warn("Operation requires CAP_SETGID, which is not available to GID %u for operations besides approved set*gid transitions\n",
-			__kuid_val(cred->uid));
+			__kgid_val(cred->gid));
 		return -EPERM;
 	default:
 		/* Error, the only capabilities were checking for is CAP_SETUID/GID */