question about CVE-2022-3169

[Keith Busch]

On Thu, Nov 10, 2022 at 06:23:20PM +0800, linan (AK) wrote:
> Hi,
> 1e866afd4bcd("nvme: ensure subsystem reset is single threaded")
> fixed CVE-2022-3169.

The description in the CVE is nonsense. That patch won't "fix":

  "the NVME_IOCTL_SUBSYS_RESET through the device file of the driver,
   resulting in a PCIe link disconnect"

That's exactly what an nvme subsystem reset is supposed to do, as is
documented in the spec since the capability was introduced:

  "When an NVM Subsystem Reset occurs, all PCIe links in the NVM
   Subsystem transition to the LTSSM Detect state."

The patch just makes sure you can't stack different reset types along
with a unbinding event, so the test case in question will just become a
no-op for the subsystem reset part.

If anything, the CVE should have been that a non-admin user could have
triggered this, but a completely different patch fixed that.

> IIUC, ERROR path I got in this CVE:
>            CPU1                                CPU2
> nvme_dev_ioctl                            nvme_dev_ioctl
>   nvme_reset_ctrl_sync                      nvme_reset_subsystem
>       reset_work
>         nvme_reset_work
>           nvme_setup_io_queues
>             nvme_remap_bar(dev, size)
>               if (size <= dev->bar_mapped_size)
>                 return 0;
>               iounmap
>                                               reg_write32   //error
>               ioremap
> 
> In nvme_remap_bar(), the premise of ioremap is "size >
> dev->bar_mapped_size".
>   size = NVME_REG_DBS + ((nr_io_queues + 1) * 8 * dev->db_stride);
>     1)nr_io_queue = dev->nr_allocated_queues - 1, Onece set to
> "nvme_max_io_queues(dev) + 1" during probe time, it could not change.
>     2)db_stride is doorbell stride, it didn't change during my test
> Therefore, I cant find a way to make iounmap happen.
> 
> Could you tell me how you trigger the ERROR?

To get a panic, something needs to happen to cause the driver to unbind
from the device at the same time as sending a subsystem reset. I'm not
sure how the original bz reporter managed to make that happen, but it
didn't matter to me since we're supposed to handle these concurrent
events. I'm guessing a previous subsystem reset triggered link-down
unbinding, which unmapped the BAR at the same time another thread wanted
to send a follow on subsystem reset.