[PATCH u-boot 04/39] api: fix a potential serious bug caused by undef CONFIG_SYS_64BIT_LBA

[Marek Behun]

On Mon, 8 Mar 2021 15:09:51 +0800
Bin Meng <bmeng.cn at gmail.com> wrote:

> Hi Marek,
> 
> On Sun, Mar 7, 2021 at 12:26 PM Marek Behún <marek.behun at nic.cz> wrote:
> >
> > The api_public.h header file undefined macro CONFIG_SYS_64BIT_LBA.
> >
> > But api/api_storage.c includes this header before including part.h,
> > causing the type of lbaint_t and subsequently the type signature of
> > blk_dread() and blk_dwrite() functions to change from the rest of U-Boot
> > (if CONFIG_SYS_64BIT_LBA is defined for the board).
> >
> > This is of course wrong, because the call to blk_dread() / blk_dwrite()
> > will recieve mangled arguments.  
> 
> typo: receive
> 
> >
> > Fix this by removing the undef of macro CONFIG_SYS_64BIT_LBA and instead
> > make the immediate code do what it would do as if the macro was not
> > defined.
> >
> > Add a FIXME to whoever is maintaining this code.
> >
> > CI managed to trigger this bug when compiling for lsxhl_defconfig, which
> > has CONFIG_API selected. The compiler complained about blk_dwrite() and
> > blk_dread() not matching original declarations:
> >
> >   include/blk.h:280:15: warning: type of ‘blk_dwrite’ does not match
> >                                  original declaration
> >                                  [-Wlto-type-mismatch]
> >   280 | unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t st
> >       |               ^
> >   drivers/block/blk-uclass.c:456:15: note: type mismatch in parameter 2
> >   456 | unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t st
> >       |               ^
> >
> > Signed-off-by: Marek Behún <marek.behun at nic.cz>
> > ---
> >  include/api_public.h | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/api_public.h b/include/api_public.h
> > index def103ce22..5a4465ea89 100644
> > --- a/include/api_public.h
> > +++ b/include/api_public.h
> > @@ -70,12 +70,25 @@ struct sys_info {
> >         int                     mr_no;  /* number of memory regions */
> >  };
> >
> > -#undef CONFIG_SYS_64BIT_LBA
> > -#ifdef CONFIG_SYS_64BIT_LBA
> > -typedef        u_int64_t lbasize_t;
> > -#else
> > +/*
> > + * FIXME: Previously this code was:
> > + *
> > + *   #undef CONFIG_SYS_64BIT_LBA
> > + *   #ifdef CONFIG_SYS_64BIT_LBA
> > + *   typedef u_int64_t lbasize_t;
> > + *   #else
> > + *   typedef unsigned long lbasize_t;
> > + *   #endif
> > + *
> > + * But we cannot just undefine CONFIG_SYS_64BIT_LBA, because then in
> > + * api/api_storage.c the type signature of lbaint_t will be different if
> > + * CONFIG_SYS_64BIT_LBA is enabled for the board, which can result in various
> > + * bugs.
> > + * So simply define lbasize_t as an unsigned long, since this was what was done
> > + * anyway for at least 13 years, but don't undefine CONFIG_SYS_64BIT_LBA.
> > + */
> >  typedef unsigned long lbasize_t;  
> 
> Does "typedef lbaint_t labsize_t" help?

That could break API applications...