linux-kernel - Re: [PATCH v2 31/34] drivers/tty: Enable capability analysis for core files

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: <CANpmjNM+0xWRUmeyQ0hb6k5zHakw=KAaQN7VZ=yMyz0eyBa4xQ@mail.gmail.com>
Date: Wed, 5 Mar 2025 10:26:33 +0100
From: Marco Elver <elver@...gle.com>
To: Jiri Slaby <jirislaby@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>, Luc Van Oostenryck <luc.vanoostenryck@...il.com>, 
	"Paul E. McKenney" <paulmck@...nel.org>, Alexander Potapenko <glider@...gle.com>, Arnd Bergmann <arnd@...db.de>, 
	Bart Van Assche <bvanassche@....org>, Bill Wendling <morbo@...gle.com>, Boqun Feng <boqun.feng@...il.com>, 
	Dmitry Vyukov <dvyukov@...gle.com>, Eric Dumazet <edumazet@...gle.com>, 
	Frederic Weisbecker <frederic@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Herbert Xu <herbert@...dor.apana.org.au>, Ingo Molnar <mingo@...nel.org>, 
	Jann Horn <jannh@...gle.com>, Joel Fernandes <joel@...lfernandes.org>, 
	Jonathan Corbet <corbet@....net>, Josh Triplett <josh@...htriplett.org>, 
	Justin Stitt <justinstitt@...gle.com>, Kees Cook <kees@...nel.org>, 
	Kentaro Takeda <takedakn@...data.co.jp>, Mark Rutland <mark.rutland@....com>, 
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Miguel Ojeda <ojeda@...nel.org>, 
	Nathan Chancellor <nathan@...nel.org>, Neeraj Upadhyay <neeraj.upadhyay@...nel.org>, 
	Peter Zijlstra <peterz@...radead.org>, Steven Rostedt <rostedt@...dmis.org>, 
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>, Thomas Gleixner <tglx@...utronix.de>, 
	Uladzislau Rezki <urezki@...il.com>, Waiman Long <longman@...hat.com>, Will Deacon <will@...nel.org>, 
	kasan-dev@...glegroups.com, linux-kernel@...r.kernel.org, 
	llvm@...ts.linux.dev, rcu@...r.kernel.org, linux-crypto@...r.kernel.org, 
	linux-serial@...r.kernel.org
Subject: Re: [PATCH v2 31/34] drivers/tty: Enable capability analysis for core files

On Wed, 5 Mar 2025 at 10:15, Jiri Slaby <jirislaby@...nel.org> wrote:
>
> On 04. 03. 25, 10:21, Marco Elver wrote:
> > Enable capability analysis for drivers/tty/*.
> >
> > This demonstrates a larger conversion to use Clang's capability
> > analysis. The benefit is additional static checking of locking rules,
> > along with better documentation.
> >
> > Signed-off-by: Marco Elver <elver@...gle.com>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Cc: Jiri Slaby <jirislaby@...nel.org>
> ...
> > --- a/drivers/tty/tty_buffer.c
> > +++ b/drivers/tty/tty_buffer.c
> > @@ -52,10 +52,8 @@
> >    */
> >   void tty_buffer_lock_exclusive(struct tty_port *port)
> >   {
> > -     struct tty_bufhead *buf = &port->buf;
> > -
> > -     atomic_inc(&buf->priority);
> > -     mutex_lock(&buf->lock);
> > +     atomic_inc(&port->buf.priority);
> > +     mutex_lock(&port->buf.lock);
>
> Here and:
>
> > @@ -73,7 +71,7 @@ void tty_buffer_unlock_exclusive(struct tty_port *port)
> >       bool restart = buf->head->commit != buf->head->read;
> >
> >       atomic_dec(&buf->priority);
> > -     mutex_unlock(&buf->lock);
> > +     mutex_unlock(&port->buf.lock);
>
> here, this appears excessive. You are changing code to adapt to one kind
> of static analysis. Adding function annotations is mostly fine, but
> changing code is too much. We don't do that. Fix the analyzer instead.

Right. So the analysis doesn't do alias analysis.

> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -167,6 +167,7 @@ static void release_tty(struct tty_struct *tty, int idx);
> >    * Locking: none. Must be called after tty is definitely unused
> >    */
> >   static void free_tty_struct(struct tty_struct *tty)
> > +     __capability_unsafe(/* destructor */)
> >   {
> >       tty_ldisc_deinit(tty);
> >       put_device(tty->dev);
> > @@ -965,7 +966,7 @@ static ssize_t iterate_tty_write(struct tty_ldisc *ld, struct tty_struct *tty,
> >       ssize_t ret, written = 0;
> >
> >       ret = tty_write_lock(tty, file->f_flags & O_NDELAY);
> > -     if (ret < 0)
> > +     if (ret)
>
> This change is not documented.

Fair point. This is because the analysis can only deal with
conditional locking when fed into zero/non-zero condition checks.

> > @@ -1154,7 +1155,7 @@ int tty_send_xchar(struct tty_struct *tty, u8 ch)
> >               return 0;
> >       }
> >
> > -     if (tty_write_lock(tty, false) < 0)
> > +     if (tty_write_lock(tty, false))
>
> And this one. And more times later.
>
> > --- a/drivers/tty/tty_ldisc.c
> > +++ b/drivers/tty/tty_ldisc.c
> ...
> > +/*
> > + * Note: Capability analysis does not like asymmetric interfaces (above types
> > + * for ref and deref are tty_struct and tty_ldisc respectively -- which are
> > + * dependent, but the compiler cannot figure that out); in this case, work
> > + * around that with this helper which takes an unused @tty argument but tells
> > + * the analysis which lock is released.
> > + */
> > +static inline void __tty_ldisc_deref(struct tty_struct *tty, struct tty_ldisc *ld)
> > +     __releases_shared(&tty->ldisc_sem)
> > +     __capability_unsafe(/* matches released with tty_ldisc_ref() */)
> > +{
> > +     tty_ldisc_deref(ld);
> > +}
>
> You want to invert the __ prefix for these two. tty_ldisc_deref() should
> be kept as the one to be called by everybody.

Ack.

I think in the near term the alias analysis issues + conditional check
of < 0 aren't solvable. Alias analysis being the bigger issue.
Two options:
1. Adding __capability_unsafe to the few functions that you weren't
happy with above.
2. Dropping the whole patch.

I'm inclined to drop the whole patch.

Thanks,
-- Marco