Joshua.Weage at dell.com
2017-Nov-21 21:55 UTC
[syslinux] [PATCH] EFI TCP buffer reuse bug
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>
---
--- 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;
};
--- 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;
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>(OP is not on list) Thanks for the patch. Marked for review. -- -Gene
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