[U-Boot] [PATCH] disk: efi: Correct backing up the MBR boot code

[U-Boot] [PATCH] disk: efi: Correct backing up the MBR boot code

[Sam Protsenko]

In commit e163a931af34 ("cmd: gpt: backup boot code before writing MBR")
there was added the procedure for storing old boot code when doing "gpt
write". But instead of storing just backup code, the whole MBR was
stored, and only specific fields were replaced further, keeping
everything else intact. That's obviously not what we want.

Fix the code to actually store only old boot code and zero out
everything else. This fixes next testing case:

    => mmc write $loadaddr 0x0 0x7b
    => gpt write mmc 1 $partitions

In case when $loadaddr address and further memory contains 0xff, the
board was bricked (ROM-code probably didn't like partition entries that
were clobbered with 0xff). With this patch applied, commands above don't
brick the board.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Cc: Alejandro Hernandez <ajhernandez@ti.com>
---
 disk/part_efi.c    | 6 ++++--
 include/part_efi.h | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 72e3997b84..5c1039f013 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -350,8 +350,6 @@ static int set_protective_mbr(struct blk_desc *dev_desc)
 {
 	/* Setup the Protective MBR */
 	ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, p_mbr, 1, dev_desc->blksz);
-	memset(p_mbr, 0, sizeof(*p_mbr));
-
 	if (p_mbr == NULL) {
 		printf("%s: calloc failed!\n", __func__);
 		return -1;
@@ -363,6 +361,10 @@ static int set_protective_mbr(struct blk_desc *dev_desc)
 		return -1;
 	}
 
+	/* Clear all data in MBR except of backed up boot code */
+	memset((char *)p_mbr + MSDOS_MBR_BOOT_CODE_SIZE, 0, sizeof(*p_mbr) -
+			MSDOS_MBR_BOOT_CODE_SIZE);
+
 	/* Append signature */
 	p_mbr->signature = MSDOS_MBR_SIGNATURE;
 	p_mbr->partition_record[0].sys_ind = EFI_PMBR_OSTYPE_EFI_GPT;
diff --git a/include/part_efi.h b/include/part_efi.h
index 6065c571f3..8525770445 100644
--- a/include/part_efi.h
+++ b/include/part_efi.h
@@ -20,6 +20,7 @@
 #include <efi.h>
 
 #define MSDOS_MBR_SIGNATURE 0xAA55
+#define MSDOS_MBR_BOOT_CODE_SIZE 440
 #define EFI_PMBR_OSTYPE_EFI 0xEF
 #define EFI_PMBR_OSTYPE_EFI_GPT 0xEE
 
@@ -111,7 +112,7 @@ typedef struct _gpt_entry {
 } __packed gpt_entry;
 
 typedef struct _legacy_mbr {
-	u8 boot_code[440];
+	u8 boot_code[MSDOS_MBR_BOOT_CODE_SIZE];
 	__le32 unique_mbr_signature;
 	__le16 unknown;
 	struct partition partition_record[4];
-- 
2.17.0

[U-Boot] [PATCH] disk: efi: Correct backing up the MBR boot code

[Andy Shevchenko]

+Cc: Ferry, he might be interested in testing this on Intel Edison.

On Tue, May 22, 2018 at 2:04 AM, Sam Protsenko
<semen.protsenko@linaro.org> wrote:
> In commit e163a931af34 ("cmd: gpt: backup boot code before writing MBR")
> there was added the procedure for storing old boot code when doing "gpt
> write". But instead of storing just backup code, the whole MBR was
> stored, and only specific fields were replaced further, keeping
> everything else intact. That's obviously not what we want.
>
> Fix the code to actually store only old boot code and zero out
> everything else. This fixes next testing case:
>
>     => mmc write $loadaddr 0x0 0x7b
>     => gpt write mmc 1 $partitions
>
> In case when $loadaddr address and further memory contains 0xff, the
> board was bricked (ROM-code probably didn't like partition entries that
> were clobbered with 0xff). With this patch applied, commands above don't
> brick the board.
>

Thanks for the patch.
It looks like a right thing to do, but let me couple of days to go
through it and recall what and why has been done on Intel Edison WRT
MBR transfering.

> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> Cc: Alejandro Hernandez <ajhernandez@ti.com>
> ---
>  disk/part_efi.c    | 6 ++++--
>  include/part_efi.h | 3 ++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 72e3997b84..5c1039f013 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -350,8 +350,6 @@ static int set_protective_mbr(struct blk_desc *dev_desc)
>  {
>         /* Setup the Protective MBR */
>         ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, p_mbr, 1, dev_desc->blksz);
> -       memset(p_mbr, 0, sizeof(*p_mbr));
> -
>         if (p_mbr == NULL) {
>                 printf("%s: calloc failed!\n", __func__);
>                 return -1;
> @@ -363,6 +361,10 @@ static int set_protective_mbr(struct blk_desc *dev_desc)
>                 return -1;
>         }
>
> +       /* Clear all data in MBR except of backed up boot code */
> +       memset((char *)p_mbr + MSDOS_MBR_BOOT_CODE_SIZE, 0, sizeof(*p_mbr) -
> +                       MSDOS_MBR_BOOT_CODE_SIZE);
> +
>         /* Append signature */
>         p_mbr->signature = MSDOS_MBR_SIGNATURE;
>         p_mbr->partition_record[0].sys_ind = EFI_PMBR_OSTYPE_EFI_GPT;
> diff --git a/include/part_efi.h b/include/part_efi.h
> index 6065c571f3..8525770445 100644
> --- a/include/part_efi.h
> +++ b/include/part_efi.h
> @@ -20,6 +20,7 @@
>  #include <efi.h>
>
>  #define MSDOS_MBR_SIGNATURE 0xAA55
> +#define MSDOS_MBR_BOOT_CODE_SIZE 440
>  #define EFI_PMBR_OSTYPE_EFI 0xEF
>  #define EFI_PMBR_OSTYPE_EFI_GPT 0xEE
>
> @@ -111,7 +112,7 @@ typedef struct _gpt_entry {
>  } __packed gpt_entry;
>
>  typedef struct _legacy_mbr {
> -       u8 boot_code[440];
> +       u8 boot_code[MSDOS_MBR_BOOT_CODE_SIZE];
>         __le32 unique_mbr_signature;
>         __le16 unknown;
>         struct partition partition_record[4];
> --
> 2.17.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH] disk: efi: Correct backing up the MBR boot code

[Andy Shevchenko]

On Tue, May 22, 2018 at 2:04 AM, Sam Protsenko
<semen.protsenko@linaro.org> wrote:
> In commit e163a931af34 ("cmd: gpt: backup boot code before writing MBR")
> there was added the procedure for storing old boot code when doing "gpt
> write". But instead of storing just backup code, the whole MBR was
> stored, and only specific fields were replaced further, keeping
> everything else intact. That's obviously not what we want.
>
> Fix the code to actually store only old boot code and zero out
> everything else. This fixes next testing case:
>
>     => mmc write $loadaddr 0x0 0x7b
>     => gpt write mmc 1 $partitions
>
> In case when $loadaddr address and further memory contains 0xff, the
> board was bricked (ROM-code probably didn't like partition entries that
> were clobbered with 0xff). With this patch applied, commands above don't
> brick the board.

Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>

At least it doesn't break Intel Edison.

>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> Cc: Alejandro Hernandez <ajhernandez@ti.com>
> ---
>  disk/part_efi.c    | 6 ++++--
>  include/part_efi.h | 3 ++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 72e3997b84..5c1039f013 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -350,8 +350,6 @@ static int set_protective_mbr(struct blk_desc *dev_desc)
>  {
>         /* Setup the Protective MBR */
>         ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, p_mbr, 1, dev_desc->blksz);
> -       memset(p_mbr, 0, sizeof(*p_mbr));
> -
>         if (p_mbr == NULL) {
>                 printf("%s: calloc failed!\n", __func__);
>                 return -1;
> @@ -363,6 +361,10 @@ static int set_protective_mbr(struct blk_desc *dev_desc)
>                 return -1;
>         }
>
> +       /* Clear all data in MBR except of backed up boot code */
> +       memset((char *)p_mbr + MSDOS_MBR_BOOT_CODE_SIZE, 0, sizeof(*p_mbr) -
> +                       MSDOS_MBR_BOOT_CODE_SIZE);
> +
>         /* Append signature */
>         p_mbr->signature = MSDOS_MBR_SIGNATURE;
>         p_mbr->partition_record[0].sys_ind = EFI_PMBR_OSTYPE_EFI_GPT;
> diff --git a/include/part_efi.h b/include/part_efi.h
> index 6065c571f3..8525770445 100644
> --- a/include/part_efi.h
> +++ b/include/part_efi.h
> @@ -20,6 +20,7 @@
>  #include <efi.h>
>
>  #define MSDOS_MBR_SIGNATURE 0xAA55
> +#define MSDOS_MBR_BOOT_CODE_SIZE 440
>  #define EFI_PMBR_OSTYPE_EFI 0xEF
>  #define EFI_PMBR_OSTYPE_EFI_GPT 0xEE
>
> @@ -111,7 +112,7 @@ typedef struct _gpt_entry {
>  } __packed gpt_entry;
>
>  typedef struct _legacy_mbr {
> -       u8 boot_code[440];
> +       u8 boot_code[MSDOS_MBR_BOOT_CODE_SIZE];
>         __le32 unique_mbr_signature;
>         __le16 unknown;
>         struct partition partition_record[4];
> --
> 2.17.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
With Best Regards,
Andy Shevchenko

[U-Boot] [PATCH] disk: efi: Correct backing up the MBR boot code

[Sam Protsenko]

On 23 May 2018 at 12:51, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, May 22, 2018 at 2:04 AM, Sam Protsenko
> <semen.protsenko@linaro.org> wrote:
>> In commit e163a931af34 ("cmd: gpt: backup boot code before writing MBR")
>> there was added the procedure for storing old boot code when doing "gpt
>> write". But instead of storing just backup code, the whole MBR was
>> stored, and only specific fields were replaced further, keeping
>> everything else intact. That's obviously not what we want.
>>
>> Fix the code to actually store only old boot code and zero out
>> everything else. This fixes next testing case:
>>
>>     => mmc write $loadaddr 0x0 0x7b
>>     => gpt write mmc 1 $partitions
>>
>> In case when $loadaddr address and further memory contains 0xff, the
>> board was bricked (ROM-code probably didn't like partition entries that
>> were clobbered with 0xff). With this patch applied, commands above don't
>> brick the board.
>
> Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> At least it doesn't break Intel Edison.
>

Hi Tom,

Can we apply this one? It's kinda critical, as some boards were
bricked while performing tests.

Thanks.

>>
>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>> Cc: Alejandro Hernandez <ajhernandez@ti.com>
>> ---
>>  disk/part_efi.c    | 6 ++++--
>>  include/part_efi.h | 3 ++-
>>  2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>> index 72e3997b84..5c1039f013 100644
>> --- a/disk/part_efi.c
>> +++ b/disk/part_efi.c
>> @@ -350,8 +350,6 @@ static int set_protective_mbr(struct blk_desc *dev_desc)
>>  {
>>         /* Setup the Protective MBR */
>>         ALLOC_CACHE_ALIGN_BUFFER_PAD(legacy_mbr, p_mbr, 1, dev_desc->blksz);
>> -       memset(p_mbr, 0, sizeof(*p_mbr));
>> -
>>         if (p_mbr == NULL) {
>>                 printf("%s: calloc failed!\n", __func__);
>>                 return -1;
>> @@ -363,6 +361,10 @@ static int set_protective_mbr(struct blk_desc *dev_desc)
>>                 return -1;
>>         }
>>
>> +       /* Clear all data in MBR except of backed up boot code */
>> +       memset((char *)p_mbr + MSDOS_MBR_BOOT_CODE_SIZE, 0, sizeof(*p_mbr) -
>> +                       MSDOS_MBR_BOOT_CODE_SIZE);
>> +
>>         /* Append signature */
>>         p_mbr->signature = MSDOS_MBR_SIGNATURE;
>>         p_mbr->partition_record[0].sys_ind = EFI_PMBR_OSTYPE_EFI_GPT;
>> diff --git a/include/part_efi.h b/include/part_efi.h
>> index 6065c571f3..8525770445 100644
>> --- a/include/part_efi.h
>> +++ b/include/part_efi.h
>> @@ -20,6 +20,7 @@
>>  #include <efi.h>
>>
>>  #define MSDOS_MBR_SIGNATURE 0xAA55
>> +#define MSDOS_MBR_BOOT_CODE_SIZE 440
>>  #define EFI_PMBR_OSTYPE_EFI 0xEF
>>  #define EFI_PMBR_OSTYPE_EFI_GPT 0xEE
>>
>> @@ -111,7 +112,7 @@ typedef struct _gpt_entry {
>>  } __packed gpt_entry;
>>
>>  typedef struct _legacy_mbr {
>> -       u8 boot_code[440];
>> +       u8 boot_code[MSDOS_MBR_BOOT_CODE_SIZE];
>>         __le32 unique_mbr_signature;
>>         __le16 unknown;
>>         struct partition partition_record[4];
>> --
>> 2.17.0
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
>
>
>
> --
> With Best Regards,
> Andy Shevchenko

[U-Boot] disk: efi: Correct backing up the MBR boot code

[Tom Rini]

On Tue, May 22, 2018 at 02:04:21AM +0300, Sam Protsenko wrote:

> In commit e163a931af34 ("cmd: gpt: backup boot code before writing MBR")
> there was added the procedure for storing old boot code when doing "gpt
> write". But instead of storing just backup code, the whole MBR was
> stored, and only specific fields were replaced further, keeping
> everything else intact. That's obviously not what we want.
> 
> Fix the code to actually store only old boot code and zero out
> everything else. This fixes next testing case:
> 
>     => mmc write $loadaddr 0x0 0x7b
>     => gpt write mmc 1 $partitions
> 
> In case when $loadaddr address and further memory contains 0xff, the
> board was bricked (ROM-code probably didn't like partition entries that
> were clobbered with 0xff). With this patch applied, commands above don't
> brick the board.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> Cc: Alejandro Hernandez <ajhernandez@ti.com>
> Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180606/8d1d131e/attachment.sig>