| 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: <64bfd4a27a1fe_3dc9bb2944e@willemb.c.googlers.com.notmuch>
Date: Tue, 25 Jul 2023 09:56:50 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Breno Leitao <leitao@...ian.org>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: asml.silence@...il.com, axboe@...nel.dk, io-uring@...r.kernel.org,
netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
edumazet@...gle.com, pabeni@...hat.com,
linux-kernel@...r.kernel.org, leit@...a.com
Subject: Re: [PATCH 2/4] io_uring/cmd: Introduce SOCKET_URING_OP_GETSOCKOPT
Breno Leitao wrote:
> On Mon, Jul 24, 2023 at 06:58:04PM -0400, Willem de Bruijn wrote:
> > Breno Leitao wrote:
> > > Add support for getsockopt command (SOCKET_URING_OP_GETSOCKOPT), where
> > > level is SOL_SOCKET. This is leveraging the sockptr_t infrastructure,
> > > where a sockptr_t is either userspace or kernel space, and handled as
> > > such.
> > >
> > > Function io_uring_cmd_getsockopt() is inspired by __sys_getsockopt().
> > >
> > > Differently from the getsockopt(2), the optlen field is not a userspace
> > > pointers. In getsockopt(2), userspace provides optlen pointer, which is
> > > overwritten by the kernel. In this implementation, userspace passes a
> > > u32, and the new value is returned in cqe->res. I.e., optlen is not a
> > > pointer.
> > >
> > > Important to say that userspace needs to keep the pointer alive until
> > > the CQE is completed.
> > >
> > > Signed-off-by: Breno Leitao <leitao@...ian.org>
> > > ---
> > > include/uapi/linux/io_uring.h | 7 ++++++
> > > io_uring/uring_cmd.c | 43 +++++++++++++++++++++++++++++++++++
> > > 2 files changed, 50 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> > > index 9fc7195f25df..8152151080db 100644
> > > --- a/include/uapi/linux/io_uring.h
> > > +++ b/include/uapi/linux/io_uring.h
> > > @@ -43,6 +43,10 @@ struct io_uring_sqe {
> > > union {
> > > __u64 addr; /* pointer to buffer or iovecs */
> > > __u64 splice_off_in;
> > > + struct {
> > > + __u32 level;
> > > + __u32 optname;
> > > + };
> > > };
> > > __u32 len; /* buffer size or number of iovecs */
> > > union {
> > > @@ -79,6 +83,7 @@ struct io_uring_sqe {
> > > union {
> > > __s32 splice_fd_in;
> > > __u32 file_index;
> > > + __u32 optlen;
> > > struct {
> > > __u16 addr_len;
> > > __u16 __pad3[1];
> > > @@ -89,6 +94,7 @@ struct io_uring_sqe {
> > > __u64 addr3;
> > > __u64 __pad2[1];
> > > };
> > > + __u64 optval;
> > > /*
> > > * If the ring is initialized with IORING_SETUP_SQE128, then
> > > * this field is used for 80 bytes of arbitrary command data
> > > @@ -729,6 +735,7 @@ struct io_uring_recvmsg_out {
> > > enum {
> > > SOCKET_URING_OP_SIOCINQ = 0,
> > > SOCKET_URING_OP_SIOCOUTQ,
> > > + SOCKET_URING_OP_GETSOCKOPT,
> > > };
> > >
> > > #ifdef __cplusplus
> > > diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c
> > > index 8e7a03c1b20e..16c857cbf3b0 100644
> > > --- a/io_uring/uring_cmd.c
> > > +++ b/io_uring/uring_cmd.c
> > > @@ -166,6 +166,47 @@ int io_uring_cmd_import_fixed(u64 ubuf, unsigned long len, int rw,
> > > }
> > > EXPORT_SYMBOL_GPL(io_uring_cmd_import_fixed);
> > >
> > > +static inline int io_uring_cmd_getsockopt(struct socket *sock,
> > > + struct io_uring_cmd *cmd)
> > > +{
> > > + void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval));
> > > + int optname = READ_ONCE(cmd->sqe->optname);
> > > + int optlen = READ_ONCE(cmd->sqe->optlen);
> > > + int level = READ_ONCE(cmd->sqe->level);
> > > + void *koptval;
> > > + int err;
> > > +
> > > + err = security_socket_getsockopt(sock, level, optname);
> > > + if (err)
> > > + return err;
> > > +
> > > + koptval = kmalloc(optlen, GFP_KERNEL);
> > > + if (!koptval)
> > > + return -ENOMEM;
> >
> > This will try to kmalloc any length that userspace passes?
>
> Yes, this value is coming directly from userspace.
>
> > That is unnecessary ..
> > > +
> > > + err = copy_from_user(koptval, optval, optlen);
> > > + if (err)
> > > + goto fail;
> > > +
> > > + err = -EOPNOTSUPP;
> > > + if (level == SOL_SOCKET) {
> > > + err = sk_getsockopt(sock->sk, level, optname,
> > > + KERNEL_SOCKPTR(koptval),
> > > + KERNEL_SOCKPTR(&optlen));
> >
> > .. sk_getsockopt defines a union of acceptable fields, which
> > are all fairly small.
>
> Right, and they are all I need for SOL_SOCKET level for now.
>
> > I notice that BPF added BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN to
> > work around the issue of pre-allocating for the worst case.
>
> I am having a hard time how to understand how
> BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN gets the MAX_OPTLEN. Reading this
> function, it seems it is conditionally get_user().
>
>
> #define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen)
> ({
> int __ret = 0;
> if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT))
> get_user(__ret, optlen);
> __ret;
> })
>
> That said, how is BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN being used to
> workaroundthe pre-allocating for the worst case?
>
> > But that also needs to deal woth other getsockopt levels.
>
> Right, and we also have a similar kmalloc() on the setsockopt path
> (SOCKET_URING_OP_SETSOCKOPT).
>
> What about if I pass the userspace sockptr (USER_SOCKPTR) to the
> {g,s}etsockopt callback directly, instead of kmalloc() in io_uring(), as
> I was doing int the RFC[1]? It avoids any extra kmalloc at all.
That looks like a great solution to me.
Avoids the whole problem of kmalloc based on untrusted user input.
> Something as:
>
> static inline int io_uring_cmd_getsockopt(struct socket *sock,
> struct io_uring_cmd *cmd)
> {
> void __user *optval = u64_to_user_ptr(READ_ONCE(cmd->sqe->optval));
> int optlen = READ_ONCE(cmd->sqe->optlen);
> int optname = READ_ONCE(cmd->sqe->optname);
> int level = READ_ONCE(cmd->sqe->level);
> int err;
>
> err = security_socket_getsockopt(sock, level, optname);
> if (err)
> return err;
>
> if (level == SOL_SOCKET) {
> err = sk_getsockopt(sock->sk, level, optname,
> USER_SOCKPTR(optval),
> KERNEL_SOCKPTR(&optlen));
> if (err < 0)
> return err;
> return optlen;
> }
Do you have a plan to extend this to other levels?
No need to implement immediately, but it would be good to know
whether it is feasible to extend the current solution when the
need (inevitably) shows up.
>
> return -EOPNOTSUPP;
>
> Thanks for the review!
>
> [1] Link: https://lore.kernel.org/all/20230719102737.2513246-3-leitao@debian.org/
Powered by blists - more mailing lists