Re: [syzbot] [mptcp?] general protection fault in proc_scheduler

On Sun, Jan 05, 2025 at 07:54:34PM +0000, Al Viro wrote:

So I suspect that current->nsproxy->netns shouldn't be used in
per-netns sysctls for consistency sake (note that it can get more
serious than just consistency, if you have e.g. a spinlock taken
in something hanging off current netns to protect access to
something table->data points to).

As for the mitigation in fs/proc/proc_sysctl.c... might be useful,
if it comes with a clear comment about the reasons it's there.

FWIW, looks like we have two such in mptcp (with sysctls next to
those definitely accessing the netns of opener rather than reader/writer),
two in rds (both inconsistent on the write side -
        struct net *net = current->nsproxy->net_ns;
        int err;

        err = proc_dointvec_minmax(ctl, write, buffer, lenp, fpos);
        if (err < 0) {
                pr_warn("Invalid input. Must be >= %d\n",
                        *(int *)(ctl->extra1));
                return err;
        }
        if (write)
                rds_tcp_sysctl_reset(net);
will modify ctl->data, which points to &rtn->{snd,rcv}buf_size, with
rtn == net_generic(net, rds_tcp_netid) and net being for opener's netns
and then call rds_tcp_sysctl_reset(net) with net being the writer's
netns) and 6 in sctp.  At least some of sctp ones are also inconsistent
on the write side; e.g.
static int proc_sctp_do_rto_min(const struct ctl_table *ctl, int write,
                                void *buffer, size_t *lenp, loff_t *ppos)
{
        struct net *net = current->nsproxy->net_ns;
        unsigned int min = *(unsigned int *) ctl->extra1;
        unsigned int max = *(unsigned int *) ctl->extra2;
        struct ctl_table tbl;
        int ret, new_value;

        memset(&tbl, 0, sizeof(struct ctl_table));
        tbl.maxlen = sizeof(unsigned int);

        if (write)
                tbl.data = &new_value;
        else
                tbl.data = &net->sctp.rto_min;

        ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
        if (write && ret == 0) {
                if (new_value > max || new_value < min)
                        return -EINVAL;

                net->sctp.rto_min = new_value;
        }

        return ret;
}
has max taken from ctl->extra2, which is &net->sctp.rto_max of the
opener's netns, but the value capped by that in stored into
net->sctp.rto_min of *writer's* netns.  So the logics that is supposed
to prevent rto_min > rto_max can be bypassed; no idea how much can that
escalate to, but it's clearly not what the code intends.

So I'd rather document the "don't assume that current->nsproxy->netns will
point to the same netns this ctl is for" and fix those 10 instances - at
least some smell seriously fishy.  It's not just the acct(2) weirdness and
the damage may be worse than an oops...