Hey,
> -----Ursprüngliche Nachricht-----
> Von: Simon Glass <s...@chromium.org>
> Gesendet: Samstag, 17. August 2024 17:58
> An: mailingli...@johanneskirchmair.de; Sean Anderson
> <sean...@gmail.com>
> Cc: u-boot@lists.denx.de; tr...@konsulko.com; Johannes Kirchmair
> <johannes.kirchm...@skidata.com>
> Betreff: Re: [PATCH] spl: fix error handling of spl_fit_read
>
> +Sean Anderson too, for this
>
> Hi,
>
> On Fri, 16 Aug 2024 at 07:36, <mailingli...@johanneskirchmair.de> wrote:
> >
> > From: Johannes Kirchmair <johannes.kirchm...@skidata.com>
> >
> > Returning negative values from spl_fit_read leads to u-boot crashing.
> > The return value of spl_fit_read is compared with an unsigned value.
> > Returning negative values leads to the check not detecting the error.
> > Not detecting the error leads to crashing.
> >
> > Returning zero in case of an reading error is fine. It indicates that
> > nothing was red.
>
> read
>
> >
> > Signed-off-by: Johannes Kirchmair <johannes.kirchm...@skidata.com>
> > ---
> > common/spl/spl_fat.c | 2 +-
> > include/spl_load.h | 4 ++--
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
>
> Perhaps instead this should be fixed in _spl_load()? We don't want to ignore
> errors, and returning -EIO is not as good as returning the actual error
> received.
> The docs of struct spl_load_info indicate that functions should return 0 on
> error, but that is somewhat surprising behaviour, which is probably why
> people are not following it?
Could also be a valid way of fixing, as Sean did in his patch [1]
Too have less confusion in the future, we should also switch the signature from
returning ulong to long and allow negative return values for errors.
>
> > diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c index
> > bd8aab253a..345bc55149 100644
> > --- a/common/spl/spl_fat.c
> > +++ b/common/spl/spl_fat.c
> > @@ -53,7 +53,7 @@ static ulong spl_fit_read(struct spl_load_info
> > *load, ulong file_offset,
> >
> > ret = fat_read_file(filename, buf, file_offset, size, &actread);
> > if (ret)
> > - return ret;
> > + return 0;
> >
> > return actread;
> > }
> > diff --git a/include/spl_load.h b/include/spl_load.h index
> > 1c2b296c0a..b411d9daa1 100644
> > --- a/include/spl_load.h
> > +++ b/include/spl_load.h
> > @@ -17,8 +17,8 @@ static inline int _spl_load(struct spl_image_info
> > *spl_image, {
> > struct legacy_img_hdr *header =
> > spl_get_load_buffer(-sizeof(*header), sizeof(*header));
> > - ulong base_offset, image_offset, overhead;
> > - int read, ret;
> > + ulong base_offset, image_offset, overhead, read;
> > + int ret;
> >
> > read = info->read(info, offset, ALIGN(sizeof(*header),
> > spl_get_bl_len(info)),
> > header);
> > --
> > 2.34.1
> >
>
> Regards,
> Simon
Best regards,
Johannes
[1] https://lore.kernel.org/u-boot/20240731130913.487121-1-fvent...@comcast.net/
