* [PATCH 1/1] ShellPkgDisconnect: zero-initialize handles
@ 2023-04-26 11:39 Gerd Hoffmann
2023-05-04 10:08 ` Gerd Hoffmann
0 siblings, 1 reply; 3+ messages in thread
From: Gerd Hoffmann @ 2023-04-26 11:39 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Pawel Polawski, Oliver Steffen, Zhichao Gao,
Gerd Hoffmann
In case ShellConvertStringToUint64() fails the Handles are left
uninitialized. That can for example happen for Handle2 and Handle3 in
case only one parameter was specified on the command line. Which can
trigger the ASSERT() in line 185.
Reproducer: boot ovmf to efi shell in qemu, using q35 machine type, then
try disconnect the sata controller in efi shell.
Fix that by explicitly setting them to NULL in that case. While being
at it also simplify the logic and avoid pointlessly calling
ShellConvertStringToUint64() in case ParamN is NULL.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
.../UefiShellDriver1CommandsLib/Disconnect.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
index fd49d1f7ceb4..fac6463e3c28 100644
--- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
+++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
@@ -160,16 +160,23 @@ ShellCommandRunDisconnect (
Param1 = ShellCommandLineGetRawValue (Package, 1);
Param2 = ShellCommandLineGetRawValue (Package, 2);
Param3 = ShellCommandLineGetRawValue (Package, 3);
- if (!EFI_ERROR (ShellConvertStringToUint64 (Param1, &Intermediate1, TRUE, FALSE))) {
- Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate1) : NULL;
+
+ if (Param1 && !EFI_ERROR (ShellConvertStringToUint64 (Param1, &Intermediate1, TRUE, FALSE))) {
+ Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate1);
+ } else {
+ Handle1 = NULL;
}
- if (!EFI_ERROR (ShellConvertStringToUint64 (Param2, &Intermediate2, TRUE, FALSE))) {
- Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate2) : NULL;
+ if (Param2 && !EFI_ERROR (ShellConvertStringToUint64 (Param2, &Intermediate2, TRUE, FALSE))) {
+ Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate2);
+ } else {
+ Handle2 = NULL;
}
- if (!EFI_ERROR (ShellConvertStringToUint64 (Param3, &Intermediate3, TRUE, FALSE))) {
- Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate3) : NULL;
+ if (Param3 && !EFI_ERROR (ShellConvertStringToUint64 (Param3, &Intermediate3, TRUE, FALSE))) {
+ Handle3 = ConvertHandleIndexToHandle ((UINTN)Intermediate3);
+ } else {
+ Handle3 = NULL;
}
if ((Param1 != NULL) && (Handle1 == NULL)) {
--
2.40.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] ShellPkgDisconnect: zero-initialize handles
2023-04-26 11:39 [PATCH 1/1] ShellPkgDisconnect: zero-initialize handles Gerd Hoffmann
@ 2023-05-04 10:08 ` Gerd Hoffmann
2023-05-10 0:54 ` Gao, Zhichao
0 siblings, 1 reply; 3+ messages in thread
From: Gerd Hoffmann @ 2023-05-04 10:08 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Pawel Polawski, Oliver Steffen, Zhichao Gao
Ping.
Any comments on this?
On Wed, Apr 26, 2023 at 01:39:27PM +0200, Gerd Hoffmann wrote:
> In case ShellConvertStringToUint64() fails the Handles are left
> uninitialized. That can for example happen for Handle2 and Handle3 in
> case only one parameter was specified on the command line. Which can
> trigger the ASSERT() in line 185.
>
> Reproducer: boot ovmf to efi shell in qemu, using q35 machine type, then
> try disconnect the sata controller in efi shell.
>
> Fix that by explicitly setting them to NULL in that case. While being
> at it also simplify the logic and avoid pointlessly calling
> ShellConvertStringToUint64() in case ParamN is NULL.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> .../UefiShellDriver1CommandsLib/Disconnect.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> index fd49d1f7ceb4..fac6463e3c28 100644
> --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> @@ -160,16 +160,23 @@ ShellCommandRunDisconnect (
> Param1 = ShellCommandLineGetRawValue (Package, 1);
> Param2 = ShellCommandLineGetRawValue (Package, 2);
> Param3 = ShellCommandLineGetRawValue (Package, 3);
> - if (!EFI_ERROR (ShellConvertStringToUint64 (Param1, &Intermediate1, TRUE, FALSE))) {
> - Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate1) : NULL;
> +
> + if (Param1 && !EFI_ERROR (ShellConvertStringToUint64 (Param1, &Intermediate1, TRUE, FALSE))) {
> + Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate1);
> + } else {
> + Handle1 = NULL;
> }
>
> - if (!EFI_ERROR (ShellConvertStringToUint64 (Param2, &Intermediate2, TRUE, FALSE))) {
> - Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate2) : NULL;
> + if (Param2 && !EFI_ERROR (ShellConvertStringToUint64 (Param2, &Intermediate2, TRUE, FALSE))) {
> + Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate2);
> + } else {
> + Handle2 = NULL;
> }
>
> - if (!EFI_ERROR (ShellConvertStringToUint64 (Param3, &Intermediate3, TRUE, FALSE))) {
> - Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle ((UINTN)Intermediate3) : NULL;
> + if (Param3 && !EFI_ERROR (ShellConvertStringToUint64 (Param3, &Intermediate3, TRUE, FALSE))) {
> + Handle3 = ConvertHandleIndexToHandle ((UINTN)Intermediate3);
> + } else {
> + Handle3 = NULL;
> }
>
> if ((Param1 != NULL) && (Handle1 == NULL)) {
> --
> 2.40.0
>
--
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] ShellPkgDisconnect: zero-initialize handles
2023-05-04 10:08 ` Gerd Hoffmann
@ 2023-05-10 0:54 ` Gao, Zhichao
0 siblings, 0 replies; 3+ messages in thread
From: Gao, Zhichao @ 2023-05-10 0:54 UTC (permalink / raw)
To: Gerd Hoffmann, devel@edk2.groups.io
Cc: Ni, Ray, Pawel Polawski, Oliver Steffen
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
Thanks,
Zhichao
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Thursday, May 4, 2023 6:08 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Pawel Polawski <ppolawsk@redhat.com>;
> Oliver Steffen <osteffen@redhat.com>; Gao, Zhichao
> <zhichao.gao@intel.com>
> Subject: Re: [PATCH 1/1] ShellPkgDisconnect: zero-initialize handles
>
> Ping.
> Any comments on this?
>
> On Wed, Apr 26, 2023 at 01:39:27PM +0200, Gerd Hoffmann wrote:
> > In case ShellConvertStringToUint64() fails the Handles are left
> > uninitialized. That can for example happen for Handle2 and Handle3 in
> > case only one parameter was specified on the command line. Which can
> > trigger the ASSERT() in line 185.
> >
> > Reproducer: boot ovmf to efi shell in qemu, using q35 machine type,
> > then try disconnect the sata controller in efi shell.
> >
> > Fix that by explicitly setting them to NULL in that case. While being
> > at it also simplify the logic and avoid pointlessly calling
> > ShellConvertStringToUint64() in case ParamN is NULL.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> > .../UefiShellDriver1CommandsLib/Disconnect.c | 19
> > +++++++++++++------
> > 1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> > b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> > index fd49d1f7ceb4..fac6463e3c28 100644
> > --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> > +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c
> > @@ -160,16 +160,23 @@ ShellCommandRunDisconnect (
> > Param1 = ShellCommandLineGetRawValue (Package, 1);
> > Param2 = ShellCommandLineGetRawValue (Package, 2);
> > Param3 = ShellCommandLineGetRawValue (Package, 3);
> > - if (!EFI_ERROR (ShellConvertStringToUint64 (Param1, &Intermediate1,
> TRUE, FALSE))) {
> > - Handle1 = Param1 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate1) : NULL;
> > +
> > + if (Param1 && !EFI_ERROR (ShellConvertStringToUint64 (Param1,
> &Intermediate1, TRUE, FALSE))) {
> > + Handle1 = ConvertHandleIndexToHandle ((UINTN)Intermediate1);
> > + } else {
> > + Handle1 = NULL;
> > }
> >
> > - if (!EFI_ERROR (ShellConvertStringToUint64 (Param2, &Intermediate2,
> TRUE, FALSE))) {
> > - Handle2 = Param2 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate2) : NULL;
> > + if (Param2 && !EFI_ERROR (ShellConvertStringToUint64 (Param2,
> &Intermediate2, TRUE, FALSE))) {
> > + Handle2 = ConvertHandleIndexToHandle ((UINTN)Intermediate2);
> > + } else {
> > + Handle2 = NULL;
> > }
> >
> > - if (!EFI_ERROR (ShellConvertStringToUint64 (Param3, &Intermediate3,
> TRUE, FALSE))) {
> > - Handle3 = Param3 != NULL ? ConvertHandleIndexToHandle
> ((UINTN)Intermediate3) : NULL;
> > + if (Param3 && !EFI_ERROR (ShellConvertStringToUint64 (Param3,
> &Intermediate3, TRUE, FALSE))) {
> > + Handle3 = ConvertHandleIndexToHandle ((UINTN)Intermediate3);
> > + } else {
> > + Handle3 = NULL;
> > }
> >
> > if ((Param1 != NULL) && (Handle1 == NULL)) {
> > --
> > 2.40.0
> >
>
> --
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-10 0:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-26 11:39 [PATCH 1/1] ShellPkgDisconnect: zero-initialize handles Gerd Hoffmann
2023-05-04 10:08 ` Gerd Hoffmann
2023-05-10 0:54 ` Gao, Zhichao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox