linux-kernel - Re: [jgunthorpe:vfio_iommufd 9/11] drivers/vfio/vfio_main.c:1690 vfio_file_enforced_coherent() warn: assigning (-95) to unsigned variable 'ret'

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] [day] [month] [year] [list]

Date:   Tue, 25 Oct 2022 13:22:00 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     kbuild@...ts.01.org, lkp@...el.com, kbuild-all@...ts.01.org,
        linux-kernel@...r.kernel.org
Subject: Re: [jgunthorpe:vfio_iommufd 9/11] drivers/vfio/vfio_main.c:1690
 vfio_file_enforced_coherent() warn: assigning (-95) to unsigned variable
 'ret'

On Tue, Oct 25, 2022 at 05:21:36PM +0300, Dan Carpenter wrote:
> tree:   https://github.com/jgunthorpe/linux vfio_iommufd
> head:   a249441ba6fd9d658f4a1b568453e3a742d12686
> commit: e44299750e287f3d64d207a5af7abb021634a31b [9/11] vfio: Make vfio_container optionally compiled
> config: openrisc-randconfig-m041-20221024
> compiler: or1k-linux-gcc (GCC) 12.1.0

Dan! Thank you for looking at this branch, I'm going to be getting it
in linux-next very soon, so I will fix whatever your tools will spot!
Let me know


> New smatch warnings:
> drivers/vfio/vfio_main.c:1690 vfio_file_enforced_coherent() warn: assigning (-95) to unsigned variable 'ret'
> 
> Old smatch warnings:
> drivers/vfio/vfio_main.c:1907 vfio_pin_pages() warn: impossible condition '(iova > (~0)) => (0-u32max > u32max)'
> drivers/vfio/vfio_main.c:1974 vfio_dma_rw() warn: impossible condition '(iova > (~0)) => (0-u32max > u32max)'
> 
> vim +/ret +1690 drivers/vfio/vfio_main.c
> 
> a905ad043f32bb drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-04  1671  /**
> a905ad043f32bb drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-04  1672   * vfio_file_enforced_coherent - True if the DMA associated with the VFIO file
> a905ad043f32bb drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-04  1673   *        is always CPU cache coherent
> 
> This comment sort of feels like returning false on error is the correct
> thing but in the rest of the code it seems like returning true on error
> is correct.

Oddly, true is the correct result. "false" means you have proven you
are worthy and that cannot happen on error cases.

> a905ad043f32bb drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-04  1674   * @file: VFIO group file
> c0560f51cf7747 drivers/vfio/vfio.c      Yan Zhao        2020-03-24  1675   *
> a905ad043f32bb drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-04  1676   * Enforced coherency means that the IOMMU ignores things like the PCIe no-snoop
> a905ad043f32bb drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-04  1677   * bit in DMA transactions. A return of false indicates that the user has
> a905ad043f32bb drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-04  1678   * rights to access additional instructions such as wbinvd on x86.
> c0560f51cf7747 drivers/vfio/vfio.c      Yan Zhao        2020-03-24  1679   */
> a905ad043f32bb drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-04  1680  bool vfio_file_enforced_coherent(struct file *file)
> c0560f51cf7747 drivers/vfio/vfio.c      Yan Zhao        2020-03-24  1681  {
> a905ad043f32bb drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-04  1682  	struct vfio_group *group = file->private_data;
> a905ad043f32bb drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-04  1683  	bool ret;
> c0560f51cf7747 drivers/vfio/vfio.c      Yan Zhao        2020-03-24  1684  
> b1b8132a651cf6 drivers/vfio/vfio_main.c Alex Williamson 2022-10-07  1685  	if (!vfio_file_is_group(file))
> a905ad043f32bb drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-04  1686  		return true;
> c0560f51cf7747 drivers/vfio/vfio.c      Yan Zhao        2020-03-24  1687  
> c82e81ab256955 drivers/vfio/vfio_main.c Jason Gunthorpe 2022-09-29  1688  	mutex_lock(&group->group_lock);
> e0e29bdb594adf drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-16  1689  	if (group->container) {
> 1408640d578887 drivers/vfio/vfio_main.c Jason Gunthorpe 2022-09-22 @1690  		ret = vfio_container_ioctl_check_extension(group->container,
> e0e29bdb594adf drivers/vfio/vfio.c      Jason Gunthorpe 2022-05-16  1691  							   VFIO_DMA_CC_IOMMU);
> 
> But this returns true if vfio_container_ioctl_check_extension() returns
> a negative error code.  (I haven't looked at the git branch and I
> suspect it's different from linux-next)

Yes, I should not take this shortcut, we would be better to explicitly
return false on error return.

But good news, I just deleted this code, so all fixed :)

Jason