On Mon, Sep 19, 2022 at 10:26:39AM -0500, Eric W. Biederman wrote:
> >
> > diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
> > index ef313ecfb53a..87eb1b1e42fa 100644
> > --- a/ipc/ipc_sysctl.c
> > +++ b/ipc/ipc_sysctl.c
> > @@ -192,23 +192,47 @@ static int set_is_seen(struct ctl_table_set *set)
> >
> > static int ipc_permissions(struct ctl_table_header *head, struct ctl_table *table)
> > {
> > - int mode = table->mode;
> > -
> > -#ifdef CONFIG_CHECKPOINT_RESTORE
> > struct ipc_namespace *ns = current->nsproxy->ipc_ns;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Historically that was the best we could do. But now that we have
> an ipc_set member in struct ipc_namespace you can use container_of
> to compute this value.
>
> For a permission check that is much safer.
Yes. It make sense.
> > +#ifdef CONFIG_CHECKPOINT_RESTORE
> > if (((table->data == &ns->ids[IPC_SEM_IDS].next_id) ||
> > (table->data == &ns->ids[IPC_MSG_IDS].next_id) ||
> > (table->data == &ns->ids[IPC_SHM_IDS].next_id)) &&
> > checkpoint_restore_ns_capable(ns->user_ns))
> > - mode = 0666;
> > + return 0666;
> > #endif
> > - return mode;
> > + if (ns->user_ns != &init_user_ns) {
> > + kuid_t ns_root_uid = make_kuid(ns->user_ns, 0);
> > + kgid_t ns_root_gid = make_kgid(ns->user_ns, 0);
> > +
> > + if (uid_valid(ns_root_uid) && uid_eq(current_euid(), ns_root_uid))
> > + return table->mode >> 6;
> > +
> > + if (gid_valid(ns_root_gid) && in_egroup_p(ns_root_gid))
> > + return table->mode >> 3;
>
> >From 10,000 fee this is fine. But this has to interact with
> test_perm in proc_systl.c. So can you please do what
> net_ctl_permissions does and replicate the chosen mode all through
> the mode line.
>
> Perhaps something like:
>
> kuid_t ns_root_uid;
> kgid_t ns_root_gid
>
> ipc_set_ownership(head, table, &ns_root_uid, &ns_root_gid);
>
> #ifdef CONFIG_CHECKPOINT_RESTORE
> if (...)
> mode = 0666;
> else
> #endif
> if (uid_eq(current_euid(), ns_root_uid))
> mode >>= 6;
>
> else if (uid_eq(in_group_p(ns_root_gid))
> mode >>= 3;
>
> mode &= 7;
> mode = (mode << 6) | (mode << 3) | mode;
> return mode;
>
>
> If we always pass through the same logic there is the advantage that we
> will always test it, and there is less room for bugs to slip through.
>
> I added a couple of unnecessary simplifications in there that I just
> saw as I was writing my example code.
Thanks! It looks better. I'll fix it and send a new version.
--
Rgrds, legion
