[syslinux] [PATCH] EFI TCP buffer reuse bug

[Gene Cumm]

On Tue, Nov 21, 2017 at 4:55 PM, Joshua.Weage--- via Syslinux
<syslinux at zytor.com> wrote:
> The attached patch resolves a buffer reuse bug in the efi/tcp.c code. The bug can be triggered when multiple TCP connections are opened when using the EFI bootloader.
>
> Signed-off-by: Joshua Weage <joshua.weage at dell.com>
>
> ---

Re-ordering to help discussion.

> --- master/efi/tcp.c    2017-07-22 14:45:46.628674366 -0500
> +++ jw/efi/tcp.c        2017-07-24 09:24:37.072922478 -0500
> @@ -199,8 +199,6 @@ void core_tcp_close_file(struct inode *i
>      socket->net.efi.binding = NULL;
>  }-
>
> -static char databuf[8192];
> -
>  void core_tcp_fill_buffer(struct inode *inode)
>  {
>      struct pxe_pvt_inode *socket = PVT(inode);
> @@ -210,7 +208,6 @@ void core_tcp_fill_buffer(struct inode *
>      EFI_TCP4_FRAGMENT_DATA *frag;
>      EFI_STATUS status;
>      EFI_TCP4 *tcp = (EFI_TCP4 *)b->this;
> -    void *data;
>      size_t len;
>
>      memset(&iotoken, 0, sizeof(iotoken));
> @@ -223,10 +220,10 @@ void core_tcp_fill_buffer(struct inode *
>
>      iotoken.Packet.RxData = &rxdata;
>      rxdata.FragmentCount = 1;
> -    rxdata.DataLength = sizeof(databuf);
> +    rxdata.DataLength = sizeof(socket->tcp_databuf);
>      frag = &rxdata.FragmentTable[0];
> -    frag->FragmentBuffer = databuf;
> -    frag->FragmentLength = sizeof(databuf);
> +    frag->FragmentBuffer = socket->tcp_databuf;
> +    frag->FragmentLength = sizeof(socket->tcp_databuf);
>
>      status = uefi_call_wrapper(tcp->Receive, 2, tcp, &iotoken);
>      if (status == EFI_CONNECTION_FIN) {
> @@ -244,10 +241,8 @@ void core_tcp_fill_buffer(struct inode *
>      cb_status = -1;
>
>      len = frag->FragmentLength;
> -    memcpy(databuf, frag->FragmentBuffer, len);
> -    data = databuf;
>
> -    socket->tftp_dataptr = data;
> +    socket->tftp_dataptr = socket->tcp_databuf;
>      socket->tftp_filepos += len;
>      socket->tftp_bytesleft = len;

The idea of this makes a lot of sense: Don't potentially re-use the
same buffer across all of Syslinux when a concurrency issue might
arise.

> --- master/core/fs/pxe/pxe.h    2017-07-22 14:45:46.561674329 -0500
> +++ jw/core/fs/pxe/pxe.h        2017-07-23 19:44:46.466498950 -0500
> @@ -144,6 +144,7 @@ struct pxe_pvt_inode {
>      uint8_t  tftp_goteof;         /* 1 if the EOF packet received */
>      uint8_t  tftp_unused[3];      /* Currently unused */
>      char    *tftp_pktbuf;         /* Packet buffer */
> +    char     tcp_databuf[8192];   /* data buffer */
>      struct inode *ctl;           /* Control connection (for FTP) */
>      const struct pxe_conn_ops *ops;
>  };

Why are we forcibly allocating an 8kiB buffer in a struct that won't
be used on a non-lwIP PXELINUX?  Why not either re-use the existing
buffer pointer, perhaps renaming it more generically and storing its
length since we won't be using the same pxe_pvt_inode struct for a
TFTP transaction and a TCP transaction?

If you'd like, I can propose an alternative patch.

-- 
-Gene