From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id ADB9ECCA473 for ; Tue, 7 Jun 2022 02:41:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236021AbiFGClp (ORCPT ); Mon, 6 Jun 2022 22:41:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36346 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233017AbiFGClp (ORCPT ); Mon, 6 Jun 2022 22:41:45 -0400 Received: from mail-yw1-x114a.google.com (mail-yw1-x114a.google.com [IPv6:2607:f8b0:4864:20::114a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4FE8EC0477 for ; Mon, 6 Jun 2022 19:41:44 -0700 (PDT) Received: by mail-yw1-x114a.google.com with SMTP id 00721157ae682-30c2f3431b4so135681097b3.21 for ; Mon, 06 Jun 2022 19:41:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=date:in-reply-to:message-id:mime-version:references:subject:from:to :cc; bh=h0VUAZ+/P4d+rcM7OhdmQUwR8fhTt/+FRpwZ4ssNaoY=; b=haYDdxDzjq/o7bMw1O8Mlq1X4m3OaUPeH0OKYGojaVvWgIZxZpmdkwii/+lK6zjWHS aOLWAHelgGztl/n/rwtDVXw+gVWOOanF88clwbCau7CDuWE1Yt4RnxmIffyC40FlU70U gEhLrZjPYc/Sen/IlkRF2QfeilU4LmlNL+T0uC/K+vHx72Bhd02UFORM8gfPSjBKSl4L grevDTjfPR6v11pX3Th+0ShdBWx0mxJwMq5TaxmyY3XURgfSKEhWdcYhKNdZom2fL+2x Oqv3HK6JJwsVcLNuauSh86gI6laZZGGCoXWFpK3jmRGxAKVH3TMWjZ8lzotzfivFTPA5 mn7w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc; bh=h0VUAZ+/P4d+rcM7OhdmQUwR8fhTt/+FRpwZ4ssNaoY=; b=jmWUHhk+XQxir26AwQpYcH2tyn+13+hC5vWTdFwuAZEPc9dUsqhE+N8SceU2gImjuf uoKbcjvnm+3fo4DIU2El8besmosk1WDBIph0e1e3fhUdLfkN83i/6oabeDk5XM0Oy/Wk ZFAKbWpaRvXkU00OjhijXA3HaBqEI5p/+0sWvL4TUr01Uxga82W+jC8fRynTx/LlOpfX 66Uh2c1vpfn4I41TkPYafZB9iWtuzKJ7uE8+xW068qVK5V9yWesQ09XuWvIBOPoKLJRS Re9e0PIXhrNp5vudzwgIadcy7g+2j0yzWP9wnJlk7vDczji9Dr9+iUwFeFWtteCO8y/7 /hQA== X-Gm-Message-State: AOAM5308ZKmirMsajDXpjhqvYR7A0NontM7ndFtCxVUP6W2yatuU7tuB YCgJnZ0bAD8lhz72hg/In7s1x1B8eT1vANk= X-Google-Smtp-Source: ABdhPJzYjvzQ2mk7L9QIklTyh4SyHlmBpJHBqK62ybGhp/eKZKog2y6pfEgoZ35c1wSZXy2FdqdGapRaNH053gA= X-Received: from sunrising.c.googlers.com ([fda3:e722:ac3:cc00:2b:ff92:c0a8:fb8]) (user=mfaltesek job=sendgmr) by 2002:a25:c694:0:b0:65c:85e6:e7d9 with SMTP id k142-20020a25c694000000b0065c85e6e7d9mr29814327ybf.333.1654569703584; Mon, 06 Jun 2022 19:41:43 -0700 (PDT) Date: Mon, 6 Jun 2022 21:41:17 -0500 In-Reply-To: <20220607024117.1344044-1-mfaltesek@google.com> Message-Id: <20220607024117.1344044-4-mfaltesek@google.com> Mime-Version: 1.0 References: <20220607024117.1344044-1-mfaltesek@google.com> X-Mailer: git-send-email 2.36.1.255.ge46751e96f-goog Subject: [PATCH net v3 3/3] nfc: st21nfca: fix incorrect sizing calculations in EVT_TRANSACTION From: Martin Faltesek To: martin.faltesek@gmail.com Cc: Martin Faltesek , stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org The transaction buffer is allocated by using the size of the packet buf, and subtracting two which seem intended to remove the two tags which are not present in the target structure. This calculation leads to under counting memory because of differences between the packet contents and the target structure. The aid_len field is a u8 in the packet, but a u32 in the structure, resulting in at least 3 bytes always being under counted. Further, the aid data is a variable length field in the packet, but fixed in the structure, so if this field is less than the max, the difference is added to the under counting. The last validation check for transaction->params_len is also incorrect since it employs the same accounting error. To fix, perform validation checks progressively to safely reach the next field, to determine the size of both buffers and verify both tags. Once all validation checks pass, allocate the buffer and copy the data. This eliminates freeing memory on the error path, as those checks are moved ahead of memory allocation. Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support") Fixes: 4fbcc1a4cb20 ("nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION") Cc: stable@vger.kernel.org Signed-off-by: Martin Faltesek --- drivers/nfc/st21nfca/se.c | 60 +++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c index 8e1113ce139b..df8d27cf2956 100644 --- a/drivers/nfc/st21nfca/se.c +++ b/drivers/nfc/st21nfca/se.c @@ -300,6 +300,8 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host, int r = 0; struct device *dev = &hdev->ndev->dev; struct nfc_evt_transaction *transaction; + u32 aid_len; + u8 params_len; pr_debug("connectivity gate event: %x\n", event); @@ -308,50 +310,48 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host, r = nfc_se_connectivity(hdev->ndev, host); break; case ST21NFCA_EVT_TRANSACTION: - /* - * According to specification etsi 102 622 + /* According to specification etsi 102 622 * 11.2.2.4 EVT_TRANSACTION Table 52 * Description Tag Length * AID 81 5 to 16 * PARAMETERS 82 0 to 255 + * + * The key differences are aid storage length is variably sized + * in the packet, but fixed in nfc_evt_transaction, and that the aid_len + * is u8 in the packet, but u32 in the structure, and the tags in + * the packet are not included in nfc_evt_transaction. + * + * size in bytes: 1 1 5-16 1 1 0-255 + * offset: 0 1 2 aid_len + 2 aid_len + 3 aid_len + 4 + * member name: aid_tag(M) aid_len aid params_tag(M) params_len params + * example: 0x81 5-16 X 0x82 0-255 X */ - if (skb->len < NFC_MIN_AID_LENGTH + 2 || - skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG) + if (skb->len < 2 || skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG) return -EPROTO; - transaction = devm_kzalloc(dev, skb->len - 2, GFP_KERNEL); - if (!transaction) - return -ENOMEM; - - transaction->aid_len = skb->data[1]; + aid_len = skb->data[1]; - /* Checking if the length of the AID is valid */ - if (transaction->aid_len > sizeof(transaction->aid)) { - devm_kfree(dev, transaction); - return -EINVAL; - } + if (skb->len < aid_len + 4 || aid_len > sizeof(transaction->aid)) + return -EPROTO; - memcpy(transaction->aid, &skb->data[2], - transaction->aid_len); + params_len = skb->data[aid_len + 3]; - /* Check next byte is PARAMETERS tag (82) */ - if (skb->data[transaction->aid_len + 2] != - NFC_EVT_TRANSACTION_PARAMS_TAG) { - devm_kfree(dev, transaction); + /* Verify PARAMETERS tag is (82), and final check that there is enough + * space in the packet to read everything. + */ + if ((skb->data[aid_len + 2] != NFC_EVT_TRANSACTION_PARAMS_TAG) || + (skb->len < aid_len + 4 + params_len)) return -EPROTO; - } - transaction->params_len = skb->data[transaction->aid_len + 3]; + transaction = devm_kzalloc(dev, sizeof(*transaction) + params_len, GFP_KERNEL); + if (!transaction) + return -ENOMEM; - /* Total size is allocated (skb->len - 2) minus fixed array members */ - if (transaction->params_len > ((skb->len - 2) - - sizeof(struct nfc_evt_transaction))) { - devm_kfree(dev, transaction); - return -EINVAL; - } + transaction->aid_len = aid_len; + transaction->params_len = params_len; - memcpy(transaction->params, skb->data + - transaction->aid_len + 4, transaction->params_len); + memcpy(transaction->aid, &skb->data[2], aid_len); + memcpy(transaction->params, &skb->data[aid_len + 4], params_len); r = nfc_se_transaction(hdev->ndev, host, transaction); break; -- 2.36.1.255.ge46751e96f-goog