linux-kernel - Re: [PATCH v2] hvc/xen: prevent concurrent accesses to the shared ring

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: <ZAm8BDGTMaI0XmMI@Air-de-Roger>
Date:   Thu, 9 Mar 2023 11:59:16 +0100
From:   Roger Pau Monné <roger.pau@...rix.com>
To:     Stefano Stabellini <sstabellini@...nel.org>,
        Juergen Gross <jgross@...e.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-kernel@...r.kernel.org, xen-devel@...ts.xenproject.org,
        Jan Beulich <jbeulich@...e.com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Chris Wright <chrisw@...s-sol.org>,
        Ingo Molnar <mingo@...e.hu>, Olof Johansson <olof@...om.net>,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v2] hvc/xen: prevent concurrent accesses to the shared
 ring

Hello,

It's been 3 months and no reply.

On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monné wrote:
> Hello,
> 
> Gentle ping regarding the locking question below.
> 
> Thanks, Roger.
> 
> On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné wrote:
> > On Wed, Nov 30, 2022 at 05:08:06PM -0800, Stefano Stabellini wrote:
> > > On Wed, 30 Nov 2022, Roger Pau Monne wrote:
> > > > The hvc machinery registers both a console and a tty device based on
> > > > the hv ops provided by the specific implementation.  Those two
> > > > interfaces however have different locks, and there's no single locks
> > > > that's shared between the tty and the console implementations, hence
> > > > the driver needs to protect itself against concurrent accesses.
> > > > Otherwise concurrent calls using the split interfaces are likely to
> > > > corrupt the ring indexes, leaving the console unusable.
> > > > 
> > > > Introduce a lock to xencons_info to serialize accesses to the shared
> > > > ring.  This is only required when using the shared memory console,
> > > > concurrent accesses to the hypercall based console implementation are
> > > > not an issue.
> > > > 
> > > > Note the conditional logic in domU_read_console() is slightly modified
> > > > so the notify_daemon() call can be done outside of the locked region:
> > > > it's an hypercall and there's no need for it to be done with the lock
> > > > held.
> > > 
> > > For domU_read_console: I don't mean to block this patch but we need to
> > > be sure about the semantics of hv_ops.get_chars. Either it is expected
> > > to be already locked, then we definitely shouldn't add another lock to
> > > domU_read_console. Or it is not expected to be already locked, then we
> > > should add the lock.
> > > 
> > > My impression is that it is expected to be already locked, but I think
> > > we need Greg or Jiri to confirm one way or the other.
> > 
> > Let me move both to the 'To:' field then.
> > 
> > My main concern is the usage of hv_ops.get_chars hook in
> > hvc_poll_get_char(), as it's not obvious to me that callers of
> > tty->poll_get_char hook as returned by tty_find_polling_driver() will
> > always do so with the tty lock held (in fact the only user right now
> > doesn't seem to hold the tty lock).
> > 
> > > Aside from that the rest looks fine.

I guess I could reluctantly remove the lock in the get_chars hook,
albeit I'm not convinced at all the lock is not needed there.

Roger.