Hi,
Submitting v2 of NFC fixes I picked up from android-4.14 tree[1]
for review and comments.
Again like to point out that I have not feature tested these patches
at all. Only made small cosmetic changes to the original patches
(removed Android-only tag and internal bug ID) and build tested for
arm, before posting them here for review.
Really appreciate any comments or feedback on how to take it forward.
Changes since v1:
* Dropped "NFC: st21nfca: Fix memory OOB and leak issues in connectivity
events handler" patch for now. I'm yet to verify if the additional
aid_len and params_len checks for buffer size are really required, and
I didn't want to hold up this patch series for one patch alone.
* Dropped redundant __func__ use dev_dbg() in "NFC: fdp: Fix possible
buffer overflow in WCS4000 NFC driver" patch.
Also drivers/nfc/fdp/ is full of __func__ parameter usage in dev_dbg(),
so submitting a new patch separately to clean that up.
Regards,
Amit Pundir
[1] https://android.googlesource.com/kernel/common/+log/android-4.14
Suren Baghdasaryan (3):
NFC: st21nfca: Fix out of bounds kernel access when handling ATR_REQ
NFC: Fix possible memory corruption when handling SHDLC I-Frame
commands
NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver
drivers/nfc/fdp/fdp.c | 22 +++++++++++-----------
drivers/nfc/fdp/i2c.c | 29 ++++++++++++++++++-----------
drivers/nfc/st21nfca/dep.c | 3 ++-
net/nfc/hci/core.c | 10 ++++++++++
4 files changed, 41 insertions(+), 23 deletions(-)
--
2.7.4
From: Suren Baghdasaryan <[email protected]>
Out of bounds kernel accesses in st21nfca's NFC HCI layer
might happen when handling ATR_REQ events if user-specified
atr_req->length is bigger than the buffer size. In
that case memcpy() inside st21nfca_tm_send_atr_res() will
read extra bytes resulting in OOB read from the kernel heap.
Signed-off-by: Suren Baghdasaryan <[email protected]>
Signed-off-by: Amit Pundir <[email protected]>
---
v2:
Resend. No changes.
drivers/nfc/st21nfca/dep.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/nfc/st21nfca/dep.c b/drivers/nfc/st21nfca/dep.c
index fd08be2..3420c51 100644
--- a/drivers/nfc/st21nfca/dep.c
+++ b/drivers/nfc/st21nfca/dep.c
@@ -217,7 +217,8 @@ static int st21nfca_tm_recv_atr_req(struct nfc_hci_dev *hdev,
atr_req = (struct st21nfca_atr_req *)skb->data;
- if (atr_req->length < sizeof(struct st21nfca_atr_req)) {
+ if (atr_req->length < sizeof(struct st21nfca_atr_req) ||
+ atr_req->length > skb->len) {
r = -EPROTO;
goto exit;
}
--
2.7.4
On Wed, 2018-05-02 at 23:18 +0530, Amit Pundir wrote:
> From: Suren Baghdasaryan <[email protected]>
>
> Possible buffer overflow when reading next_read_size bytes into
> tmp buffer after next_read_size was extracted from a previous packet.
>
> Signed-off-by: Suren Baghdasaryan <[email protected]>
> Signed-off-by: Amit Pundir <[email protected]>
> ---
> v2:
> Remove redundant __func__ from dev_dgb().
>
> drivers/nfc/fdp/i2c.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/nfc/fdp/i2c.c b/drivers/nfc/fdp/i2c.c
> index c4da50e..b80d1ad 100644
> --- a/drivers/nfc/fdp/i2c.c
> +++ b/drivers/nfc/fdp/i2c.c
> @@ -176,6 +176,15 @@ static int fdp_nci_i2c_read(struct fdp_i2c_phy
> *phy, struct sk_buff **skb)
> /* Packet that contains a length */
> if (tmp[0] == 0 && tmp[1] == 0) {
> phy->next_read_size = (tmp[2] << 8) + tmp[3]
> + 3;
> + /*
> + * Ensure next_read_size does not exceed
> sizeof(tmp)
> + * for reading that many bytes during next
> iteration
> + */
> + if (phy->next_read_size >
> FDP_NCI_I2C_MAX_PAYLOAD) {
> + dev_dbg(&client->dev, "corrupted
> packet\n");
> + phy->next_read_size = 5;
Shouldn't be this magic replaced by
phy->next_read_size = FDP_NCI_I2C_MIN_PAYLOAD;
?
> + goto flush;
> + }
> } else {
> phy->next_read_size =
> FDP_NCI_I2C_MIN_PAYLOAD;
>
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
From: Suren Baghdasaryan <[email protected]>
When handling SHDLC I-Frame commands "pipe" field used for indexing
into an array should be checked before usage. If left unchecked it
might access memory outside of the array of size NFC_HCI_MAX_PIPES(127).
Signed-off-by: Suren Baghdasaryan <[email protected]>
Signed-off-by: Amit Pundir <[email protected]>
---
v2:
Resend. No changes.
net/nfc/hci/core.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/net/nfc/hci/core.c b/net/nfc/hci/core.c
index ac8030c4..19cb2e4 100644
--- a/net/nfc/hci/core.c
+++ b/net/nfc/hci/core.c
@@ -209,6 +209,11 @@ void nfc_hci_cmd_received(struct nfc_hci_dev *hdev, u8 pipe, u8 cmd,
}
create_info = (struct hci_create_pipe_resp *)skb->data;
+ if (create_info->pipe >= NFC_HCI_MAX_PIPES) {
+ status = NFC_HCI_ANY_E_NOK;
+ goto exit;
+ }
+
/* Save the new created pipe and bind with local gate,
* the description for skb->data[3] is destination gate id
* but since we received this cmd from host controller, we
@@ -232,6 +237,11 @@ void nfc_hci_cmd_received(struct nfc_hci_dev *hdev, u8 pipe, u8 cmd,
}
delete_info = (struct hci_delete_pipe_noti *)skb->data;
+ if (delete_info->pipe >= NFC_HCI_MAX_PIPES) {
+ status = NFC_HCI_ANY_E_NOK;
+ goto exit;
+ }
+
hdev->pipes[delete_info->pipe].gate = NFC_HCI_INVALID_GATE;
hdev->pipes[delete_info->pipe].dest_host = NFC_HCI_INVALID_HOST;
break;
--
2.7.4
On Wed, 2018-05-02 at 23:18 +0530, Amit Pundir wrote:
> Hi,
>
> Submitting v2 of NFC fixes I picked up from android-4.14 tree[1]
> for review and comments.
>
> Again like to point out that I have not feature tested these patches
> at all. Only made small cosmetic changes to the original patches
> (removed Android-only tag and internal bug ID) and build tested for
> arm, before posting them here for review.
>
> Really appreciate any comments or feedback on how to take it forward.
>
> Changes since v1:
> * Dropped "NFC: st21nfca: Fix memory OOB and leak issues in
> connectivity
> events handler" patch for now. I'm yet to verify if the additional
> aid_len and params_len checks for buffer size are really required,
> and
> I didn't want to hold up this patch series for one patch alone.
> * Dropped redundant __func__ use dev_dbg() in "NFC: fdp: Fix possible
> buffer overflow in WCS4000 NFC driver" patch.
>
> Also drivers/nfc/fdp/ is full of __func__ parameter usage in
> dev_dbg(),
> so submitting a new patch separately to clean that up.
>
After addressing one comment, FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>
> Regards,
> Amit Pundir
> [1] https://android.googlesource.com/kernel/common/+log/android-4.14
>
> Suren Baghdasaryan (3):
> NFC: st21nfca: Fix out of bounds kernel access when handling ATR_REQ
> NFC: Fix possible memory corruption when handling SHDLC I-Frame
> commands
> NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver
>
> drivers/nfc/fdp/fdp.c | 22 +++++++++++-----------
> drivers/nfc/fdp/i2c.c | 29 ++++++++++++++++++-----------
> drivers/nfc/st21nfca/dep.c | 3 ++-
> net/nfc/hci/core.c | 10 ++++++++++
> 4 files changed, 41 insertions(+), 23 deletions(-)
>
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
On 3 May 2018 at 15:50, Andy Shevchenko
<[email protected]> wrote:
> On Wed, 2018-05-02 at 23:18 +0530, Amit Pundir wrote:
>> From: Suren Baghdasaryan <[email protected]>
>>
>> Possible buffer overflow when reading next_read_size bytes into
>> tmp buffer after next_read_size was extracted from a previous packet.
>>
>> Signed-off-by: Suren Baghdasaryan <[email protected]>
>> Signed-off-by: Amit Pundir <[email protected]>
>> ---
>> v2:
>> Remove redundant __func__ from dev_dgb().
>>
>> drivers/nfc/fdp/i2c.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/nfc/fdp/i2c.c b/drivers/nfc/fdp/i2c.c
>> index c4da50e..b80d1ad 100644
>> --- a/drivers/nfc/fdp/i2c.c
>> +++ b/drivers/nfc/fdp/i2c.c
>> @@ -176,6 +176,15 @@ static int fdp_nci_i2c_read(struct fdp_i2c_phy
>> *phy, struct sk_buff **skb)
>> /* Packet that contains a length */
>> if (tmp[0] == 0 && tmp[1] == 0) {
>> phy->next_read_size = (tmp[2] << 8) + tmp[3]
>> + 3;
>> + /*
>> + * Ensure next_read_size does not exceed
>> sizeof(tmp)
>> + * for reading that many bytes during next
>> iteration
>> + */
>> + if (phy->next_read_size >
>> FDP_NCI_I2C_MAX_PAYLOAD) {
>> + dev_dbg(&client->dev, "corrupted
>> packet\n");
>
>> + phy->next_read_size = 5;
>
> Shouldn't be this magic replaced by
>
> phy->next_read_size = FDP_NCI_I2C_MIN_PAYLOAD;
>
> ?
Ack. Fixing it in v3.
Regards,
Amit Pundir
>
>> + goto flush;
>> + }
>> } else {
>> phy->next_read_size =
>> FDP_NCI_I2C_MIN_PAYLOAD;
>>
>
> --
> Andy Shevchenko <[email protected]>
> Intel Finland Oy
From: Suren Baghdasaryan <[email protected]>
Possible buffer overflow when reading next_read_size bytes into
tmp buffer after next_read_size was extracted from a previous packet.
Signed-off-by: Suren Baghdasaryan <[email protected]>
Signed-off-by: Amit Pundir <[email protected]>
---
v2:
Remove redundant __func__ from dev_dgb().
drivers/nfc/fdp/i2c.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/nfc/fdp/i2c.c b/drivers/nfc/fdp/i2c.c
index c4da50e..b80d1ad 100644
--- a/drivers/nfc/fdp/i2c.c
+++ b/drivers/nfc/fdp/i2c.c
@@ -176,6 +176,15 @@ static int fdp_nci_i2c_read(struct fdp_i2c_phy *phy, struct sk_buff **skb)
/* Packet that contains a length */
if (tmp[0] == 0 && tmp[1] == 0) {
phy->next_read_size = (tmp[2] << 8) + tmp[3] + 3;
+ /*
+ * Ensure next_read_size does not exceed sizeof(tmp)
+ * for reading that many bytes during next iteration
+ */
+ if (phy->next_read_size > FDP_NCI_I2C_MAX_PAYLOAD) {
+ dev_dbg(&client->dev, "corrupted packet\n");
+ phy->next_read_size = 5;
+ goto flush;
+ }
} else {
phy->next_read_size = FDP_NCI_I2C_MIN_PAYLOAD;
--
2.7.4