Re: [PATCH v3 08/10] net: dhcp: allow receiving DHCP OFFER and ACK packets

[Daniel Kiper]

On Thu, Mar 07, 2019 at 03:14:14PM +0000, Andre Przywara wrote:
> From: Andrei Borzenkov <address@hidden>
>
> In respone to a BOOTREQUEST packet a BOOTP server would answer with a
> BOOTREPLY packet, which ends the conversation for good.
> DHCP uses a 4-way handshake, where the initial server respone is an OFFER,
> which has to be answered with REQUEST by the client again, only to be
> completed by an ACKNOWLEDGE packet from the server.
>
> Teach the grub_net_process_dhcp() function to deal with OFFER packets,
> and treat ACK packets the same es BOOTREPLY packets.
>
> Signed-off-by: Andre Przywara <address@hidden>
> Reviewed-by: Daniel Kiper <address@hidden>

This patch has changed and you retained my RB and not explained why it
has changed. Could you tell us what has happened? Next time please drop
RB if you change the code significantly.

Hmmm... It seems to me that at least partially this is due to change in
patch #5.

Daniel

> ---
>  grub-core/net/bootp.c | 78 +++++++++++++++++++++++++++++++++++--------
>  include/grub/net.h    |  5 +++
>  2 files changed, 70 insertions(+), 13 deletions(-)
>
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index bfde6ef10..d0482a7d5 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -30,6 +30,21 @@ enum
>    GRUB_DHCP_OPT_OVERLOAD_FILE = 1,
>    GRUB_DHCP_OPT_OVERLOAD_SNAME = 2,
>  };
> +enum
> +{
> +  GRUB_DHCP_MESSAGE_UNKNOWN,
> +  GRUB_DHCP_MESSAGE_DISCOVER,
> +  GRUB_DHCP_MESSAGE_OFFER,
> +  GRUB_DHCP_MESSAGE_REQUEST,
> +  GRUB_DHCP_MESSAGE_DECLINE,
> +  GRUB_DHCP_MESSAGE_ACK,
> +  GRUB_DHCP_MESSAGE_NAK,
> +  GRUB_DHCP_MESSAGE_RELEASE,
> +  GRUB_DHCP_MESSAGE_INFORM,
> +};
> +
> +/* Max timeout when waiting for BOOTP/DHCP reply */
> +#define GRUB_DHCP_MAX_PACKET_TIMEOUT 32
>
>  #define GRUB_BOOTP_MAX_OPTIONS_SIZE 64
>
> @@ -443,22 +458,59 @@ grub_net_process_dhcp (struct grub_net_buff *nb,
>    struct grub_net_card *card = iface->card;
>    const struct grub_net_bootp_packet *bp = (const struct 
> grub_net_bootp_packet *) nb->data;
>    grub_size_t size = nb->tail - nb->data;
> +  const grub_uint8_t *opt;
> +  grub_uint8_t opt_len, type;
> +  grub_uint32_t srv_id = 0;
> +
> +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_MESSAGE_TYPE, &opt_len);
> +  if (opt && opt_len == 1)
> +    type = *opt;
> +  else
> +    type = GRUB_DHCP_MESSAGE_UNKNOWN;
> +
> +  opt = find_dhcp_option (bp, size, GRUB_NET_DHCP_SERVER_IDENTIFIER, 
> &opt_len);
> +  if (opt && opt_len == sizeof (srv_id))
> +    srv_id = grub_get_unaligned32 (opt);
>
> -  name = grub_xasprintf ("%s:dhcp", card->name);
> -  if (!name)
> +  /*
> +   * If we received BOOTP reply or DHCPACK, proceed with configuration.
> +   * Otherwise store offered address and server id for later processing
> +   * of DHCPACK.
> +   * xid and srv_id are stored in network order so do not need conversion.
> +   */
> +  if ((!iface->srv_id && type == GRUB_DHCP_MESSAGE_UNKNOWN)
> +      || (iface->srv_id && type == GRUB_DHCP_MESSAGE_ACK
> +       && bp->ident == iface->xid
> +       && srv_id == iface->srv_id))
>      {
> -      grub_print_error ();
> -      return;
> +      name = grub_xasprintf ("%s:dhcp", card->name);
> +      if (!name)
> +     {
> +       grub_print_error ();
> +       return;
> +     }
> +      grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
> +      grub_free (name);
> +      if (grub_errno)
> +     grub_print_error ();
> +      else
> +     grub_net_network_level_interface_unregister (iface);
> +    }
> +  else if (!iface->srv_id && type == GRUB_DHCP_MESSAGE_OFFER && srv_id)
> +    {
> +      iface->srv_id = srv_id;
> +      iface->my_ip = bp->your_ip;
> +      /* Reset retransmission timer */
> +      iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
> +    }
> +  else if (iface->srv_id && type == GRUB_DHCP_MESSAGE_NAK
> +        && bp->ident == iface->xid
> +        && srv_id == iface->srv_id)
> +    {
> +      iface->xid = iface->srv_id = iface->my_ip = 0;
> +      /* Reset retransmission timer */
> +      iface->dhcp_tmo = iface->dhcp_tmo_left = 1;
>      }
> -  grub_net_configure_by_dhcp_ack (name, card, 0, bp, size, 0, 0, 0);
> -  grub_free (name);
> -  if (grub_errno)
> -    grub_print_error ();
> -  else
> -    if (grub_memcmp (iface->name, card->name, grub_strlen (card->name)) == 0 
> &&
> -     grub_memcmp (iface->name + grub_strlen (card->name),
> -                       ":dhcp_tmp", sizeof (":dhcp_tmp") - 1) == 0)
> -      grub_net_network_level_interface_unregister (iface);
>  }
>
>  static char
> diff --git a/include/grub/net.h b/include/grub/net.h
> index f7b546446..68a9f1311 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -292,6 +292,9 @@ struct grub_net_network_level_interface
>    struct grub_net_bootp_packet *dhcp_ack;
>    grub_size_t dhcp_acklen;
>    grub_uint16_t vlantag;
> +  grub_uint32_t xid;      /* DHCPv4 transaction id */
> +  grub_uint32_t srv_id;   /* DHCPv4 server_identifier */
> +  grub_uint32_t my_ip;    /* DHCPv4 offered IP address */
>    unsigned dhcp_tmo_left; /* DHCPv4 running retransmission timeout */
>    unsigned dhcp_tmo;      /* DHCPv4 current retransmission timeout */
>    void *data;
> @@ -460,6 +463,8 @@ enum
>      GRUB_NET_BOOTP_ROOT_PATH = 0x11,
>      GRUB_NET_BOOTP_EXTENSIONS_PATH = 0x12,
>      GRUB_NET_DHCP_OVERLOAD = 52,
> +    GRUB_NET_DHCP_MESSAGE_TYPE = 53,
> +    GRUB_NET_DHCP_SERVER_IDENTIFIER = 54,
>      GRUB_NET_DHCP_TFTP_SERVER_NAME = 66,
>      GRUB_NET_DHCP_BOOTFILE_NAME = 67,
>      GRUB_NET_BOOTP_END = 0xff
> --
> 2.17.1