| Message ID | 20221126161056.127376-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [1/1] efi_loader: fix handling of DHCP acknowledge | expand |
Hi Heinrich, On Sat, Nov 26, 2022 at 05:10:56PM +0100, Heinrich Schuchardt wrote: > The dhcp command may be executed after the first UEFI command. > We should still update the EFI_PXE_BASE_CODE_PROTOCOL. > > Don't leak content of prior acknowledge packages. > > Handle out of memory. > The patch looks correct, but the description is a bit confusing. Apart from what you mention this patch also fixes - An unchecked allocation - shadowing of the global netobj Can we please update the commit message to something that mentions all of these? Thanks /Ilias > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > lib/efi_loader/efi_net.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c > index 69276b275d..96a5bcca27 100644 > --- a/lib/efi_loader/efi_net.c > +++ b/lib/efi_loader/efi_net.c > @@ -30,6 +30,7 @@ static uchar **receive_buffer; > static size_t *receive_lengths; > static int rx_packet_idx; > static int rx_packet_num; > +static struct efi_net_obj *netobj; > > /* > * The notification function of this event is called in every timer cycle > @@ -660,10 +661,16 @@ void efi_net_set_dhcp_ack(void *pkt, int len) > { > int maxsize = sizeof(*dhcp_ack); > > - if (!dhcp_ack) > + if (!dhcp_ack) { > dhcp_ack = malloc(maxsize); > - > + if (!dhcp_ack) > + return; > + } > + memset(dhcp_ack, 0, maxsize); > memcpy(dhcp_ack, pkt, min(len, maxsize)); > + > + if (netobj) > + netobj->pxe_mode.dhcp_ack = *dhcp_ack; > } > > /** > @@ -853,7 +860,6 @@ static efi_status_t EFIAPI efi_pxe_base_code_set_packets( > */ > efi_status_t efi_net_register(void) > { > - struct efi_net_obj *netobj = NULL; > efi_status_t r; > int i; > > @@ -982,6 +988,7 @@ failure_to_add_protocol: > return r; > out_of_resources: > free(netobj); > + netobj = NULL; > free(transmit_buffer); > if (receive_buffer) > for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) > -- > 2.37.2 >
On 11/30/22 08:37, Ilias Apalodimas wrote: > Hi Heinrich, > > On Sat, Nov 26, 2022 at 05:10:56PM +0100, Heinrich Schuchardt wrote: >> The dhcp command may be executed after the first UEFI command. >> We should still update the EFI_PXE_BASE_CODE_PROTOCOL. >> >> Don't leak content of prior acknowledge packages. >> >> Handle out of memory. >> > > The patch looks correct, but the description is a bit confusing. Apart > from what you mention this patch also fixes > - An unchecked allocation I can add this. > - shadowing of the global netobj netobj was a local variable before this patch. Thanks for reviewing. Best regards Heinrich > Can we please update the commit message to something that mentions all of > these? > > Thanks > /Ilias > >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> lib/efi_loader/efi_net.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c >> index 69276b275d..96a5bcca27 100644 >> --- a/lib/efi_loader/efi_net.c >> +++ b/lib/efi_loader/efi_net.c >> @@ -30,6 +30,7 @@ static uchar **receive_buffer; >> static size_t *receive_lengths; >> static int rx_packet_idx; >> static int rx_packet_num; >> +static struct efi_net_obj *netobj; >> >> /* >> * The notification function of this event is called in every timer cycle >> @@ -660,10 +661,16 @@ void efi_net_set_dhcp_ack(void *pkt, int len) >> { >> int maxsize = sizeof(*dhcp_ack); >> >> - if (!dhcp_ack) >> + if (!dhcp_ack) { >> dhcp_ack = malloc(maxsize); >> - >> + if (!dhcp_ack) >> + return; >> + } >> + memset(dhcp_ack, 0, maxsize); >> memcpy(dhcp_ack, pkt, min(len, maxsize)); >> + >> + if (netobj) >> + netobj->pxe_mode.dhcp_ack = *dhcp_ack; >> } >> >> /** >> @@ -853,7 +860,6 @@ static efi_status_t EFIAPI efi_pxe_base_code_set_packets( >> */ >> efi_status_t efi_net_register(void) >> { >> - struct efi_net_obj *netobj = NULL; >> efi_status_t r; >> int i; >> >> @@ -982,6 +988,7 @@ failure_to_add_protocol: >> return r; >> out_of_resources: >> free(netobj); >> + netobj = NULL; >> free(transmit_buffer); >> if (receive_buffer) >> for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) >> -- >> 2.37.2 >>
On Wed, Nov 30, 2022 at 08:50:07AM +0100, Heinrich Schuchardt wrote: > > > On 11/30/22 08:37, Ilias Apalodimas wrote: > > Hi Heinrich, > > > > On Sat, Nov 26, 2022 at 05:10:56PM +0100, Heinrich Schuchardt wrote: > > > The dhcp command may be executed after the first UEFI command. > > > We should still update the EFI_PXE_BASE_CODE_PROTOCOL. > > > > > > Don't leak content of prior acknowledge packages. > > > > > > Handle out of memory. > > > > > > > The patch looks correct, but the description is a bit confusing. Apart > > from what you mention this patch also fixes > > > - An unchecked allocation > > I can add this. > > > - shadowing of the global netobj > > netobj was a local variable before this patch. > Ah missed that Thanks! /Ilias > Thanks for reviewing. > > Best regards > > Heinrich > > > Can we please update the commit message to something that mentions all of > > these? > > > > Thanks > > /Ilias > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > > --- > > > lib/efi_loader/efi_net.c | 13 ++++++++++--- > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c > > > index 69276b275d..96a5bcca27 100644 > > > --- a/lib/efi_loader/efi_net.c > > > +++ b/lib/efi_loader/efi_net.c > > > @@ -30,6 +30,7 @@ static uchar **receive_buffer; > > > static size_t *receive_lengths; > > > static int rx_packet_idx; > > > static int rx_packet_num; > > > +static struct efi_net_obj *netobj; > > > /* > > > * The notification function of this event is called in every timer cycle > > > @@ -660,10 +661,16 @@ void efi_net_set_dhcp_ack(void *pkt, int len) > > > { > > > int maxsize = sizeof(*dhcp_ack); > > > - if (!dhcp_ack) > > > + if (!dhcp_ack) { > > > dhcp_ack = malloc(maxsize); > > > - > > > + if (!dhcp_ack) > > > + return; > > > + } > > > + memset(dhcp_ack, 0, maxsize); > > > memcpy(dhcp_ack, pkt, min(len, maxsize)); > > > + > > > + if (netobj) > > > + netobj->pxe_mode.dhcp_ack = *dhcp_ack; > > > } > > > /** > > > @@ -853,7 +860,6 @@ static efi_status_t EFIAPI efi_pxe_base_code_set_packets( > > > */ > > > efi_status_t efi_net_register(void) > > > { > > > - struct efi_net_obj *netobj = NULL; > > > efi_status_t r; > > > int i; > > > @@ -982,6 +988,7 @@ failure_to_add_protocol: > > > return r; > > > out_of_resources: > > > free(netobj); > > > + netobj = NULL; > > > free(transmit_buffer); > > > if (receive_buffer) > > > for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++) > > > -- > > > 2.37.2 > > >
On 11/30/22 09:44, Ilias Apalodimas wrote: > On Wed, Nov 30, 2022 at 08:50:07AM +0100, Heinrich Schuchardt wrote: >> >> >> On 11/30/22 08:37, Ilias Apalodimas wrote: >>> Hi Heinrich, >>> >>> On Sat, Nov 26, 2022 at 05:10:56PM +0100, Heinrich Schuchardt wrote: >>>> The dhcp command may be executed after the first UEFI command. >>>> We should still update the EFI_PXE_BASE_CODE_PROTOCOL. >>>> >>>> Don't leak content of prior acknowledge packages. >>>> >>>> Handle out of memory. >>>> >>> >>> The patch looks correct, but the description is a bit confusing. Apart >>> from what you mention this patch also fixes >> >>> - An unchecked allocation Above I wrote "Handle out of memory." Regards Heinrich >> >> I can add this. >> >>> - shadowing of the global netobj >> >> netobj was a local variable before this patch. >> > > Ah missed that > > Thanks! > /Ilias >> Thanks for reviewing. >> >> Best regards >> >> Heinrich >> >>> Can we please update the commit message to something that mentions all of >>> these? >>> >>> Thanks >>> /Ilias
Hi Henrich On Wed, 30 Nov 2022 at 14:45, Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: > > On 11/30/22 09:44, Ilias Apalodimas wrote: > > On Wed, Nov 30, 2022 at 08:50:07AM +0100, Heinrich Schuchardt wrote: > >> > >> > >> On 11/30/22 08:37, Ilias Apalodimas wrote: > >>> Hi Heinrich, > >>> > >>> On Sat, Nov 26, 2022 at 05:10:56PM +0100, Heinrich Schuchardt wrote: > >>>> The dhcp command may be executed after the first UEFI command. > >>>> We should still update the EFI_PXE_BASE_CODE_PROTOCOL. > >>>> > >>>> Don't leak content of prior acknowledge packages. > >>>> > >>>> Handle out of memory. > >>>> > >>> > >>> The patch looks correct, but the description is a bit confusing. Apart > >>> from what you mention this patch also fixes > >> > >>> - An unchecked allocation > > Above I wrote "Handle out of memory." ah, well that wasn't too clear when I first read the commit message. Can you please amend ti while merging? Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Regards > > Heinrich > > > >> > >> I can add this. > >> > >>> - shadowing of the global netobj > >> > >> netobj was a local variable before this patch. > >> > > > > Ah missed that > > > > Thanks! > > /Ilias > >> Thanks for reviewing. > >> > >> Best regards > >> > >> Heinrich > >> > >>> Can we please update the commit message to something that mentions all of > >>> these? > >>> > >>> Thanks > >>> /Ilias >
diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c index 69276b275d..96a5bcca27 100644 --- a/lib/efi_loader/efi_net.c +++ b/lib/efi_loader/efi_net.c @@ -30,6 +30,7 @@ static uchar **receive_buffer; static size_t *receive_lengths; static int rx_packet_idx; static int rx_packet_num; +static struct efi_net_obj *netobj; /* * The notification function of this event is called in every timer cycle @@ -660,10 +661,16 @@ void efi_net_set_dhcp_ack(void *pkt, int len) { int maxsize = sizeof(*dhcp_ack); - if (!dhcp_ack) + if (!dhcp_ack) { dhcp_ack = malloc(maxsize); - + if (!dhcp_ack) + return; + } + memset(dhcp_ack, 0, maxsize); memcpy(dhcp_ack, pkt, min(len, maxsize)); + + if (netobj) + netobj->pxe_mode.dhcp_ack = *dhcp_ack; } /** @@ -853,7 +860,6 @@ static efi_status_t EFIAPI efi_pxe_base_code_set_packets( */ efi_status_t efi_net_register(void) { - struct efi_net_obj *netobj = NULL; efi_status_t r; int i; @@ -982,6 +988,7 @@ failure_to_add_protocol: return r; out_of_resources: free(netobj); + netobj = NULL; free(transmit_buffer); if (receive_buffer) for (i = 0; i < ETH_PACKETS_BATCH_RECV; i++)
The dhcp command may be executed after the first UEFI command. We should still update the EFI_PXE_BASE_CODE_PROTOCOL. Don't leak content of prior acknowledge packages. Handle out of memory. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- lib/efi_loader/efi_net.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)