Re: [edk2-devel] [PATCH V2 2/2] NetworkPkg: Add WiFi profile sync protocol support
Thank you Heng and all,
My responses:
1. Fixed.
2. Fixed, it was a missed change now added to the new patch.
3. Added CC.
Sending an updated patch file for reviewal.
Thanks,
Zack
-Original Message-
From: Luo, Heng
Sent: Tuesday, September 27, 2022 12:31 AM
To: devel@edk2.groups.io; Clark-williams, Zachary
Cc: Zachary Clark-Williams
Subject: RE: [edk2-devel] [PATCH V2 2/2] NetworkPkg: Add WiFi profile sync
protocol support
Hi Zack,
1.
> + Status = UnicodeStrToAsciiStrS (Profile->Password, (CHAR8
> + *)AsciiPassword, ((StrLen (Profile->Password) + 1) * sizeof
> + (CHAR8)));
I think we should remove '* sizeof (CHAR8)' because the third parameter is the
length of string but not size of the memory:
Status = UnicodeStrToAsciiStrS (Profile->Password, (CHAR8
*)AsciiPassword, ((StrLen (Profile->Password) + 1)));
2.
> + if (StrLen (Profile->Password) > PASSWORD_STORAGE_SIZE) {
> +ASSERT (EFI_INVALID_PARAMETER);
> +return EFI_INVALID_PARAMETER;
> + }
Need null terminator at end of password, so I think the conditional should be:
if (StrLen (Profile->Password) >= PASSWORD_STORAGE_SIZE) in order to
support 32 bytes PSK passwords with null terminator. Maybe need to change
PASSWORD_STORAGE_SIZE to 65? But PASSWORD_MAX_LEN should still be 63, because
according to the 802.11i specification:
A pass-phrase is a sequence of between 8 and 63 ASCII-encoded characters. The
limit of 63 comes from the desire to distinguish between a pass-phrase and a
PSK displayed as 64 hexadecimal characters.
3.
Suggest to add maintainers in Cc of the commit msg to speed up review.
Fox example:
WifiProfileSyncProtocol and if found will operate on the premise of a One
Click Recovery, or KVM flow with a Wifi profile provided by AMT.
Cc: Maciej Rabeda
Cc: Fu Siyuan
Cc: Wu Jiaxin
Signed-off-by: Zachary Clark-Williams
Thanks,
Heng
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Clark-
> williams, Zachary
> Sent: Tuesday, September 27, 2022 2:20 AM
> To: devel@edk2.groups.io
> Cc: Zachary Clark-Williams ; Clark-williams,
> Zachary
> Subject: [edk2-devel] [PATCH V2 2/2] NetworkPkg: Add WiFi profile sync
> protocol support
>
> From: Zachary Clark-Williams
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3845
>
> Enables KVM and One Click Recovery WLAN capability with WiFi Profile
> Sync feature and protocol. Adding WiFiProfileSyncProtocol, which
> supports the profilesync driver operations for transferring WiFi
> profiles from AMT to the Supplicant. WiFiConnectionManager will check
> for the WifiProfileSyncProtocol and if found will operate on the
> premise of a One Click Recovery, or KVM flow with a Wifi profile provided by
> AMT.
>
> Signed-off-by: Zachary Clark-Williams
>
> ---
> .../Protocol/WiFiProfileSyncProtocol.h| 83
> NetworkPkg/NetworkPkg.dec | 3 +
> .../WifiConnectionManagerDxe.inf | 3 +-
> .../WifiConnectionMgrDriver.c | 126
> .../WifiConnectionMgrDxe.h| 4 +-
> .../WifiConnectionMgrImpl.c | 193 --
> .../WifiConnectionMgrMisc.c | 13 ++
> 7 files changed, 366 insertions(+), 59 deletions(-) create mode
> 100644 NetworkPkg/Include/Protocol/WiFiProfileSyncProtocol.h
>
> diff --git a/NetworkPkg/Include/Protocol/WiFiProfileSyncProtocol.h
> b/NetworkPkg/Include/Protocol/WiFiProfileSyncProtocol.h
> new file mode 100644
> index 00..e36daceabf
> --- /dev/null
> +++ b/NetworkPkg/Include/Protocol/WiFiProfileSyncProtocol.h
> @@ -0,0 +1,83 @@
> +/** @file
> + WiFi profile sync protocol. Supports One Click Recovery or KVM OS
> +recovery
> + boot flow over WiFi.
> +
> + Copyright (c) 2022, Intel Corporation. All rights reserved.
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent **/
> +
> +#ifndef WIFI_PROFILE_SYNC_PROTOCOL_H_ #define
> +WIFI_PROFILE_SYNC_PROTOCOL_H_
> +
> +#include
> +
> +//
> +// WiFi Profile Sync Protocol GUID variable.
> +//
> +extern EFI_GUID gEfiWiFiProfileSyncProtocolGuid;
> +
> +/**
> + Used by the WiFi connection manager to get the WiFi profile that
> +AMT shared
> + and was stored in WiFi profile protocol. Aligns the AMT WiFi
> +profile data to
> + the WiFi connection manager profile structure fo connection use.
> +
> + @param[in, out] WcmProfile WiFi Connection Manager profile
> structure
> + @param[in, out] MacAddress MAC address from AMT saved to NiC
> MAC address
> +
> + @retval EFI_SUCCESS Stored WiFi profile converted and
> returned
> succefully
> + @retval EFI_UNSUPPORTED Pro
Re: [edk2-devel] [PATCH V2 2/2] NetworkPkg: Add WiFi profile sync protocol support
Hi Zack,
1.
> + Status = UnicodeStrToAsciiStrS (Profile->Password, (CHAR8
> + *)AsciiPassword, ((StrLen (Profile->Password) + 1) * sizeof (CHAR8)));
I think we should remove '* sizeof (CHAR8)' because the third parameter is the
length of string but not size of the memory:
Status = UnicodeStrToAsciiStrS (Profile->Password, (CHAR8
*)AsciiPassword, ((StrLen (Profile->Password) + 1)));
2.
> + if (StrLen (Profile->Password) > PASSWORD_STORAGE_SIZE) {
> +ASSERT (EFI_INVALID_PARAMETER);
> +return EFI_INVALID_PARAMETER;
> + }
Need null terminator at end of password, so I think the conditional should be:
if (StrLen (Profile->Password) >= PASSWORD_STORAGE_SIZE)
in order to support 32 bytes PSK passwords with null terminator. Maybe need to
change PASSWORD_STORAGE_SIZE to 65? But PASSWORD_MAX_LEN should still be 63,
because according to the 802.11i specification:
A pass-phrase is a sequence of between 8 and 63 ASCII-encoded characters. The
limit of 63 comes from the desire to distinguish between a pass-phrase and a
PSK displayed as 64 hexadecimal characters.
3.
Suggest to add maintainers in Cc of the commit msg to speed up review.
Fox example:
WifiProfileSyncProtocol and if found will operate on the premise of a One
Click Recovery, or KVM flow with a Wifi profile provided by AMT.
Cc: Maciej Rabeda
Cc: Fu Siyuan
Cc: Wu Jiaxin
Signed-off-by: Zachary Clark-Williams
Thanks,
Heng
> -Original Message-
> From: devel@edk2.groups.io On Behalf Of Clark-
> williams, Zachary
> Sent: Tuesday, September 27, 2022 2:20 AM
> To: devel@edk2.groups.io
> Cc: Zachary Clark-Williams ; Clark-williams, Zachary
>
> Subject: [edk2-devel] [PATCH V2 2/2] NetworkPkg: Add WiFi profile sync
> protocol support
>
> From: Zachary Clark-Williams
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3845
>
> Enables KVM and One Click Recovery WLAN capability with WiFi Profile Sync
> feature and protocol. Adding WiFiProfileSyncProtocol, which supports the
> profilesync driver operations for transferring WiFi profiles from AMT to the
> Supplicant. WiFiConnectionManager will check for the
> WifiProfileSyncProtocol and if found will operate on the premise of a One
> Click Recovery, or KVM flow with a Wifi profile provided by AMT.
>
> Signed-off-by: Zachary Clark-Williams
> ---
> .../Protocol/WiFiProfileSyncProtocol.h| 83
> NetworkPkg/NetworkPkg.dec | 3 +
> .../WifiConnectionManagerDxe.inf | 3 +-
> .../WifiConnectionMgrDriver.c | 126
> .../WifiConnectionMgrDxe.h| 4 +-
> .../WifiConnectionMgrImpl.c | 193 --
> .../WifiConnectionMgrMisc.c | 13 ++
> 7 files changed, 366 insertions(+), 59 deletions(-) create mode 100644
> NetworkPkg/Include/Protocol/WiFiProfileSyncProtocol.h
>
> diff --git a/NetworkPkg/Include/Protocol/WiFiProfileSyncProtocol.h
> b/NetworkPkg/Include/Protocol/WiFiProfileSyncProtocol.h
> new file mode 100644
> index 00..e36daceabf
> --- /dev/null
> +++ b/NetworkPkg/Include/Protocol/WiFiProfileSyncProtocol.h
> @@ -0,0 +1,83 @@
> +/** @file
> + WiFi profile sync protocol. Supports One Click Recovery or KVM OS
> +recovery
> + boot flow over WiFi.
> +
> + Copyright (c) 2022, Intel Corporation. All rights reserved.
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent **/
> +
> +#ifndef WIFI_PROFILE_SYNC_PROTOCOL_H_
> +#define WIFI_PROFILE_SYNC_PROTOCOL_H_
> +
> +#include
> +
> +//
> +// WiFi Profile Sync Protocol GUID variable.
> +//
> +extern EFI_GUID gEfiWiFiProfileSyncProtocolGuid;
> +
> +/**
> + Used by the WiFi connection manager to get the WiFi profile that AMT
> +shared
> + and was stored in WiFi profile protocol. Aligns the AMT WiFi profile
> +data to
> + the WiFi connection manager profile structure fo connection use.
> +
> + @param[in, out] WcmProfile WiFi Connection Manager profile
> structure
> + @param[in, out] MacAddress MAC address from AMT saved to NiC
> MAC address
> +
> + @retval EFI_SUCCESS Stored WiFi profile converted and
> returned
> succefully
> + @retval EFI_UNSUPPORTED Profile protocol sharing not supported or
> enabled
> + @retval EFI_NOT_FOUND No profiles to returned
> + @retval OthersError Occurred
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI *WIFI_PROFILE_GET)(
> + IN OUT WIFI_MGR_NETWORK_PROFILE *Profile,
> + IN OUT EFI_80211_MAC_ADDRESS MacAddress
> + );
> +
> +/**
> + Saves the WiFi connection status recieved by the
> +WiFiConnectionManager when
> + in a KVM OR One Click Recovery WLAN recovery flow. Input as
> + EFI_80211_CONNECT_NETWORK_RESULT_CODE then converted and
> stored as EFI_STATUS type.
> +
> + @param[in] ConnectionStatus WiFi connection attempt results
> +**/
> +typedef
> +VOID
> +(EFIAPI *WIFI_SET_CONNECT_STATE)(
> + IN
