| Message ID | 20211013061631.2958938-1-wanghai38@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | ata: ahci_platform: fix null-ptr-deref in ahci_platform_enable_regulators() | expand |
Hi, On 10/13/21 8:16 AM, Wang Hai wrote: > I got a null-ptr-deref report: > > KASAN: null-ptr-deref in range [0x0000000000000090-0x0000000000000097] > ... > RIP: 0010:regulator_enable+0x84/0x260 > ... > Call Trace: > ahci_platform_enable_regulators+0xae/0x320 > ahci_platform_enable_resources+0x1a/0x120 > ahci_probe+0x4f/0x1b9 > platform_probe+0x10b/0x280 > ... > entry_SYSCALL_64_after_hwframe+0x44/0xae > > If devm_regulator_get() in ahci_platform_get_resources() fails, > hpriv->phy_regulator will point to NULL, when enabling or disabling it, > null-ptr-deref will occur. > > ahci_probe() > ahci_platform_get_resources() > devm_regulator_get(, "phy") // failed, let phy_regulator = NULL > ahci_platform_enable_resources() > ahci_platform_enable_regulators() > regulator_enable(hpriv->phy_regulator) // null-ptr-deref > > commit 962399bb7fbf ("ata: libahci_platform: Fix regulator_get_optional() > misuse") replaces devm_regulator_get_optional() with devm_regulator_get(), > but PHY regulator omits to delete "hpriv->phy_regulator = NULL;" like AHCI. > Delete it like AHCI regulator to fix this bug. > > Fixes: commit 962399bb7fbf ("ata: libahci_platform: Fix regulator_get_optional() misuse") > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Wang Hai <wanghai38@huawei.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/ata/libahci_platform.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index b2f552088291..0910441321f7 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -440,10 +440,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > hpriv->phy_regulator = devm_regulator_get(dev, "phy"); > if (IS_ERR(hpriv->phy_regulator)) { > rc = PTR_ERR(hpriv->phy_regulator); > - if (rc == -EPROBE_DEFER) > - goto err_out; > - rc = 0; > - hpriv->phy_regulator = NULL; > + goto err_out; > } > > if (flags & AHCI_PLATFORM_GET_RESETS) { >
On 10/13/21 15:16, Wang Hai wrote: > I got a null-ptr-deref report: > > KASAN: null-ptr-deref in range [0x0000000000000090-0x0000000000000097] > ... > RIP: 0010:regulator_enable+0x84/0x260 > ... > Call Trace: > ahci_platform_enable_regulators+0xae/0x320 > ahci_platform_enable_resources+0x1a/0x120 > ahci_probe+0x4f/0x1b9 > platform_probe+0x10b/0x280 > ... > entry_SYSCALL_64_after_hwframe+0x44/0xae > > If devm_regulator_get() in ahci_platform_get_resources() fails, > hpriv->phy_regulator will point to NULL, when enabling or disabling it, > null-ptr-deref will occur. > > ahci_probe() > ahci_platform_get_resources() > devm_regulator_get(, "phy") // failed, let phy_regulator = NULL > ahci_platform_enable_resources() > ahci_platform_enable_regulators() > regulator_enable(hpriv->phy_regulator) // null-ptr-deref > > commit 962399bb7fbf ("ata: libahci_platform: Fix regulator_get_optional() > misuse") replaces devm_regulator_get_optional() with devm_regulator_get(), > but PHY regulator omits to delete "hpriv->phy_regulator = NULL;" like AHCI. > Delete it like AHCI regulator to fix this bug. > > Fixes: commit 962399bb7fbf ("ata: libahci_platform: Fix regulator_get_optional() misuse") > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Wang Hai <wanghai38@huawei.com> > --- > drivers/ata/libahci_platform.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index b2f552088291..0910441321f7 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -440,10 +440,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > hpriv->phy_regulator = devm_regulator_get(dev, "phy"); > if (IS_ERR(hpriv->phy_regulator)) { > rc = PTR_ERR(hpriv->phy_regulator); > - if (rc == -EPROBE_DEFER) > - goto err_out; > - rc = 0; > - hpriv->phy_regulator = NULL; > + goto err_out; > } > > if (flags & AHCI_PLATFORM_GET_RESETS) { > I applied this to for-5.15-fixes. Note that the code right before the hunk you fixed looks like this: hpriv->ahci_regulator = devm_regulator_get(dev, "ahci"); if (IS_ERR(hpriv->ahci_regulator)) { rc = PTR_ERR(hpriv->ahci_regulator); if (rc != 0) goto err_out; } This looks very strange to me. The "if (rc != 0)" seems bogus since if IS_ERR() is true, then hpriv->ahci_regulator is not NULL, it is an error pointer. Some cleanup seems necessary to me.
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index b2f552088291..0910441321f7 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -440,10 +440,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, hpriv->phy_regulator = devm_regulator_get(dev, "phy"); if (IS_ERR(hpriv->phy_regulator)) { rc = PTR_ERR(hpriv->phy_regulator); - if (rc == -EPROBE_DEFER) - goto err_out; - rc = 0; - hpriv->phy_regulator = NULL; + goto err_out; } if (flags & AHCI_PLATFORM_GET_RESETS) {
I got a null-ptr-deref report: KASAN: null-ptr-deref in range [0x0000000000000090-0x0000000000000097] ... RIP: 0010:regulator_enable+0x84/0x260 ... Call Trace: ahci_platform_enable_regulators+0xae/0x320 ahci_platform_enable_resources+0x1a/0x120 ahci_probe+0x4f/0x1b9 platform_probe+0x10b/0x280 ... entry_SYSCALL_64_after_hwframe+0x44/0xae If devm_regulator_get() in ahci_platform_get_resources() fails, hpriv->phy_regulator will point to NULL, when enabling or disabling it, null-ptr-deref will occur. ahci_probe() ahci_platform_get_resources() devm_regulator_get(, "phy") // failed, let phy_regulator = NULL ahci_platform_enable_resources() ahci_platform_enable_regulators() regulator_enable(hpriv->phy_regulator) // null-ptr-deref commit 962399bb7fbf ("ata: libahci_platform: Fix regulator_get_optional() misuse") replaces devm_regulator_get_optional() with devm_regulator_get(), but PHY regulator omits to delete "hpriv->phy_regulator = NULL;" like AHCI. Delete it like AHCI regulator to fix this bug. Fixes: commit 962399bb7fbf ("ata: libahci_platform: Fix regulator_get_optional() misuse") Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Wang Hai <wanghai38@huawei.com> --- drivers/ata/libahci_platform.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)