[PATCH] android_ab: Fix backup offset calculation

[PATCH] android_ab: Fix backup offset calculation

[Joshua Watt]

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>
---
 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

Re: [PATCH] android_ab: Fix backup offset calculation

[Mattijs Korpershoek]

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

Re: [PATCH] android_ab: Fix backup offset calculation

[Sam Protsenko]

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
>

[PATCH v2] android_ab: Fixes: Fix backup offset calculation

[Joshua Watt]

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

Re: [PATCH v2] android_ab: Fixes: Fix backup offset calculation

[Mattijs Korpershoek]

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

Re: [PATCH v2] android_ab: Fixes: Fix backup offset calculation

[Mattijs Korpershoek]

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