* [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands @ 2018-08-13 22:39 Suren Baghdasaryan 2018-08-14 0:28 ` Kees Cook ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Suren Baghdasaryan @ 2018-08-13 22:39 UTC (permalink / raw) Cc: security, kdeus, surenb, Samuel Ortiz, David S. Miller, Allen Pais, Kees Cook, linux-wireless, netdev, linux-kernel 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). Malformed NFC HCI frames could be injected by a malicious NFC device communicating with the device being attacked (remote attack vector), or even by an attacker with physical access to the I2C bus such that they could influence the data transfers on that bus (local attack vector). skb->data is controlled by the attacker and has only been sanitized in the most trivial ways (CRC check), therefore we can consider the create_info struct and all of its members to tainted. 'create_info->pipe' with max value of 255 (uint8) is used to take an offset of the hdev->pipes array of 127 elements which can lead to OOB write. Suggested-by: Kevin Deus <kdeus@google.com> Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- 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 ac8030c4bcf8..19cb2e473ea6 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.18.0.597.ga71716f1ad-goog ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-13 22:39 [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands Suren Baghdasaryan @ 2018-08-14 0:28 ` Kees Cook 2018-08-14 9:21 ` Greg KH 2018-08-14 9:54 ` Dan Carpenter 2 siblings, 0 replies; 15+ messages in thread From: Kees Cook @ 2018-08-14 0:28 UTC (permalink / raw) To: Suren Baghdasaryan Cc: Security Officers, kdeus, Samuel Ortiz, David S. Miller, Allen Pais, linux-wireless, Network Development, LKML On Mon, Aug 13, 2018 at 3:39 PM, Suren Baghdasaryan <surenb@google.com> wrote: > 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). > > Malformed NFC HCI frames could be injected by a malicious NFC device > communicating with the device being attacked (remote attack vector), > or even by an attacker with physical access to the I2C bus such that > they could influence the data transfers on that bus (local attack vector). > skb->data is controlled by the attacker and has only been sanitized in > the most trivial ways (CRC check), therefore we can consider the > create_info struct and all of its members to tainted. 'create_info->pipe' > with max value of 255 (uint8) is used to take an offset of the > hdev->pipes array of 127 elements which can lead to OOB write. > > Suggested-by: Kevin Deus <kdeus@google.com> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> Nice find! Acked-by: Kees Cook <keescook@chromium.org> > --- > 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 ac8030c4bcf8..19cb2e473ea6 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.18.0.597.ga71716f1ad-goog > -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-13 22:39 [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands Suren Baghdasaryan 2018-08-14 0:28 ` Kees Cook @ 2018-08-14 9:21 ` Greg KH 2018-08-14 9:54 ` Dan Carpenter 2 siblings, 0 replies; 15+ messages in thread From: Greg KH @ 2018-08-14 9:21 UTC (permalink / raw) To: Suren Baghdasaryan Cc: security, kdeus, Samuel Ortiz, David S. Miller, Allen Pais, Kees Cook, linux-wireless, netdev, linux-kernel On Mon, Aug 13, 2018 at 03:39:08PM -0700, Suren Baghdasaryan wrote: > 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). > > Malformed NFC HCI frames could be injected by a malicious NFC device > communicating with the device being attacked (remote attack vector), > or even by an attacker with physical access to the I2C bus such that > they could influence the data transfers on that bus (local attack vector). > skb->data is controlled by the attacker and has only been sanitized in > the most trivial ways (CRC check), therefore we can consider the > create_info struct and all of its members to tainted. 'create_info->pipe' > with max value of 255 (uint8) is used to take an offset of the > hdev->pipes array of 127 elements which can lead to OOB write. > > Suggested-by: Kevin Deus <kdeus@google.com> > Signed-off-by: Suren Baghdasaryan <surenb@google.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-13 22:39 [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands Suren Baghdasaryan 2018-08-14 0:28 ` Kees Cook 2018-08-14 9:21 ` Greg KH @ 2018-08-14 9:54 ` Dan Carpenter 2018-08-14 16:57 ` Suren Baghdasaryan 2 siblings, 1 reply; 15+ messages in thread From: Dan Carpenter @ 2018-08-14 9:54 UTC (permalink / raw) To: Suren Baghdasaryan Cc: security, kdeus, Samuel Ortiz, David S. Miller, Allen Pais, Kees Cook, linux-wireless, netdev, linux-kernel Thanks. This is great. I'm so glad these are finally getting fixed. Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT (0x7f) so that's one element beyond the end of the array and the NFC_HCI_HCP_RESPONSE isn't checked. Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use NCI_HCP_MSG_GET_PIPE() so those could be off by one. regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-14 9:54 ` Dan Carpenter @ 2018-08-14 16:57 ` Suren Baghdasaryan 2018-08-14 20:26 ` Suren Baghdasaryan 0 siblings, 1 reply; 15+ messages in thread From: Suren Baghdasaryan @ 2018-08-14 16:57 UTC (permalink / raw) To: Dan Carpenter Cc: security, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais, Kees Cook, linux-wireless, netdev, linux-kernel On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > Thanks. This is great. I'm so glad these are finally getting fixed. > > Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as > well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT > (0x7f) so that's one element beyond the end of the array and the > NFC_HCI_HCP_RESPONSE isn't checked. > > Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use > NCI_HCP_MSG_GET_PIPE() so those could be off by one. Good point. From hci.h: /* * According to specification 102 622 chapter 4.4 Pipes, * the pipe identifier is 7 bits long. */ #define NFC_HCI_MAX_PIPES 127 And then: struct nfc_hci_dev { ... struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES]; ... } I think the correct fix would be to change it to: struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1]; What do you think? > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-14 16:57 ` Suren Baghdasaryan @ 2018-08-14 20:26 ` Suren Baghdasaryan 2018-08-14 20:33 ` Kees Cook 0 siblings, 1 reply; 15+ messages in thread From: Suren Baghdasaryan @ 2018-08-14 20:26 UTC (permalink / raw) To: Dan Carpenter Cc: security, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais, Kees Cook, linux-wireless, netdev, linux-kernel On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@google.com> wrote: > On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: >> Thanks. This is great. I'm so glad these are finally getting fixed. >> >> Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as >> well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT >> (0x7f) so that's one element beyond the end of the array and the >> NFC_HCI_HCP_RESPONSE isn't checked. >> >> Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use >> NCI_HCP_MSG_GET_PIPE() so those could be off by one. > > Good point. From hci.h: > > /* > * According to specification 102 622 chapter 4.4 Pipes, > * the pipe identifier is 7 bits long. > */ > #define NFC_HCI_MAX_PIPES 127 > > And then: > > struct nfc_hci_dev { > ... > struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES]; > ... > } > > I think the correct fix would be to change it to: > > struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1]; > > What do you think? > Just to be clear, this would fix the problem Dan described in his reply and it should be implemented in a separate patch. The original fix is still valid. >> >> regards, >> dan carpenter >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-14 20:26 ` Suren Baghdasaryan @ 2018-08-14 20:33 ` Kees Cook 2018-08-14 20:55 ` Suren Baghdasaryan 0 siblings, 1 reply; 15+ messages in thread From: Kees Cook @ 2018-08-14 20:33 UTC (permalink / raw) To: Suren Baghdasaryan Cc: Dan Carpenter, Security Officers, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais, linux-wireless, Network Development, LKML On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan <surenb@google.com> wrote: > On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@google.com> wrote: >> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: >>> Thanks. This is great. I'm so glad these are finally getting fixed. >>> >>> Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as >>> well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT >>> (0x7f) so that's one element beyond the end of the array and the >>> NFC_HCI_HCP_RESPONSE isn't checked. >>> >>> Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use >>> NCI_HCP_MSG_GET_PIPE() so those could be off by one. >> >> Good point. From hci.h: >> >> /* >> * According to specification 102 622 chapter 4.4 Pipes, >> * the pipe identifier is 7 bits long. >> */ >> #define NFC_HCI_MAX_PIPES 127 >> >> And then: >> >> struct nfc_hci_dev { >> ... >> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES]; >> ... >> } >> >> I think the correct fix would be to change it to: >> >> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1]; >> >> What do you think? >> > > Just to be clear, this would fix the problem Dan described in his > reply and it should be implemented in a separate patch. The original > fix is still valid. I think you could merge the fixes into a single patch. -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-14 20:33 ` Kees Cook @ 2018-08-14 20:55 ` Suren Baghdasaryan 2018-08-14 21:49 ` Kees Cook 0 siblings, 1 reply; 15+ messages in thread From: Suren Baghdasaryan @ 2018-08-14 20:55 UTC (permalink / raw) To: Kees Cook Cc: Dan Carpenter, Security Officers, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais, linux-wireless, Network Development, LKML On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook <keescook@chromium.org> wrote: > On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan <surenb@google.com> wrote: >> On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@google.com> wrote: >>> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: >>>> Thanks. This is great. I'm so glad these are finally getting fixed. >>>> >>>> Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as >>>> well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT >>>> (0x7f) so that's one element beyond the end of the array and the >>>> NFC_HCI_HCP_RESPONSE isn't checked. >>>> >>>> Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use >>>> NCI_HCP_MSG_GET_PIPE() so those could be off by one. >>> >>> Good point. From hci.h: >>> >>> /* >>> * According to specification 102 622 chapter 4.4 Pipes, >>> * the pipe identifier is 7 bits long. >>> */ >>> #define NFC_HCI_MAX_PIPES 127 >>> >>> And then: >>> >>> struct nfc_hci_dev { >>> ... >>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES]; >>> ... >>> } >>> >>> I think the correct fix would be to change it to: >>> >>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1]; >>> >>> What do you think? >>> >> >> Just to be clear, this would fix the problem Dan described in his >> reply and it should be implemented in a separate patch. The original >> fix is still valid. > > I think you could merge the fixes into a single patch. Couple reasons I would prefer to keep them separate: - I feel that descriptions for these two issues should be different and it's easier if we don't mix them up - This one is already merged into Android kernels, so would be easier to introduce the second fix separately - I would like to give credit to people who noticed the problem (in this case those are different people) However if more people think we should fix both issues in the same patch I'll happily do that. Thanks! > > -Kees > > -- > Kees Cook > Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-14 20:55 ` Suren Baghdasaryan @ 2018-08-14 21:49 ` Kees Cook 2018-08-14 22:19 ` Suren Baghdasaryan 0 siblings, 1 reply; 15+ messages in thread From: Kees Cook @ 2018-08-14 21:49 UTC (permalink / raw) To: Suren Baghdasaryan Cc: Dan Carpenter, Security Officers, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais, linux-wireless, Network Development, LKML On Tue, Aug 14, 2018 at 1:55 PM, Suren Baghdasaryan <surenb@google.com> wrote: > On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook <keescook@chromium.org> wrote: >> On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan <surenb@google.com> wrote: >>> On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@google.com> wrote: >>>> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: >>>>> Thanks. This is great. I'm so glad these are finally getting fixed. >>>>> >>>>> Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as >>>>> well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT >>>>> (0x7f) so that's one element beyond the end of the array and the >>>>> NFC_HCI_HCP_RESPONSE isn't checked. >>>>> >>>>> Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use >>>>> NCI_HCP_MSG_GET_PIPE() so those could be off by one. >>>> >>>> Good point. From hci.h: >>>> >>>> /* >>>> * According to specification 102 622 chapter 4.4 Pipes, >>>> * the pipe identifier is 7 bits long. >>>> */ >>>> #define NFC_HCI_MAX_PIPES 127 >>>> >>>> And then: >>>> >>>> struct nfc_hci_dev { >>>> ... >>>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES]; >>>> ... >>>> } >>>> >>>> I think the correct fix would be to change it to: >>>> >>>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1]; >>>> >>>> What do you think? >>>> >>> >>> Just to be clear, this would fix the problem Dan described in his >>> reply and it should be implemented in a separate patch. The original >>> fix is still valid. >> >> I think you could merge the fixes into a single patch. > > Couple reasons I would prefer to keep them separate: > - I feel that descriptions for these two issues should be different > and it's easier if we don't mix them up > - This one is already merged into Android kernels, so would be easier > to introduce the second fix separately > - I would like to give credit to people who noticed the problem (in > this case those are different people) > > However if more people think we should fix both issues in the same > patch I'll happily do that. > Thanks! If it's already landed separately somewhere else, then yeah, 2 patches sounds good. No objection either way from me! -Kees -- Kees Cook Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-14 21:49 ` Kees Cook @ 2018-08-14 22:19 ` Suren Baghdasaryan 2018-08-14 22:38 ` Suren Baghdasaryan 0 siblings, 1 reply; 15+ messages in thread From: Suren Baghdasaryan @ 2018-08-14 22:19 UTC (permalink / raw) To: Kees Cook Cc: Dan Carpenter, Security Officers, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais, linux-wireless, Network Development, LKML On Tue, Aug 14, 2018 at 2:49 PM, Kees Cook <keescook@chromium.org> wrote: > On Tue, Aug 14, 2018 at 1:55 PM, Suren Baghdasaryan <surenb@google.com> wrote: >> On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook <keescook@chromium.org> wrote: >>> On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan <surenb@google.com> wrote: >>>> On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@google.com> wrote: >>>>> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: >>>>>> Thanks. This is great. I'm so glad these are finally getting fixed. >>>>>> >>>>>> Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as >>>>>> well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT >>>>>> (0x7f) so that's one element beyond the end of the array and the >>>>>> NFC_HCI_HCP_RESPONSE isn't checked. >>>>>> >>>>>> Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use >>>>>> NCI_HCP_MSG_GET_PIPE() so those could be off by one. >>>>> >>>>> Good point. From hci.h: >>>>> >>>>> /* >>>>> * According to specification 102 622 chapter 4.4 Pipes, >>>>> * the pipe identifier is 7 bits long. >>>>> */ >>>>> #define NFC_HCI_MAX_PIPES 127 >>>>> >>>>> And then: >>>>> >>>>> struct nfc_hci_dev { >>>>> ... >>>>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES]; >>>>> ... >>>>> } >>>>> >>>>> I think the correct fix would be to change it to: >>>>> >>>>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1]; >>>>> >>>>> What do you think? >>>>> Actually, after looking more closely, NFC_HCI_MAX_PIPES is always used as the number of supported pipes and not as the max pipe ID, so the right fix would be: -#define NFC_HCI_MAX_PIPES 127 +#define NFC_HCI_MAX_PIPES 128 I would prefer to rename it into NFC_HCI_PIPE_COUNT but don't want to introduce unnecessary churn for one-line change, so will keep the name. Will post a separate fix for this shortly. >>>> >>>> Just to be clear, this would fix the problem Dan described in his >>>> reply and it should be implemented in a separate patch. The original >>>> fix is still valid. >>> >>> I think you could merge the fixes into a single patch. >> >> Couple reasons I would prefer to keep them separate: >> - I feel that descriptions for these two issues should be different >> and it's easier if we don't mix them up >> - This one is already merged into Android kernels, so would be easier >> to introduce the second fix separately >> - I would like to give credit to people who noticed the problem (in >> this case those are different people) >> >> However if more people think we should fix both issues in the same >> patch I'll happily do that. >> Thanks! > > If it's already landed separately somewhere else, then yeah, 2 patches > sounds good. No objection either way from me! > > -Kees > > -- > Kees Cook > Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-14 22:19 ` Suren Baghdasaryan @ 2018-08-14 22:38 ` Suren Baghdasaryan 2018-08-15 8:29 ` Dan Carpenter 0 siblings, 1 reply; 15+ messages in thread From: Suren Baghdasaryan @ 2018-08-14 22:38 UTC (permalink / raw) To: Kees Cook Cc: Dan Carpenter, Security Officers, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais, linux-wireless, Network Development, LKML The separate fix for the size of pipes[] array is posted here: https://lkml.org/lkml/2018/8/14/1034 Thanks! On Tue, Aug 14, 2018 at 3:19 PM, Suren Baghdasaryan <surenb@google.com> wrote: > On Tue, Aug 14, 2018 at 2:49 PM, Kees Cook <keescook@chromium.org> wrote: >> On Tue, Aug 14, 2018 at 1:55 PM, Suren Baghdasaryan <surenb@google.com> wrote: >>> On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook <keescook@chromium.org> wrote: >>>> On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan <surenb@google.com> wrote: >>>>> On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@google.com> wrote: >>>>>> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: >>>>>>> Thanks. This is great. I'm so glad these are finally getting fixed. >>>>>>> >>>>>>> Do we need to fix nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as >>>>>>> well? In nfc_hci_recv_from_llc() we allow pipe to be NFC_HCI_FRAGMENT >>>>>>> (0x7f) so that's one element beyond the end of the array and the >>>>>>> NFC_HCI_HCP_RESPONSE isn't checked. >>>>>>> >>>>>>> Also nci_hci_msg_rx_work() and nci_hci_data_received_cb() use >>>>>>> NCI_HCP_MSG_GET_PIPE() so those could be off by one. >>>>>> >>>>>> Good point. From hci.h: >>>>>> >>>>>> /* >>>>>> * According to specification 102 622 chapter 4.4 Pipes, >>>>>> * the pipe identifier is 7 bits long. >>>>>> */ >>>>>> #define NFC_HCI_MAX_PIPES 127 >>>>>> >>>>>> And then: >>>>>> >>>>>> struct nfc_hci_dev { >>>>>> ... >>>>>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES]; >>>>>> ... >>>>>> } >>>>>> >>>>>> I think the correct fix would be to change it to: >>>>>> >>>>>> struct nfc_hci_pipe pipes[NFC_HCI_MAX_PIPES + 1]; >>>>>> >>>>>> What do you think? >>>>>> > > Actually, after looking more closely, NFC_HCI_MAX_PIPES is always used > as the number of supported pipes and not as the max pipe ID, so the > right fix would be: > > -#define NFC_HCI_MAX_PIPES 127 > +#define NFC_HCI_MAX_PIPES 128 > > I would prefer to rename it into NFC_HCI_PIPE_COUNT but don't want to > introduce unnecessary churn for one-line change, so will keep the > name. Will post a separate fix for this shortly. > >>>>> >>>>> Just to be clear, this would fix the problem Dan described in his >>>>> reply and it should be implemented in a separate patch. The original >>>>> fix is still valid. >>>> >>>> I think you could merge the fixes into a single patch. >>> >>> Couple reasons I would prefer to keep them separate: >>> - I feel that descriptions for these two issues should be different >>> and it's easier if we don't mix them up >>> - This one is already merged into Android kernels, so would be easier >>> to introduce the second fix separately >>> - I would like to give credit to people who noticed the problem (in >>> this case those are different people) >>> >>> However if more people think we should fix both issues in the same >>> patch I'll happily do that. >>> Thanks! >> >> If it's already landed separately somewhere else, then yeah, 2 patches >> sounds good. No objection either way from me! >> >> -Kees >> >> -- >> Kees Cook >> Pixel Security ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-14 22:38 ` Suren Baghdasaryan @ 2018-08-15 8:29 ` Dan Carpenter 2018-08-15 16:40 ` Suren Baghdasaryan 0 siblings, 1 reply; 15+ messages in thread From: Dan Carpenter @ 2018-08-15 8:29 UTC (permalink / raw) To: Suren Baghdasaryan Cc: Kees Cook, Security Officers, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais, linux-wireless, Network Development, LKML On Tue, Aug 14, 2018 at 03:38:14PM -0700, Suren Baghdasaryan wrote: > The separate fix for the size of pipes[] array is posted here: > https://lkml.org/lkml/2018/8/14/1034 > Thanks! > That's great! Let's add some bounds checking to nfc_hci_msg_rx_work() and nfc_hci_recv_from_llc() as well and then we can close the chapter on these bugs. regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-15 8:29 ` Dan Carpenter @ 2018-08-15 16:40 ` Suren Baghdasaryan 2018-08-17 12:06 ` Dan Carpenter 0 siblings, 1 reply; 15+ messages in thread From: Suren Baghdasaryan @ 2018-08-15 16:40 UTC (permalink / raw) To: Dan Carpenter Cc: Kees Cook, Security Officers, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais, linux-wireless, Network Development, LKML On Wed, Aug 15, 2018 at 1:29 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Tue, Aug 14, 2018 at 03:38:14PM -0700, Suren Baghdasaryan wrote: >> The separate fix for the size of pipes[] array is posted here: >> https://lkml.org/lkml/2018/8/14/1034 >> Thanks! >> > > That's great! Let's add some bounds checking to nfc_hci_msg_rx_work() > and nfc_hci_recv_from_llc() as well and then we can close the chapter on > these bugs. Dan, I don't think we need additional checks there. Here are the relevant parts of the code in nfc_hci_recv_from_llc(): static void nfc_hci_recv_from_llc(struct nfc_hci_dev *hdev, struct sk_buff *skb) { ... packet = (struct hcp_packet *)skb->data; ... /* it's the last fragment. Does it need re-aggregation? */ if (skb_queue_len(&hdev->rx_hcp_frags)) { pipe = packet->header & NFC_HCI_FRAGMENT; ... hcp_skb = nfc_alloc_recv_skb(NFC_HCI_HCP_PACKET_HEADER_LEN + msg_len, GFP_KERNEL); ... *skb_put(hcp_skb, NFC_HCI_HCP_PACKET_HEADER_LEN) = pipe; ... } else { packet->header &= NFC_HCI_FRAGMENT; hcp_skb = skb; } AFAIU in both cases the pipe field in hcp_skb can't exceed 127 after we applied NFC_HCI_FRAGMENT(0x7f) mask. > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-15 16:40 ` Suren Baghdasaryan @ 2018-08-17 12:06 ` Dan Carpenter 2018-08-17 14:47 ` Dan Carpenter 0 siblings, 1 reply; 15+ messages in thread From: Dan Carpenter @ 2018-08-17 12:06 UTC (permalink / raw) To: Suren Baghdasaryan Cc: Kees Cook, Security Officers, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais, linux-wireless, Network Development, LKML On Wed, Aug 15, 2018 at 09:40:13AM -0700, Suren Baghdasaryan wrote: > On Wed, Aug 15, 2018 at 1:29 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Tue, Aug 14, 2018 at 03:38:14PM -0700, Suren Baghdasaryan wrote: > >> The separate fix for the size of pipes[] array is posted here: > >> https://lkml.org/lkml/2018/8/14/1034 > >> Thanks! > >> > > > > That's great! Let's add some bounds checking to nfc_hci_msg_rx_work() > > and nfc_hci_recv_from_llc() as well and then we can close the chapter on > > these bugs. > > Dan, I don't think we need additional checks there. Here are the > relevant parts of the code in nfc_hci_recv_from_llc(): > Sorry, I meant after that at the end of the function: net/nfc/hci/core.c 902 /* if this is a response, dispatch immediately to 903 * unblock waiting cmd context. Otherwise, enqueue to dispatch 904 * in separate context where handler can also execute command. 905 */ 906 packet = (struct hcp_packet *)hcp_skb->data; 907 type = HCP_MSG_GET_TYPE(packet->message.header); 908 if (type == NFC_HCI_HCP_RESPONSE) { 909 pipe = packet->header; ^^^^^^^^^^^^^^^^^^^^^ Pipe can go up to 255. 910 instruction = HCP_MSG_GET_CMD(packet->message.header); 911 skb_pull(hcp_skb, NFC_HCI_HCP_PACKET_HEADER_LEN + 912 NFC_HCI_HCP_MESSAGE_HEADER_LEN); 913 nfc_hci_hcp_message_rx(hdev, pipe, type, instruction, hcp_skb); ^^^^ Then inside the nfc_hci_hcp_message_rx() function we call nfc_hci_cmd_received() and nfc_hci_event_received() which use it as an array index. 914 } else { 915 skb_queue_tail(&hdev->msg_rx_queue, hcp_skb); 916 schedule_work(&hdev->msg_rx_work); 917 } 918 } It's the same thing when nfc_hci_hcp_message_rx() is called from nfc_hci_msg_rx_work(): 138 static void nfc_hci_msg_rx_work(struct work_struct *work) 139 { 140 struct nfc_hci_dev *hdev = container_of(work, struct nfc_hci_dev, 141 msg_rx_work); 142 struct sk_buff *skb; 143 struct hcp_message *message; 144 u8 pipe; 145 u8 type; 146 u8 instruction; 147 148 while ((skb = skb_dequeue(&hdev->msg_rx_queue)) != NULL) { 149 pipe = skb->data[0]; ^^^^^^^^^^^^^^^^^^^ 150 skb_pull(skb, NFC_HCI_HCP_PACKET_HEADER_LEN); 151 message = (struct hcp_message *)skb->data; 152 type = HCP_MSG_GET_TYPE(message->header); 153 instruction = HCP_MSG_GET_CMD(message->header); 154 skb_pull(skb, NFC_HCI_HCP_MESSAGE_HEADER_LEN); 155 156 nfc_hci_hcp_message_rx(hdev, pipe, type, instruction, skb); ^^^^ 157 } 158 } regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands 2018-08-17 12:06 ` Dan Carpenter @ 2018-08-17 14:47 ` Dan Carpenter 0 siblings, 0 replies; 15+ messages in thread From: Dan Carpenter @ 2018-08-17 14:47 UTC (permalink / raw) To: Suren Baghdasaryan Cc: Kees Cook, Security Officers, Kevin Deus, Samuel Ortiz, David S. Miller, Allen Pais, linux-wireless, Network Development, LKML Never mind. This code is a bit subtle for static analysis so those warnings are false positives. Thanks for taking a look at this, Suren. regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-08-17 14:48 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-13 22:39 [PATCH 1/1] NFC: Fix possible memory corruption when handling SHDLC I-Frame commands Suren Baghdasaryan 2018-08-14 0:28 ` Kees Cook 2018-08-14 9:21 ` Greg KH 2018-08-14 9:54 ` Dan Carpenter 2018-08-14 16:57 ` Suren Baghdasaryan 2018-08-14 20:26 ` Suren Baghdasaryan 2018-08-14 20:33 ` Kees Cook 2018-08-14 20:55 ` Suren Baghdasaryan 2018-08-14 21:49 ` Kees Cook 2018-08-14 22:19 ` Suren Baghdasaryan 2018-08-14 22:38 ` Suren Baghdasaryan 2018-08-15 8:29 ` Dan Carpenter 2018-08-15 16:40 ` Suren Baghdasaryan 2018-08-17 12:06 ` Dan Carpenter 2018-08-17 14:47 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).