linux-kernel - Re: [PATCH net v2] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message

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

Message-ID: <670b87a9.1d1aa.180d6d8952e.Coremail.duoming@zju.edu.cn>
Date:   Wed, 18 May 2022 19:05:59 +0800 (GMT+08:00)
From:   duoming@....edu.cn
To:     "Krzysztof Kozlowski" <krzysztof.kozlowski@...aro.org>
Cc:     linux-kernel@...r.kernel.org, kuba@...nel.org, davem@...emloft.net,
        edumazet@...gle.com, pabeni@...hat.com, gregkh@...uxfoundation.org,
        alexander.deucher@....com, broonie@...nel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH net v2] NFC: hci: fix sleep in atomic context bugs in
 nfc_hci_hcp_message_tx

Hello,

Date: Wed, 18 May 2022 11:39:27 +0200 Krzysztof wrote:

> >> There is.
> >>
> >> nfc_hci_failure -> spin lock -> nfc_driver_failure -> nfc_targets_found
> >> -> device_lock
> >>
> >> I found it just by a very quick look, so I suspect there are several
> >> other places, not really checked.
> > 
> > I agree with you, the spin_lock is not a good solution to this problem. There is another solution:
> > 
> > We could put the nfc_hci_send_event() of st21nfca_se_wt_timeout() in a work item, then, using
> > schedule_work() in st21nfca_se_wt_timeout() to execute the work item. The schedule_work() will
> > wake up another kernel thread which is in process context to execute the bottom half of the interrupt, 
> > so it allows sleep.
> > 
> > The following is the details.
> 
> Yes, this seems good solution. You might also need to add
> cancel_work_sync to all places removing the timer.

Thanks for your approval.

There are two del_timer_sync() functions related with bwi_timer timer.
I neglect one site in st21nfca_apdu_reader_event_received(), I add
cancel_work_sync() in it this time. 

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index c922f10d0d7..7e213f8ddc9 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -241,7 +241,7 @@ int st21nfca_hci_se_io(struct nfc_hci_dev *hdev, u32 se_idx,
 }
 EXPORT_SYMBOL(st21nfca_hci_se_io);

-static void st21nfca_se_wt_timeout(struct timer_list *t)
+static void st21nfca_se_wt_work(struct work_struct *work)
 {
        /* 
         * No answer from the secure element
@@ -254,8 +254,9 @@ static void st21nfca_se_wt_timeout(struct timer_list *t)
         */
        /* hardware reset managed through VCC_UICC_OUT power supply */
        u8 param = 0x01;
-       struct st21nfca_hci_info *info = from_timer(info, t,
-                                                   se_info.bwi_timer);
+       struct st21nfca_hci_info *info = container_of(work,
+                                               struct st21nfca_hci_info,
+                                               se_info.timeout_work);

        info->se_info.bwi_active = false;
 
@@ -271,6 +272,13 @@ static void st21nfca_se_wt_timeout(struct timer_list *t)
        info->se_info.cb(info->se_info.cb_context, NULL, 0, -ETIME);
 }

+static void st21nfca_se_wt_timeout(struct timer_list *t)
+{
+       struct st21nfca_hci_info *info = from_timer(info, t, se_info.bwi_timer);
+
+       schedule_work(&info->se_info.timeout_work);
+}
+
 static void st21nfca_se_activation_timeout(struct timer_list *t)
 {
        struct st21nfca_hci_info *info = from_timer(info, t,
@@ -360,6 +368,7 @@ int st21nfca_apdu_reader_event_received(struct nfc_hci_dev *hdev,
        switch (event) {
        case ST21NFCA_EVT_TRANSMIT_DATA:
                del_timer_sync(&info->se_info.bwi_timer);
+               cancel_work_sync(&info->se_info.timeout_work);
                info->se_info.bwi_active = false;
                r = nfc_hci_send_event(hdev, ST21NFCA_DEVICE_MGNT_GATE,
                                ST21NFCA_EVT_SE_END_OF_APDU_TRANSFER, NULL, 0);
@@ -389,6 +398,7 @@ void st21nfca_se_init(struct nfc_hci_dev *hdev)
        struct st21nfca_hci_info *info = nfc_hci_get_clientdata(hdev);

        init_completion(&info->se_info.req_completion);
+       INIT_WORK(&info->se_info.timeout_work, st21nfca_se_wt_work);
        /* initialize timers */
        timer_setup(&info->se_info.bwi_timer, st21nfca_se_wt_timeout, 0);
        info->se_info.bwi_active = false;
@@ -416,6 +426,7 @@ void st21nfca_se_deinit(struct nfc_hci_dev *hdev)
        if (info->se_info.se_active)
                del_timer_sync(&info->se_info.se_active_timer);

+       cancel_work_sync(&info->se_info.timeout_work);
        info->se_info.bwi_active = false;
        info->se_info.se_active = false;
 }
diff --git a/drivers/nfc/st21nfca/st21nfca.h b/drivers/nfc/st21nfca/st21nfca.h
index cb6ad916be9..ae6771cc989 100644
--- a/drivers/nfc/st21nfca/st21nfca.h
+++ b/drivers/nfc/st21nfca/st21nfca.h
@@ -141,6 +141,7 @@ struct st21nfca_se_info {

        se_io_cb_t cb;
        void *cb_context;
+       struct work_struct timeout_work;
 };

 struct st21nfca_hci_info {

If you think this solution is ok, I will send "PATCH v3".

Best regards,
Duoming Zhou