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

Message-ID: <CAMn1gO5OmHg_10s698tNqf4X-hJ_gn17D8afyRhbW1nKpvLzWQ@mail.gmail.com>
Date:   Wed, 1 Sep 2021 11:01:32 -0700
From:   Peter Collingbourne <pcc@...gle.com>
To:     Jakub Kicinski <kuba@...nel.org>
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, Sep 1, 2021 at 7:04 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 1 Sep 2021 08:22:42 +0000 David Laight wrote:
> > From: Jakub Kicinski
> > > Sent: 31 August 2021 17:30
> > >
> > > On Thu, 26 Aug 2021 12:46:01 -0700 Peter Collingbourne wrote:
> > > > @@ -3306,6 +3308,8 @@ static int compat_ifr_data_ioctl(struct net *net, unsigned int cmd,
> > > >   struct ifreq ifreq;
> > > >   u32 data32;
> > > >
> > > > + if (!is_socket_ioctl_cmd(cmd))
> > > > +         return -ENOTTY;
> > > >   if (copy_from_user(ifreq.ifr_name, u_ifreq32->ifr_name, IFNAMSIZ))
> > > >           return -EFAULT;
> > > >   if (get_user(data32, &u_ifreq32->ifr_data))
> > >
> > > Hi Peter, when resolving the net -> net-next merge conflict I couldn't
> > > figure out why this chunk is needed. It seems all callers of
> > > compat_ifr_data_ioctl() already made sure it's a socket IOCTL.
> > > Please double check my resolution (tip of net-next) and if this is
> > > indeed unnecessary perhaps send a cleanup? Thanks!
> >
> > 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?

Peter