linux-kernel - Re: [PATCH] Module Name: drivers/nfc/pn544/pn544.c fix a bug

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

Message-ID: <2e7b0365-69c1-e680-8383-bebe3fb8e8f8@canonical.com>
Date:   Sun, 26 Dec 2021 12:31:49 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To:     hrshy0629 <hongqiang601217@...il.com>, davem@...emloft.net
Cc:     kuba@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Module Name: drivers/nfc/pn544/pn544.c fix a bug

On 25/12/2021 14:27, hrshy0629 wrote:
>     I noticed that the same usage for API nfc_hci_send_cmd() in line 541 and line 552 of the file pn544.c.
>     And the variable r is checked on line 545 but not checked on line 552.
>     The r in line 552 should be checked.

Why 'r' should be tested and returned early in line 552? Just because
some other code has slightly similar pattern, does not mean you should
apply it everywhere blindly.

Patch title and description is not matching coding style. Please, go via
git history to find how it is being written.

Title must describe what bug exactly you fix. Use imperative mode in the
commit description. Describe what is the bug and h ow it should be fixed.

> 
> Signed-off-by: hrshy0629 <hongqiang601217@...il.com>
> ---
>  drivers/nfc/pn544/pn544.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/nfc/pn544/pn544.c b/drivers/nfc/pn544/pn544.c
> index 32a61a185142..531eda0d11a2 100644
> --- a/drivers/nfc/pn544/pn544.c
> +++ b/drivers/nfc/pn544/pn544.c
> @@ -552,6 +552,8 @@ static int pn544_hci_complete_target_discovered(struct nfc_hci_dev *hdev,
>  			r = nfc_hci_send_cmd(hdev, PN544_RF_READER_F_GATE,
>  					     PN544_RF_READER_CMD_ACTIVATE_NEXT,
>  					     uid_skb->data, uid_skb->len, NULL);
> +			if (r < 0)
> +				return r;

This looks like creating a leak, so NAK. Please provide explanation why
such error patch should be used.

>  			kfree_skb(uid_skb);


Best regards,
Krzysztof