[bug report] ACPI: Add new IORT functions to support MSI domain handling

[bug report] ACPI: Add new IORT functions to support MSI domain handling

[Dan Carpenter]

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

Re: [bug report] ACPI: Add new IORT functions to support MSI domain handling

[Robin Murphy]

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
> 

Re: [bug report] ACPI: Add new IORT functions to support MSI domain handling

[Hanjun Guo]

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

Re: [bug report] ACPI: Add new IORT functions to support MSI domain handling

[Tomasz Nowicki]

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

Re: [bug report] ACPI: Add new IORT functions to support MSI domain handling

[Lorenzo Pieralisi]

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