Re: [PATCH] nfc: fix memory leak of se_io context in nfc_genl_se_io

[Krzysztof Kozlowski]

On 28/02/2023 12:25, Fedor Pchelkin wrote:
> On Tue, Feb 28, 2023 at 11:14:03AM +0100, Krzysztof Kozlowski wrote:
>> On 27/02/2023 16:05, Fedor Pchelkin wrote:
>>>>> Fixes: 5ce3f32b5264 ("NFC: netlink: SE API implementation")
>>>>> Reported-by: syzbot+df64c0a2e8d68e78a4fa@xxxxxxxxxxxxxxxxxxxxxxxxx
>>>>> Signed-off-by: Fedor Pchelkin <pchelkin@xxxxxxxxx>
>>>>> Signed-off-by: Alexey Khoroshilov <khoroshilov@xxxxxxxxx>
>>>>
>>>> SoB order is a bit odd. Who is the author?
>>>>
>>>
>>> The author is me (Fedor). I thought the authorship is expressed with the
>>> first Signed-off-by line, isn't it?
>>
>> Yes and since you are sending it, then what is Alexey's Sob for? The
>> tags are in order...
>>
> 
> Now I get what you mean. Alexey is my supervisor and the patches I make
> are passed through him (even though they are sent by me). If this is not
> a customary thing, then I'll take that into account for further
> submissions. I guess something like Acked-by is more appropriate?

Different people abuse these tags in different way, so it happens, but
it's not necessarily the correct way. I, for example, see little value
of some tags added from some internal and private arrangements. If
Alexey wants to ack something, sure, please ack - we have mailing list
for that. Storing acks for some of your private process is not relevant
to upstream process.

> 
>>>
>>>>> ---
>>>>>  drivers/nfc/st-nci/se.c   | 6 ++++++
>>>>>  drivers/nfc/st21nfca/se.c | 6 ++++++
>>>>>  net/nfc/netlink.c         | 4 ++++
>>>>>  3 files changed, 16 insertions(+)
>>>>>
>>>>> diff --git a/drivers/nfc/st-nci/se.c b/drivers/nfc/st-nci/se.c
>>>>> index ec87dd21e054..b2f1ced8e6dd 100644
>>>>> --- a/drivers/nfc/st-nci/se.c
>>>>> +++ b/drivers/nfc/st-nci/se.c
>>>>> @@ -672,6 +672,12 @@ int st_nci_se_io(struct nci_dev *ndev, u32 se_idx,
>>>>>  					ST_NCI_EVT_TRANSMIT_DATA, apdu,
>>>>>  					apdu_length)
>>>> nci_hci_send_event() should also free it in its error paths.
>>>> nci_data_exchange_complete() as well? Who eventually frees it? These
>>>> might be separate patches.
>>>>
>>>>
>>>
>>> nci_hci_send_event(), as I can see, should not free the callback context.
>>> I should have probably better explained that in the commit info (will
>>> include this in the patch v2), but the main thing is: nfc_se_io() is
>>> called with se_io_cb callback function as an argument and that callback is 
>>> the exact place where an allocated se_io_ctx context should be freed. And
>>> it is actually freed there unless some error path happens that leads the
>>
>> Exactly, so why nci_hci_send_event() error path should not free it?
>>
> 
> nci_hci_send_event() should not free it on its error path because the
> bwi_timer is already charged before nci_hci_send_event() is called.
> 
> The pattern in the .se_io functions of the corresponding drivers (st-nci,
> st21nfca) is following:
> 
> 	info->se_info.cb = cb;
> 	info->se_info.cb_context = cb_context;
> 	mod_timer(&info->se_info.bwi_timer, jiffies +
> 		  msecs_to_jiffies(info->se_info.wt_timeout)); // <-charged
> 	info->se_info.bwi_active = true;
> 	return nci_hci_send_event(...);
> 
> As the timer is charged, it will eventually call se_io_cb() to free the
> context, even if the error path is taken inside nci_hci_send_event().
> 
> Am I missing something?

Hm, sounds right.

Best regards,
Krzysztof