netdev - Re: [PATCH net-next v3] net: ioctl: Use kernel memory on protocol ioctl callbacks

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]

Message-ID: <CAF=yD-LXcufhJBpkEcUuphFpR1TA4=QwUXw4sKFsSiEL_mwG4Q@mail.gmail.com>
Date: Thu, 25 May 2023 12:06:00 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: David Ahern <dsahern@...nel.org>
Cc: Breno Leitao <leitao@...ian.org>, Remi Denis-Courmont <courmisch@...il.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Alexander Aring <alex.aring@...il.com>, Stefan Schmidt <stefan@...enfreihafen.org>, 
	Miquel Raynal <miquel.raynal@...tlin.com>, Matthieu Baerts <matthieu.baerts@...sares.net>, 
	Mat Martineau <martineau@...nel.org>, Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, 
	Xin Long <lucien.xin@...il.com>, leit@...com, axboe@...nel.dk, asml.silence@...il.com, 
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org, dccp@...r.kernel.org, 
	linux-wpan@...r.kernel.org, mptcp@...ts.linux.dev, linux-sctp@...r.kernel.org
Subject: Re: [PATCH net-next v3] net: ioctl: Use kernel memory on protocol
 ioctl callbacks

On Thu, May 25, 2023 at 11:34 AM David Ahern <dsahern@...nel.org> wrote:
>
> On 5/25/23 9:05 AM, Willem de Bruijn wrote:
> >> +/* A wrapper around sock ioctls, which copies the data from userspace
> >> + * (depending on the protocol/ioctl), and copies back the result to userspace.
> >> + * The main motivation for this function is to pass kernel memory to the
> >> + * protocol ioctl callbacks, instead of userspace memory.
> >> + */
> >> +int sk_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
> >> +{
> >> +       int rc = 1;
> >> +
> >> +       if (ipmr_is_sk(sk))
> >> +               rc = ipmr_sk_ioctl(sk, cmd, arg);
> >> +       else if (ip6mr_is_sk(sk))
> >> +               rc = ip6mr_sk_ioctl(sk, cmd, arg);
> >> +       else if (phonet_is_sk(sk))
> >> +               rc = phonet_sk_ioctl(sk, cmd, arg);
> >
> > I don't understand what this buys us vs testing the sk_family,
> > sk_protocol and cmd here.
>
> To keep protocol specific code out of core files is the reason I
> suggested it.

I guess you object to demultiplexing based on per-family
protocol and ioctl cmd constants directly in this file?

That only requires including the smaller uapi headers.

But now net/core/sock.h now still has to add includes
linux/mroute.h, linux/mroute6.h and net/phonet/phonet.h.

Aside on phonet_is_sk, if we're keeping this: this should be
sk_is_phonet? Analogous to sk_is_tcp and such. And, it should suffice
to  demultiplex based on the protocol family, without testing the
type or protocol. The family is defined in protocol-independent header
linux/socket.h. The differences between
PN_PROTO_PHONET and PN_PROTO_PIPE should be handled inside the family
code. So I think it is cleaner just to open-coded as `if
(sk->sk_family == PF_PHONET)`