question about CVE-2022-3169

question about CVE-2022-3169

[linan (AK)]

Hi,
1e866afd4bcd("nvme: ensure subsystem reset is single threaded")
fixed CVE-2022-3169.

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?

Thanks,
Nan.

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

Re: question about CVE-2022-3169

[Chaitanya Kulkarni]

Lian,

On 11/10/22 02:23, linan (AK) wrote:
> Hi,
> 1e866afd4bcd("nvme: ensure subsystem reset is single threaded")
> fixed CVE-2022-3169.
> 
> 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?
> 
> Thanks,
> Nan.
> 

Do you have a script to reproduce this ?

-ck

Re: question about CVE-2022-3169

[linan (AK)]

在 2022/11/11 9:37, Chaitanya Kulkarni 写道:
> Lian,
> 
> On 11/10/22 02:23, linan (AK) wrote:
>> Hi,
>> 1e866afd4bcd("nvme: ensure subsystem reset is single threaded")
>> fixed CVE-2022-3169.
>>
>> 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?
>>
>> Thanks,
>> Nan.
>>
> 
> Do you have a script to reproduce this ?
> 
> -ck
> 

I can't reproduce it. "cause the driver to unbind from the device at the 
same time as sending a subsystem reset", you can have a try like Keith said.