netdev - Re: [syzbot] [mptcp?] general protection fault in proc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250105205056.GK1977892@ZenIV>
Date: Sun, 5 Jan 2025 20:50:56 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Matthieu Baerts <matttbe@...nel.org>, davem@...emloft.net,
	geliang@...nel.org, horms@...nel.org, kuba@...nel.org,
	linux-kernel@...r.kernel.org, martineau@...nel.org,
	mptcp@...ts.linux.dev, netdev@...r.kernel.org, pabeni@...hat.com,
	syzkaller-bugs@...glegroups.com,
	syzbot <syzbot+e364f774c6f57f2c86d1@...kaller.appspotmail.com>
Subject: 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...