[PATCH] spl: fix error handling of spl_fit_read
From: Johannes Kirchmair johannes.kirchmair@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.
Signed-off-by: Johannes Kirchmair johannes.kirchmair@skidata.com --- common/spl/spl_fat.c | 2 +- include/spl_load.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
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);
+Sean Anderson too, for this
Hi,
On Fri, 16 Aug 2024 at 07:36, mailinglist1@johanneskirchmair.de wrote:
From: Johannes Kirchmair johannes.kirchmair@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.kirchmair@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?
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
Hey,
-----Ursprüngliche Nachricht----- Von: Simon Glass sjg@chromium.org Gesendet: Samstag, 17. August 2024 17:58 An: mailinglist1@johanneskirchmair.de; Sean Anderson seanga2@gmail.com Cc: u-boot@lists.denx.de; trini@konsulko.com; Johannes Kirchmair johannes.kirchmair@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, mailinglist1@johanneskirchmair.de wrote:
From: Johannes Kirchmair johannes.kirchmair@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.kirchmair@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-fventuri@comcast.net/
On 8/16/24 03:33, mailinglist1@johanneskirchmair.de wrote:
From: Johannes Kirchmair johannes.kirchmair@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.
Signed-off-by: Johannes Kirchmair johannes.kirchmair@skidata.com
common/spl/spl_fat.c | 2 +- include/spl_load.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
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);
Does [1] fix your problem?
--Sean
[1] https://lore.kernel.org/u-boot/20240731130913.487121-1-fventuri@comcast.net/
participants (3)
-
mailinglist1＠johanneskirchmair.de -
Sean Anderson -
Simon Glass