Syslinux - Jul 2015 - [PATCH] Updated udp.c to use real client ip and subnetmask values if on local subnet

>>>
 Jeff, Patrick: Could you try my code from my github repo branch
 efi-multinic?? It's derived from Patrick's code and I finally see good
 responses with a VMware VM's e1000e NIC (never saw ANYTHING good from
 it until now).
 
 git://github.com/geneC/syslinux.git
 https://github.com/geneC/syslinux.git
 
 -- 
 -Gene
 <<<

Hi there
I think in the case of a particular driver that implements Pxebc but it does not
implement UDPv4Sb/TCPv4Sb
(HP Elitebook 2560p/8460p) your patch crashes;

In that case the following for(;;) structure will never have a MAC match, then
the
BS->OpenProtocol will never fail returning the needed "status !=
EFI_SUCCESS"
to avoid de-referencing an undefined pointer (sbp->CreateChild) in the next
code line.

...
for (i = 0; i < nr_handles; i++) {
	    DevicePath = DevicePathFromHandle(handles[i]);
	    if (efi_get_MAC(DevicePath, &mac_2, PXE_MAC_LENGTH)
		    && memcmp(mac_1, mac_2, PXE_MAC_LENGTH) == 0) {
		 sb_handle = handles[i];
		status = uefi_call_wrapper(BS->OpenProtocol, 6, sb_handle,
					bguid, (void **)&sbp,
					image_handle, sb_handle,
					EFI_OPEN_PROTOCOL_GET_PROTOCOL);
		if (status == EFI_SUCCESS) {
		    mnpsb_handle = sb_handle;
		    break;
		}
	    }

	}

 }
    if (status != EFI_SUCCESS)
       goto free_binding;

    child = NULL;

    status = uefi_call_wrapper(sbp->CreateChild, 2, sbp, (EFI_HANDLE
*)&child);  <<<< Potential crash here !

...


I think these changes would solve the thing.

...
-EFI_SERVICE_BINDING *sbp;
+EFI_SERVICE_BINDING *sbp =NULL;
...

-    if (status != EFI_SUCCESS)
+    if (status != EFI_SUCCESS || sbp != NULL)
       goto free_binding;
...


Please take a look;
BTW It's just only me or the adopted Linux kernel coding style makes
following C code really a PITA ??

Best,
Patrick

small typo

-    if (status != EFI_SUCCESS)
+    if (status != EFI_SUCCESS || sbp == NULL)
       goto free_binding;


Best,
Patrick

On Wed, Jul 22, 2015 at 5:13 AM, Patrick Masotta <masottaus at yahoo.com>
wrote:
>>>>
>  Jeff, Patrick: Could you try my code from my github repo branch
>  efi-multinic?  It's derived from Patrick's code and I finally see
good
>  responses with a VMware VM's e1000e NIC (never saw ANYTHING good from
>  it until now).
>
>  git://github.com/geneC/syslinux.git
>  https://github.com/geneC/syslinux.git
>
>  --
>  -Gene
>  <<<
>
> Hi there
> I think in the case of a particular driver that implements Pxebc but it
does not implement UDPv4Sb/TCPv4Sb
> (HP Elitebook 2560p/8460p) your patch crashes;
>
> In that case the following for(;;) structure will never have a MAC match,
then the
> BS->OpenProtocol will never fail returning the needed "status !=
EFI_SUCCESS"
> to avoid de-referencing an undefined pointer (sbp->CreateChild) in the
next code line.
>
> ...
> for (i = 0; i < nr_handles; i++) {
>             DevicePath = DevicePathFromHandle(handles[i]);
>             if (efi_get_MAC(DevicePath, &mac_2, PXE_MAC_LENGTH)
>                     && memcmp(mac_1, mac_2, PXE_MAC_LENGTH) == 0) {
>                  sb_handle = handles[i];
>                 status = uefi_call_wrapper(BS->OpenProtocol, 6,
sb_handle,
>                                         bguid, (void **)&sbp,
>                                         image_handle, sb_handle,
>                                         EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>                 if (status == EFI_SUCCESS) {
>                     mnpsb_handle = sb_handle;
>                     break;
>                 }
>             }
>
>         }
>
>  }
>     if (status != EFI_SUCCESS)
>        goto free_binding;
>
>     child = NULL;
>
>     status = uefi_call_wrapper(sbp->CreateChild, 2, sbp, (EFI_HANDLE
*)&child);  <<<< Potential crash here !
>
> ...
>
>
> I think these changes would solve the thing.
>
> ...
> -EFI_SERVICE_BINDING *sbp;
> +EFI_SERVICE_BINDING *sbp =NULL;
> ...
>
> -    if (status != EFI_SUCCESS)
> +    if (status != EFI_SUCCESS || sbp == NULL)
>        goto free_binding;
> ...
The presumption was that we'd see an error rather than non-error from
LibLocateHandle() when nr_handles is 0 and be guaranteed to find a
matching handle.  Your additional measures seem the best choice.

-- 
-Gene

>>>
 > I think these changes would solve the thing.
 >
 > ...
 > -EFI_SERVICE_BINDING *sbp;
 > +EFI_SERVICE_BINDING *sbp =NULL;
 > ...
 >
 > -? ? if (status != EFI_SUCCESS)
 > +? ? if (status != EFI_SUCCESS || sbp = NULL)
 >? ? ? ? goto free_binding;
 > ...
 
 The presumption was that we'd see an error rather than
 non-error from LibLocateHandle() when nr_handles is 0 and be guaranteed to find
a
 matching handle.? Your additional measures seem the best choice.
 -- 
 -Gene
<<<

OK I have adopted this code approach; testing it since last
week, so far so good.
Presumably there are other people testing this; feedback appreciated.

Best,
Patrick