Re: [PATCH V2, 0/1] PCI/AER: fix use-after-free in pcie_do_fatal_recovery

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 07/15/2018 02:55 PM, poza@xxxxxxxxxxxxxx wrote:
On 2018-07-13 03:21, Bjorn Helgaas wrote:
[+cc Keith for DPC question]

On Thu, Jul 12, 2018 at 11:33:42AM -0600, Thomas Tai wrote:
Hi Bjorn,
Thank you very much to review the V1 patch. I reworked the patch as
you have suggested. Your suggestion is indeed better and cleaner.
Would you please have a look for me?

FYI, I did not clear the device structure in aer_isr_one_error()
just to avoid over complicated the fix.

I think that was a bad idea anyway.  I think the problem there is that
add_error_device() copies the pci_dev pointer without taking a
reference on it:

  aer_isr
    get_e_source              # dequeue from rpc->e_sources[]
      *e_src = rpc->e_sources[x]
    aer_isr_one_error
      pdev = rpc->rpd         # pdev = Root Port pci_dev
      find_source_device(pdev)
    find_device_iter
      add_error_device
        e_info->dev[i] = dev   # <-- save pci_dev pointer
      aer_process_err_devices
    handle_error_source(e_info->dev[i])
      pcie_do_fatal_recovery

At this point, we have pci_dev pointers in e_info->dev[] that are
potentially stale.  Right now nobody uses them, but I think it's
sloppy that we still have them at all.

I think a better way to fix this would be to call pci_dev_get() in
add_error_device() when we save the pointer, and then do the
corresponding pci_dev_put() in aer_isr_one_error(), after we return
from aer_process_err_devices().

This could be done with a little helper function that iterates through
e_info->dev[i], calls pci_dev_put() for each, and clears
e_info->error_dev_num (which could then be removed from
find_source_device()).

I think this would fix the problem you're seeing, so we wouldn't need
the change in pcie_do_fatal_recovery().

However, I think we're also slightly exposed in dpc_work(), in basically
the same (possibly harmless) way.

  dpc_irq
    schedule_work(&dpc->work)
  ...
  dpc_work
    pdev = dpc->dev->port
    pcie_do_fatal_recovery(pdev)

pdev may be removed by pcie_do_fatal_recovery(), but dpc_work() is still
holding onto a pointer (which it never uses again).

The DPC driver should be holding a reference to pdev (through some black
magic I don't understand), but that would be released when pdev is removed,
and I don't know what ensures that dpc_work() runs before that release.


Is not that dpc_work() is calling pcie_do_fatal_recovery() which in-turn is removing pdev.
I do not see DPC is using any more reference beyond that.

Agree.

So, isnt it safe to assume that no matter when the dpc_work() run, the pdev it reference is always intact. this is with the fact, that both AER and DPC can not trigger simultaneously, and and hence no race for removal.
who else would remove that pdev other than AER and DPC ?

Looks like no one is removing it other than the pcie_do_fatal_recovery(pdev)


With Keith's patch, are not we just improving the latency with threaded irq instead of workqueue ?

I think the most important is that the threaded_irq hold the device until it finished the bottom half. Although I can't see where in the threaded_irq code increase the device reference count. I may be missing some info here, either way I am trying out the threaded_irq to see if it fixed the issue or not.


the potential problem is another interrupt arriving while we have acknowledged it in dpc_reset_link() and we are about to remove the devices. The acknowledgement of DPC status trigger and re-enabling interrupt can be done in dpc_work() or threaded_handler() after we are done with pcie_do_fatal_recovery()

Good point. the pcie_do_fatal_recovery() use pci_stop_and_remove_bus_device() to remove the device, isn't it will stop the device from sending out anymore interrupts? Once we remove the device, we probably no need to reset the device.

-Thomas


dpc_reset_link()
{
         following can be moved after pcie_do_fatal_recovery() ??
     pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
                   PCI_EXP_DPC_STATUS_TRIGGER);

     pci_read_config_word(pdev, cap + PCI_EXP_DPC_CTL, &ctl);
     pci_write_config_word(pdev, cap + PCI_EXP_DPC_CTL,
                   ctl | PCI_EXP_DPC_CTL_INT_EN);
}

and probably following also one also can be done after recovery.

pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
                   PCI_EXP_DPC_STATUS_INTERRUPT);

I am not sure if this was the concern or I did talk about something else all together !

Bjorn



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux