[PATCH] efi/libstub: Cast away type warning in use of max()

[PATCH] efi/libstub: Cast away type warning in use of max()

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

Add a missing (u64) cast to alloc_min, which is passed into
efi_random_alloc() as unsigned long, while efi_physical_addr_t is u64.

Fixes: 3cb4a4827596abc82e ("efi/libstub: fix efi_random_alloc() ...")
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/randomalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/randomalloc.c b/drivers/firmware/efi/libstub/randomalloc.c
index 7e1852859550..fa81528150fe 100644
--- a/drivers/firmware/efi/libstub/randomalloc.c
+++ b/drivers/firmware/efi/libstub/randomalloc.c
@@ -120,7 +120,7 @@ efi_status_t efi_random_alloc(unsigned long size,
 			continue;
 		}
 
-		target = round_up(max(md->phys_addr, alloc_min), align) + target_slot * align;
+		target = round_up(max(md->phys_addr, (u64)alloc_min), align) + target_slot * align;
 		pages = size / EFI_PAGE_SIZE;
 
 		status = efi_bs_call(allocate_pages, EFI_ALLOCATE_ADDRESS,
-- 
2.44.0.396.g6e790dbe36-goog

Re: [PATCH] efi/libstub: Cast away type warning in use of max()

[Lukas Wunner]

On Tue, Mar 26, 2024 at 11:18:51AM +0100, Ard Biesheuvel wrote:
> Add a missing (u64) cast to alloc_min, which is passed into
> efi_random_alloc() as unsigned long, while efi_physical_addr_t is u64.
[...]
> --- a/drivers/firmware/efi/libstub/randomalloc.c
> +++ b/drivers/firmware/efi/libstub/randomalloc.c
> @@ -120,7 +120,7 @@ efi_status_t efi_random_alloc(unsigned long size,
>  			continue;
>  		}
>  
> -		target = round_up(max(md->phys_addr, alloc_min), align) + target_slot * align;
> +		target = round_up(max(md->phys_addr, (u64)alloc_min), align) + target_slot * align;

Why not

    max_t(u64, md->phys_addr, alloc_min)

?

Re: [PATCH] efi/libstub: Cast away type warning in use of max()

[Ard Biesheuvel]

On Thu, 28 Mar 2024 at 10:21, Lukas Wunner <lukas@wunner.de> wrote:
>
> On Tue, Mar 26, 2024 at 11:18:51AM +0100, Ard Biesheuvel wrote:
> > Add a missing (u64) cast to alloc_min, which is passed into
> > efi_random_alloc() as unsigned long, while efi_physical_addr_t is u64.
> [...]
> > --- a/drivers/firmware/efi/libstub/randomalloc.c
> > +++ b/drivers/firmware/efi/libstub/randomalloc.c
> > @@ -120,7 +120,7 @@ efi_status_t efi_random_alloc(unsigned long size,
> >                       continue;
> >               }
> >
> > -             target = round_up(max(md->phys_addr, alloc_min), align) + target_slot * align;
> > +             target = round_up(max(md->phys_addr, (u64)alloc_min), align) + target_slot * align;
>
> Why not
>
>     max_t(u64, md->phys_addr, alloc_min)
>

Why is that better?

Re: [PATCH] efi/libstub: Cast away type warning in use of max()

[Lukas Wunner]

On Thu, Mar 28, 2024 at 11:13:07AM +0200, Ard Biesheuvel wrote:
> On Thu, 28 Mar 2024 at 10:21, Lukas Wunner <lukas@wunner.de> wrote:
> > On Tue, Mar 26, 2024 at 11:18:51AM +0100, Ard Biesheuvel wrote:
> > > Add a missing (u64) cast to alloc_min, which is passed into
> > > efi_random_alloc() as unsigned long, while efi_physical_addr_t is u64.
> > [...]
> > > --- a/drivers/firmware/efi/libstub/randomalloc.c
> > > +++ b/drivers/firmware/efi/libstub/randomalloc.c
> > > @@ -120,7 +120,7 @@ efi_status_t efi_random_alloc(unsigned long size,
> > >                       continue;
> > >               }
> > >
> > > -             target = round_up(max(md->phys_addr, alloc_min), align) + target_slot * align;
> > > +             target = round_up(max(md->phys_addr, (u64)alloc_min), align) + target_slot * align;
> >
> > Why not
> >
> >     max_t(u64, md->phys_addr, alloc_min)
> 
> Why is that better?

It just seems to be the idiomatic way to handle these casts in the kernel.

It's also what checkpatch suggests, so by not using it you risk getting
"helpful" fixup patches from the usual suspects.

It's your call buddy. :)

Re: [PATCH] efi/libstub: Cast away type warning in use of max()

[Ard Biesheuvel]

On Thu, 28 Mar 2024 at 15:38, Lukas Wunner <lukas@wunner.de> wrote:
>
> On Thu, Mar 28, 2024 at 11:13:07AM +0200, Ard Biesheuvel wrote:
> > On Thu, 28 Mar 2024 at 10:21, Lukas Wunner <lukas@wunner.de> wrote:
> > > On Tue, Mar 26, 2024 at 11:18:51AM +0100, Ard Biesheuvel wrote:
> > > > Add a missing (u64) cast to alloc_min, which is passed into
> > > > efi_random_alloc() as unsigned long, while efi_physical_addr_t is u64.
> > > [...]
> > > > --- a/drivers/firmware/efi/libstub/randomalloc.c
> > > > +++ b/drivers/firmware/efi/libstub/randomalloc.c
> > > > @@ -120,7 +120,7 @@ efi_status_t efi_random_alloc(unsigned long size,
> > > >                       continue;
> > > >               }
> > > >
> > > > -             target = round_up(max(md->phys_addr, alloc_min), align) + target_slot * align;
> > > > +             target = round_up(max(md->phys_addr, (u64)alloc_min), align) + target_slot * align;
> > >
> > > Why not
> > >
> > >     max_t(u64, md->phys_addr, alloc_min)
> >
> > Why is that better?
>
> It just seems to be the idiomatic way to handle these casts in the kernel.
>

In this particular case, I prefer max() with the cast, because it
matches the other occurrence, where alloc_min is also used but there
it is u64 not unsigned long.

> It's also what checkpatch suggests, so by not using it you risk getting
> "helpful" fixup patches from the usual suspects.
>

Ugh yeah good point.

> It's your call buddy. :)

Thanks for the head's up