| Message ID | 20240827084822.18785-1-amishin@t-argos.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | febccb39255f9df35527b88c953b2e0deae50e53 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] nfc: pn533: Add poll mod list filling check | expand |
On 8/27/24 10:48, Aleksandr Mishin wrote: > In case of im_protocols value is 1 and tm_protocols value is 0 this > combination successfully passes the check > 'if (!im_protocols && !tm_protocols)' in the nfc_start_poll(). > But then after pn533_poll_create_mod_list() call in pn533_start_poll() > poll mod list will remain empty and dev->poll_mod_count will remain 0 > which lead to division by zero. > > Normally no im protocol has value 1 in the mask, so this combination is > not expected by driver. But these protocol values actually come from > userspace via Netlink interface (NFC_CMD_START_POLL operation). So a > broken or malicious program may pass a message containing a "bad" > combination of protocol parameter values so that dev->poll_mod_count > is not incremented inside pn533_poll_create_mod_list(), thus leading > to division by zero. > Call trace looks like: > nfc_genl_start_poll() > nfc_start_poll() > ->start_poll() > pn533_start_poll() > > Add poll mod list filling check. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: dfccd0f58044 ("NFC: pn533: Add some polling entropy") > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> The issue looks real to me and the proposed fix the correct one, but waiting a little more for Krzysztof feedback, as he expressed concerns on v1. Thanks, Paolo
On 8/29/24 10:26, Paolo Abeni wrote: > The issue looks real to me and the proposed fix the correct one, but > waiting a little more for Krzysztof feedback, as he expressed concerns > on v1. I almost forgot: for next submissions, please include the target tree in the subj prefix ('net' in this case) and avoid reply to the same thread with the new version: that usually confuses the patchbot. Thanks, Paolo
On 29/08/2024 10:26, Paolo Abeni wrote: > > > On 8/27/24 10:48, Aleksandr Mishin wrote: >> In case of im_protocols value is 1 and tm_protocols value is 0 this >> combination successfully passes the check >> 'if (!im_protocols && !tm_protocols)' in the nfc_start_poll(). >> But then after pn533_poll_create_mod_list() call in pn533_start_poll() >> poll mod list will remain empty and dev->poll_mod_count will remain 0 >> which lead to division by zero. >> >> Normally no im protocol has value 1 in the mask, so this combination is >> not expected by driver. But these protocol values actually come from >> userspace via Netlink interface (NFC_CMD_START_POLL operation). So a >> broken or malicious program may pass a message containing a "bad" >> combination of protocol parameter values so that dev->poll_mod_count >> is not incremented inside pn533_poll_create_mod_list(), thus leading >> to division by zero. >> Call trace looks like: >> nfc_genl_start_poll() >> nfc_start_poll() >> ->start_poll() >> pn533_start_poll() >> >> Add poll mod list filling check. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Fixes: dfccd0f58044 ("NFC: pn533: Add some polling entropy") >> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> > > The issue looks real to me and the proposed fix the correct one, but > waiting a little more for Krzysztof feedback, as he expressed concerns > on v1. There was one month delay between my reply and clarifications from Fedor, so original patch is neither in my mailbox nor in my brain. Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> However different problem is: shouldn't as well or instead nfc_genl_start_poll() validate the attributes received by netlink? We just pass them directly to the drivers and several other drivers might not expect random stuff there. Best regards, Krzysztof
On 8/29/24 11:06, Krzysztof Kozlowski wrote: > On 29/08/2024 10:26, Paolo Abeni wrote: >> >> >> On 8/27/24 10:48, Aleksandr Mishin wrote: >>> In case of im_protocols value is 1 and tm_protocols value is 0 this >>> combination successfully passes the check >>> 'if (!im_protocols && !tm_protocols)' in the nfc_start_poll(). >>> But then after pn533_poll_create_mod_list() call in pn533_start_poll() >>> poll mod list will remain empty and dev->poll_mod_count will remain 0 >>> which lead to division by zero. >>> >>> Normally no im protocol has value 1 in the mask, so this combination is >>> not expected by driver. But these protocol values actually come from >>> userspace via Netlink interface (NFC_CMD_START_POLL operation). So a >>> broken or malicious program may pass a message containing a "bad" >>> combination of protocol parameter values so that dev->poll_mod_count >>> is not incremented inside pn533_poll_create_mod_list(), thus leading >>> to division by zero. >>> Call trace looks like: >>> nfc_genl_start_poll() >>> nfc_start_poll() >>> ->start_poll() >>> pn533_start_poll() >>> >>> Add poll mod list filling check. >>> >>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>> >>> Fixes: dfccd0f58044 ("NFC: pn533: Add some polling entropy") >>> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> >> >> The issue looks real to me and the proposed fix the correct one, but >> waiting a little more for Krzysztof feedback, as he expressed concerns >> on v1. > > There was one month delay between my reply and clarifications from > Fedor, so original patch is neither in my mailbox nor in my brain. > > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > However different problem is: shouldn't as well or instead > nfc_genl_start_poll() validate the attributes received by netlink? > > We just pass them directly to the drivers and several other drivers > might not expect random stuff there. FTR, I had a similar thought and skimmed over other nfc drivers. I did not see similar issues there. Additionally I fear that existing user-space could feed to the kernel such random stuff and work happily because the kernel is currently ignoring it - on other drivers. Such cases will suddenly stop working. I think we could/should merge the patch as-is, please LMK your thought. Thanks, Paolo
On 8/29/24 11:06, Krzysztof Kozlowski wrote: > On 29/08/2024 10:26, Paolo Abeni wrote: >> >> >> On 8/27/24 10:48, Aleksandr Mishin wrote: >>> In case of im_protocols value is 1 and tm_protocols value is 0 this >>> combination successfully passes the check >>> 'if (!im_protocols && !tm_protocols)' in the nfc_start_poll(). >>> But then after pn533_poll_create_mod_list() call in pn533_start_poll() >>> poll mod list will remain empty and dev->poll_mod_count will remain 0 >>> which lead to division by zero. >>> >>> Normally no im protocol has value 1 in the mask, so this combination is >>> not expected by driver. But these protocol values actually come from >>> userspace via Netlink interface (NFC_CMD_START_POLL operation). So a >>> broken or malicious program may pass a message containing a "bad" >>> combination of protocol parameter values so that dev->poll_mod_count >>> is not incremented inside pn533_poll_create_mod_list(), thus leading >>> to division by zero. >>> Call trace looks like: >>> nfc_genl_start_poll() >>> nfc_start_poll() >>> ->start_poll() >>> pn533_start_poll() >>> >>> Add poll mod list filling check. >>> >>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>> >>> Fixes: dfccd0f58044 ("NFC: pn533: Add some polling entropy") >>> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> >> >> The issue looks real to me and the proposed fix the correct one, but >> waiting a little more for Krzysztof feedback, as he expressed concerns >> on v1. > > There was one month delay between my reply and clarifications from > Fedor, so original patch is neither in my mailbox nor in my brain. > > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > However different problem is: shouldn't as well or instead > nfc_genl_start_poll() validate the attributes received by netlink? > > We just pass them directly to the drivers and several other drivers > might not expect random stuff there. FTR, I had a similar thought and skimmed over other nfc drivers. I did not see similar issues there. Additionally I fear that existing user-space could feed to the kernel such random stuff and work happily because the kernel is currently ignoring it - on other drivers. Such cases will suddenly stop working. I think we could/should merge the patch as-is, please LMK your thought. Thanks, Paolo
On 29/08/2024 11:34, Paolo Abeni wrote: > On 8/29/24 11:06, Krzysztof Kozlowski wrote: >> On 29/08/2024 10:26, Paolo Abeni wrote: >>> >>> >>> On 8/27/24 10:48, Aleksandr Mishin wrote: >>>> In case of im_protocols value is 1 and tm_protocols value is 0 this >>>> combination successfully passes the check >>>> 'if (!im_protocols && !tm_protocols)' in the nfc_start_poll(). >>>> But then after pn533_poll_create_mod_list() call in pn533_start_poll() >>>> poll mod list will remain empty and dev->poll_mod_count will remain 0 >>>> which lead to division by zero. >>>> >>>> Normally no im protocol has value 1 in the mask, so this combination is >>>> not expected by driver. But these protocol values actually come from >>>> userspace via Netlink interface (NFC_CMD_START_POLL operation). So a >>>> broken or malicious program may pass a message containing a "bad" >>>> combination of protocol parameter values so that dev->poll_mod_count >>>> is not incremented inside pn533_poll_create_mod_list(), thus leading >>>> to division by zero. >>>> Call trace looks like: >>>> nfc_genl_start_poll() >>>> nfc_start_poll() >>>> ->start_poll() >>>> pn533_start_poll() >>>> >>>> Add poll mod list filling check. >>>> >>>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>>> >>>> Fixes: dfccd0f58044 ("NFC: pn533: Add some polling entropy") >>>> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> >>> >>> The issue looks real to me and the proposed fix the correct one, but >>> waiting a little more for Krzysztof feedback, as he expressed concerns >>> on v1. >> >> There was one month delay between my reply and clarifications from >> Fedor, so original patch is neither in my mailbox nor in my brain. >> >> >> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> However different problem is: shouldn't as well or instead >> nfc_genl_start_poll() validate the attributes received by netlink? >> >> We just pass them directly to the drivers and several other drivers >> might not expect random stuff there. > > FTR, I had a similar thought and skimmed over other nfc drivers. I did > not see similar issues there. > > Additionally I fear that existing user-space could feed to the kernel > such random stuff and work happily because the kernel is currently > ignoring it - on other drivers. Such cases will suddenly stop working. > > I think we could/should merge the patch as-is, please LMK your thought. Yeah, the patch I already acked, can go in. I am just thinking whether we need a follow-up for core NFC code. Best regards, Krzysztof
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Tue, 27 Aug 2024 11:48:22 +0300 you wrote: > In case of im_protocols value is 1 and tm_protocols value is 0 this > combination successfully passes the check > 'if (!im_protocols && !tm_protocols)' in the nfc_start_poll(). > But then after pn533_poll_create_mod_list() call in pn533_start_poll() > poll mod list will remain empty and dev->poll_mod_count will remain 0 > which lead to division by zero. > > [...] Here is the summary with links: - [v2] nfc: pn533: Add poll mod list filling check https://git.kernel.org/netdev/net/c/febccb39255f You are awesome, thank you!
diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c index b19c39dcfbd9..e2bc67300a91 100644 --- a/drivers/nfc/pn533/pn533.c +++ b/drivers/nfc/pn533/pn533.c @@ -1723,6 +1723,11 @@ static int pn533_start_poll(struct nfc_dev *nfc_dev, } pn533_poll_create_mod_list(dev, im_protocols, tm_protocols); + if (!dev->poll_mod_count) { + nfc_err(dev->dev, + "Poll mod list is empty\n"); + return -EINVAL; + } /* Do not always start polling from the same modulation */ get_random_bytes(&rand_mod, sizeof(rand_mod));
In case of im_protocols value is 1 and tm_protocols value is 0 this combination successfully passes the check 'if (!im_protocols && !tm_protocols)' in the nfc_start_poll(). But then after pn533_poll_create_mod_list() call in pn533_start_poll() poll mod list will remain empty and dev->poll_mod_count will remain 0 which lead to division by zero. Normally no im protocol has value 1 in the mask, so this combination is not expected by driver. But these protocol values actually come from userspace via Netlink interface (NFC_CMD_START_POLL operation). So a broken or malicious program may pass a message containing a "bad" combination of protocol parameter values so that dev->poll_mod_count is not incremented inside pn533_poll_create_mod_list(), thus leading to division by zero. Call trace looks like: nfc_genl_start_poll() nfc_start_poll() ->start_poll() pn533_start_poll() Add poll mod list filling check. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: dfccd0f58044 ("NFC: pn533: Add some polling entropy") Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> --- v2: Update comment message for a more detailed description of the problem drivers/nfc/pn533/pn533.c | 5 +++++ 1 file changed, 5 insertions(+)