* Re: [PATCH] android_ab: Fix backup offset calculation
2024-08-07 14:13 [PATCH] android_ab: Fix backup offset calculation Joshua Watt
@ 2024-08-16 8:23 ` Mattijs Korpershoek
2024-08-28 0:38 ` Sam Protsenko
2024-08-28 14:37 ` [PATCH v2] android_ab: Fixes: " Joshua Watt
2 siblings, 0 replies; 6+ messages in thread
From: Mattijs Korpershoek @ 2024-08-16 8:23 UTC (permalink / raw)
To: Joshua Watt, u-boot; +Cc: Joshua Watt, Igor Opaniuk, Sam Protsenko, Tom Rini
Hi Joshua,
Thank you for the patch.
On mer., août 07, 2024 at 08:13, Joshua Watt <jpewhacker@gmail.com> wrote:
> The backup offset is in bytes, but was incorrectly be interpreted as
> blocks, leading to it being written to the wrong location. Fix the
> calculation and clarify that ANDROID_AB_BACKUP_OFFSET is in bytes and
> must be a multiple of the block size.
Nice finding !
>
> Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
> ---
> boot/android_ab.c | 4 ++--
> common/Kconfig | 3 ++-
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index 143f373aae..b253200161 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -139,8 +139,8 @@ static int ab_control_store(struct blk_desc *dev_desc,
> {
> ulong abc_offset, abc_blocks, ret;
>
Can we add a safeguard in here to make sure that offset
is always blksz aligned ?
We could do something similar to ab_control_create_from_disk():
if (offset % part_info->blksz) {
log_err("ANDROID: offset not block aligned.\n");
return -EINVAL;
}
> - abc_offset = offset +
> - offsetof(struct bootloader_message_ab, slot_suffix) /
> + abc_offset = (offset +
> + offsetof(struct bootloader_message_ab, slot_suffix)) /
> part_info->blksz;
> abc_blocks = DIV_ROUND_UP(sizeof(struct bootloader_control),
> part_info->blksz);
I've also noticed that ab_control_store()'s documentation does not
document the @param[in] offset.
Could we add that as well (either in a seperate patch or in this one) ?
> diff --git a/common/Kconfig b/common/Kconfig
> index 83c81edac2..e1b8557e0c 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -986,7 +986,8 @@ config ANDROID_AB_BACKUP_OFFSET
> help
> If non-zero, a backup bootloader message starting at this offset in
> the partition will tried in the event that the primary one (starting
> - at offset 0) fails its checksum.
> + at offset 0) fails its checksum. The offset is in bytes and must be
> + multiple of the block size.
>
> endmenu
>
> --
> 2.45.2
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] android_ab: Fix backup offset calculation
2024-08-07 14:13 [PATCH] android_ab: Fix backup offset calculation Joshua Watt
2024-08-16 8:23 ` Mattijs Korpershoek
@ 2024-08-28 0:38 ` Sam Protsenko
2024-08-28 14:37 ` [PATCH v2] android_ab: Fixes: " Joshua Watt
2 siblings, 0 replies; 6+ messages in thread
From: Sam Protsenko @ 2024-08-28 0:38 UTC (permalink / raw)
To: Joshua Watt; +Cc: u-boot, Igor Opaniuk, Mattijs Korpershoek, Tom Rini
On Wed, Aug 7, 2024 at 9:14 AM Joshua Watt <jpewhacker@gmail.com> wrote:
>
> The backup offset is in bytes, but was incorrectly be interpreted as
> blocks, leading to it being written to the wrong location. Fix the
> calculation and clarify that ANDROID_AB_BACKUP_OFFSET is in bytes and
> must be a multiple of the block size.
>
> Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
> ---
In addition to what Mattijs said, I think it makes sense to add
"Fixes:" tag for this patch.
> boot/android_ab.c | 4 ++--
> common/Kconfig | 3 ++-
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index 143f373aae..b253200161 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -139,8 +139,8 @@ static int ab_control_store(struct blk_desc *dev_desc,
> {
> ulong abc_offset, abc_blocks, ret;
>
> - abc_offset = offset +
> - offsetof(struct bootloader_message_ab, slot_suffix) /
> + abc_offset = (offset +
> + offsetof(struct bootloader_message_ab, slot_suffix)) /
> part_info->blksz;
> abc_blocks = DIV_ROUND_UP(sizeof(struct bootloader_control),
> part_info->blksz);
> diff --git a/common/Kconfig b/common/Kconfig
> index 83c81edac2..e1b8557e0c 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -986,7 +986,8 @@ config ANDROID_AB_BACKUP_OFFSET
> help
> If non-zero, a backup bootloader message starting at this offset in
> the partition will tried in the event that the primary one (starting
> - at offset 0) fails its checksum.
> + at offset 0) fails its checksum. The offset is in bytes and must be
> + multiple of the block size.
>
> endmenu
>
> --
> 2.45.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] android_ab: Fixes: Fix backup offset calculation
2024-08-07 14:13 [PATCH] android_ab: Fix backup offset calculation Joshua Watt
2024-08-16 8:23 ` Mattijs Korpershoek
2024-08-28 0:38 ` Sam Protsenko
@ 2024-08-28 14:37 ` Joshua Watt
2024-08-29 8:56 ` Mattijs Korpershoek
2024-09-05 6:56 ` Mattijs Korpershoek
2 siblings, 2 replies; 6+ messages in thread
From: Joshua Watt @ 2024-08-28 14:37 UTC (permalink / raw)
To: u-boot
Cc: Joshua Watt, Igor Opaniuk, Mattijs Korpershoek, Sam Protsenko,
Tom Rini
The backup offset is in bytes, but was incorrectly be interpreted as
blocks, leading to it being written to the wrong location. Fix the
calculation, clarify that ANDROID_AB_BACKUP_OFFSET is in bytes and must
be a multiple of the block size, and add a runtime check to validate the
offset.
Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
---
boot/android_ab.c | 9 +++++++--
common/Kconfig | 3 ++-
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c
index 143f373aae..1196a189ed 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -139,8 +139,13 @@ static int ab_control_store(struct blk_desc *dev_desc,
{
ulong abc_offset, abc_blocks, ret;
- abc_offset = offset +
- offsetof(struct bootloader_message_ab, slot_suffix) /
+ if (offset % part_info->blksz) {
+ log_err("ANDROID: offset not block aligned\n");
+ return -EINVAL;
+ }
+
+ abc_offset = (offset +
+ offsetof(struct bootloader_message_ab, slot_suffix)) /
part_info->blksz;
abc_blocks = DIV_ROUND_UP(sizeof(struct bootloader_control),
part_info->blksz);
diff --git a/common/Kconfig b/common/Kconfig
index 83c81edac2..e1b8557e0c 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -986,7 +986,8 @@ config ANDROID_AB_BACKUP_OFFSET
help
If non-zero, a backup bootloader message starting at this offset in
the partition will tried in the event that the primary one (starting
- at offset 0) fails its checksum.
+ at offset 0) fails its checksum. The offset is in bytes and must be
+ multiple of the block size.
endmenu
--
2.45.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] android_ab: Fixes: Fix backup offset calculation
2024-08-28 14:37 ` [PATCH v2] android_ab: Fixes: " Joshua Watt
@ 2024-08-29 8:56 ` Mattijs Korpershoek
2024-09-05 6:56 ` Mattijs Korpershoek
1 sibling, 0 replies; 6+ messages in thread
From: Mattijs Korpershoek @ 2024-08-29 8:56 UTC (permalink / raw)
To: Joshua Watt, u-boot; +Cc: Joshua Watt, Igor Opaniuk, Sam Protsenko, Tom Rini
Hi Joshua,
Thank you for the patch.
On mer., août 28, 2024 at 08:37, Joshua Watt <jpewhacker@gmail.com> wrote:
> The backup offset is in bytes, but was incorrectly be interpreted as
> blocks, leading to it being written to the wrong location. Fix the
> calculation, clarify that ANDROID_AB_BACKUP_OFFSET is in bytes and must
> be a multiple of the block size, and add a runtime check to validate the
> offset.
>
> Signed-off-by: Joshua Watt <JPEWhacker@gmail.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
As Igor suggested, I will add:
Fixes: 3430f24bc69d ("android_ab: Try backup booloader_message")
When applying.
> ---
> boot/android_ab.c | 9 +++++++--
> common/Kconfig | 3 ++-
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index 143f373aae..1196a189ed 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -139,8 +139,13 @@ static int ab_control_store(struct blk_desc *dev_desc,
> {
> ulong abc_offset, abc_blocks, ret;
>
> - abc_offset = offset +
> - offsetof(struct bootloader_message_ab, slot_suffix) /
> + if (offset % part_info->blksz) {
> + log_err("ANDROID: offset not block aligned\n");
> + return -EINVAL;
> + }
> +
> + abc_offset = (offset +
> + offsetof(struct bootloader_message_ab, slot_suffix)) /
> part_info->blksz;
> abc_blocks = DIV_ROUND_UP(sizeof(struct bootloader_control),
> part_info->blksz);
> diff --git a/common/Kconfig b/common/Kconfig
> index 83c81edac2..e1b8557e0c 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -986,7 +986,8 @@ config ANDROID_AB_BACKUP_OFFSET
> help
> If non-zero, a backup bootloader message starting at this offset in
> the partition will tried in the event that the primary one (starting
> - at offset 0) fails its checksum.
> + at offset 0) fails its checksum. The offset is in bytes and must be
> + multiple of the block size.
>
> endmenu
>
> --
> 2.45.2
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] android_ab: Fixes: Fix backup offset calculation
2024-08-28 14:37 ` [PATCH v2] android_ab: Fixes: " Joshua Watt
2024-08-29 8:56 ` Mattijs Korpershoek
@ 2024-09-05 6:56 ` Mattijs Korpershoek
1 sibling, 0 replies; 6+ messages in thread
From: Mattijs Korpershoek @ 2024-09-05 6:56 UTC (permalink / raw)
To: u-boot, Joshua Watt; +Cc: Joshua Watt, Igor Opaniuk, Sam Protsenko, Tom Rini
Hi,
On Wed, 28 Aug 2024 08:37:57 -0600, Joshua Watt wrote:
> The backup offset is in bytes, but was incorrectly be interpreted as
> blocks, leading to it being written to the wrong location. Fix the
> calculation, clarify that ANDROID_AB_BACKUP_OFFSET is in bytes and must
> be a multiple of the block size, and add a runtime check to validate the
> offset.
>
>
> [...]
Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)
[1/1] android_ab: Fixes: Fix backup offset calculation
https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/cc2f60c13f6aaf1bd277cf10c8b375c2e4a695b7
--
Mattijs
^ permalink raw reply [flat|nested] 6+ messages in thread