[PATCH v2 0/3] Few NFC fixes from android-4.14 tree

[PATCH v2 0/3] Few NFC fixes from android-4.14 tree

[Amit Pundir]

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

[PATCH v2 1/3] NFC: st21nfca: Fix out of bounds kernel access when handling ATR_REQ

[Amit Pundir]

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

[PATCH v2 2/3] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands

[Amit Pundir]

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

[PATCH v2 3/3] NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver

[Amit Pundir]

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

Re: [PATCH v2 3/3] NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver

[Andy Shevchenko]

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

Re: [PATCH v2 3/3] NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver

[Andy Shevchenko]

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

Re: [PATCH v2 0/3] Few NFC fixes from android-4.14 tree

[Andy Shevchenko]

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

Re: [PATCH v2 0/3] Few NFC fixes from android-4.14 tree

[Andy Shevchenko]

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

Re: [PATCH v2 3/3] NFC: fdp: Fix possible buffer overflow in WCS4000 NFC driver

[Amit Pundir]

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