• Duoming Zhou's avatar
    nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs · d270453a
    Duoming Zhou authored
    There are destructive operations such as nfcmrvl_fw_dnld_abort and
    gpio_free in nfcmrvl_nci_unregister_dev. The resources such as firmware,
    gpio and so on could be destructed while the upper layer functions such as
    nfcmrvl_fw_dnld_start and nfcmrvl_nci_recv_frame is executing, which leads
    to double-free, use-after-free and null-ptr-deref bugs.
    
    There are three situations that could lead to double-free bugs.
    
    The first situation is shown below:
    
       (Thread 1)                 |      (Thread 2)
    nfcmrvl_fw_dnld_start         |
     ...                          |  nfcmrvl_nci_unregister_dev
     release_firmware()           |   nfcmrvl_fw_dnld_abort
      kfree(fw) //(1)             |    fw_dnld_over
                                  |     release_firmware
      ...                         |      kfree(fw) //(2)
                                  |     ...
    
    The second situation is shown below:
    
       (Thread 1)                 |      (Thread 2)
    nfcmrvl_fw_dnld_start         |
     ...                          |
     mod_timer                    |
     (wait a time)                |
     fw_dnld_timeout              |  nfcmrvl_nci_unregister_dev
       fw_dnld_over               |   nfcmrvl_fw_dnld_abort
        release_firmware          |    fw_dnld_over
         kfree(fw) //(1)          |     release_firmware
         ...                      |      kfree(fw) //(2)
    
    The third situation is shown below:
    
           (Thread 1)               |       (Thread 2)
    nfcmrvl_nci_recv_frame          |
     if(..->fw_download_in_progress)|
      nfcmrvl_fw_dnld_recv_frame    |
       queue_work                   |
                                    |
    fw_dnld_rx_work                 | nfcmrvl_nci_unregister_dev
     fw_dnld_over                   |  nfcmrvl_fw_dnld_abort
      release_firmware              |   fw_dnld_over
       kfree(fw) //(1)              |    release_firmware
                                    |     kfree(fw) //(2)
    
    The firmware struct is deallocated in position (1) and deallocated
    in position (2) again.
    
    The crash trace triggered by POC is like below:
    
    BUG: KASAN: double-free or invalid-free in fw_dnld_over
    Call Trace:
      kfree
      fw_dnld_over
      nfcmrvl_nci_unregister_dev
      nci_uart_tty_close
      tty_ldisc_kill
      tty_ldisc_hangup
      __tty_hangup.part.0
      tty_release
      ...
    
    What's more, there are also use-after-free and null-ptr-deref bugs
    in nfcmrvl_fw_dnld_start. If we deallocate firmware struct, gpio or
    set null to the members of priv->fw_dnld in nfcmrvl_nci_unregister_dev,
    then, we dereference firmware, gpio or the members of priv->fw_dnld in
    nfcmrvl_fw_dnld_start, the UAF or NPD bugs will happen.
    
    This patch reorders destructive operations after nci_unregister_device
    in order to synchronize between cleanup routine and firmware download
    routine.
    
    The nci_unregister_device is well synchronized. If the device is
    detaching, the firmware download routine will goto error. If firmware
    download routine is executing, nci_unregister_device will wait until
    firmware download routine is finished.
    
    Fixes: 3194c687
    
     ("NFC: nfcmrvl: add firmware download support")
    Signed-off-by: default avatarDuoming Zhou <duoming@zju.edu.cn>
    Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
    d270453a
main.c 6.28 KB