linux-kernel - Re: [PATCH v2 1/2] vfio: Prevent open

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: <20250618160801.6ccec26e@DESKTOP-0403QTC.>
Date: Wed, 18 Jun 2025 16:08:01 -0700
From: Jacob Pan <jacob.pan@...ux.microsoft.com>
To: Alex Williamson <alex.williamson@...hat.com>
Cc: Jason Gunthorpe <jgg@...dia.com>, linux-kernel@...r.kernel.org,
 "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>, "Liu, Yi L"
 <yi.l.liu@...el.com>, Zhang Yu <zhangyu1@...rosoft.com>, Easwar Hariharan
 <eahariha@...ux.microsoft.com>, Saurabh Sengar
 <ssengar@...ux.microsoft.com>, jacob.pan@...ux.microsoft.com
Subject: Re: [PATCH v2 1/2] vfio: Prevent open_count decrement to negative

Hi Alex,

On Mon, 16 Jun 2025 08:40:01 -0600
Alex Williamson <alex.williamson@...hat.com> wrote:

> On Fri, 13 Jun 2025 21:09:26 -0300
> Jason Gunthorpe <jgg@...dia.com> wrote:
> 
> > On Fri, Jun 13, 2025 at 04:31:00PM -0600, Alex Williamson wrote:  
> > > Hi Jacob,
> > > 
> > > On Tue,  3 Jun 2025 08:23:42 -0700
> > > Jacob Pan <jacob.pan@...ux.microsoft.com> wrote:
> > >     
> > > > When vfio_df_close() is called with open_count=0, it triggers a
> > > > warning in vfio_assert_device_open() but still decrements
> > > > open_count to -1. This allows a subsequent open to incorrectly
> > > > pass the open_count == 0 check, leading to unintended behavior,
> > > > such as setting df->access_granted = true.
> > > > 
> > > > For example, running an IOMMUFD compat no-IOMMU device with
> > > > VFIO tests
> > > > (https://github.com/awilliam/tests/blob/master/vfio-noiommu-pci-device-open.c)
> > > > results in a warning and a failed VFIO_GROUP_GET_DEVICE_FD
> > > > ioctl on the first run, but the second run succeeds incorrectly.
> > > > 
> > > > Add checks to avoid decrementing open_count below zero.    
> > > 
> > > The example above suggests to me that this is a means by which we
> > > could see this, but in reality it seems it is the only means by
> > > which we can create this scenario, right?    
> > 
> > I understood this as an assertion hit because of the bug fixed in
> > patch 2 and thus the missed assertion error handling flow was
> > noticed.
> > 
> > Obviously the assertion should never happen, but if it does we
> > should try to recover better than we currently do.  
> 
> Certainly.  My statement is trying to determine the scope of the issue
> from a stable perspective.  Maybe I'm interpreting "[f]or example" too
> broadly, but I think this is unreachable outside of the specific
> described scenario, ie. using iommufd in compatibility mode with
> no-iommu.  Further, it only became reachable with 6086efe73498.
> 
> In any case, it fixes something and we should attribute that
> something, whether it's 6086efe73498 or we want to reach back to when
> the assert was introduced and claim it should have had a return even
> if it was unreachable.
> 
> It seems these patches should also be re-ordered if not rolled into
> one.  Fixing the issue in 2/ makes this once again unreachable, so I
> don't mind it coming along as a "also handle this error case better."
> This alone doesn't really do much.  Thanks,
> 
IMHO, this is an independent exception handling fix, perhaps I just add
the missing tag below?

Fixes: 05f37e1c03b6 ("vfio: Pass struct vfio_device_file * to
vfio_device_open/close()")

Thanks,

Jacob