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
| ||
|
Message-ID: <df1c06c2a2b516e4adb5d74cf1f50935e745abdc.camel@redhat.com> Date: Thu, 16 Feb 2023 14:12:08 +0100 From: Paolo Abeni <pabeni@...hat.com> To: Chuck Lever <chuck.lever@...cle.com>, kuba@...nel.org, edumazet@...gle.com Cc: netdev@...r.kernel.org, hare@...e.com, dhowells@...hat.com, bcodding@...hat.com, kolga@...app.com, jmeneghi@...hat.com Subject: Re: [PATCH v4 1/2] net/handshake: Create a NETLINK service for handling handshake requests [partial feedback /me is still a bit lost in the code ;] On Wed, 2023-02-15 at 14:23 -0500, Chuck Lever wrote: > +/* > + * This function is careful to not close the socket. It merely removes > + * it from the file descriptor table so that it is no longer visible > + * to the calling process. > + */ > +static int handshake_genl_cmd_done(struct sk_buff *skb, struct genl_info *gi) > +{ > + struct nlattr *tb[HANDSHAKE_GENL_ATTR_MAX + 1]; > + struct handshake_req *req; > + struct socket *sock; > + int fd, status, err; > + > + err = genlmsg_parse(nlmsg_hdr(skb), &handshake_genl_family, tb, > + HANDSHAKE_GENL_ATTR_MAX, handshake_genl_policy, > + NULL); > + if (err) { > + pr_err_ratelimited("%s: genlmsg_parse() returned %d\n", > + __func__, err); > + return err; > + } > + > + if (!tb[HANDSHAKE_GENL_ATTR_SOCKFD]) > + return handshake_genl_status_reply(skb, gi, -EINVAL); > + err = 0; > + fd = nla_get_u32(tb[HANDSHAKE_GENL_ATTR_SOCKFD]); > + sock = sockfd_lookup(fd, &err); > + if (err) > + return handshake_genl_status_reply(skb, gi, -EBADF); > + > + req = sock->sk->sk_handshake_req; > + if (req->hr_fd != fd) /* sanity */ > + return handshake_genl_status_reply(skb, gi, -EBADF); > + > + status = -EIO; > + if (tb[HANDSHAKE_GENL_ATTR_SESS_STATUS]) > + status = nla_get_u32(tb[HANDSHAKE_GENL_ATTR_SESS_STATUS]); > + > + put_unused_fd(req->hr_fd); If I read correctly, at this point the user-space is expected to have already closed hr_fd , but that is not enforced, right? a buggy or malicious user-space could cause bad things not closing such fd. Can we use sockfd_put(sock) instead? will make the code more readable, I think. BTW I don't think there is any problem with the sock->sk dereference above, the fd reference count will prevent __sock_release from being called. [...] > +static void __net_exit handshake_net_exit(struct net *net) > +{ > + struct handshake_req *req; > + LIST_HEAD(requests); > + > + /* > + * XXX: This drains the net's pending list, but does > + * nothing about requests that have been accepted > + * and are in progress. > + */ > + spin_lock(&net->hs_lock); > + list_splice_init(&requests, &net->hs_requests); > + spin_unlock(&net->hs_lock); If I read correctly accepted, uncompleted reqs are leaked. I think that could be prevented installing a custom sk_destructor in sock->sk tacking care of freeing the sk->sk_handshake_req. The existing/old sk_destructor - if any - could be stored in an additional sk_handshake_req field and tail-called by the req's one. [...] > +/* > + * This limit is to prevent slow remotes from causing denial of service. > + * A ulimit-style tunable might be used instead. > + */ > +#define HANDSHAKE_PENDING_MAX (10) I liked the idea of a core mem based limit ;) not a big deal anyway ;) > + > +struct handshake_req *handshake_req_get(struct handshake_req *req) > +{ > + return likely(refcount_inc_not_zero(&req->hr_ref)) ? req : NULL; > +} It's unclear to me under which circumstances the refcount should be > 1: AFAICS the req should have always a single owner: initially the creator, then the accept queue and finally the user-space serving the request. Cheers, Paolo
Powered by blists - more mailing lists