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

[Sam Protsenko]

On 23 May 2018 at 12:51, Andy Shevchenko  wrote:
> On Tue, May 22, 2018 at 2:04 AM, 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.
>
> Tested-by: Andy Shevchenko 
>
> 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 
>> Cc: Alejandro Hernandez 
>> ---
>>  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 
>>
>>  #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@lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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
 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 

At least it doesn't break Intel Edison.

>
> Signed-off-by: Sam Protsenko 
> Cc: Alejandro Hernandez 
> ---
>  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 
>
>  #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@lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
With Best Regards,
Andy Shevchenko
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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
 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 
> Cc: Alejandro Hernandez 
> ---
>  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 
>
>  #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@lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
With Best Regards,
Andy Shevchenko
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot