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