[<prev] [next>] [day] [month] [year] [list]
Message-ID: <CAA-qYXj44McN44UVOCx_vanHXddR5ToVBTEir7mrmWO5GmoGcA@mail.gmail.com>
Date: Fri, 21 May 2021 22:09:12 +0800
From: Jinmeng Zhou <jjjinmeng.zhou@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc: netdev@...r.kernel.org, shenwenbosmile@...il.com
Subject: Fwd: A missing check bug in unix_create1().
Dear maintainers,
hi, our team has reported two bugs on Linux kernel v5.10.7 using
static analysis.
The first one is confirmed and we submit a patch to fix the bug, and
it is accepted.
For the second bug in function unix_create1(),
we are looking forward to having more experts' eyes on this. Thank you!
> On Fri, May 7, 2021 at 1:57 AM Jakub Kicinski <kuba@...nel.org> wrote:
> > On Thu, 6 May 2021 15:15:08 +0800 Jinmeng Zhou wrote:
> > hi, our team has found two bugs on Linux kernel v5.10.7 using static
> > analysis, a missing check bug and an inconsistent check bug.
> Thanks a lot for the report!
> > Before calling sk_alloc(), rawsock_create() uses capable() to check,
> > xsk_create() uses ns_capable() to check, while unix_create1() does not
> > check anything. As shown below, capable() actually checks the capability
> > of init_user_ns, however, ns_capable() is possibly checks other user namespace.
> >
> > 1. bool capable(int cap)
> > 2. {
> > 3. return ns_capable(&init_user_ns, cap);
> > 4. }
> >
> > As shown, rawsock_create() uses capable() to check CAP_NET_RAW that checks the capability of init_user_ns.
> > 1. // check capable() ////////////////////////////////////
> > 2. static int rawsock_create(struct net *net, struct socket *sock,
> > 3. const struct nfc_protocol *nfc_proto, int kern) {
> > 4. ...
> > 5. if (sock->type == SOCK_RAW) {
> > 6. if (!capable(CAP_NET_RAW))
> > 7. return -EPERM;
> > 8. sock->ops = &rawsock_raw_ops;
> > 9. } else {
> > 10. sock->ops = &rawsock_ops;
> > 11. }
> > 12. sk = sk_alloc(net, PF_NFC, GFP_ATOMIC, nfc_proto->proto, kern);
> > 13. ...
> > 14. }
>
> Indeed, this one should likely use the ns-aware check. Would you mind
> sending a fix? (please make sure you CC the authors of this code,
> folks who touched it most recently and other relevant developers you
> can think of).
>
> > Function xsk_create() checks the capability of net->user_ns before calling sk_alloc().
> > 1. // check ns_capable() ////////////////////////////////////
> > 2. static int xsk_create(struct net *net, struct socket *sock, int protocol,
> > 3. int kern)
> > 4. {
> > 5. struct xdp_sock *xs;
> > 6. struct sock *sk;
> > 7. if (!ns_capable(net->user_ns, CAP_NET_RAW))
> > 8. return -EPERM;
> > 9. if (sock->type != SOCK_RAW)
> > 10. return -ESOCKTNOSUPPORT;
> > 11. if (protocol)
> > 12. return -EPROTONOSUPPORT;
> > 13. sock->state = SS_UNCONNECTED;
> > 14. sk = sk_alloc(net, PF_XDP, GFP_KERNEL, &xsk_proto, kern);
> > 15. ...
> > 16. }
> >
> > There is no check before calling sk_alloc().
> > 1. // no check ////////////////////////////////////
> > 2. static struct sock *unix_create1(struct net *net, struct socket *sock, int kern)
> > 3. {
> > 4. struct sock *sk = NULL;
> > 5. struct unix_sock *u;
> > 6. atomic_long_inc(&unix_nr_socks);
> > 7. if (atomic_long_read(&unix_nr_socks) > 2 * get_max_files())
> > 8. goto out;
> > 9. sk = sk_alloc(net, PF_UNIX, GFP_KERNEL, &unix_proto, kern);
> > 10. ...
> > 11. }
>
> I believe this is fine, unix sockets don't really have a meaningful
> notion of raw packets. But please feel free to report this to other
> folks and perhaps CC the netdev@...r.kernel.org<mailto:netdev@...r.kernel.org> mailing list to get
> more eyes on this.
>
> > All these three functions are assigned to the create field of their corresponding struct.
> > 1. static const struct nfc_protocol rawsock_nfc_proto = {
> > 2. ...
> > 3. .create = rawsock_create
> > 4. };
> > 5. static const struct net_proto_family xsk_family_ops = {
> > 6. ...
> > 7. .create = xsk_create,
> > 8. ...
> > 9. };
> > 10. static const struct net_proto_family unix_family_ops = {
> > 11. ...
> > 12. .create = unix_create,
> > 13. ...
> > 14. };
> > (unix_create => unix_create1)
> >
> > They are used to create sock similarly, especially the last two
> > functions assigned to the same struct type, net_proto_family. Therefore,
> > we believe these functions share the similar functionality. There will
> > be bugs if they use different permission checks or some functions do not
> > have any check.
> >
> > Thanks!
> >
> >
> > Best regards,
> > Jinmeng Zhou
Sorry for the late reply, thanks again!
Best regards,
Jinmeng Zhou
Powered by blists - more mailing lists