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@xxxxxxxxxx> wrote: > On Tue, Aug 14, 2018 at 2:49 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote: >> On Tue, Aug 14, 2018 at 1:55 PM, Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote: >>> On Tue, Aug 14, 2018 at 1:33 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote: >>>> On Tue, Aug 14, 2018 at 1:26 PM, Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote: >>>>> On Tue, Aug 14, 2018 at 9:57 AM, Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote: >>>>>> On Tue, Aug 14, 2018 at 2:54 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
