[PATCH 0/2] efinet: correct closing of SNP protocol

[PATCH 0/2] efinet: correct closing of SNP protocol

[Heinrich Schuchardt]

In the context of the implementation of the EFI_LOAD_FILE2_PROTOCOL for
the initial ramdisk it was observed that opening the SNP protocol failed
(https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00020.html).
This is due to an incorrect call to CloseProtocol().

This is corrected in the first patch.

The second patch provides a new function grub_efi_close_protocol() to
simplify the coding.

Heinrich Schuchardt (2):
  efinet: correct closing of SNP protocol
  efi: library function grub_efi_close_protocol()

 grub-core/kern/efi/efi.c           | 18 ++++++++++++++++++
 grub-core/net/drivers/efi/efinet.c | 30 +++++++++++++++++++++---------
 include/grub/efi/efi.h             |  3 +++
 3 files changed, 42 insertions(+), 9 deletions(-)

-- 
2.32.0

[PATCH 1/2] efinet: correct closing of SNP protocol

[Heinrich Schuchardt]

In the context of the implementation of the EFI_LOAD_FILE2_PROTOCOL for
the initial ramdisk it was observed that opening the SNP protocol failed.
https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00020.html
This is due to an incorrect call to CloseProtocol().

The first parameter of CloseProtocol() is the handle, not the interface.

We call OpenProtocol() with ControllerHandle = NULL. Hence we must also
call CloseProtcol with ControllerHandel = NULL.

Each call of OpenProtocol() for the same network card handle is expected to
return the same interface pointer. If we want to close the protocol which
we opened non-exclusively when searching for a card, we have to do this
before opening the protocol exclusively.

As there is no guarantee that we successfully open the protocol add checks
in the transmit and receive functions.

Reported-by: Andreas Schwab <schwab@linux-m68k.org>
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 grub-core/net/drivers/efi/efinet.c | 32 ++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
index 5388f952b..3f2ff03f5 100644
--- a/grub-core/net/drivers/efi/efinet.c
+++ b/grub-core/net/drivers/efi/efinet.c
@@ -39,6 +39,8 @@ send_card_buffer (struct grub_net_card *dev,
   grub_uint64_t limit_time = grub_get_time_ms () + 4000;
   void *txbuf;
 
+  if (!net)
+    return grub_error (GRUB_ERR_IO, N_("couldn't send network packet"));
   if (dev->txbusy)
     while (1)
       {
@@ -101,6 +103,9 @@ get_card_packet (struct grub_net_card *dev)
   struct grub_net_buff *nb;
   int i;
 
+  if (!net)
+    return NULL;
+
   for (i = 0; i < 2; i++)
     {
       if (!dev->rcvbuf)
@@ -148,11 +153,23 @@ open_card (struct grub_net_card *dev)
 {
   grub_efi_simple_network_t *net;
 
-  /* Try to reopen SNP exlusively to close any active MNP protocol instance
-     that may compete for packet polling
+  if (dev->efi_net)
+    {
+      efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
+		  dev->efi_handle, &net_io_guid,
+		  grub_efi_image_handle, 0);
+      dev->efi_net = NULL;
+    }
+  /*
+   * Try to reopen SNP exlusively to close any active MNP protocol instance
+   * that may compete for packet polling
    */
   net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid,
 				GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE);
+  /* If exclusively fails, try non-exclusively. */
+  if (!net)
+      net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid,
+				    GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
   if (net)
     {
       if (net->mode->state == GRUB_EFI_NETWORK_STOPPED
@@ -192,13 +209,12 @@ open_card (struct grub_net_card *dev)
 	  efi_call_6 (net->receive_filters, net, filters, 0, 0, 0, NULL);
 	}
 
-      efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
-		  dev->efi_net, &net_io_guid,
-		  grub_efi_image_handle, dev->efi_handle);
       dev->efi_net = net;
+    } else {
+      return grub_error (GRUB_ERR_NET_NO_CARD, "%s: can't open protocol",
+			 dev->name);
     }
 
-  /* If it failed we just try to run as best as we can */
   return GRUB_ERR_NONE;
 }
 
@@ -208,8 +224,8 @@ close_card (struct grub_net_card *dev)
   efi_call_1 (dev->efi_net->shutdown, dev->efi_net);
   efi_call_1 (dev->efi_net->stop, dev->efi_net);
   efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
-	      dev->efi_net, &net_io_guid,
-	      grub_efi_image_handle, dev->efi_handle);
+	      dev->efi_handle, &net_io_guid,
+	      grub_efi_image_handle, 0);
 }
 
 static struct grub_net_card_driver efidriver =
-- 
2.32.0

[PATCH 2/2] efi: library function grub_efi_close_protocol()

[Heinrich Schuchardt]

Create a library function for CloseProtocol() and use it for the SNP
driver.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 grub-core/kern/efi/efi.c           | 18 ++++++++++++++++++
 grub-core/net/drivers/efi/efinet.c |  8 ++------
 include/grub/efi/efi.h             |  3 +++
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 8cff7be02..1d331d858 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -117,6 +117,24 @@ grub_efi_open_protocol (grub_efi_handle_t handle,
   return interface;
 }
 
+grub_efi_status_t
+grub_efi_close_protocol (grub_efi_handle_t handle,
+			 grub_efi_guid_t *protocol)
+{
+  grub_efi_boot_services_t *b;
+  grub_efi_status_t status;
+
+
+  b = grub_efi_system_table->boot_services;
+  status = efi_call_4 (b->close_protocol,
+		       handle,
+		       protocol,
+		       grub_efi_image_handle,
+		       0);
+
+  return status;
+}
+
 int
 grub_efi_set_text_mode (int on)
 {
diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
index 3f2ff03f5..2febbf84a 100644
--- a/grub-core/net/drivers/efi/efinet.c
+++ b/grub-core/net/drivers/efi/efinet.c
@@ -155,9 +155,7 @@ open_card (struct grub_net_card *dev)
 
   if (dev->efi_net)
     {
-      efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
-		  dev->efi_handle, &net_io_guid,
-		  grub_efi_image_handle, 0);
+      grub_efi_close_protocol (dev->efi_handle, &net_io_guid);
       dev->efi_net = NULL;
     }
   /*
@@ -223,9 +221,7 @@ close_card (struct grub_net_card *dev)
 {
   efi_call_1 (dev->efi_net->shutdown, dev->efi_net);
   efi_call_1 (dev->efi_net->stop, dev->efi_net);
-  efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
-	      dev->efi_handle, &net_io_guid,
-	      grub_efi_image_handle, 0);
+  grub_efi_close_protocol (dev->efi_handle, &net_io_guid);
 }
 
 static struct grub_net_card_driver efidriver =
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 83d958f99..12d041f2e 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -35,6 +35,9 @@ EXPORT_FUNC(grub_efi_locate_handle) (grub_efi_locate_search_type_t search_type,
 void *EXPORT_FUNC(grub_efi_open_protocol) (grub_efi_handle_t handle,
 					   grub_efi_guid_t *protocol,
 					   grub_efi_uint32_t attributes);
+grub_efi_status_t
+EXPORT_FUNC(grub_efi_close_protocol) (grub_efi_handle_t handle,
+				      grub_efi_guid_t *protocol);
 int EXPORT_FUNC(grub_efi_set_text_mode) (int on);
 void EXPORT_FUNC(grub_efi_stall) (grub_efi_uintn_t microseconds);
 void *
-- 
2.32.0

Re: [PATCH 0/2] efinet: correct closing of SNP protocol

[Andreas Schwab]

On Okt 06 2021, Heinrich Schuchardt wrote:

> In the context of the implementation of the EFI_LOAD_FILE2_PROTOCOL for
> the initial ramdisk it was observed that opening the SNP protocol failed
> (https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00020.html).
> This is due to an incorrect call to CloseProtocol().
>
> This is corrected in the first patch.
>
> The second patch provides a new function grub_efi_close_protocol() to
> simplify the coding.

Sucessfully tested with
https://download.opensuse.org/ports/riscv/tumbleweed/iso/openSUSE-Tumbleweed-NET-riscv64-Media.iso
on the Hifive Unmatched.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 1/2] efinet: correct closing of SNP protocol

[Daniel Kiper]

First of all, sorry for late reply but I am busy...

On Wed, Oct 06, 2021 at 10:30:42AM +0200, Heinrich Schuchardt wrote:
> In the context of the implementation of the EFI_LOAD_FILE2_PROTOCOL for
> the initial ramdisk it was observed that opening the SNP protocol failed.
> https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00020.html
> This is due to an incorrect call to CloseProtocol().
>
> The first parameter of CloseProtocol() is the handle, not the interface.
>
> We call OpenProtocol() with ControllerHandle = NULL. Hence we must also
> call CloseProtcol with ControllerHandel = NULL.
>
> Each call of OpenProtocol() for the same network card handle is expected to
> return the same interface pointer. If we want to close the protocol which
> we opened non-exclusively when searching for a card, we have to do this
> before opening the protocol exclusively.
>
> As there is no guarantee that we successfully open the protocol add checks
> in the transmit and receive functions.
>
> Reported-by: Andreas Schwab <schwab@linux-m68k.org>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  grub-core/net/drivers/efi/efinet.c | 32 ++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
> index 5388f952b..3f2ff03f5 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -39,6 +39,8 @@ send_card_buffer (struct grub_net_card *dev,
>    grub_uint64_t limit_time = grub_get_time_ms () + 4000;
>    void *txbuf;
>
> +  if (!net)

if (net == NULL)

> +    return grub_error (GRUB_ERR_IO, N_("couldn't send network packet"));

Could you display more specific error here?

>    if (dev->txbusy)
>      while (1)
>        {
> @@ -101,6 +103,9 @@ get_card_packet (struct grub_net_card *dev)
>    struct grub_net_buff *nb;
>    int i;
>
> +  if (!net)

if (net == NULL)

> +    return NULL;
> +
>    for (i = 0; i < 2; i++)
>      {
>        if (!dev->rcvbuf)
> @@ -148,11 +153,23 @@ open_card (struct grub_net_card *dev)
>  {
>    grub_efi_simple_network_t *net;
>
> -  /* Try to reopen SNP exlusively to close any active MNP protocol instance
> -     that may compete for packet polling
> +  if (dev->efi_net)

if (dev->efi_net != NULL)

... and please fix this in the other places in the patch(es) too...

> +    {
> +      efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
> +		  dev->efi_handle, &net_io_guid,
> +		  grub_efi_image_handle, 0);

s/0/NULL/

> +      dev->efi_net = NULL;
> +    }
> +  /*
> +   * Try to reopen SNP exlusively to close any active MNP protocol instance
> +   * that may compete for packet polling
>     */
>    net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid,
>  				GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE);
> +  /* If exclusively fails, try non-exclusively. */
> +  if (!net)
> +      net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid,
> +				    GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);

I am not convinced it is correct. Please take a look at send_card_buffer()...

46         st = efi_call_3 (net->get_status, net, 0, &txbuf);
47         if (st != GRUB_EFI_SUCCESS)
48           return grub_error (GRUB_ERR_IO,
49                              N_("couldn't send network packet"));
50         /*
51            Some buggy firmware could return an arbitrary address instead of the
52            txbuf address we trasmitted, so just check that txbuf is non NULL
53            for success.  This is ok because we open the SNP protocol in
54            exclusive mode so we know we're the only ones transmitting on this
    Here ---> ^^^^^^^^^
55            box and since we only transmit one packet at a time we know our
56            transmit was successfull.
57          */
58         if (txbuf)
59           {
60             dev->txbusy = 0;
61             break;
62           }

>    if (net)
>      {
>        if (net->mode->state == GRUB_EFI_NETWORK_STOPPED
> @@ -192,13 +209,12 @@ open_card (struct grub_net_card *dev)
>  	  efi_call_6 (net->receive_filters, net, filters, 0, 0, 0, NULL);
>  	}
>
> -      efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
> -		  dev->efi_net, &net_io_guid,
> -		  grub_efi_image_handle, dev->efi_handle);
>        dev->efi_net = net;
> +    } else {
> +      return grub_error (GRUB_ERR_NET_NO_CARD, "%s: can't open protocol",
> +			 dev->name);
>      }
>
> -  /* If it failed we just try to run as best as we can */
>    return GRUB_ERR_NONE;
>  }
>
> @@ -208,8 +224,8 @@ close_card (struct grub_net_card *dev)
>    efi_call_1 (dev->efi_net->shutdown, dev->efi_net);
>    efi_call_1 (dev->efi_net->stop, dev->efi_net);
>    efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
> -	      dev->efi_net, &net_io_guid,
> -	      grub_efi_image_handle, dev->efi_handle);
> +	      dev->efi_handle, &net_io_guid,
> +	      grub_efi_image_handle, 0);

s/0/NULL/

Daniel

Re: [PATCH 2/2] efi: library function grub_efi_close_protocol()

[Daniel Kiper]

On Wed, Oct 06, 2021 at 10:30:43AM +0200, Heinrich Schuchardt wrote:
> Create a library function for CloseProtocol() and use it for the SNP
> driver.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  grub-core/kern/efi/efi.c           | 18 ++++++++++++++++++
>  grub-core/net/drivers/efi/efinet.c |  8 ++------
>  include/grub/efi/efi.h             |  3 +++
>  3 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index 8cff7be02..1d331d858 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -117,6 +117,24 @@ grub_efi_open_protocol (grub_efi_handle_t handle,
>    return interface;
>  }
>
> +grub_efi_status_t
> +grub_efi_close_protocol (grub_efi_handle_t handle,
> +			 grub_efi_guid_t *protocol)

grub_efi_close_protocol (grub_efi_handle_t handle, grub_efi_guid_t *protocol)

> +{
> +  grub_efi_boot_services_t *b;
> +  grub_efi_status_t status;
> +
> +

Please drop this redundant empty line...

> +  b = grub_efi_system_table->boot_services;

I think you could do this above:

grub_efi_boot_services_t *b = grub_efi_system_table->boot_services;

> +  status = efi_call_4 (b->close_protocol,
> +		       handle,
> +		       protocol,
> +		       grub_efi_image_handle,
> +		       0);

status = efi_call_4 (b->close_protocol, handle, protocol, grub_efi_image_handle, NULL);

I am OK with lines a bit longer than 80 chars.

Otherwise LGTM.

Daniel