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: <569186c5-8663-43df-a01c-d543f57ce5ca@kernel.org>
Date: Wed, 5 Mar 2025 10:15:05 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: Marco Elver <elver@...gle.com>
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 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.

> --- 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.

> @@ -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.

thanks,
-- 
js
suse labs