Fwd: A missing check bug in unix_create1().

Fwd: A missing check bug in unix_create1().

[Jinmeng Zhou]

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@kernel.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@vger.kernel.org<mailto:netdev@vger.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