[PATCH 1/1] test: stabilize test_efi_secboot

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Heinrich,

On Fri, May 08, 2020 at 08:10:28AM +0900, AKASHI Takahiro wrote:
> On Thu, May 07, 2020 at 11:14:17PM +0200, Heinrich Schuchardt wrote:
> > On 5/7/20 2:36 AM, AKASHI Takahiro wrote:
> > > Heinrich,
> > >
> > > On Mon, May 04, 2020 at 12:33:26PM +0200, Heinrich Schuchardt wrote:
> > >> When setting up the console via function efi_console_register() we call
> > >> query_console_serial(). This functions sends an escape sequence to the
> > >> terminal to query the display size. The response is another escape
> > >> sequence.
> > >>
> > >> console.run_command_list() is looking for a regular expression '^==>'.
> > >> If the escape sequence for the screen size precedes the prompt without a
> > >> line break, no match is found.
> > >>
> > >> When efi_disk_register() is called before efi_console_register() this leads
> > >> to a test failuere of the UEFI secure boot tests.
> > >
> > > Why does the order of those calls affect test results?
> > 
> > Please, have a look at function query_console_serial() and at
> > run_command_list().
> > 
> > As described above:
> > '\e7\e[r\e[999;999H\e[6n\e8==>' cannot be matched by regular expression
> > '^==>'.
> 
> (Probably) right, but what I don't get here is why efi_disk_register()
> have an impact on efi_console_register(). The former function has
> nothing to do with any console behaviors.

You have merged your patch without replying to my comment.

> Moreover, I wonder
> - why you want to move efi_console_register() after efi_disk_register().
> - why python script can see such escape sequences.

I don't like your fix.
With your fixes, my secure boot pytest now fails to run
on sandbox locally.

Instead, I propose:
1. revert your commits
   commit 16ad946f41d3 ("efi_loader: change setup sequence")
   commit 5827c2545849 ("test: stabilize test_efi_secboot")
2. move efi_console_register() forward *before* efi_console_register()


Then my secure boot test should work again without any modification.
I believe that my solution is much better than your workaround.

-Takahiro Akashi

> > 
> > >
> > >> We can avoid the problem if the first UEFI command passed to
> > >> u_boot_console.run_command_list() produces output. This patch achieves this
> > >> by appending '; echo' to the first UEFI related command of the problematic
> > >> tests.
> > >
> > > It looks to be a workaround.
> > > We'd better have another approach to fix the problem.
> > > Otherwise, similar issues will occur again.
> > 
> > I would not like to change the logic in Python to detect the U-Boot
> > prompt for something UEFI specific. And we need a method to determine
> > the size of a serial console.
> > 
> > The current approach allowed me to merge your patch series which
> > otherwise might not have reached the v2020.07 release. Did the problem
> > not show up in your Travis CI testing?
> 
> No. If your saying is correct, this can happen only if efi_console_register()
> is moved after efi_disk_register(). Right?
> 
> > If you have a better solution, we can easily merge your patch.
> 
> First, I want to understand what's happening.
> 
> -Takahiro Akashi
> 
> > Best regards
> > 
> > Heinrich
> > 
> > >
> > > Thanks,
> > > -Takahiro Akashi
> > >
> > >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > >> ---
> > >>  test/py/tests/test_efi_secboot/test_authvar.py  | 8 ++++----
> > >>  test/py/tests/test_efi_secboot/test_signed.py   | 4 ++--
> > >>  test/py/tests/test_efi_secboot/test_unsigned.py | 6 +++---
> > >>  3 files changed, 9 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/test/py/tests/test_efi_secboot/test_authvar.py b/test/py/tests/test_efi_secboot/test_authvar.py
> > >> index 55dcaa95f1..9912694a3e 100644
> > >> --- a/test/py/tests/test_efi_secboot/test_authvar.py
> > >> +++ b/test/py/tests/test_efi_secboot/test_authvar.py
> > >> @@ -133,7 +133,7 @@ class TestEfiAuthVar(object):
> > >>              output = u_boot_console.run_command_list([
> > >>                  'host bind 0 %s' % disk_img,
> > >>                  'fatload host 0:1 4000000 PK.auth',
> > >> -                'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK',
> > >> +                'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK; echo',
> > >>                  'fatload host 0:1 4000000 KEK.auth',
> > >>                  'setenv -e -nv -bs -rt -at -i 4000000,$filesize KEK',
> > >>                  'fatload host 0:1 4000000 db.auth',
> > >> @@ -174,7 +174,7 @@ class TestEfiAuthVar(object):
> > >>              output = u_boot_console.run_command_list([
> > >>                  'host bind 0 %s' % disk_img,
> > >>                  'fatload host 0:1 4000000 PK.auth',
> > >> -                'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK',
> > >> +                'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK; echo',
> > >>                  'fatload host 0:1 4000000 KEK.auth',
> > >>                  'setenv -e -nv -bs -rt -at -i 4000000,$filesize KEK',
> > >>                  'fatload host 0:1 4000000 db.auth',
> > >> @@ -215,7 +215,7 @@ class TestEfiAuthVar(object):
> > >>              output = u_boot_console.run_command_list([
> > >>                  'host bind 0 %s' % disk_img,
> > >>                  'fatload host 0:1 4000000 PK.auth',
> > >> -                'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK',
> > >> +                'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK; echo',
> > >>                  'fatload host 0:1 4000000 KEK.auth',
> > >>                  'setenv -e -nv -bs -rt -at -i 4000000,$filesize KEK',
> > >>                  'fatload host 0:1 4000000 db.auth',
> > >> @@ -249,7 +249,7 @@ class TestEfiAuthVar(object):
> > >>              output = u_boot_console.run_command_list([
> > >>                  'host bind 0 %s' % disk_img,
> > >>                  'fatload host 0:1 4000000 PK.auth',
> > >> -                'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK',
> > >> +                'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK; echo',
> > >>                  'fatload host 0:1 4000000 KEK.auth',
> > >>                  'setenv -e -nv -bs -rt -at -i 4000000,$filesize KEK',
> > >>                  'fatload host 0:1 4000000 db.auth',
> > >> diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
> > >> index 584282b338..fc722ab506 100644
> > >> --- a/test/py/tests/test_efi_secboot/test_signed.py
> > >> +++ b/test/py/tests/test_efi_secboot/test_signed.py
> > >> @@ -29,7 +29,7 @@ class TestEfiSignedImage(object):
> > >>              # Test Case 1a, run signed image if no db/dbx
> > >>              output = u_boot_console.run_command_list([
> > >>                  'host bind 0 %s' % disk_img,
> > >> -                'efidebug boot add 1 HELLO1 host 0:1 /helloworld.efi.signed ""',
> > >> +                'efidebug boot add 1 HELLO1 host 0:1 /helloworld.efi.signed ""; echo',
> > >>                  'efidebug boot next 1',
> > >>                  'bootefi bootmgr'])
> > >>              assert(re.search('Hello, world!', ''.join(output)))
> > >> @@ -81,7 +81,7 @@ class TestEfiSignedImage(object):
> > >>              output = u_boot_console.run_command_list([
> > >>                  'host bind 0 %s' % disk_img,
> > >>                  'fatload host 0:1 4000000 db.auth',
> > >> -                'setenv -e -nv -bs -rt -at -i 4000000,$filesize dbx',
> > >> +                'setenv -e -nv -bs -rt -at -i 4000000,$filesize dbx; echo',
> > >>                  'fatload host 0:1 4000000 KEK.auth',
> > >>                  'setenv -e -nv -bs -rt -at -i 4000000,$filesize KEK',
> > >>                  'fatload host 0:1 4000000 PK.auth',
> > >> diff --git a/test/py/tests/test_efi_secboot/test_unsigned.py b/test/py/tests/test_efi_secboot/test_unsigned.py
> > >> index 22d849afb8..a4af845c51 100644
> > >> --- a/test/py/tests/test_efi_secboot/test_unsigned.py
> > >> +++ b/test/py/tests/test_efi_secboot/test_unsigned.py
> > >> @@ -30,7 +30,7 @@ class TestEfiUnsignedImage(object):
> > >>              output = u_boot_console.run_command_list([
> > >>                  'host bind 0 %s' % disk_img,
> > >>  		'fatload host 0:1 4000000 KEK.auth',
> > >> -		'setenv -e -nv -bs -rt -at -i 4000000,$filesize KEK',
> > >> +		'setenv -e -nv -bs -rt -at -i 4000000,$filesize KEK; echo',
> > >>  		'fatload host 0:1 4000000 PK.auth',
> > >>  		'setenv -e -nv -bs -rt -at -i 4000000,$filesize PK'])
> > >>              assert(not re.search('Failed to set EFI variable', ''.join(output)))
> > >> @@ -58,7 +58,7 @@ class TestEfiUnsignedImage(object):
> > >>              output = u_boot_console.run_command_list([
> > >>                  'host bind 0 %s' % disk_img,
> > >>  		'fatload host 0:1 4000000 db_hello.auth',
> > >> -		'setenv -e -nv -bs -rt -at -i 4000000,$filesize db',
> > >> +		'setenv -e -nv -bs -rt -at -i 4000000,$filesize db; echo',
> > >>  		'fatload host 0:1 4000000 KEK.auth',
> > >>  		'setenv -e -nv -bs -rt -at -i 4000000,$filesize KEK',
> > >>  		'fatload host 0:1 4000000 PK.auth',
> > >> @@ -82,7 +82,7 @@ class TestEfiUnsignedImage(object):
> > >>              output = u_boot_console.run_command_list([
> > >>                  'host bind 0 %s' % disk_img,
> > >>  		'fatload host 0:1 4000000 db_hello.auth',
> > >> -		'setenv -e -nv -bs -rt -at -i 4000000,$filesize dbx',
> > >> +		'setenv -e -nv -bs -rt -at -i 4000000,$filesize dbx; echo',
> > >>  		'fatload host 0:1 4000000 KEK.auth',
> > >>  		'setenv -e -nv -bs -rt -at -i 4000000,$filesize KEK',
> > >>  		'fatload host 0:1 4000000 PK.auth',
> > >> --
> > >> 2.26.2
> > >>
> >