On 02/09/21 13:33, Michael Brown wrote:

On 09/02/2021 11:54, Laszlo Ersek wrote:

On 02/08/21 16:28, Michael Brown wrote:

This reminds me of a very longstanding question I've had around
OpenProtocol(): is there any defined way for something that is an
ordinary consumer (rather than a driver or child controller) to obtain a
long-lived pointer to a protocol interface?

The CloseProtocol() specification has parts as follows (relevant passage
is the last one below):

     The caller is responsible for ensuring that there are no references
     to a protocol interface that has been removed. In some cases,
     outstanding reference information is not available in the protocol,
     so the protocol, once added, cannot be removed. Examples include
     Console I/O, Block I/O, Disk I/O, and (in general) handles to device
     protocols.

<snip>

Thank you!  I see that text in the documentation for the deprecated call
UninstallProtocolInterface(), which explains why I hadn't previously
noticed it.

Right, my apologies. I'm not sure why I wrote CloseProtocol(). Perhaps a
typo, or maybe I lost track of where I was in the PDF with "evince".

My comments:

(1) I don't really understand what the last quoted passage means by
"closing an agent".

Based on what the EDK2 implementation actually does, I'm pretty sure
this means that any opens recorded with attributes of
BY_HANDLE_PROTOCOL, GET_PROTOCOL, or TEST_PROTOCOL are simply discarded:

https://github.com/tianocore/edk2/blob/stable/202011/MdeModulePkg/Core/Dxe/Hand/Handle.c#L659-L674


The code that performed the BY_HANDLE_PROTOCOL, GET_PROTOCOL, or
TEST_PROTOCOL open is not notified in any way.

(2) I've never tested this, nor attempted to verify it from the edk2
source, myself. It does suggest however that GET_PROTOCOL opens are
tracked too, and thus, *not* closing a GET_PROTOCOL open is technically
still a leak.

They are tracked, but as far as I can tell only for informative purposes
(e.g. debugging using the EFI shell "openinfo" command, or with iPXE's
DBG_EFI_OPENERS() call).

So: technically a leak, but a leak that is arguably useful from the
perspective of someone (e.g. quite often me) trying to debug what code
has tried to do something with a particular handle.  And a leak that
will get automatically cleaned up when the protocol is uninstalled.


But this still leaves my original problem: there is no way for code to
ever know that a protocol it opened with GET_PROTOCOL has ceased to be
valid, and nothing that stops said code from dereferencing the
potentially invalid pointer.

That seems to be the case, yes.

As far as I can tell, the only way to be notified when a protocol
pointer ceases to be valid is to open BY_DRIVER, and we can't open some
protocols (such as EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL) with BY_DRIVER
attributes because there will be multiple openers of that protocol.

Agreed.

Laszlo

On 09/02/2021 11:54, Laszlo Ersek wrote:

On 02/08/21 16:28, Michael Brown wrote:

This reminds me of a very longstanding question I've had around
OpenProtocol(): is there any defined way for something that is an
ordinary consumer (rather than a driver or child controller) to obtain a
long-lived pointer to a protocol interface?

The CloseProtocol() specification has parts as follows (relevant passage
is the last one below):
The caller is responsible for ensuring that there are no references
to a protocol interface that has been removed. In some cases,
outstanding reference information is not available in the protocol,
so the protocol, once added, cannot be removed. Examples include
Console I/O, Block I/O, Disk I/O, and (in general) handles to device
protocols.
<snip>

Thank you! I see that text in the documentation for the deprecated call UninstallProtocolInterface(), which explains why I hadn't previously noticed it.

My comments:
(1) I don't really understand what the last quoted passage means by
"closing an agent".

Based on what the EDK2 implementation actually does, I'm pretty sure this means that any opens recorded with attributes of BY_HANDLE_PROTOCOL, GET_PROTOCOL, or TEST_PROTOCOL are simply discarded:

https://github.com/tianocore/edk2/blob/stable/202011/MdeModulePkg/Core/Dxe/Hand/Handle.c#L659-L674

The code that performed the BY_HANDLE_PROTOCOL, GET_PROTOCOL, or TEST_PROTOCOL open is not notified in any way.

(2) I've never tested this, nor attempted to verify it from the edk2
source, myself. It does suggest however that GET_PROTOCOL opens are
tracked too, and thus, *not* closing a GET_PROTOCOL open is technically
still a leak.

They are tracked, but as far as I can tell only for informative purposes (e.g. debugging using the EFI shell "openinfo" command, or with iPXE's DBG_EFI_OPENERS() call).

So: technically a leak, but a leak that is arguably useful from the perspective of someone (e.g. quite often me) trying to debug what code has tried to do something with a particular handle. And a leak that will get automatically cleaned up when the protocol is uninstalled.


But this still leaves my original problem: there is no way for code to ever know that a protocol it opened with GET_PROTOCOL has ceased to be valid, and nothing that stops said code from dereferencing the potentially invalid pointer.

As far as I can tell, the only way to be notified when a protocol pointer ceases to be valid is to open BY_DRIVER, and we can't open some protocols (such as EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL) with BY_DRIVER attributes because there will be multiple openers of that protocol.

Michael

On 02/08/21 16:28, Michael Brown wrote:

On 08/02/2021 14:22, Laszlo Ersek wrote:

gBS->OpenProtocol() calls that pass the EFI_OPEN_PROTOCOL_GET_PROTOCOL
argument for the "Attributes" parameter *need not* be mirrored with
gBS->CloseProtocol() calls.

Please refer to the UEFI spec on the OpenProtocol() boot service,
Attributes=EFI_OPEN_PROTOCOL_GET_PROTOCOL:

     [...] The caller is also not required to close the protocol
     interface with EFI_BOOT_SERVICES.CloseProtocol().

So you *can* call CloseProtocol(), but you don't have to.

This reminds me of a very longstanding question I've had around
OpenProtocol(): is there any defined way for something that is an
ordinary consumer (rather than a driver or child controller) to obtain a
long-lived pointer to a protocol interface?

For example:
ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c seems
to open EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL using just HandleProtocol()
and then continues to use the pointer for the lifetime of the
application.  But, as far as I can tell, there's nothing that guarantees
that this pointer remains valid?

The CloseProtocol() specification has parts as follows (relevant passage
is the last one below):

The caller is responsible for ensuring that there are no references
to a protocol interface that has been removed. In some cases,
outstanding reference information is not available in the protocol,
so the protocol, once added, cannot be removed. Examples include
Console I/O, Block I/O, Disk I/O, and (in general) handles to device
protocols.

[...]

EFI 1.10 Extension

The extension to this service directly addresses the limitations
described in the section above. There may be some drivers that are
currently consuming the protocol interface that needs to be
uninstalled, so it may be dangerous to just blindly remove a
protocol interface from the system. Since the usage of protocol
interfaces is now being tracked for components that use the
EFI_BOOT_SERVICES.OpenProtocol() and
EFI_BOOT_SERVICES.CloseProtocol() boot services, a safe version of
this function can be implemented.

[...]

Lastly, all of the agents that have the protocol interface open with
an attribute of EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL,
EFI_OPEN_PROTOCOL_GET_PROTOCOL, or EFI_OPEN_PROTOCOL_TEST_PROTOCOL
are closed. If there are any agents remaining that still have the
protocol interface open, the protocol interface is not removed from
the handle and EFI_ACCESS_DENIED is returned. In addition, all of
the drivers that were disconnected with the boot service
DisconnectController() earlier, are reconnected with the boot
service EFI_BOOT_SERVICES.ConnectController(). If there are no
agents remaining that are consuming the protocol interface, then the
protocol interface is removed from the handle as described above.

My comments:

(1) I don't really understand what the last quoted passage means by
"closing an agent".

(2) I've never tested this, nor attempted to verify it from the edk2
source, myself. It does suggest however that GET_PROTOCOL opens are
tracked too, and thus, *not* closing a GET_PROTOCOL open is technically
still a leak.

Thanks
Laszlo

Hi Laszlo,

I was almost exclusively checking edk2 headers but should have checked with
the specs. Thank you for clarifying it for me.

Best,
Satoshi

On 08/02/2021 14:22, Laszlo Ersek wrote:

gBS->OpenProtocol() calls that pass the EFI_OPEN_PROTOCOL_GET_PROTOCOL
argument for the "Attributes" parameter *need not* be mirrored with
gBS->CloseProtocol() calls.
Please refer to the UEFI spec on the OpenProtocol() boot service,
Attributes=EFI_OPEN_PROTOCOL_GET_PROTOCOL:
[...] The caller is also not required to close the protocol
interface with EFI_BOOT_SERVICES.CloseProtocol().
So you *can* call CloseProtocol(), but you don't have to.

This reminds me of a very longstanding question I've had around OpenProtocol(): is there any defined way for something that is an ordinary consumer (rather than a driver or child controller) to obtain a long-lived pointer to a protocol interface?

For example: ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c seems to open EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL using just HandleProtocol() and then continues to use the pointer for the lifetime of the application. But, as far as I can tell, there's nothing that guarantees that this pointer remains valid?

Michael

Hi,

On 02/06/21 19:56, Satoshi Tanda wrote:

There are a number of places that do not call CloseProtocol() while it
appears to be required, in EDK2. Can someone confirm if (some of) those are
indeed errors, or there are actually cases where skipping CloseProtocol()
after OpenProtocol() is appropriate?

Here are the places I would expect a CloseProtocol() call but apparently
missing it.

MdeModulePkg/Universal/DebugSupportDxe/DebugSupport.c
- InitializeDebugSupportDriver().

ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c
- GetDriverName() / GetDriverImageName()

ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
- *ProtocolDumpInformation(). DevicePathProtocolDumpInformationEx() does
call it. So lack of call seems like an error to me.

ShellPkg/Library/UefiShellDriver1CommandsLib/DevTree.c
- DoDevTreeForHandle()

I am new to UEFI and trying to learn basics like how to use OpenProtocol().
I have not observed or encountered any concrete issue.

gBS->OpenProtocol() calls that pass the EFI_OPEN_PROTOCOL_GET_PROTOCOL
argument for the "Attributes" parameter *need not* be mirrored with
gBS->CloseProtocol() calls.

Please refer to the UEFI spec on the OpenProtocol() boot service,
Attributes=EFI_OPEN_PROTOCOL_GET_PROTOCOL:

[...] The caller is also not required to close the protocol
interface with EFI_BOOT_SERVICES.CloseProtocol().

So you *can* call CloseProtocol(), but you don't have to.

Thanks
Laszlo

There are a number of places that do not call CloseProtocol() while it
appears to be required, in EDK2. Can someone confirm if (some of) those are
indeed errors, or there are actually cases where skipping CloseProtocol()
after OpenProtocol() is appropriate?

Here are the places I would expect a CloseProtocol() call but apparently
missing it.

MdeModulePkg/Universal/DebugSupportDxe/DebugSupport.c
- InitializeDebugSupportDriver().

ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c
- GetDriverName() / GetDriverImageName()

ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
- *ProtocolDumpInformation(). DevicePathProtocolDumpInformationEx() does
call it. So lack of call seems like an error to me.

ShellPkg/Library/UefiShellDriver1CommandsLib/DevTree.c
- DoDevTreeForHandle()

I am new to UEFI and trying to learn basics like how to use OpenProtocol().
I have not observed or encountered any concrete issue.

Best,
Satoshi