[v3,09/23] ata: libahci_platform: Sanity check the DT child nodes number

Having greater than (AHCI_MAX_PORTS = 32) ports detected isn't that
critical from the further AHCI-platform initialization point of view since
exceeding the ports upper limit will cause allocating more resources than
will be used afterwards. But detecting too many child DT-nodes doesn't
seem right since it's very unlikely to have it on an ordinary platform. In
accordance with the AHCI specification there can't be more than 32 ports
implemented at least due to having the CAP.NP field of 4 bits wide and the
PI register of dword size. Thus if such situation is found the DTB must
have been corrupted and the data read from it shouldn't be reliable. Let's
consider that as an erroneous situation and halt further resources
allocation.

Note it's logically more correct to have the nports set only after the
initialization value is checked for being sane. So while at it let's make
sure nports is assigned with a correct value.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Changelog v2:
- Drop the else word from the child_nodes value checking if-else-if
  statement (@Damien) and convert the after-else part into the ternary
  operator-based statement.
---
 drivers/ata/libahci_platform.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

On 5/12/22 01:17, Serge Semin wrote:
> Having greater than (AHCI_MAX_PORTS = 32) ports detected isn't that
> critical from the further AHCI-platform initialization point of view since
> exceeding the ports upper limit will cause allocating more resources than
> will be used afterwards. But detecting too many child DT-nodes doesn't
> seem right since it's very unlikely to have it on an ordinary platform. In
> accordance with the AHCI specification there can't be more than 32 ports
> implemented at least due to having the CAP.NP field of 4 bits wide and the
> PI register of dword size. Thus if such situation is found the DTB must
> have been corrupted and the data read from it shouldn't be reliable. Let's
> consider that as an erroneous situation and halt further resources
> allocation.
> 
> Note it's logically more correct to have the nports set only after the
> initialization value is checked for being sane. So while at it let's make
> sure nports is assigned with a correct value.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> ---
> 
> Changelog v2:
> - Drop the else word from the child_nodes value checking if-else-if
>    statement (@Damien) and convert the after-else part into the ternary
>    operator-based statement.
> ---
>   drivers/ata/libahci_platform.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 7ff6626fd569..4e54e19f07b2 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -480,15 +480,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>   		}
>   	}
>   
> -	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
> +	/*
> +	 * Too many sub-nodes most likely means having something wrong with
> +	 * the firmware.
> +	 */
> +	child_nodes = of_get_child_count(dev->of_node);
> +	if (child_nodes > AHCI_MAX_PORTS) {
> +		rc = -EINVAL;
> +		goto err_out;
> +	}
>   
>   	/*
>   	 * If no sub-node was found, we still need to set nports to
>   	 * one in order to be able to use the
>   	 * ahci_platform_[en|dis]able_[phys|regulators] functions.
>   	 */
> -	if (!child_nodes)
> -		hpriv->nports = 1;
> +	hpriv->nports = child_nodes ?: 1;
>   
>   	hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
>   	if (!hpriv->phys) {

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes

On 5/12/22 2:17 AM, Serge Semin wrote:

> Having greater than (AHCI_MAX_PORTS = 32) ports detected isn't that

   Having greater than AHCI_MAX_PORTS (32) ports detected?

> critical from the further AHCI-platform initialization point of view since
> exceeding the ports upper limit will cause allocating more resources than
> will be used afterwards. But detecting too many child DT-nodes doesn't
> seem right since it's very unlikely to have it on an ordinary platform. In
> accordance with the AHCI specification there can't be more than 32 ports
> implemented at least due to having the CAP.NP field of 4 bits wide and the

   It's 5 bits wide, actually...

> PI register of dword size. Thus if such situation is found the DTB must
> have been corrupted and the data read from it shouldn't be reliable. Let's
> consider that as an erroneous situation and halt further resources
> allocation.
> 
> Note it's logically more correct to have the nports set only after the
> initialization value is checked for being sane. So while at it let's make
> sure nports is assigned with a correct value.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

[...]

MBR, Sergey

On Thu, May 12, 2022 at 11:24:22AM +0300, Sergei Shtylyov wrote:
> On 5/12/22 2:17 AM, Serge Semin wrote:
> 
> > Having greater than (AHCI_MAX_PORTS = 32) ports detected isn't that
> 

>    Having greater than AHCI_MAX_PORTS (32) ports detected?

Ok.

> 
> > critical from the further AHCI-platform initialization point of view since
> > exceeding the ports upper limit will cause allocating more resources than
> > will be used afterwards. But detecting too many child DT-nodes doesn't
> > seem right since it's very unlikely to have it on an ordinary platform. In
> > accordance with the AHCI specification there can't be more than 32 ports
> > implemented at least due to having the CAP.NP field of 4 bits wide and the
> 

>    It's 5 bits wide, actually...

Right =)

The denoted comments will be taken into account in v4. Thanks.

-Sergey

> 
> > PI register of dword size. Thus if such situation is found the DTB must
> > have been corrupted and the data read from it shouldn't be reliable. Let's
> > consider that as an erroneous situation and halt further resources
> > allocation.
> > 
> > Note it's logically more correct to have the nports set only after the
> > initialization value is checked for being sane. So while at it let's make
> > sure nports is assigned with a correct value.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> [...]
> 
> MBR, Sergey