Crash with "ata: ahci_platform: convert kcalloc to devm_kcalloc"

Hello,

the recently applied "ata: ahci_platform: convert kcalloc to 
devm_kcalloc" seems to be causing boot failures on Tegra124 Jetson TK1. 
The patch is as follows:

  static int ahci_platform_get_phy(struct ahci_host_priv *hpriv, u32 port,
@@ -408,7 +406,7 @@ struct ahci_host_priv 
*ahci_platform_get_resources(struct platform_device *pdev)
                 rc = -ENOMEM;
                 goto err_out;
         }
-       hpriv->target_pwrs = kcalloc(hpriv->nports, 
sizeof(*hpriv->target_pwrs), GFP_KERNEL);
+       hpriv->target_pwrs = devm_kcalloc(dev, hpriv->nports, 
sizeof(*hpriv->target_pwrs), GFP_KERNEL);
         if (!hpriv->target_pwrs) {
                 rc = -ENOMEM;
                 goto err_out;

However, this is not valid, as it will cause hpriv->target_pwrs to be 
freed before ahci_platform_put_resources is called. With the older code, 
the free happened intentionally only after the regulator_put calls were 
done.

Thanks,
Mikko
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jul 16, 2018 at 04:11:44PM +0300, Mikko Perttunen wrote:
> Hello,
> 
> the recently applied "ata: ahci_platform: convert kcalloc to 
> devm_kcalloc" seems to be causing boot failures on Tegra124 Jetson TK1. 
> The patch is as follows:
> 
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index be9f54423a9b..fe8939e161ea 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -271,8 +271,6 @@ static void ahci_platform_put_resources(struct 
> device *dev, void *res)
>          for (c = 0; c < hpriv->nports; c++)
>                  if (hpriv->target_pwrs && hpriv->target_pwrs[c])
>                          regulator_put(hpriv->target_pwrs[c]);
> -
> -       kfree(hpriv->target_pwrs);
>   }
> 
>   static int ahci_platform_get_phy(struct ahci_host_priv *hpriv, u32 port,
> @@ -408,7 +406,7 @@ struct ahci_host_priv 
> *ahci_platform_get_resources(struct platform_device *pdev)
>                  rc = -ENOMEM;
>                  goto err_out;
>          }
> -       hpriv->target_pwrs = kcalloc(hpriv->nports, 
> sizeof(*hpriv->target_pwrs), GFP_KERNEL);
> +       hpriv->target_pwrs = devm_kcalloc(dev, hpriv->nports, 
> sizeof(*hpriv->target_pwrs), GFP_KERNEL);
>          if (!hpriv->target_pwrs) {
>                  rc = -ENOMEM;
>                  goto err_out;
> 
> However, this is not valid, as it will cause hpriv->target_pwrs to be 
> freed before ahci_platform_put_resources is called. With the older code, 
> the free happened intentionally only after the regulator_put calls were 
> done.
> 

Hello

I am surprised, since I have tested all my AHCI patch on a Tegra124 Jetson TK1.
Could you print the boot crash ?

Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 07/16/2018 05:55 PM, LABBE Corentin wrote:
> On Mon, Jul 16, 2018 at 04:11:44PM +0300, Mikko Perttunen wrote:
>> Hello,
>>
>> the recently applied "ata: ahci_platform: convert kcalloc to
>> devm_kcalloc" seems to be causing boot failures on Tegra124 Jetson TK1.
>> The patch is as follows:
>>
>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
>> index be9f54423a9b..fe8939e161ea 100644
>> --- a/drivers/ata/libahci_platform.c
>> +++ b/drivers/ata/libahci_platform.c
>> @@ -271,8 +271,6 @@ static void ahci_platform_put_resources(struct
>> device *dev, void *res)
>>           for (c = 0; c < hpriv->nports; c++)
>>                   if (hpriv->target_pwrs && hpriv->target_pwrs[c])
>>                           regulator_put(hpriv->target_pwrs[c]);
>> -
>> -       kfree(hpriv->target_pwrs);
>>    }
>>
>>    static int ahci_platform_get_phy(struct ahci_host_priv *hpriv, u32 port,
>> @@ -408,7 +406,7 @@ struct ahci_host_priv
>> *ahci_platform_get_resources(struct platform_device *pdev)
>>                   rc = -ENOMEM;
>>                   goto err_out;
>>           }
>> -       hpriv->target_pwrs = kcalloc(hpriv->nports,
>> sizeof(*hpriv->target_pwrs), GFP_KERNEL);
>> +       hpriv->target_pwrs = devm_kcalloc(dev, hpriv->nports,
>> sizeof(*hpriv->target_pwrs), GFP_KERNEL);
>>           if (!hpriv->target_pwrs) {
>>                   rc = -ENOMEM;
>>                   goto err_out;
>>
>> However, this is not valid, as it will cause hpriv->target_pwrs to be
>> freed before ahci_platform_put_resources is called. With the older code,
>> the free happened intentionally only after the regulator_put calls were
>> done.
>>
> 
> Hello
> 
> I am surprised, since I have tested all my AHCI patch on a Tegra124 Jetson TK1.
> Could you print the boot crash ?

I don't have the crash log in front of me now (can get it to you 
tomorrow), but basically it was ahci_platform_put_resources calling 
eventually _regulator_put which was dereferencing 0x6b6b6bbf, quite 
clearly an offset of 0x6b6b6b6b which is the use-after-free poison.

It actually only happens on tegra_defconfig -- I assume there's some 
different dependency situation that doesn't happen on 
multi_v7_defconfig, that causes ahci-tegra to defer probe, causing the 
error path to be triggered.

Thanks,
Mikko

> 
> Regards
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jul 16, 2018 at 06:33:52PM +0300, Mikko Perttunen wrote:
> 
> 
> On 07/16/2018 05:55 PM, LABBE Corentin wrote:
> > On Mon, Jul 16, 2018 at 04:11:44PM +0300, Mikko Perttunen wrote:
> >> Hello,
> >>
> >> the recently applied "ata: ahci_platform: convert kcalloc to
> >> devm_kcalloc" seems to be causing boot failures on Tegra124 Jetson TK1.
> >> The patch is as follows:
> >>
> >> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> >> index be9f54423a9b..fe8939e161ea 100644
> >> --- a/drivers/ata/libahci_platform.c
> >> +++ b/drivers/ata/libahci_platform.c
> >> @@ -271,8 +271,6 @@ static void ahci_platform_put_resources(struct
> >> device *dev, void *res)
> >>           for (c = 0; c < hpriv->nports; c++)
> >>                   if (hpriv->target_pwrs && hpriv->target_pwrs[c])
> >>                           regulator_put(hpriv->target_pwrs[c]);
> >> -
> >> -       kfree(hpriv->target_pwrs);
> >>    }
> >>
> >>    static int ahci_platform_get_phy(struct ahci_host_priv *hpriv, u32 port,
> >> @@ -408,7 +406,7 @@ struct ahci_host_priv
> >> *ahci_platform_get_resources(struct platform_device *pdev)
> >>                   rc = -ENOMEM;
> >>                   goto err_out;
> >>           }
> >> -       hpriv->target_pwrs = kcalloc(hpriv->nports,
> >> sizeof(*hpriv->target_pwrs), GFP_KERNEL);
> >> +       hpriv->target_pwrs = devm_kcalloc(dev, hpriv->nports,
> >> sizeof(*hpriv->target_pwrs), GFP_KERNEL);
> >>           if (!hpriv->target_pwrs) {
> >>                   rc = -ENOMEM;
> >>                   goto err_out;
> >>
> >> However, this is not valid, as it will cause hpriv->target_pwrs to be
> >> freed before ahci_platform_put_resources is called. With the older code,
> >> the free happened intentionally only after the regulator_put calls were
> >> done.
> >>
> > 
> > Hello
> > 
> > I am surprised, since I have tested all my AHCI patch on a Tegra124 Jetson TK1.
> > Could you print the boot crash ?
> 
> I don't have the crash log in front of me now (can get it to you 
> tomorrow), but basically it was ahci_platform_put_resources calling 
> eventually _regulator_put which was dereferencing 0x6b6b6bbf, quite 
> clearly an offset of 0x6b6b6b6b which is the use-after-free poison.
> 
> It actually only happens on tegra_defconfig -- I assume there's some 
> different dependency situation that doesn't happen on 
> multi_v7_defconfig, that causes ahci-tegra to defer probe, causing the 
> error path to be triggered.
> 

I have just checked on kernelci.org, and see what you said.
And yes it was the PHY defer which cause this, and explains why I didnt hit the case (I was not using defconfig).

I will send a commit that revert the change add add a warning on why it must remains a simple kcalloc.

Thanks
Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html