netdev - Re: [PATCH] tun: support not enabling carrier in TUNSETIFF

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: <CANP3RGdMEJMDcB8X_YD-PM7X6pqypvSn7_q4x=B8rzLd+CAqXA@mail.gmail.com>
Date:   Mon, 19 Sep 2022 17:01:07 -0700
From:   Maciej Żenczykowski <maze@...gle.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     Patrick Rohr <prohr@...gle.com>,
        "David S . Miller" <davem@...emloft.net>,
        Linux Network Development Mailing List 
        <netdev@...r.kernel.org>, Lorenzo Colitti <lorenzo@...gle.com>,
        Jason Wang <jasowang@...hat.com>
Subject: Re: [PATCH] tun: support not enabling carrier in TUNSETIFF

On Mon, Sep 19, 2022 at 10:18 AM Stephen Hemminger
<stephen@...workplumber.org> wrote:
> On Fri, 16 Sep 2022 16:45:52 -0700
> Patrick Rohr <prohr@...gle.com> wrote:
> >  #define IFF_DETACH_QUEUE 0x0400
> > +/* Used in TUNSETIFF to bring up tun/tap without carrier */
> > +#define IFF_NO_CARRIER IFF_DETACH_QUEUE
>
> Overloading a flag in existing user API is likely to break
> some application somewhere...

We could of course burn a bit (0x0040 and 0x0080 are both currently
utterly unused)... but that just seemed wasteful...
Do you think that would be better?

I find it exceedingly unlikely that any application is specifying this
flag to TUNSETIFF currently.

This flag has barely any hits in the code base, indeed ignoring the
Documentation, tests, and #define's we have:

$ git grep IFF_DETACH_QUEUE
drivers/net/tap.c:928:  else if (flags & IFF_DETACH_QUEUE)
drivers/net/tun.c:2954: } else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
drivers/net/tun.c:3115:                 ifr.ifr_flags |= IFF_DETACH_QUEUE;

The first two implement ioctl(TUNSETQUEUE) -- that's the only spot
where IFF_DETACH_QUEUE is currently supposed to be used.

The third one is the most interesting, see drivers/net/tun.c:3111

 case TUNGETIFF:
         tun_get_iff(tun, &ifr);
         if (tfile->detached)
                 ifr.ifr_flags |= IFF_DETACH_QUEUE;
         if (!tfile->socket.sk->sk_filter)
                 ifr.ifr_flags |= IFF_NOFILTER;

This means TUNGETIFF can return this flag for a detached queue.  However:

(a) multiqueue tun/tap is pretty niche, and detached queues are even more niche.

(b) the TUNGETIFF returned ifr_flags field already cannot be safely
used as input to TUNSETIFF, because IFF_NOFILTER == IFF_NO_PI ==
0x1000

(this overlap of IFF_NO_PI and IFF_NOFILTER is why we thought it'd be
ok to overlap here as well)

(c) if this actually turns out to be a problem it shouldn't be that
hard to fix the 1 or 2 userspace programs to mask out the flag
and not pass in garbage... Do we really want / need to maintain
compatibility with extremely badly written userspace?
It's really hard to even imagine how such code would come into existence...

Arguably the TUNSETIFF api should have always returned an error for
invalid flags... should we make that change now?