netdev - Re: [PATCH 0/5] add initial io_uring

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 18 Apr 2023 15:41:24 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Breno Leitao <leitao@...ian.org>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        kuba@...nel.org
Cc:     Jens Axboe <axboe@...nel.dk>, David Ahern <dsahern@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        io-uring@...r.kernel.org, netdev@...r.kernel.org, kuba@...nel.org,
        asml.silence@...il.com, leit@...com, edumazet@...gle.com,
        pabeni@...hat.com, davem@...emloft.net, dccp@...r.kernel.org,
        mptcp@...ts.linux.dev, linux-kernel@...r.kernel.org,
        matthieu.baerts@...sares.net, marcelo.leitner@...il.com
Subject: Re: [PATCH 0/5] add initial io_uring_cmd support for sockets

Breno Leitao wrote:
> On Thu, Apr 13, 2023 at 10:24:31AM -0400, Willem de Bruijn wrote:
> > > How to handle these contradictory behaviour ahead of time (at callee
> > > time, where the buffers will be prepared)?
> > 
> > Ah you found a counter-example to the simple pattern of put_user.
> > 
> > The answer perhaps depends on how many such counter-examples you
> > encounter in the list you gave. If this is the only one, exceptions
> > in the wrapper are reasonable. Not if there are many.
> 
> 
> Hello Williem,
> 
> I spend sometime dealing with it, and the best way for me to figure out
> how much work this is, was implementing a PoC. You can find a basic PoC
> in the link below. It is not 100% complete (still need to convert 4
> simple ioctls), but, it deals with the most complicated cases. The
> missing parts are straighforward if we are OK with this approach.
> 
> 	https://github.com/leitao/linux/commits/ioctl_refactor
> 
> Details
> =======
> 
> 1)  Change the ioctl callback to use kernel memory arguments. This
> changes a lot of files but most of them are trivial. This is the new
> ioctl callback:
> 
> struct proto {
> 
>         int                     (*ioctl)(struct sock *sk, int cmd,
> -                                        unsigned long arg);
> +                                        int *karg);
> 
> 	You can see the full changeset in the following commit (which is
> 	the last in the tree above)
> 	https://github.com/leitao/linux/commit/ad78da14601b078c4b6a9f63a86032467ab59bf7
> 
> 2) Create a wrapper (sock_skprot_ioctl()) that should be called instead
> of sk->sk_prot->ioctl(). For every exception, calls a specific function
> for the exception (basically ipmr_ioctl and ipmr_ioctl) (see more on 3)
> 
> 	This is the commit https://github.com/leitao/linux/commit/511592e549c39ef0de19efa2eb4382cac5786227
> 
> 3) There are two exceptions, they are ip{6}mr_ioctl() and pn_ioctl().
> ip{6}mr is the hardest one, and I implemented the exception flow for it.
> 
> 	You could find ipmr changes here:
> 	https://github.com/leitao/linux/commit/659a76dc0547ab2170023f31e20115520ebe33d9
> 
> Is this what you had in mind?
> 
> Thank you!

Thanks for the series, Breno. Yes, this looks very much what I hoped for.

The series shows two cases of ioctls: getters that return an int, and
combined getter/setters that take a struct of a certain size and
return the exact same.

I would deduplicate the four ipmr/ip6mr cases that constitute the second
type, by having a single helper for this type. sock_skprot_ioctl_struct,
which takes an argument for the struct size to copy in/out.

Did this series cover all proto ioctls, or is this still a subset just
for demonstration purposes -- and might there still be other types
lurking elsewhere?

If this is all, this looks like a reasonable amount of code churn to me.

Three small points

* please keep the __user annotation. Use make C=2 when unsure to warn
  about mismatched annotation
* minor: special case the ipmr (type 2) ioctls in sock_skprot_ioctl
  and treat the "return int" (type 1) ioctls as the default case.
* introduce code in a patch together with its use-case, so no separate
  patches for sock_skprot_ioctl and sock_skprot_ioctl_ipmr. Either one
  patch, or two, for each type of conversion.