Re: [PATCH 1/2] efi/esrt: fix unsupported version initialization failure

[Ard Biesheuvel]

On 8 March 2018 at 18:15, Ard Biesheuvel  wrote:
> On 8 March 2018 at 18:00, Ard Biesheuvel  wrote:
>> On 8 March 2018 at 16:11, Tyler Baicar  wrote:
>>> On 2/24/2018 2:20 AM, Dave Young wrote:

 On 02/23/18 at 12:42pm, Tyler Baicar wrote:
>
> If ESRT initialization fails due to an unsupported version, the
> early_memremap allocation is never unmapped. This will cause an
> early ioremap leak. So, make sure to unmap the memory allocation
> before returning from efi_esrt_init().
>
> Signed-off-by: Tyler Baicar 
> ---
>   drivers/firmware/efi/esrt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index c47e0c6..504f3c3 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -285,7 +285,7 @@ void __init efi_esrt_init(void)
> } else {
> pr_err("Unsupported ESRT version %lld.\n",
>tmpesrt.fw_resource_version);
> -   return;
> +   goto err_memunmap;
> }
> if (tmpesrt.fw_resource_count > 0 && max - size < entry_size) {
> --

 Reviewed-by: Dave Young 
>>>
>>> Thank you Dave for your review here and input on the other patch.
>>>
>>> Ard,
>>>
>>> Can this patch be picked up? I understand patch 2 is not acceptable, but
>>> this one should
>>> be good to go I think.
>>>
>>
>> Yeah you're right. I'll pick it up as a bugfix.
>
> Actually, on second thought, could you respin this patch, and just
> move the memunamp() to right after the memcpy()? That way, we can get
> rid of all the 'goto err_memunmap's afaict
>

OK, now I'm confused. Does anyone have a clue why in efi_esrt_init()
the memremap() is done a second time? AFAICT it just reserves the
region, it does not actually access the second mapping at all.

Re: [PATCH 1/2] efi/esrt: fix unsupported version initialization failure

[Ard Biesheuvel]

On 8 March 2018 at 18:00, Ard Biesheuvel  wrote:
> On 8 March 2018 at 16:11, Tyler Baicar  wrote:
>> On 2/24/2018 2:20 AM, Dave Young wrote:
>>>
>>> On 02/23/18 at 12:42pm, Tyler Baicar wrote:

 If ESRT initialization fails due to an unsupported version, the
 early_memremap allocation is never unmapped. This will cause an
 early ioremap leak. So, make sure to unmap the memory allocation
 before returning from efi_esrt_init().

 Signed-off-by: Tyler Baicar 
 ---
   drivers/firmware/efi/esrt.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
 index c47e0c6..504f3c3 100644
 --- a/drivers/firmware/efi/esrt.c
 +++ b/drivers/firmware/efi/esrt.c
 @@ -285,7 +285,7 @@ void __init efi_esrt_init(void)
 } else {
 pr_err("Unsupported ESRT version %lld.\n",
tmpesrt.fw_resource_version);
 -   return;
 +   goto err_memunmap;
 }
 if (tmpesrt.fw_resource_count > 0 && max - size < entry_size) {
 --
>>>
>>> Reviewed-by: Dave Young 
>>
>> Thank you Dave for your review here and input on the other patch.
>>
>> Ard,
>>
>> Can this patch be picked up? I understand patch 2 is not acceptable, but
>> this one should
>> be good to go I think.
>>
>
> Yeah you're right. I'll pick it up as a bugfix.

Actually, on second thought, could you respin this patch, and just
move the memunamp() to right after the memcpy()? That way, we can get
rid of all the 'goto err_memunmap's afaict

Thanks,
Ard.

Re: [PATCH 1/2] efi/esrt: fix unsupported version initialization failure

[Ard Biesheuvel]

On 8 March 2018 at 16:11, Tyler Baicar  wrote:
> On 2/24/2018 2:20 AM, Dave Young wrote:
>>
>> On 02/23/18 at 12:42pm, Tyler Baicar wrote:
>>>
>>> If ESRT initialization fails due to an unsupported version, the
>>> early_memremap allocation is never unmapped. This will cause an
>>> early ioremap leak. So, make sure to unmap the memory allocation
>>> before returning from efi_esrt_init().
>>>
>>> Signed-off-by: Tyler Baicar 
>>> ---
>>>   drivers/firmware/efi/esrt.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
>>> index c47e0c6..504f3c3 100644
>>> --- a/drivers/firmware/efi/esrt.c
>>> +++ b/drivers/firmware/efi/esrt.c
>>> @@ -285,7 +285,7 @@ void __init efi_esrt_init(void)
>>> } else {
>>> pr_err("Unsupported ESRT version %lld.\n",
>>>tmpesrt.fw_resource_version);
>>> -   return;
>>> +   goto err_memunmap;
>>> }
>>> if (tmpesrt.fw_resource_count > 0 && max - size < entry_size) {
>>> --
>>
>> Reviewed-by: Dave Young 
>
> Thank you Dave for your review here and input on the other patch.
>
> Ard,
>
> Can this patch be picked up? I understand patch 2 is not acceptable, but
> this one should
> be good to go I think.
>

Yeah you're right. I'll pick it up as a bugfix.

Re: [PATCH 1/2] efi/esrt: fix unsupported version initialization failure

[Tyler Baicar]

On 2/24/2018 2:20 AM, Dave Young wrote:

On 02/23/18 at 12:42pm, Tyler Baicar wrote:

If ESRT initialization fails due to an unsupported version, the
early_memremap allocation is never unmapped. This will cause an
early ioremap leak. So, make sure to unmap the memory allocation
before returning from efi_esrt_init().

Signed-off-by: Tyler Baicar 
---
  drivers/firmware/efi/esrt.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index c47e0c6..504f3c3 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -285,7 +285,7 @@ void __init efi_esrt_init(void)
} else {
pr_err("Unsupported ESRT version %lld.\n",
   tmpesrt.fw_resource_version);
-   return;
+   goto err_memunmap;
}
  
  	if (tmpesrt.fw_resource_count > 0 && max - size < entry_size) {

--

Reviewed-by: Dave Young 

Thank you Dave for your review here and input on the other patch.

Ard,

Can this patch be picked up? I understand patch 2 is not acceptable, but this 
one should

be good to go I think.

Thanks,
Tyler

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH 1/2] efi/esrt: fix unsupported version initialization failure

[Dave Young]

On 02/23/18 at 12:42pm, Tyler Baicar wrote:
> If ESRT initialization fails due to an unsupported version, the
> early_memremap allocation is never unmapped. This will cause an
> early ioremap leak. So, make sure to unmap the memory allocation
> before returning from efi_esrt_init().
> 
> Signed-off-by: Tyler Baicar 
> ---
>  drivers/firmware/efi/esrt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index c47e0c6..504f3c3 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -285,7 +285,7 @@ void __init efi_esrt_init(void)
>   } else {
>   pr_err("Unsupported ESRT version %lld.\n",
>  tmpesrt.fw_resource_version);
> - return;
> + goto err_memunmap;
>   }
>  
>   if (tmpesrt.fw_resource_count > 0 && max - size < entry_size) {
> -- 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
> Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reviewed-by: Dave Young 

Thanks
Dave

[PATCH 1/2] efi/esrt: fix unsupported version initialization failure

[Tyler Baicar]

If ESRT initialization fails due to an unsupported version, the
early_memremap allocation is never unmapped. This will cause an
early ioremap leak. So, make sure to unmap the memory allocation
before returning from efi_esrt_init().

Signed-off-by: Tyler Baicar 
---
 drivers/firmware/efi/esrt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index c47e0c6..504f3c3 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -285,7 +285,7 @@ void __init efi_esrt_init(void)
} else {
pr_err("Unsupported ESRT version %lld.\n",
   tmpesrt.fw_resource_version);
-   return;
+   goto err_memunmap;
}
 
if (tmpesrt.fw_resource_count > 0 && max - size < entry_size) {
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.