netdev - Re: [PATCH v2] net: don't unconditionally copy_from

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] [day] [month] [year] [list]

Message-ID: <20210901162011.3ce5f012@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 1 Sep 2021 16:20:11 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Peter Collingbourne <pcc@...gle.com>
Cc:     David Laight <David.Laight@...lab.com>,
        "David S. Miller" <davem@...emloft.net>,
        Colin Ian King <colin.king@...onical.com>,
        Cong Wang <cong.wang@...edance.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Greg KH <gregkh@...uxfoundation.org>,
        Arnd Bergmann <arnd@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v2] net: don't unconditionally copy_from_user a struct
 ifreq for socket ioctls

On Wed, 1 Sep 2021 11:01:32 -0700 Peter Collingbourne wrote:
> > > To stop the copy_from_user() faulting when the user buffer
> > > isn't long enough.
> > > In particular for iasatty() on arm with tagged pointers.  
> >
> > Let me rephrase. is_socket_ioctl_cmd() is always true here. There were
> > only two callers, both check cmd is of specific, "sockety" type.  
> 
> I see, it looks like we don't need the check on the compat path then.
> 
> I can send a followup to clean this up but given that I got a comment
> from another reviewer saying that we should try to make the native and
> compat paths as similar as possible, maybe it isn't too bad to leave
> things as is?

I have a weak preference to get rid of it, the code is a little
complex and extra dead code makes it harder to follow, but up to you.

IMO the "right place" for the check is:

static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
[...]
		default:
			/* --> here <-- */
			err = sock_do_ioctl(net, sock, cmd, arg);
			break;

Since that's the point where we take all the remaining cmd values and
call a function which assumes struct ifreq.

Compat code does not have a default statement.

But as I said no big deal, feel free to ignore.