* [bug report] ACPI: Add new IORT functions to support MSI domain handling @ 2019-06-13 6:54 Dan Carpenter 2019-06-13 9:30 ` Robin Murphy 0 siblings, 1 reply; 5+ messages in thread From: Dan Carpenter @ 2019-06-13 6:54 UTC (permalink / raw) To: tn; +Cc: linux-acpi, linux-arm-kernel Hello Tomasz Nowicki, The patch 4bf2efd26d76: "ACPI: Add new IORT functions to support MSI domain handling" from Sep 12, 2016, leads to the following static checker warning: drivers/acpi/arm64/iort.c:628 iort_dev_find_its_id() warn: array off by one? 'its->identifiers[idx]' drivers/acpi/arm64/iort.c 589 /** 590 * iort_dev_find_its_id() - Find the ITS identifier for a device 591 * @dev: The device. 592 * @req_id: Device's requester ID 593 * @idx: Index of the ITS identifier list. 594 * @its_id: ITS identifier. 595 * 596 * Returns: 0 on success, appropriate error value otherwise 597 */ 598 static int iort_dev_find_its_id(struct device *dev, u32 req_id, 599 unsigned int idx, int *its_id) 600 { 601 struct acpi_iort_its_group *its; 602 struct acpi_iort_node *node; 603 604 node = iort_find_dev_node(dev); 605 if (!node) 606 return -ENXIO; 607 608 node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE); 609 if (!node) 610 return -ENXIO; 611 612 /* Move to ITS specific data */ 613 its = (struct acpi_iort_its_group *)node->node_data; 614 if (idx > its->its_count) { ^^^^^^^^^^^^^^^^^^^^ I wasn't able to find any information about how its->its_count is set but it looks to me that is off by one. 615 dev_err(dev, "requested ITS ID index [%d] is greater than available [%d]\n", 616 idx, its->its_count); 617 return -ENXIO; 618 } 619 620 *its_id = its->identifiers[idx]; 621 return 0; 622 } regards, dan carpenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] ACPI: Add new IORT functions to support MSI domain handling 2019-06-13 6:54 [bug report] ACPI: Add new IORT functions to support MSI domain handling Dan Carpenter @ 2019-06-13 9:30 ` Robin Murphy 2019-06-14 1:03 ` Hanjun Guo 0 siblings, 1 reply; 5+ messages in thread From: Robin Murphy @ 2019-06-13 9:30 UTC (permalink / raw) To: Dan Carpenter, tn; +Cc: linux-acpi, linux-arm-kernel On 13/06/2019 07:54, Dan Carpenter wrote: > Hello Tomasz Nowicki, > > The patch 4bf2efd26d76: "ACPI: Add new IORT functions to support MSI > domain handling" from Sep 12, 2016, leads to the following static > checker warning: > > drivers/acpi/arm64/iort.c:628 iort_dev_find_its_id() > warn: array off by one? 'its->identifiers[idx]' > > drivers/acpi/arm64/iort.c > 589 /** > 590 * iort_dev_find_its_id() - Find the ITS identifier for a device > 591 * @dev: The device. > 592 * @req_id: Device's requester ID > 593 * @idx: Index of the ITS identifier list. > 594 * @its_id: ITS identifier. > 595 * > 596 * Returns: 0 on success, appropriate error value otherwise > 597 */ > 598 static int iort_dev_find_its_id(struct device *dev, u32 req_id, > 599 unsigned int idx, int *its_id) > 600 { > 601 struct acpi_iort_its_group *its; > 602 struct acpi_iort_node *node; > 603 > 604 node = iort_find_dev_node(dev); > 605 if (!node) > 606 return -ENXIO; > 607 > 608 node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE); > 609 if (!node) > 610 return -ENXIO; > 611 > 612 /* Move to ITS specific data */ > 613 its = (struct acpi_iort_its_group *)node->node_data; > 614 if (idx > its->its_count) { > ^^^^^^^^^^^^^^^^^^^^ > I wasn't able to find any information about how its->its_count is set > but it looks to me that is off by one. its->count is read directly from the firmware table. Currently it seems this condition can never be hit anyway, since this is only ever called with idx == 0. TBH I can't really see how the code could evolve such that this check should ever be necessary (i.e. it makes no sense for callers to pull idx values out if thin air, so they'd presumably end up being derived from its->count in the first place), but if we are going to have it then I agree it should be ">=". Robin. > > 615 dev_err(dev, "requested ITS ID index [%d] is greater than available [%d]\n", > 616 idx, its->its_count); > 617 return -ENXIO; > 618 } > 619 > 620 *its_id = its->identifiers[idx]; > 621 return 0; > 622 } > > regards, > dan carpenter > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] ACPI: Add new IORT functions to support MSI domain handling 2019-06-13 9:30 ` Robin Murphy @ 2019-06-14 1:03 ` Hanjun Guo 2019-06-14 10:09 ` Tomasz Nowicki 0 siblings, 1 reply; 5+ messages in thread From: Hanjun Guo @ 2019-06-14 1:03 UTC (permalink / raw) To: Robin Murphy, Dan Carpenter, tn Cc: linux-acpi, linux-arm-kernel, Lorenzo Pieralisi On 2019/6/13 17:30, Robin Murphy wrote: > On 13/06/2019 07:54, Dan Carpenter wrote: >> Hello Tomasz Nowicki, >> >> The patch 4bf2efd26d76: "ACPI: Add new IORT functions to support MSI >> domain handling" from Sep 12, 2016, leads to the following static >> checker warning: >> >> drivers/acpi/arm64/iort.c:628 iort_dev_find_its_id() >> warn: array off by one? 'its->identifiers[idx]' >> >> drivers/acpi/arm64/iort.c >> 589 /** >> 590 * iort_dev_find_its_id() - Find the ITS identifier for a device >> 591 * @dev: The device. >> 592 * @req_id: Device's requester ID >> 593 * @idx: Index of the ITS identifier list. >> 594 * @its_id: ITS identifier. >> 595 * >> 596 * Returns: 0 on success, appropriate error value otherwise >> 597 */ >> 598 static int iort_dev_find_its_id(struct device *dev, u32 req_id, >> 599 unsigned int idx, int *its_id) >> 600 { >> 601 struct acpi_iort_its_group *its; >> 602 struct acpi_iort_node *node; >> 603 >> 604 node = iort_find_dev_node(dev); >> 605 if (!node) >> 606 return -ENXIO; >> 607 >> 608 node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE); >> 609 if (!node) >> 610 return -ENXIO; >> 611 >> 612 /* Move to ITS specific data */ >> 613 its = (struct acpi_iort_its_group *)node->node_data; >> 614 if (idx > its->its_count) { >> ^^^^^^^^^^^^^^^^^^^^ >> I wasn't able to find any information about how its->its_count is set >> but it looks to me that is off by one. > > its->count is read directly from the firmware table. Currently it seems this condition can never be hit anyway, since this is only ever called with idx == 0. TBH I can't really see how the code could evolve such that this check should ever be necessary (i.e. it makes no sense for callers to pull idx values out if thin air, so they'd presumably end up being derived from its->count in the first place), but if we are going to have it then I agree it should be ">=". For now seems we only got systems which map a device to a single ITS, but in the IORT spec, it assumes that maybe there is a ITS group for mapping, so I think we can just use ">=" as you suggested to align with the spec. Thanks Hanjun ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] ACPI: Add new IORT functions to support MSI domain handling 2019-06-14 1:03 ` Hanjun Guo @ 2019-06-14 10:09 ` Tomasz Nowicki 2019-06-14 14:28 ` Lorenzo Pieralisi 0 siblings, 1 reply; 5+ messages in thread From: Tomasz Nowicki @ 2019-06-14 10:09 UTC (permalink / raw) To: Hanjun Guo, Robin Murphy, Dan Carpenter Cc: linux-acpi, linux-arm-kernel, Lorenzo Pieralisi On 14.06.2019 03:03, Hanjun Guo wrote: > On 2019/6/13 17:30, Robin Murphy wrote: >> On 13/06/2019 07:54, Dan Carpenter wrote: >>> Hello Tomasz Nowicki, >>> >>> The patch 4bf2efd26d76: "ACPI: Add new IORT functions to support MSI >>> domain handling" from Sep 12, 2016, leads to the following static >>> checker warning: >>> >>> drivers/acpi/arm64/iort.c:628 iort_dev_find_its_id() >>> warn: array off by one? 'its->identifiers[idx]' >>> >>> drivers/acpi/arm64/iort.c >>> 589 /** >>> 590 * iort_dev_find_its_id() - Find the ITS identifier for a device >>> 591 * @dev: The device. >>> 592 * @req_id: Device's requester ID >>> 593 * @idx: Index of the ITS identifier list. >>> 594 * @its_id: ITS identifier. >>> 595 * >>> 596 * Returns: 0 on success, appropriate error value otherwise >>> 597 */ >>> 598 static int iort_dev_find_its_id(struct device *dev, u32 req_id, >>> 599 unsigned int idx, int *its_id) >>> 600 { >>> 601 struct acpi_iort_its_group *its; >>> 602 struct acpi_iort_node *node; >>> 603 >>> 604 node = iort_find_dev_node(dev); >>> 605 if (!node) >>> 606 return -ENXIO; >>> 607 >>> 608 node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE); >>> 609 if (!node) >>> 610 return -ENXIO; >>> 611 >>> 612 /* Move to ITS specific data */ >>> 613 its = (struct acpi_iort_its_group *)node->node_data; >>> 614 if (idx > its->its_count) { >>> ^^^^^^^^^^^^^^^^^^^^ >>> I wasn't able to find any information about how its->its_count is set >>> but it looks to me that is off by one. >> >> its->count is read directly from the firmware table. Currently it seems this condition can never be hit anyway, since this is only ever called with idx == 0. TBH I can't really see how the code could evolve such that this check should ever be necessary (i.e. it makes no sense for callers to pull idx values out if thin air, so they'd presumably end up being derived from its->count in the first place), but if we are going to have it then I agree it should be ">=". > > For now seems we only got systems which map a device to a single > ITS, but in the IORT spec, it assumes that maybe there is a ITS group > for mapping, so I think we can just use ">=" as you suggested to > align with the spec. > Yes, should be ">=" and the error massage should be fixed as well: /* Move to ITS specific data */ its = (struct acpi_iort_its_group *)node->node_data; if (idx >= its->its_count) { dev_err(dev, "requested ITS ID index [%d] exceeds max permitted [%d] index\n", idx, its->its_count - 1); return -ENXIO; } Thanks, Tomasz ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] ACPI: Add new IORT functions to support MSI domain handling 2019-06-14 10:09 ` Tomasz Nowicki @ 2019-06-14 14:28 ` Lorenzo Pieralisi 0 siblings, 0 replies; 5+ messages in thread From: Lorenzo Pieralisi @ 2019-06-14 14:28 UTC (permalink / raw) To: Tomasz Nowicki Cc: Hanjun Guo, Robin Murphy, Dan Carpenter, linux-acpi, linux-arm-kernel On Fri, Jun 14, 2019 at 12:09:17PM +0200, Tomasz Nowicki wrote: > On 14.06.2019 03:03, Hanjun Guo wrote: > > On 2019/6/13 17:30, Robin Murphy wrote: > > > On 13/06/2019 07:54, Dan Carpenter wrote: > > > > Hello Tomasz Nowicki, > > > > > > > > The patch 4bf2efd26d76: "ACPI: Add new IORT functions to support MSI > > > > domain handling" from Sep 12, 2016, leads to the following static > > > > checker warning: > > > > > > > > drivers/acpi/arm64/iort.c:628 iort_dev_find_its_id() > > > > warn: array off by one? 'its->identifiers[idx]' > > > > > > > > drivers/acpi/arm64/iort.c > > > > 589 /** > > > > 590 * iort_dev_find_its_id() - Find the ITS identifier for a device > > > > 591 * @dev: The device. > > > > 592 * @req_id: Device's requester ID > > > > 593 * @idx: Index of the ITS identifier list. > > > > 594 * @its_id: ITS identifier. > > > > 595 * > > > > 596 * Returns: 0 on success, appropriate error value otherwise > > > > 597 */ > > > > 598 static int iort_dev_find_its_id(struct device *dev, u32 req_id, > > > > 599 unsigned int idx, int *its_id) > > > > 600 { > > > > 601 struct acpi_iort_its_group *its; > > > > 602 struct acpi_iort_node *node; > > > > 603 > > > > 604 node = iort_find_dev_node(dev); > > > > 605 if (!node) > > > > 606 return -ENXIO; > > > > 607 > > > > 608 node = iort_node_map_id(node, req_id, NULL, IORT_MSI_TYPE); > > > > 609 if (!node) > > > > 610 return -ENXIO; > > > > 611 > > > > 612 /* Move to ITS specific data */ > > > > 613 its = (struct acpi_iort_its_group *)node->node_data; > > > > 614 if (idx > its->its_count) { > > > > ^^^^^^^^^^^^^^^^^^^^ > > > > I wasn't able to find any information about how its->its_count is set > > > > but it looks to me that is off by one. > > > > > > its->count is read directly from the firmware table. Currently it seems this condition can never be hit anyway, since this is only ever called with idx == 0. TBH I can't really see how the code could evolve such that this check should ever be necessary (i.e. it makes no sense for callers to pull idx values out if thin air, so they'd presumably end up being derived from its->count in the first place), but if we are going to have it then I agree it should be ">=". > > > > For now seems we only got systems which map a device to a single > > ITS, but in the IORT spec, it assumes that maybe there is a ITS group > > for mapping, so I think we can just use ">=" as you suggested to > > align with the spec. > > > > Yes, should be ">=" and the error massage should be fixed as well: > > /* Move to ITS specific data */ > its = (struct acpi_iort_its_group *)node->node_data; > if (idx >= its->its_count) { > dev_err(dev, "requested ITS ID index [%d] exceeds max permitted [%d] > index\n", > idx, its->its_count - 1); > return -ENXIO; > } I will pick it up, reformat and resend it. Thanks, Lorenzo ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-14 14:28 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-13 6:54 [bug report] ACPI: Add new IORT functions to support MSI domain handling Dan Carpenter 2019-06-13 9:30 ` Robin Murphy 2019-06-14 1:03 ` Hanjun Guo 2019-06-14 10:09 ` Tomasz Nowicki 2019-06-14 14:28 ` Lorenzo Pieralisi
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).