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
| ||
|
Message-Id: <20220329175431.3175472-1-mfaltesek@google.com> Date: Tue, 29 Mar 2022 12:54:31 -0500 From: Martin Faltesek <mfaltesek@...omium.org> To: netdev@...r.kernel.org, krzk@...nel.org, christophe.ricard@...il.com, jordy@...ing.systems Cc: sameo@...ux.intel.com, wklin@...gle.com, groeck@...gle.com, surenb@...gle.com, mfaltesek@...gle.com, gregkh@...uxfoundation.org Subject: [PATCH] nfc: st21nfca: Refactor EVT_TRANSACTION EVT_TRANSACTION has four different bugs: 1. First conditional has logical AND but should be OR. It should always check if it isn't NFC_EVT_TRANSACTION_AID_TAG, then bail. 2. Potential under allocating memory:devm_kzalloc (skb->len - 2) when the aid_len specified in the packet is less than the fixed NFC_MAX_AID_LENGTH in struct nfc_evt_transaction. In addition, aid_len is u32 in the data structure, and u8 in the packet, under counting 3 more bytes. 3. Memory leaks after kzalloc when returning error. 4. The final conditional check is also incorrect, for the same reasons explained in #2. Fixes: 26fc6c7f02cb ("NFC: st21nfca: Add HCI transaction event support") Fixes: 4fbcc1a4cb20 ("nfc: st21nfca: Fix potential buffer overflows in EVT_TRANSACTION") Signed-off-by: Martin Faltesek <mfaltesek@...gle.com> --- drivers/nfc/st21nfca/se.c | 65 ++++++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c index c922f10d0d7b..acc8d831246a 100644 --- a/drivers/nfc/st21nfca/se.c +++ b/drivers/nfc/st21nfca/se.c @@ -292,6 +292,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); @@ -306,37 +308,56 @@ int st21nfca_connectivity_event_received(struct nfc_hci_dev *hdev, u8 host, * 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 part of nfc_evt_transaction. + * + * size in bytes: 1 1 5-16 1 1 0-255 + * offset: 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) - return -EPROTO; - - transaction = devm_kzalloc(dev, skb->len - 2, GFP_KERNEL); - if (!transaction) - return -ENOMEM; - transaction->aid_len = skb->data[1]; + /* + * Validate the packet is large enough to read the first two bytes + * containing the aid_tag and aid_len, and then read both. Capacity + * checks are expanded incrementally after this, for clarity. + */ + if (skb->len < 2 || skb->data[0] != NFC_EVT_TRANSACTION_AID_TAG) + return -EPROTO; - /* Checking if the length of the AID is valid */ - if (transaction->aid_len > sizeof(transaction->aid)) - return -EINVAL; + aid_len = skb->data[1]; + /* + * With the actual aid_len, verify there is enough space in + * the packet to read params_tag and params_len, and that + * aid_len does not exceed destination capacity. Reference + * offset comment above for +4 +3 +2 offsets used. + */ + 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) + /* + * Verify PARAMETERS tag is (82), and final validation that enough + * space in 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(struct nfc_evt_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))) - 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.35.1.1021.g381101b075-goog
Powered by blists - more mailing lists