grub-install deleting long UEFI entries bug ?

grub-install deleting long UEFI entries bug ?

[adrian15]

grub-install seems to be deleting long UEFI entries

(*) What the bug is

* Add an UEFI entry with this label (Remove the single quotes):
 '(Rescapp added) \EFI\ubuntu\MokManager.efi'

Example:

efibootmgr -c \
 -d /dev/sda \
 -p 2 \
 -L '(Rescapp added) \EFI\ubuntu\MokManager.efi' \
 -l '\EFI\ubuntu\MokManager.efi'

* Run grub-install /dev/sda or maybe just grub-install

I expect the newly added uefi entry to be there.
What I find is that the entry has been lost or deleted!


(*) Exact version where is found

2.02~beta2-36ubuntu3.2 (Ubuntu 16.04's grub)

(*) Can be this replicated?

Can anyone replicate this is in upstream's git head?

Maybe is it a well known bug which was already fixed?


(*) Video of the bug

This is a video of the bug: https://www.youtube.com/watch?v=rhAg_ojj3VQ .

At 18 minutes 9 seconds I start to add an uefi entry.

At 21 minutes 3 seconds I run an option so that update-grub and
grub-install is run.

At 22 minutes 31 seconds I find the bug for the first time.

At 27 minutes 37 seconds I start to the debug the problem manually to
discard any of the Rescapp scripts being involved in the problem.

(*) What might be the problem.

I initially thought that the problem was because of '(', ')' or '\'
characters.

After additional tests there seems to be a problem with the length of
the UEFI boot entry.

Maybe grub-install uses efibootmgr as an auxiliar tool and the problem
is in Ubuntu's efibootmgr?

It would be nice if someone could point us on where does grub-install
handles the uefi boot entries so that we can take a deeper look into it.



Thank you!


adrian15
-- 
Support free software. Donate to Super Grub Disk. Apoya el software
libre. Dona a Super Grub Disk. http://www.supergrubdisk.org/donate/

Re: grub-install deleting long UEFI entries bug ?

[Andrei Borzenkov]

23.04.2017 03:54, adrian15 пишет:
> grub-install seems to be deleting long UEFI entries
> 
> (*) What the bug is
> 
> * Add an UEFI entry with this label (Remove the single quotes):
>  '(Rescapp added) \EFI\ubuntu\MokManager.efi'
> 
> Example:
> 
> efibootmgr -c \
>  -d /dev/sda \
>  -p 2 \
>  -L '(Rescapp added) \EFI\ubuntu\MokManager.efi' \
>  -l '\EFI\ubuntu\MokManager.efi'
> 
> * Run grub-install /dev/sda or maybe just grub-install
> 
> I expect the newly added uefi entry to be there.
> What I find is that the entry has been lost or deleted!
> 

What is value of GRUB_DISTRIBUTOR in /etc/default/grub?
...
> 
> Maybe grub-install uses efibootmgr as an auxiliar tool and the problem
> is in Ubuntu's efibootmgr?
> 

Yes.

> It would be nice if someone could point us on where does grub-install
> handles the uefi boot entries so that we can take a deeper look into it.
> 

grub-core/osdep/unix/platform.c

Re: grub-install deleting long UEFI entries bug ?

[adrian15 adrian15]

[-- Attachment #1: Type: text/plain, Size: 2822 bytes --]

2017-04-23 6:36 GMT+02:00 Andrei Borzenkov <arvidjaar@gmail.com>:

> 23.04.2017 03:54, adrian15 пишет:
> > grub-install seems to be deleting long UEFI entries
> >
> > (*) What the bug is
> >
> > * Add an UEFI entry with this label (Remove the single quotes):
> >  '(Rescapp added) \EFI\ubuntu\MokManager.efi'
> >
> > Example:
> >
> > efibootmgr -c \
> >  -d /dev/sda \
> >  -p 2 \
> >  -L '(Rescapp added) \EFI\ubuntu\MokManager.efi' \
> >  -l '\EFI\ubuntu\MokManager.efi'
> >
> > * Run grub-install /dev/sda or maybe just grub-install
> >
> > I expect the newly added uefi entry to be there.
> > What I find is that the entry has been lost or deleted!
> >
>
> What is value of GRUB_DISTRIBUTOR in /etc/default/grub?
>

After evaluating the bash expression the GRUB_DISTRIBUTOR value is Ubuntu.

> ...
> >
> > Maybe grub-install uses efibootmgr as an auxiliar tool and the problem
> > is in Ubuntu's efibootmgr?
> >
>
> Yes.
>

Yeah, I see it right in the source code. More to come.

>
> > It would be nice if someone could point us on where does grub-install
> > handles the uefi boot entries so that we can take a deeper look into it.
> >
>
> grub-core/osdep/unix/platform.c
>
Thank you.

I've taken a look at: grub-core/osdep/unix/platform.c file (on 2.02-rc2
tag) and I have some comments about it.

There's the function: grub_install_remove_efi_entries_by_distributor which
has some interesting snippets:

1) First of all this matches all the line:

if (!strcasestr (line, efi_distributor))
continue;

That means that if you add a custom label which matches the efi distributor
then it gets removed. I think that's what happened to me. I would prefer
something more precise that would check the complete efi file path agains
e.g. EFI/vendor/ .

2) Then there's:

      if (grub_memcmp (line, "Boot", sizeof ("Boot") - 1) != 0
 || line[sizeof ("Boot") - 1] < '0'
 || line[sizeof ("Boot") - 1] > '9')
continue;

which might be wrong because of 0 and 9 and maybe not because of the array
indexes.

Let's go into details about that.

2.1) Boot0000 First entry
BootA000 Second entry

Shouldn't the look for A to F hexadecimal letters too?

And...

2.2) line[sizeof ("Boot") - 1] < '0'

Am I doing it right?

sizeof ("Boot") = 4

So it's line [4 - 1] and therefore: line [3] .
And as a consequence... line[3] = 't'.

line[3] is always going to be 't' when the first OR condition is not
true...so no need of the third or the fourth condition then.

Or is there  something about indexes or character size (unicode being
16bit?) that I am missing?



Thank you.

adrian15
-- 
Support free software. Donate to Super Grub Disk. Apoya el software libre.
Dona a Super Grub Disk. http://www.supergrubdisk.org/donate/ .

[-- Attachment #2: Type: text/html, Size: 5040 bytes --]

Re: grub-install deleting long UEFI entries bug ?

[Andrei Borzenkov]

23.04.2017 11:21, adrian15 adrian15 пишет:
> 2017-04-23 6:36 GMT+02:00 Andrei Borzenkov <arvidjaar@gmail.com>:
> 
>> 23.04.2017 03:54, adrian15 пишет:
>>> grub-install seems to be deleting long UEFI entries
>>>
>>> (*) What the bug is
>>>
>>> * Add an UEFI entry with this label (Remove the single quotes):
>>>  '(Rescapp added) \EFI\ubuntu\MokManager.efi'
>>>
>>> Example:
>>>
>>> efibootmgr -c \
>>>  -d /dev/sda \
>>>  -p 2 \
>>>  -L '(Rescapp added) \EFI\ubuntu\MokManager.efi' \
>>>  -l '\EFI\ubuntu\MokManager.efi'
>>>
>>> * Run grub-install /dev/sda or maybe just grub-install
>>>
>>> I expect the newly added uefi entry to be there.
>>> What I find is that the entry has been lost or deleted!
>>>
>>
>> What is value of GRUB_DISTRIBUTOR in /etc/default/grub?
>>
> 
> After evaluating the bash expression the GRUB_DISTRIBUTOR value is Ubuntu.
> 

Yes, historically grub did case insensitive substring search. This
probably is wrong, we should just take everything after boot number
literally.
...
> 1) First of all this matches all the line:
> 
> if (!strcasestr (line, efi_distributor))
> continue;
> 
> That means that if you add a custom label which matches the efi distributor
> then it gets removed. I think that's what happened to me. I would prefer
> something more precise that would check the complete efi file path agains
> e.g. EFI/vendor/ .
> 
> 2) Then there's:
> 
>       if (grub_memcmp (line, "Boot", sizeof ("Boot") - 1) != 0
>  || line[sizeof ("Boot") - 1] < '0'
>  || line[sizeof ("Boot") - 1] > '9')
> continue;
> 
> which might be wrong because of 0 and 9 and maybe not because of the array
> indexes.
> 
> Let's go into details about that.
> 
> 2.1) Boot0000 First entry
> BootA000 Second entry
> 
> Shouldn't the look for A to F hexadecimal letters too?
> 

Yes. Patches are welcome for both problems. Second one is actually bug
fix so should be independent.

> And...
> 
> 2.2) line[sizeof ("Boot") - 1] < '0'
> 
> Am I doing it right?
> 
> sizeof ("Boot") = 4
> 

It is 5.

Re: grub-install deleting long UEFI entries bug ?

[adrian15]

El 23/04/17 a las 10:45, Andrei Borzenkov escribió:
> 23.04.2017 11:21, adrian15 adrian15 пишет:
>> 2017-04-23 6:36 GMT+02:00 Andrei Borzenkov <arvidjaar@gmail.com>:
>>
>>> 23.04.2017 03:54, adrian15 пишет:
>>>> grub-install seems to be deleting long UEFI entries
>>>>
>>>> (*) What the bug is
>>>>
>>>> * Add an UEFI entry with this label (Remove the single quotes):
>>>>  '(Rescapp added) \EFI\ubuntu\MokManager.efi'
>>>>
>>>> Example:
>>>>
>>>> efibootmgr -c \
>>>>  -d /dev/sda \
>>>>  -p 2 \
>>>>  -L '(Rescapp added) \EFI\ubuntu\MokManager.efi' \
>>>>  -l '\EFI\ubuntu\MokManager.efi'
>>>>
>>>> * Run grub-install /dev/sda or maybe just grub-install
>>>>
>>>> I expect the newly added uefi entry to be there.
>>>> What I find is that the entry has been lost or deleted!
>>>>
>>>
>>> What is value of GRUB_DISTRIBUTOR in /etc/default/grub?
>>>
>>
>> After evaluating the bash expression the GRUB_DISTRIBUTOR value is Ubuntu.
>>
> 
> Yes, historically grub did case insensitive substring search. This
> probably is wrong, we should just take everything after boot number
> literally.

I see, like removing what you are about to add I guess.
The problem that I see is that efibootmgr output (even if --verbose
switch) it's not machine readable.

I guess efibootmgr itself would need an specific switch in order to
produce output suitable for scripts. Another option is include some of
the efibootmgr functionality/libraries into grub itself.

Maybe there's something on upstream's efibootmgr. Not a clue about that.
I have only checked Debian stretch's efibootmgr. I might ask about it in
debian-efi mailing list.

> ...
>> 1) First of all this matches all the line:
>>
>> if (!strcasestr (line, efi_distributor))
>> continue;
>>
>> That means that if you add a custom label which matches the efi distributor
>> then it gets removed. I think that's what happened to me. I would prefer
>> something more precise that would check the complete efi file path agains
>> e.g. EFI/vendor/ .
>>
>> 2) Then there's:
>>
>>       if (grub_memcmp (line, "Boot", sizeof ("Boot") - 1) != 0
>>  || line[sizeof ("Boot") - 1] < '0'
>>  || line[sizeof ("Boot") - 1] > '9')
>> continue;
>>
>> which might be wrong because of 0 and 9 and maybe not because of the array
>> indexes.
>>
>> Let's go into details about that.
>>
>> 2.1) Boot0000 First entry
>> BootA000 Second entry
>>
>> Shouldn't the look for A to F hexadecimal letters too?
>>
> 
> Yes. Patches are welcome for both problems. Second one is actually bug
> fix so should be independent.
> 
>> And...

Well, I think just checking 0 to 9 in the first character is a good
compromise.

Some outputs have: BootCurrent . So 'BootC' can be found in e.g.
'BootC001' too. So that would be adding another problem because
'BootCurrent' would be considered as a right entry.

Just checking the first character leaves place for 16^3 = (2^4)^3
= 2 ^ (4 * 3 ) = 2 ^12 =  4096 .

That should be enough for most of the usecases.


>>
>> 2.2) line[sizeof ("Boot") - 1] < '0'
>>
>> Am I doing it right?
>>
>> sizeof ("Boot") = 4
>>
> 
> It is 5.
Ok, yes, sizeof is not length so... it shows what it takes to save it.
So... 4 bytes and the 'finish string byte' so that makes 5.


Well, I have finally decided not to put the full path to efi file and
only the basename of it. That will avoid custom entries being suddenly
removed by grub-install.

Thank you for your feedback.


adrian15
-- 
Support free software. Donate to Super Grub Disk. Apoya el software
libre. Dona a Super Grub Disk. http://www.supergrubdisk.org/donate/

Re: grub-install deleting long UEFI entries bug ?

[Andrei Borzenkov]

23.04.2017 23:33, adrian15 пишет:
> El 23/04/17 a las 10:45, Andrei Borzenkov escribió:
>> 23.04.2017 11:21, adrian15 adrian15 пишет:
>>> 2017-04-23 6:36 GMT+02:00 Andrei Borzenkov <arvidjaar@gmail.com>:
>>>
>>>> 23.04.2017 03:54, adrian15 пишет:
>>>>> grub-install seems to be deleting long UEFI entries
>>>>>
>>>>> (*) What the bug is
>>>>>
>>>>> * Add an UEFI entry with this label (Remove the single quotes):
>>>>>  '(Rescapp added) \EFI\ubuntu\MokManager.efi'
>>>>>
>>>>> Example:
>>>>>
>>>>> efibootmgr -c \
>>>>>  -d /dev/sda \
>>>>>  -p 2 \
>>>>>  -L '(Rescapp added) \EFI\ubuntu\MokManager.efi' \
>>>>>  -l '\EFI\ubuntu\MokManager.efi'
>>>>>
>>>>> * Run grub-install /dev/sda or maybe just grub-install
>>>>>
>>>>> I expect the newly added uefi entry to be there.
>>>>> What I find is that the entry has been lost or deleted!
>>>>>
>>>>
>>>> What is value of GRUB_DISTRIBUTOR in /etc/default/grub?
>>>>
>>>
>>> After evaluating the bash expression the GRUB_DISTRIBUTOR value is Ubuntu.
>>>
>>
>> Yes, historically grub did case insensitive substring search. This
>> probably is wrong, we should just take everything after boot number
>> literally.
> 
> I see, like removing what you are about to add I guess.
> The problem that I see is that efibootmgr output (even if --verbose
> switch) it's not machine readable.
> 

We are using plain "efibootmgr" without --verbose and output is variable
name followed by either '*' or space followed by space followed by
description if present. Unless description contains newline, it looks
straightforward to parse.

> I guess efibootmgr itself would need an specific switch in order to
> produce output suitable for scripts.

It would need somehow escape newline

> Another option is include some of
> the efibootmgr functionality/libraries into grub itself.
> 

Yes, I have half-done patch but as I see now it is probably not really
needed unless we actually have case of description containing newlines.

>>>
>>> 2) Then there's:
>>>
>>>       if (grub_memcmp (line, "Boot", sizeof ("Boot") - 1) != 0
>>>  || line[sizeof ("Boot") - 1] < '0'
>>>  || line[sizeof ("Boot") - 1] > '9')
>>> continue;
>>>
>>> which might be wrong because of 0 and 9 and maybe not because of the array
>>> indexes.
>>>
>>> Let's go into details about that.
>>>
>>> 2.1) Boot0000 First entry
>>> BootA000 Second entry
>>>
>>> Shouldn't the look for A to F hexadecimal letters too?
>>>
>>
>> Yes. Patches are welcome for both problems. Second one is actually bug
>> fix so should be independent.
>>
>>> And...
> 
> Well, I think just checking 0 to 9 in the first character is a good
> compromise.
> 
> Some outputs have: BootCurrent . So 'BootC' can be found in e.g.
> 'BootC001' too. So that would be adding another problem because
> 'BootCurrent' would be considered as a right entry.
> 

This simply means that we have to check for exactly 4 hexadecimal
numbers, not shortcut this.