* [PATCH v2 1/3] NFC: st21nfca: Fix out of bounds kernel access when handling ATR_REQ
2018-05-02 17:48 [PATCH v2 0/3] Few NFC fixes from android-4.14 tree Amit Pundir
@ 2018-05-02 17:48 ` Amit Pundir
2018-05-02 17:48 ` [PATCH v2 2/3] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands Amit Pundir
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Amit Pundir @ 2018-05-02 17:48 UTC (permalink / raw)
To: lkml, linux-wireless
Cc: Suren Baghdasaryan, Samuel Ortiz, Christophe Ricard,
Andy Shevchenko, Greg KH, John Stultz, Dmitry Shmidt, Todd Kjos,
Android Kernel Team
From: Suren Baghdasaryan <surenb@google.com>
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 <surenb@google.com>
Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands
2018-05-02 17:48 [PATCH v2 0/3] Few NFC fixes from android-4.14 tree Amit Pundir
2018-05-02 17:48 ` [PATCH v2 1/3] NFC: st21nfca: Fix out of bounds kernel access when handling ATR_REQ Amit Pundir
@ 2018-05-02 17:48 ` Amit Pundir
2018-05-02 17:48 ` [PATCH v2 3/3] NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver Amit Pundir
2018-05-03 10:21 ` Andy Shevchenko
3 siblings, 0 replies; 9+ messages in thread
From: Amit Pundir @ 2018-05-02 17:48 UTC (permalink / raw)
To: lkml, linux-wireless
Cc: Suren Baghdasaryan, Samuel Ortiz, Christophe Ricard,
Andy Shevchenko, Greg KH, John Stultz, Dmitry Shmidt, Todd Kjos,
Android Kernel Team
From: Suren Baghdasaryan <surenb@google.com>
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 <surenb@google.com>
Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver
2018-05-02 17:48 [PATCH v2 0/3] Few NFC fixes from android-4.14 tree Amit Pundir
2018-05-02 17:48 ` [PATCH v2 1/3] NFC: st21nfca: Fix out of bounds kernel access when handling ATR_REQ Amit Pundir
2018-05-02 17:48 ` [PATCH v2 2/3] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands Amit Pundir
@ 2018-05-02 17:48 ` Amit Pundir
2018-05-03 10:20 ` Andy Shevchenko
2018-05-03 10:21 ` Andy Shevchenko
3 siblings, 1 reply; 9+ messages in thread
From: Amit Pundir @ 2018-05-02 17:48 UTC (permalink / raw)
To: lkml, linux-wireless
Cc: Suren Baghdasaryan, Samuel Ortiz, Christophe Ricard,
Andy Shevchenko, Greg KH, John Stultz, Dmitry Shmidt, Todd Kjos,
Android Kernel Team
From: Suren Baghdasaryan <surenb@google.com>
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 <surenb@google.com>
Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver
2018-05-02 17:48 ` [PATCH v2 3/3] NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver Amit Pundir
@ 2018-05-03 10:20 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2018-05-03 10:20 UTC (permalink / raw)
To: Amit Pundir, lkml, linux-wireless
Cc: Suren Baghdasaryan, Samuel Ortiz, Christophe Ricard, Greg KH,
John Stultz, Dmitry Shmidt, Todd Kjos, Android Kernel Team
On Wed, 2018-05-02 at 23:18 +0530, Amit Pundir wrote:
> From: Suren Baghdasaryan <surenb@google.com>
>
> 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 <surenb@google.com>
> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
> ---
> 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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver
@ 2018-05-03 10:20 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2018-05-03 10:20 UTC (permalink / raw)
To: Amit Pundir, lkml, linux-wireless
Cc: Suren Baghdasaryan, Samuel Ortiz, Christophe Ricard, Greg KH,
John Stultz, Dmitry Shmidt, Todd Kjos, Android Kernel Team
On Wed, 2018-05-02 at 23:18 +0530, Amit Pundir wrote:
> From: Suren Baghdasaryan <surenb@google.com>
>
> 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 <surenb@google.com>
> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
> ---
> 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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver
2018-05-03 10:20 ` Andy Shevchenko
(?)
@ 2018-05-03 18:26 ` Amit Pundir
-1 siblings, 0 replies; 9+ messages in thread
From: Amit Pundir @ 2018-05-03 18:26 UTC (permalink / raw)
To: Andy Shevchenko
Cc: lkml, linux-wireless, Suren Baghdasaryan, Samuel Ortiz,
Christophe Ricard, Greg KH, John Stultz, Dmitry Shmidt,
Todd Kjos, Android Kernel Team
On 3 May 2018 at 15:50, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, 2018-05-02 at 23:18 +0530, Amit Pundir wrote:
>> From: Suren Baghdasaryan <surenb@google.com>
>>
>> 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 <surenb@google.com>
>> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
>> ---
>> 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 <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/3] Few NFC fixes from android-4.14 tree
2018-05-02 17:48 [PATCH v2 0/3] Few NFC fixes from android-4.14 tree Amit Pundir
@ 2018-05-03 10:21 ` Andy Shevchenko
2018-05-02 17:48 ` [PATCH v2 2/3] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands Amit Pundir
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2018-05-03 10:21 UTC (permalink / raw)
To: Amit Pundir, lkml, linux-wireless
Cc: Suren Baghdasaryan, Samuel Ortiz, Christophe Ricard, Greg KH,
John Stultz, Dmitry Shmidt, Todd Kjos, Android Kernel Team
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 <andriy.shevchenko@linux.intel.com>
> 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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/3] Few NFC fixes from android-4.14 tree
@ 2018-05-03 10:21 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2018-05-03 10:21 UTC (permalink / raw)
To: Amit Pundir, lkml, linux-wireless
Cc: Suren Baghdasaryan, Samuel Ortiz, Christophe Ricard, Greg KH,
John Stultz, Dmitry Shmidt, Todd Kjos, Android Kernel Team
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 <andriy.shevchenko@linux.intel.com>
> 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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 9+ messages in thread