* [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
@ 2017-07-23 10:10 Marvin Häuser
2017-07-24 16:31 ` Kinney, Michael D
0 siblings, 1 reply; 8+ messages in thread
From: Marvin Häuser @ 2017-07-23 10:10 UTC (permalink / raw)
To: edk2-devel@lists.01.org; +Cc: michael.d.kinney@intel.com, liming.gao@intel.com
This patch adds IsNodeInList() to BaseLib, which verifies the given
Node is part of the doubly-linked List provided.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
---
MdePkg/Library/BaseLib/LinkedList.c | 70 +++++++++++++++++++-
MdePkg/Include/Library/BaseLib.h | 25 +++++++
MdePkg/Library/BaseLib/BaseLibInternals.h | 28 --------
3 files changed, 94 insertions(+), 29 deletions(-)
diff --git a/MdePkg/Library/BaseLib/LinkedList.c b/MdePkg/Library/BaseLib/LinkedList.c
index ba373f4b7be3..b364ae41c647 100644
--- a/MdePkg/Library/BaseLib/LinkedList.c
+++ b/MdePkg/Library/BaseLib/LinkedList.c
@@ -1,7 +1,7 @@
/** @file
Linked List Library Functions.
- Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD License
which accompanies this distribution. The full text of the license may be found at
@@ -113,6 +113,74 @@ InternalBaseLibIsNodeInList (
}
/**
+ Checks whether Node is part of a doubly-linked list.
+
+ If List is NULL, then ASSERT().
+ If List->ForwardLink is NULL, then ASSERT().
+ If List->BackLink is NULL, then ASSERT().
+ If Node is NULL, then ASSERT();
+ If PcdMaximumLinkedListLength is not zero, and List contains more than
+ PcdMaximumLinkedListLength nodes, then ASSERT().
+
+ @param List A pointer to a node in a linked list.
+ @param Node A pointer to the node to locate.
+
+ @retval TRUE Node is in List.
+ @retval FALSE Node isn't in List, or List is invalid.
+
+**/
+BOOLEAN
+EFIAPI
+IsNodeInList (
+ IN CONST LIST_ENTRY *List,
+ IN CONST LIST_ENTRY *Node
+ )
+{
+ UINTN Count;
+ CONST LIST_ENTRY *Ptr;
+
+ //
+ // Test the validity of List and Node
+ //
+ ASSERT (List != NULL);
+ ASSERT (List->ForwardLink != NULL);
+ ASSERT (List->BackLink != NULL);
+ ASSERT (Node != NULL);
+
+ //
+ // ASSERT List not too long
+ //
+ ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE));
+
+ Count = 0;
+ Ptr = List;
+
+ //
+ // Check to see if Node is a member of List.
+ // Exit early if the number of nodes in List >= PcdMaximumLinkedListLength
+ //
+ do {
+ Ptr = Ptr->ForwardLink;
+ if (PcdGet32 (PcdMaximumLinkedListLength) > 0) {
+ Count++;
+
+ //
+ // Return if the linked list is too long
+ //
+ if (Count == PcdGet32 (PcdMaximumLinkedListLength)) {
+ return (BOOLEAN)(Ptr == Node);
+ }
+ }
+
+ if (Ptr == Node) {
+ return TRUE;
+ }
+ } while (Ptr != List);
+
+ return FALSE;
+}
+
+/**
Initializes the head node of a doubly-linked list, and returns the pointer to
the head node of the doubly-linked list.
diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 791849b80406..4f3f4fd51f3f 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -2869,6 +2869,31 @@ PathCleanUpDirectories(
/**
+ Checks whether Node is part of a doubly-linked list.
+
+ If List is NULL, then ASSERT().
+ If List->ForwardLink is NULL, then ASSERT().
+ If List->BackLink is NULL, then ASSERT().
+ If Node is NULL, then ASSERT();
+ If PcdMaximumLinkedListLength is not zero, and List contains more than
+ PcdMaximumLinkedListLength nodes, then ASSERT().
+
+ @param List A pointer to a node in a linked list.
+ @param Node A pointer to the node to locate.
+
+ @retval TRUE Node is in List.
+ @retval FALSE Node isn't in List, or List is invalid.
+
+**/
+BOOLEAN
+EFIAPI
+IsNodeInList (
+ IN CONST LIST_ENTRY *List,
+ IN CONST LIST_ENTRY *Node
+ );
+
+
+/**
Initializes the head node of a doubly linked list, and returns the pointer to
the head node of the doubly linked list.
diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h b/MdePkg/Library/BaseLib/BaseLibInternals.h
index ea387ce37d27..9dca97a0dcc9 100644
--- a/MdePkg/Library/BaseLib/BaseLibInternals.h
+++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
@@ -340,34 +340,6 @@ InternalSwitchStack (
/**
- Worker function that locates the Node in the List.
-
- By searching the List, finds the location of the Node in List. At the same time,
- verifies the validity of this list.
-
- If List is NULL, then ASSERT().
- If List->ForwardLink is NULL, then ASSERT().
- If List->backLink is NULL, then ASSERT().
- If Node is NULL, then ASSERT();
- If PcdMaximumLinkedListLength is not zero, and prior to insertion the number
- of nodes in ListHead, including the ListHead node, is greater than or
- equal to PcdMaximumLinkedListLength, then ASSERT().
-
- @param List A pointer to a node in a linked list.
- @param Node A pointer to one nod.
-
- @retval TRUE Node is in List.
- @retval FALSE Node isn't in List, or List is invalid.
-
-**/
-BOOLEAN
-EFIAPI
-IsNodeInList (
- IN CONST LIST_ENTRY *List,
- IN CONST LIST_ENTRY *Node
- );
-
-/**
Worker function that returns a bit field from Operand.
Returns the bitfield specified by the StartBit and the EndBit from Operand.
--
2.12.2.windows.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
2017-07-23 10:10 [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function Marvin Häuser
@ 2017-07-24 16:31 ` Kinney, Michael D
2017-07-24 16:52 ` Marvin Häuser
0 siblings, 1 reply; 8+ messages in thread
From: Kinney, Michael D @ 2017-07-24 16:31 UTC (permalink / raw)
To: Marvin Häuser, edk2-devel@lists.01.org, Kinney, Michael D
Cc: Gao, Liming
Hi Marvin,
Can you provide a few more details on why you would like
to see tis internal function promoted to a library class
API?
Thanks,
Mike
> -----Original Message-----
> From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com]
> Sent: Sunday, July 23, 2017 3:11 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>
> This patch adds IsNodeInList() to BaseLib, which verifies the
> given
> Node is part of the doubly-linked List provided.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> ---
> MdePkg/Library/BaseLib/LinkedList.c | 70
> +++++++++++++++++++-
> MdePkg/Include/Library/BaseLib.h | 25 +++++++
> MdePkg/Library/BaseLib/BaseLibInternals.h | 28 --------
> 3 files changed, 94 insertions(+), 29 deletions(-)
>
> diff --git a/MdePkg/Library/BaseLib/LinkedList.c
> b/MdePkg/Library/BaseLib/LinkedList.c
> index ba373f4b7be3..b364ae41c647 100644
> --- a/MdePkg/Library/BaseLib/LinkedList.c
> +++ b/MdePkg/Library/BaseLib/LinkedList.c
> @@ -1,7 +1,7 @@
> /** @file
> Linked List Library Functions.
>
> - Copyright (c) 2006 - 2013, Intel Corporation. All rights
> reserved.<BR>
> + Copyright (c) 2006 - 2017, Intel Corporation. All rights
> reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions
> of the BSD License
> which accompanies this distribution. The full text of the
> license may be found at
> @@ -113,6 +113,74 @@ InternalBaseLibIsNodeInList (
> }
>
> /**
> + Checks whether Node is part of a doubly-linked list.
> +
> + If List is NULL, then ASSERT().
> + If List->ForwardLink is NULL, then ASSERT().
> + If List->BackLink is NULL, then ASSERT().
> + If Node is NULL, then ASSERT();
> + If PcdMaximumLinkedListLength is not zero, and List contains
> more than
> + PcdMaximumLinkedListLength nodes, then ASSERT().
> +
> + @param List A pointer to a node in a linked list.
> + @param Node A pointer to the node to locate.
> +
> + @retval TRUE Node is in List.
> + @retval FALSE Node isn't in List, or List is invalid.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +IsNodeInList (
> + IN CONST LIST_ENTRY *List,
> + IN CONST LIST_ENTRY *Node
> + )
> +{
> + UINTN Count;
> + CONST LIST_ENTRY *Ptr;
> +
> + //
> + // Test the validity of List and Node
> + //
> + ASSERT (List != NULL);
> + ASSERT (List->ForwardLink != NULL);
> + ASSERT (List->BackLink != NULL);
> + ASSERT (Node != NULL);
> +
> + //
> + // ASSERT List not too long
> + //
> + ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE));
> +
> + Count = 0;
> + Ptr = List;
> +
> + //
> + // Check to see if Node is a member of List.
> + // Exit early if the number of nodes in List >=
> PcdMaximumLinkedListLength
> + //
> + do {
> + Ptr = Ptr->ForwardLink;
> + if (PcdGet32 (PcdMaximumLinkedListLength) > 0) {
> + Count++;
> +
> + //
> + // Return if the linked list is too long
> + //
> + if (Count == PcdGet32 (PcdMaximumLinkedListLength)) {
> + return (BOOLEAN)(Ptr == Node);
> + }
> + }
> +
> + if (Ptr == Node) {
> + return TRUE;
> + }
> + } while (Ptr != List);
> +
> + return FALSE;
> +}
> +
> +/**
> Initializes the head node of a doubly-linked list, and returns
> the pointer to
> the head node of the doubly-linked list.
>
> diff --git a/MdePkg/Include/Library/BaseLib.h
> b/MdePkg/Include/Library/BaseLib.h
> index 791849b80406..4f3f4fd51f3f 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -2869,6 +2869,31 @@ PathCleanUpDirectories(
>
>
> /**
> + Checks whether Node is part of a doubly-linked list.
> +
> + If List is NULL, then ASSERT().
> + If List->ForwardLink is NULL, then ASSERT().
> + If List->BackLink is NULL, then ASSERT().
> + If Node is NULL, then ASSERT();
> + If PcdMaximumLinkedListLength is not zero, and List contains
> more than
> + PcdMaximumLinkedListLength nodes, then ASSERT().
> +
> + @param List A pointer to a node in a linked list.
> + @param Node A pointer to the node to locate.
> +
> + @retval TRUE Node is in List.
> + @retval FALSE Node isn't in List, or List is invalid.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +IsNodeInList (
> + IN CONST LIST_ENTRY *List,
> + IN CONST LIST_ENTRY *Node
> + );
> +
> +
> +/**
> Initializes the head node of a doubly linked list, and returns
> the pointer to
> the head node of the doubly linked list.
>
> diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h
> b/MdePkg/Library/BaseLib/BaseLibInternals.h
> index ea387ce37d27..9dca97a0dcc9 100644
> --- a/MdePkg/Library/BaseLib/BaseLibInternals.h
> +++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
> @@ -340,34 +340,6 @@ InternalSwitchStack (
>
>
> /**
> - Worker function that locates the Node in the List.
> -
> - By searching the List, finds the location of the Node in List.
> At the same time,
> - verifies the validity of this list.
> -
> - If List is NULL, then ASSERT().
> - If List->ForwardLink is NULL, then ASSERT().
> - If List->backLink is NULL, then ASSERT().
> - If Node is NULL, then ASSERT();
> - If PcdMaximumLinkedListLength is not zero, and prior to
> insertion the number
> - of nodes in ListHead, including the ListHead node, is greater
> than or
> - equal to PcdMaximumLinkedListLength, then ASSERT().
> -
> - @param List A pointer to a node in a linked list.
> - @param Node A pointer to one nod.
> -
> - @retval TRUE Node is in List.
> - @retval FALSE Node isn't in List, or List is invalid.
> -
> -**/
> -BOOLEAN
> -EFIAPI
> -IsNodeInList (
> - IN CONST LIST_ENTRY *List,
> - IN CONST LIST_ENTRY *Node
> - );
> -
> -/**
> Worker function that returns a bit field from Operand.
>
> Returns the bitfield specified by the StartBit and the EndBit
> from Operand.
> --
> 2.12.2.windows.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
2017-07-24 16:31 ` Kinney, Michael D
@ 2017-07-24 16:52 ` Marvin Häuser
2017-07-27 15:31 ` Gao, Liming
0 siblings, 1 reply; 8+ messages in thread
From: Marvin Häuser @ 2017-07-24 16:52 UTC (permalink / raw)
To: edk2-devel@lists.01.org; +Cc: Kinney, Michael D
Hey Mike,
In contrast to setting 'BackLink' and 'ForwardLink' to dummy values such as EFI_BAD_POINTER, verifying that a node is part of a known list is a safe way to determine whether a link is still valid without needing to keep the memory allocated. If the Node, which had its links set to dummys, is freed, the dummy values may be overwriten and the check would pass despite the node being invalid. This can, for example, be used in OpenKabylake's PchSmiDispatcher, which does the described check against EFI_BAD_POINTER (though if I remember correctly, the value is actually never explicitely set). The issue is that some function may pass a handle that was unregistered to the function. IsNodeInList() can be used to determine whether it was previously removed.
Thanks for your response!
Regards,
Marvin.
> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Monday, July 24, 2017 6:31 PM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>
> Hi Marvin,
>
> Can you provide a few more details on why you would like to see tis internal
> function promoted to a library class API?
>
> Thanks,
>
> Mike
>
> > -----Original Message-----
> > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com]
> > Sent: Sunday, July 23, 2017 3:11 AM
> > To: edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > <liming.gao@intel.com>
> > Subject: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
> >
> > This patch adds IsNodeInList() to BaseLib, which verifies the given
> > Node is part of the doubly-linked List provided.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> > ---
> > MdePkg/Library/BaseLib/LinkedList.c | 70
> > +++++++++++++++++++-
> > MdePkg/Include/Library/BaseLib.h | 25 +++++++
> > MdePkg/Library/BaseLib/BaseLibInternals.h | 28 --------
> > 3 files changed, 94 insertions(+), 29 deletions(-)
> >
> > diff --git a/MdePkg/Library/BaseLib/LinkedList.c
> > b/MdePkg/Library/BaseLib/LinkedList.c
> > index ba373f4b7be3..b364ae41c647 100644
> > --- a/MdePkg/Library/BaseLib/LinkedList.c
> > +++ b/MdePkg/Library/BaseLib/LinkedList.c
> > @@ -1,7 +1,7 @@
> > /** @file
> > Linked List Library Functions.
> >
> > - Copyright (c) 2006 - 2013, Intel Corporation. All rights
> > reserved.<BR>
> > + Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > reserved.<BR>
> > This program and the accompanying materials
> > are licensed and made available under the terms and conditions of
> > the BSD License
> > which accompanies this distribution. The full text of the license
> > may be found at @@ -113,6 +113,74 @@ InternalBaseLibIsNodeInList ( }
> >
> > /**
> > + Checks whether Node is part of a doubly-linked list.
> > +
> > + If List is NULL, then ASSERT().
> > + If List->ForwardLink is NULL, then ASSERT().
> > + If List->BackLink is NULL, then ASSERT().
> > + If Node is NULL, then ASSERT();
> > + If PcdMaximumLinkedListLength is not zero, and List contains
> > more than
> > + PcdMaximumLinkedListLength nodes, then ASSERT().
> > +
> > + @param List A pointer to a node in a linked list.
> > + @param Node A pointer to the node to locate.
> > +
> > + @retval TRUE Node is in List.
> > + @retval FALSE Node isn't in List, or List is invalid.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +IsNodeInList (
> > + IN CONST LIST_ENTRY *List,
> > + IN CONST LIST_ENTRY *Node
> > + )
> > +{
> > + UINTN Count;
> > + CONST LIST_ENTRY *Ptr;
> > +
> > + //
> > + // Test the validity of List and Node // ASSERT (List != NULL);
> > + ASSERT (List->ForwardLink != NULL); ASSERT (List->BackLink !=
> > + NULL); ASSERT (Node != NULL);
> > +
> > + //
> > + // ASSERT List not too long
> > + //
> > + ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE));
> > +
> > + Count = 0;
> > + Ptr = List;
> > +
> > + //
> > + // Check to see if Node is a member of List.
> > + // Exit early if the number of nodes in List >=
> > PcdMaximumLinkedListLength
> > + //
> > + do {
> > + Ptr = Ptr->ForwardLink;
> > + if (PcdGet32 (PcdMaximumLinkedListLength) > 0) {
> > + Count++;
> > +
> > + //
> > + // Return if the linked list is too long
> > + //
> > + if (Count == PcdGet32 (PcdMaximumLinkedListLength)) {
> > + return (BOOLEAN)(Ptr == Node);
> > + }
> > + }
> > +
> > + if (Ptr == Node) {
> > + return TRUE;
> > + }
> > + } while (Ptr != List);
> > +
> > + return FALSE;
> > +}
> > +
> > +/**
> > Initializes the head node of a doubly-linked list, and returns the
> > pointer to
> > the head node of the doubly-linked list.
> >
> > diff --git a/MdePkg/Include/Library/BaseLib.h
> > b/MdePkg/Include/Library/BaseLib.h
> > index 791849b80406..4f3f4fd51f3f 100644
> > --- a/MdePkg/Include/Library/BaseLib.h
> > +++ b/MdePkg/Include/Library/BaseLib.h
> > @@ -2869,6 +2869,31 @@ PathCleanUpDirectories(
> >
> >
> > /**
> > + Checks whether Node is part of a doubly-linked list.
> > +
> > + If List is NULL, then ASSERT().
> > + If List->ForwardLink is NULL, then ASSERT().
> > + If List->BackLink is NULL, then ASSERT().
> > + If Node is NULL, then ASSERT();
> > + If PcdMaximumLinkedListLength is not zero, and List contains
> > more than
> > + PcdMaximumLinkedListLength nodes, then ASSERT().
> > +
> > + @param List A pointer to a node in a linked list.
> > + @param Node A pointer to the node to locate.
> > +
> > + @retval TRUE Node is in List.
> > + @retval FALSE Node isn't in List, or List is invalid.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +IsNodeInList (
> > + IN CONST LIST_ENTRY *List,
> > + IN CONST LIST_ENTRY *Node
> > + );
> > +
> > +
> > +/**
> > Initializes the head node of a doubly linked list, and returns the
> > pointer to
> > the head node of the doubly linked list.
> >
> > diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h
> > b/MdePkg/Library/BaseLib/BaseLibInternals.h
> > index ea387ce37d27..9dca97a0dcc9 100644
> > --- a/MdePkg/Library/BaseLib/BaseLibInternals.h
> > +++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
> > @@ -340,34 +340,6 @@ InternalSwitchStack (
> >
> >
> > /**
> > - Worker function that locates the Node in the List.
> > -
> > - By searching the List, finds the location of the Node in List.
> > At the same time,
> > - verifies the validity of this list.
> > -
> > - If List is NULL, then ASSERT().
> > - If List->ForwardLink is NULL, then ASSERT().
> > - If List->backLink is NULL, then ASSERT().
> > - If Node is NULL, then ASSERT();
> > - If PcdMaximumLinkedListLength is not zero, and prior to insertion
> > the number
> > - of nodes in ListHead, including the ListHead node, is greater than
> > or
> > - equal to PcdMaximumLinkedListLength, then ASSERT().
> > -
> > - @param List A pointer to a node in a linked list.
> > - @param Node A pointer to one nod.
> > -
> > - @retval TRUE Node is in List.
> > - @retval FALSE Node isn't in List, or List is invalid.
> > -
> > -**/
> > -BOOLEAN
> > -EFIAPI
> > -IsNodeInList (
> > - IN CONST LIST_ENTRY *List,
> > - IN CONST LIST_ENTRY *Node
> > - );
> > -
> > -/**
> > Worker function that returns a bit field from Operand.
> >
> > Returns the bitfield specified by the StartBit and the EndBit from
> > Operand.
> > --
> > 2.12.2.windows.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
2017-07-24 16:52 ` Marvin Häuser
@ 2017-07-27 15:31 ` Gao, Liming
2017-07-27 16:19 ` Marvin Häuser
0 siblings, 1 reply; 8+ messages in thread
From: Gao, Liming @ 2017-07-27 15:31 UTC (permalink / raw)
To: Marvin.Haeuser@outlook.com, edk2-devel@lists.01.org; +Cc: Kinney, Michael D
Marvin:
Once this API is exposed as the public library API, what code will you update to consume this API?
And, for the patch, I have two comments.
1) Original logic uses PcdVerifyNodeInList to turn on the verification of NodeInList. New logic always turns on it. Because this check takes much overhead, this PCD is FALSE as the default value in MdePkg.dec.
2) In IsListEmpty(), ASSERT() is missing for InternalBaseLibIsListValid (ListHead).
Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Marvin H?user
> Sent: Tuesday, July 25, 2017 12:53 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>
> Hey Mike,
>
> In contrast to setting 'BackLink' and 'ForwardLink' to dummy values such as EFI_BAD_POINTER, verifying that a node is part of a known
> list is a safe way to determine whether a link is still valid without needing to keep the memory allocated. If the Node, which had its links
> set to dummys, is freed, the dummy values may be overwriten and the check would pass despite the node being invalid. This can, for
> example, be used in OpenKabylake's PchSmiDispatcher, which does the described check against EFI_BAD_POINTER (though if I
> remember correctly, the value is actually never explicitely set). The issue is that some function may pass a handle that was unregistered
> to the function. IsNodeInList() can be used to determine whether it was previously removed.
>
> Thanks for your response!
>
> Regards,
> Marvin.
>
> > -----Original Message-----
> > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > Sent: Monday, July 24, 2017 6:31 PM
> > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Gao, Liming <liming.gao@intel.com>
> > Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
> >
> > Hi Marvin,
> >
> > Can you provide a few more details on why you would like to see tis internal
> > function promoted to a library class API?
> >
> > Thanks,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com]
> > > Sent: Sunday, July 23, 2017 3:11 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>
> > > Subject: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
> > >
> > > This patch adds IsNodeInList() to BaseLib, which verifies the given
> > > Node is part of the doubly-linked List provided.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> > > ---
> > > MdePkg/Library/BaseLib/LinkedList.c | 70
> > > +++++++++++++++++++-
> > > MdePkg/Include/Library/BaseLib.h | 25 +++++++
> > > MdePkg/Library/BaseLib/BaseLibInternals.h | 28 --------
> > > 3 files changed, 94 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/MdePkg/Library/BaseLib/LinkedList.c
> > > b/MdePkg/Library/BaseLib/LinkedList.c
> > > index ba373f4b7be3..b364ae41c647 100644
> > > --- a/MdePkg/Library/BaseLib/LinkedList.c
> > > +++ b/MdePkg/Library/BaseLib/LinkedList.c
> > > @@ -1,7 +1,7 @@
> > > /** @file
> > > Linked List Library Functions.
> > >
> > > - Copyright (c) 2006 - 2013, Intel Corporation. All rights
> > > reserved.<BR>
> > > + Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > > reserved.<BR>
> > > This program and the accompanying materials
> > > are licensed and made available under the terms and conditions of
> > > the BSD License
> > > which accompanies this distribution. The full text of the license
> > > may be found at @@ -113,6 +113,74 @@ InternalBaseLibIsNodeInList ( }
> > >
> > > /**
> > > + Checks whether Node is part of a doubly-linked list.
> > > +
> > > + If List is NULL, then ASSERT().
> > > + If List->ForwardLink is NULL, then ASSERT().
> > > + If List->BackLink is NULL, then ASSERT().
> > > + If Node is NULL, then ASSERT();
> > > + If PcdMaximumLinkedListLength is not zero, and List contains
> > > more than
> > > + PcdMaximumLinkedListLength nodes, then ASSERT().
> > > +
> > > + @param List A pointer to a node in a linked list.
> > > + @param Node A pointer to the node to locate.
> > > +
> > > + @retval TRUE Node is in List.
> > > + @retval FALSE Node isn't in List, or List is invalid.
> > > +
> > > +**/
> > > +BOOLEAN
> > > +EFIAPI
> > > +IsNodeInList (
> > > + IN CONST LIST_ENTRY *List,
> > > + IN CONST LIST_ENTRY *Node
> > > + )
> > > +{
> > > + UINTN Count;
> > > + CONST LIST_ENTRY *Ptr;
> > > +
> > > + //
> > > + // Test the validity of List and Node // ASSERT (List != NULL);
> > > + ASSERT (List->ForwardLink != NULL); ASSERT (List->BackLink !=
> > > + NULL); ASSERT (Node != NULL);
> > > +
> > > + //
> > > + // ASSERT List not too long
> > > + //
> > > + ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE));
> > > +
> > > + Count = 0;
> > > + Ptr = List;
> > > +
> > > + //
> > > + // Check to see if Node is a member of List.
> > > + // Exit early if the number of nodes in List >=
> > > PcdMaximumLinkedListLength
> > > + //
> > > + do {
> > > + Ptr = Ptr->ForwardLink;
> > > + if (PcdGet32 (PcdMaximumLinkedListLength) > 0) {
> > > + Count++;
> > > +
> > > + //
> > > + // Return if the linked list is too long
> > > + //
> > > + if (Count == PcdGet32 (PcdMaximumLinkedListLength)) {
> > > + return (BOOLEAN)(Ptr == Node);
> > > + }
> > > + }
> > > +
> > > + if (Ptr == Node) {
> > > + return TRUE;
> > > + }
> > > + } while (Ptr != List);
> > > +
> > > + return FALSE;
> > > +}
> > > +
> > > +/**
> > > Initializes the head node of a doubly-linked list, and returns the
> > > pointer to
> > > the head node of the doubly-linked list.
> > >
> > > diff --git a/MdePkg/Include/Library/BaseLib.h
> > > b/MdePkg/Include/Library/BaseLib.h
> > > index 791849b80406..4f3f4fd51f3f 100644
> > > --- a/MdePkg/Include/Library/BaseLib.h
> > > +++ b/MdePkg/Include/Library/BaseLib.h
> > > @@ -2869,6 +2869,31 @@ PathCleanUpDirectories(
> > >
> > >
> > > /**
> > > + Checks whether Node is part of a doubly-linked list.
> > > +
> > > + If List is NULL, then ASSERT().
> > > + If List->ForwardLink is NULL, then ASSERT().
> > > + If List->BackLink is NULL, then ASSERT().
> > > + If Node is NULL, then ASSERT();
> > > + If PcdMaximumLinkedListLength is not zero, and List contains
> > > more than
> > > + PcdMaximumLinkedListLength nodes, then ASSERT().
> > > +
> > > + @param List A pointer to a node in a linked list.
> > > + @param Node A pointer to the node to locate.
> > > +
> > > + @retval TRUE Node is in List.
> > > + @retval FALSE Node isn't in List, or List is invalid.
> > > +
> > > +**/
> > > +BOOLEAN
> > > +EFIAPI
> > > +IsNodeInList (
> > > + IN CONST LIST_ENTRY *List,
> > > + IN CONST LIST_ENTRY *Node
> > > + );
> > > +
> > > +
> > > +/**
> > > Initializes the head node of a doubly linked list, and returns the
> > > pointer to
> > > the head node of the doubly linked list.
> > >
> > > diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h
> > > b/MdePkg/Library/BaseLib/BaseLibInternals.h
> > > index ea387ce37d27..9dca97a0dcc9 100644
> > > --- a/MdePkg/Library/BaseLib/BaseLibInternals.h
> > > +++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
> > > @@ -340,34 +340,6 @@ InternalSwitchStack (
> > >
> > >
> > > /**
> > > - Worker function that locates the Node in the List.
> > > -
> > > - By searching the List, finds the location of the Node in List.
> > > At the same time,
> > > - verifies the validity of this list.
> > > -
> > > - If List is NULL, then ASSERT().
> > > - If List->ForwardLink is NULL, then ASSERT().
> > > - If List->backLink is NULL, then ASSERT().
> > > - If Node is NULL, then ASSERT();
> > > - If PcdMaximumLinkedListLength is not zero, and prior to insertion
> > > the number
> > > - of nodes in ListHead, including the ListHead node, is greater than
> > > or
> > > - equal to PcdMaximumLinkedListLength, then ASSERT().
> > > -
> > > - @param List A pointer to a node in a linked list.
> > > - @param Node A pointer to one nod.
> > > -
> > > - @retval TRUE Node is in List.
> > > - @retval FALSE Node isn't in List, or List is invalid.
> > > -
> > > -**/
> > > -BOOLEAN
> > > -EFIAPI
> > > -IsNodeInList (
> > > - IN CONST LIST_ENTRY *List,
> > > - IN CONST LIST_ENTRY *Node
> > > - );
> > > -
> > > -/**
> > > Worker function that returns a bit field from Operand.
> > >
> > > Returns the bitfield specified by the StartBit and the EndBit from
> > > Operand.
> > > --
> > > 2.12.2.windows.2
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
2017-07-27 15:31 ` Gao, Liming
@ 2017-07-27 16:19 ` Marvin Häuser
2017-08-02 12:12 ` Gao, Liming
0 siblings, 1 reply; 8+ messages in thread
From: Marvin Häuser @ 2017-07-27 16:19 UTC (permalink / raw)
To: edk2-devel@lists.01.org; +Cc: Gao, Liming, michael.d.kinney@intel.com
Hi Liming,
Comments are inline. Thanks for your response!
Regards,
Marvin.
> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Thursday, July 27, 2017 5:32 PM
> To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>
> Marvin:
> Once this API is exposed as the public library API, what code will you update
> to consume this API?
The code which gave me the idea for this patch is this: https://github.com/tianocore/edk2-platforms/blob/devel-MinPlatform/Silicon/Intel/KabylakeSiliconPkg/Pch/PchSmiDispatcher/Smm/IoTrap.c#L508
ForwardLink is obviously checked against EFI_BAD_POINTER to verify whether the entry is still valid. Though, after being freed, the value stored in this location may just be overwritten.
The code linked currently doesn't seem to do anything as ForwardLink is never set to EFI_BAD_POINTER, though it can be updated with IsNodeInList() to function (regardless of the entry being freed).
It seems to me that this is a leftover from the EDK era as EDK's RemoveEntryList() does set the Links to EFI_BAD_POINTER.
>
> And, for the patch, I have two comments.
> 1) Original logic uses PcdVerifyNodeInList to turn on the verification of
> NodeInList. New logic always turns on it. Because this check takes much
> overhead, this PCD is FALSE as the default value in MdePkg.dec.
> 2) In IsListEmpty(), ASSERT() is missing for InternalBaseLibIsListValid
> (ListHead).
Sorry for these oversights. Would you prefer an internal, static function or a macro to do the PCD check and following the ASSERTin v2?
>
> Thanks
> Liming
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Marvin H?user
> > Sent: Tuesday, July 25, 2017 12:53 AM
> > To: edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList()
> function.
> >
> > Hey Mike,
> >
> > In contrast to setting 'BackLink' and 'ForwardLink' to dummy values
> > such as EFI_BAD_POINTER, verifying that a node is part of a known list
> > is a safe way to determine whether a link is still valid without
> > needing to keep the memory allocated. If the Node, which had its links
> > set to dummys, is freed, the dummy values may be overwriten and the
> > check would pass despite the node being invalid. This can, for example, be
> used in OpenKabylake's PchSmiDispatcher, which does the described check
> against EFI_BAD_POINTER (though if I remember correctly, the value is
> actually never explicitely set). The issue is that some function may pass a
> handle that was unregistered to the function. IsNodeInList() can be used to
> determine whether it was previously removed.
> >
> > Thanks for your response!
> >
> > Regards,
> > Marvin.
> >
> > > -----Original Message-----
> > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> > > Sent: Monday, July 24, 2017 6:31 PM
> > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> > > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: Gao, Liming <liming.gao@intel.com>
> > > Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
> > >
> > > Hi Marvin,
> > >
> > > Can you provide a few more details on why you would like to see tis
> > > internal function promoted to a library class API?
> > >
> > > Thanks,
> > >
> > > Mike
> > >
> > > > -----Original Message-----
> > > > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com]
> > > > Sent: Sunday, July 23, 2017 3:11 AM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > > <liming.gao@intel.com>
> > > > Subject: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
> > > >
> > > > This patch adds IsNodeInList() to BaseLib, which verifies the
> > > > given Node is part of the doubly-linked List provided.
> > > >
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> > > > ---
> > > > MdePkg/Library/BaseLib/LinkedList.c | 70
> > > > +++++++++++++++++++-
> > > > MdePkg/Include/Library/BaseLib.h | 25 +++++++
> > > > MdePkg/Library/BaseLib/BaseLibInternals.h | 28 --------
> > > > 3 files changed, 94 insertions(+), 29 deletions(-)
> > > >
> > > > diff --git a/MdePkg/Library/BaseLib/LinkedList.c
> > > > b/MdePkg/Library/BaseLib/LinkedList.c
> > > > index ba373f4b7be3..b364ae41c647 100644
> > > > --- a/MdePkg/Library/BaseLib/LinkedList.c
> > > > +++ b/MdePkg/Library/BaseLib/LinkedList.c
> > > > @@ -1,7 +1,7 @@
> > > > /** @file
> > > > Linked List Library Functions.
> > > >
> > > > - Copyright (c) 2006 - 2013, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > + Copyright (c) 2006 - 2017, Intel Corporation. All rights
> > > > reserved.<BR>
> > > > This program and the accompanying materials
> > > > are licensed and made available under the terms and conditions
> > > > of the BSD License
> > > > which accompanies this distribution. The full text of the
> > > > license may be found at @@ -113,6 +113,74 @@
> > > > InternalBaseLibIsNodeInList ( }
> > > >
> > > > /**
> > > > + Checks whether Node is part of a doubly-linked list.
> > > > +
> > > > + If List is NULL, then ASSERT().
> > > > + If List->ForwardLink is NULL, then ASSERT().
> > > > + If List->BackLink is NULL, then ASSERT().
> > > > + If Node is NULL, then ASSERT(); If PcdMaximumLinkedListLength
> > > > + is not zero, and List contains
> > > > more than
> > > > + PcdMaximumLinkedListLength nodes, then ASSERT().
> > > > +
> > > > + @param List A pointer to a node in a linked list.
> > > > + @param Node A pointer to the node to locate.
> > > > +
> > > > + @retval TRUE Node is in List.
> > > > + @retval FALSE Node isn't in List, or List is invalid.
> > > > +
> > > > +**/
> > > > +BOOLEAN
> > > > +EFIAPI
> > > > +IsNodeInList (
> > > > + IN CONST LIST_ENTRY *List,
> > > > + IN CONST LIST_ENTRY *Node
> > > > + )
> > > > +{
> > > > + UINTN Count;
> > > > + CONST LIST_ENTRY *Ptr;
> > > > +
> > > > + //
> > > > + // Test the validity of List and Node // ASSERT (List !=
> > > > + NULL); ASSERT (List->ForwardLink != NULL); ASSERT
> > > > + (List->BackLink != NULL); ASSERT (Node != NULL);
> > > > +
> > > > + //
> > > > + // ASSERT List not too long
> > > > + //
> > > > + ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE));
> > > > +
> > > > + Count = 0;
> > > > + Ptr = List;
> > > > +
> > > > + //
> > > > + // Check to see if Node is a member of List.
> > > > + // Exit early if the number of nodes in List >=
> > > > PcdMaximumLinkedListLength
> > > > + //
> > > > + do {
> > > > + Ptr = Ptr->ForwardLink;
> > > > + if (PcdGet32 (PcdMaximumLinkedListLength) > 0) {
> > > > + Count++;
> > > > +
> > > > + //
> > > > + // Return if the linked list is too long
> > > > + //
> > > > + if (Count == PcdGet32 (PcdMaximumLinkedListLength)) {
> > > > + return (BOOLEAN)(Ptr == Node);
> > > > + }
> > > > + }
> > > > +
> > > > + if (Ptr == Node) {
> > > > + return TRUE;
> > > > + }
> > > > + } while (Ptr != List);
> > > > +
> > > > + return FALSE;
> > > > +}
> > > > +
> > > > +/**
> > > > Initializes the head node of a doubly-linked list, and returns
> > > > the pointer to
> > > > the head node of the doubly-linked list.
> > > >
> > > > diff --git a/MdePkg/Include/Library/BaseLib.h
> > > > b/MdePkg/Include/Library/BaseLib.h
> > > > index 791849b80406..4f3f4fd51f3f 100644
> > > > --- a/MdePkg/Include/Library/BaseLib.h
> > > > +++ b/MdePkg/Include/Library/BaseLib.h
> > > > @@ -2869,6 +2869,31 @@ PathCleanUpDirectories(
> > > >
> > > >
> > > > /**
> > > > + Checks whether Node is part of a doubly-linked list.
> > > > +
> > > > + If List is NULL, then ASSERT().
> > > > + If List->ForwardLink is NULL, then ASSERT().
> > > > + If List->BackLink is NULL, then ASSERT().
> > > > + If Node is NULL, then ASSERT(); If PcdMaximumLinkedListLength
> > > > + is not zero, and List contains
> > > > more than
> > > > + PcdMaximumLinkedListLength nodes, then ASSERT().
> > > > +
> > > > + @param List A pointer to a node in a linked list.
> > > > + @param Node A pointer to the node to locate.
> > > > +
> > > > + @retval TRUE Node is in List.
> > > > + @retval FALSE Node isn't in List, or List is invalid.
> > > > +
> > > > +**/
> > > > +BOOLEAN
> > > > +EFIAPI
> > > > +IsNodeInList (
> > > > + IN CONST LIST_ENTRY *List,
> > > > + IN CONST LIST_ENTRY *Node
> > > > + );
> > > > +
> > > > +
> > > > +/**
> > > > Initializes the head node of a doubly linked list, and returns
> > > > the pointer to
> > > > the head node of the doubly linked list.
> > > >
> > > > diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h
> > > > b/MdePkg/Library/BaseLib/BaseLibInternals.h
> > > > index ea387ce37d27..9dca97a0dcc9 100644
> > > > --- a/MdePkg/Library/BaseLib/BaseLibInternals.h
> > > > +++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
> > > > @@ -340,34 +340,6 @@ InternalSwitchStack (
> > > >
> > > >
> > > > /**
> > > > - Worker function that locates the Node in the List.
> > > > -
> > > > - By searching the List, finds the location of the Node in List.
> > > > At the same time,
> > > > - verifies the validity of this list.
> > > > -
> > > > - If List is NULL, then ASSERT().
> > > > - If List->ForwardLink is NULL, then ASSERT().
> > > > - If List->backLink is NULL, then ASSERT().
> > > > - If Node is NULL, then ASSERT();
> > > > - If PcdMaximumLinkedListLength is not zero, and prior to
> > > > insertion the number
> > > > - of nodes in ListHead, including the ListHead node, is greater
> > > > than or
> > > > - equal to PcdMaximumLinkedListLength, then ASSERT().
> > > > -
> > > > - @param List A pointer to a node in a linked list.
> > > > - @param Node A pointer to one nod.
> > > > -
> > > > - @retval TRUE Node is in List.
> > > > - @retval FALSE Node isn't in List, or List is invalid.
> > > > -
> > > > -**/
> > > > -BOOLEAN
> > > > -EFIAPI
> > > > -IsNodeInList (
> > > > - IN CONST LIST_ENTRY *List,
> > > > - IN CONST LIST_ENTRY *Node
> > > > - );
> > > > -
> > > > -/**
> > > > Worker function that returns a bit field from Operand.
> > > >
> > > > Returns the bitfield specified by the StartBit and the EndBit
> > > > from Operand.
> > > > --
> > > > 2.12.2.windows.2
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
2017-07-27 16:19 ` Marvin Häuser
@ 2017-08-02 12:12 ` Gao, Liming
2017-08-02 12:23 ` Marvin Häuser
0 siblings, 1 reply; 8+ messages in thread
From: Gao, Liming @ 2017-08-02 12:12 UTC (permalink / raw)
To: Marvin.Haeuser@outlook.com, edk2-devel@lists.01.org; +Cc: Kinney, Michael D
Marvin:
How about directly use PCD checker with IsNodeInList()?
ASSERT (IsNodeInList (FirstEntry, SecondEntry));
==>
ASSERT (FeaturePcdGet (PcdVerifyNodeInList) && IsNodeInList (FirstEntry, SecondEntry))|| InternalBaseLibIsListValid (FirstEntry));
Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Marvin H?user
>Sent: Friday, July 28, 2017 12:20 AM
>To: edk2-devel@lists.01.org
>Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
><liming.gao@intel.com>
>Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>
>Hi Liming,
>
>Comments are inline. Thanks for your response!
>
>Regards,
>Marvin.
>
>> -----Original Message-----
>> From: Gao, Liming [mailto:liming.gao@intel.com]
>> Sent: Thursday, July 27, 2017 5:32 PM
>> To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
>> Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>>
>> Marvin:
>> Once this API is exposed as the public library API, what code will you
>update
>> to consume this API?
>
>The code which gave me the idea for this patch is this:
>https://github.com/tianocore/edk2-platforms/blob/devel-
>MinPlatform/Silicon/Intel/KabylakeSiliconPkg/Pch/PchSmiDispatcher/Smm/Io
>Trap.c#L508
>ForwardLink is obviously checked against EFI_BAD_POINTER to verify whether
>the entry is still valid. Though, after being freed, the value stored in this
>location may just be overwritten.
>The code linked currently doesn't seem to do anything as ForwardLink is never
>set to EFI_BAD_POINTER, though it can be updated with IsNodeInList() to
>function (regardless of the entry being freed).
>It seems to me that this is a leftover from the EDK era as EDK's
>RemoveEntryList() does set the Links to EFI_BAD_POINTER.
>
>>
>> And, for the patch, I have two comments.
>> 1) Original logic uses PcdVerifyNodeInList to turn on the verification of
>> NodeInList. New logic always turns on it. Because this check takes much
>> overhead, this PCD is FALSE as the default value in MdePkg.dec.
>> 2) In IsListEmpty(), ASSERT() is missing for InternalBaseLibIsListValid
>> (ListHead).
>
>Sorry for these oversights. Would you prefer an internal, static function or a
>macro to do the PCD check and following the ASSERTin v2?
>
>>
>> Thanks
>> Liming
>> > -----Original Message-----
>> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> > Marvin H?user
>> > Sent: Tuesday, July 25, 2017 12:53 AM
>> > To: edk2-devel@lists.01.org
>> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
>> > Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList()
>> function.
>> >
>> > Hey Mike,
>> >
>> > In contrast to setting 'BackLink' and 'ForwardLink' to dummy values
>> > such as EFI_BAD_POINTER, verifying that a node is part of a known list
>> > is a safe way to determine whether a link is still valid without
>> > needing to keep the memory allocated. If the Node, which had its links
>> > set to dummys, is freed, the dummy values may be overwriten and the
>> > check would pass despite the node being invalid. This can, for example, be
>> used in OpenKabylake's PchSmiDispatcher, which does the described check
>> against EFI_BAD_POINTER (though if I remember correctly, the value is
>> actually never explicitely set). The issue is that some function may pass a
>> handle that was unregistered to the function. IsNodeInList() can be used to
>> determine whether it was previously removed.
>> >
>> > Thanks for your response!
>> >
>> > Regards,
>> > Marvin.
>> >
>> > > -----Original Message-----
>> > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
>> > > Sent: Monday, July 24, 2017 6:31 PM
>> > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
>> > > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
>> > > Cc: Gao, Liming <liming.gao@intel.com>
>> > > Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>> > >
>> > > Hi Marvin,
>> > >
>> > > Can you provide a few more details on why you would like to see tis
>> > > internal function promoted to a library class API?
>> > >
>> > > Thanks,
>> > >
>> > > Mike
>> > >
>> > > > -----Original Message-----
>> > > > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com]
>> > > > Sent: Sunday, July 23, 2017 3:11 AM
>> > > > To: edk2-devel@lists.01.org
>> > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> > > > <liming.gao@intel.com>
>> > > > Subject: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>> > > >
>> > > > This patch adds IsNodeInList() to BaseLib, which verifies the
>> > > > given Node is part of the doubly-linked List provided.
>> > > >
>> > > > Contributed-under: TianoCore Contribution Agreement 1.1
>> > > > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
>> > > > ---
>> > > > MdePkg/Library/BaseLib/LinkedList.c | 70
>> > > > +++++++++++++++++++-
>> > > > MdePkg/Include/Library/BaseLib.h | 25 +++++++
>> > > > MdePkg/Library/BaseLib/BaseLibInternals.h | 28 --------
>> > > > 3 files changed, 94 insertions(+), 29 deletions(-)
>> > > >
>> > > > diff --git a/MdePkg/Library/BaseLib/LinkedList.c
>> > > > b/MdePkg/Library/BaseLib/LinkedList.c
>> > > > index ba373f4b7be3..b364ae41c647 100644
>> > > > --- a/MdePkg/Library/BaseLib/LinkedList.c
>> > > > +++ b/MdePkg/Library/BaseLib/LinkedList.c
>> > > > @@ -1,7 +1,7 @@
>> > > > /** @file
>> > > > Linked List Library Functions.
>> > > >
>> > > > - Copyright (c) 2006 - 2013, Intel Corporation. All rights
>> > > > reserved.<BR>
>> > > > + Copyright (c) 2006 - 2017, Intel Corporation. All rights
>> > > > reserved.<BR>
>> > > > This program and the accompanying materials
>> > > > are licensed and made available under the terms and conditions
>> > > > of the BSD License
>> > > > which accompanies this distribution. The full text of the
>> > > > license may be found at @@ -113,6 +113,74 @@
>> > > > InternalBaseLibIsNodeInList ( }
>> > > >
>> > > > /**
>> > > > + Checks whether Node is part of a doubly-linked list.
>> > > > +
>> > > > + If List is NULL, then ASSERT().
>> > > > + If List->ForwardLink is NULL, then ASSERT().
>> > > > + If List->BackLink is NULL, then ASSERT().
>> > > > + If Node is NULL, then ASSERT(); If PcdMaximumLinkedListLength
>> > > > + is not zero, and List contains
>> > > > more than
>> > > > + PcdMaximumLinkedListLength nodes, then ASSERT().
>> > > > +
>> > > > + @param List A pointer to a node in a linked list.
>> > > > + @param Node A pointer to the node to locate.
>> > > > +
>> > > > + @retval TRUE Node is in List.
>> > > > + @retval FALSE Node isn't in List, or List is invalid.
>> > > > +
>> > > > +**/
>> > > > +BOOLEAN
>> > > > +EFIAPI
>> > > > +IsNodeInList (
>> > > > + IN CONST LIST_ENTRY *List,
>> > > > + IN CONST LIST_ENTRY *Node
>> > > > + )
>> > > > +{
>> > > > + UINTN Count;
>> > > > + CONST LIST_ENTRY *Ptr;
>> > > > +
>> > > > + //
>> > > > + // Test the validity of List and Node // ASSERT (List !=
>> > > > + NULL); ASSERT (List->ForwardLink != NULL); ASSERT
>> > > > + (List->BackLink != NULL); ASSERT (Node != NULL);
>> > > > +
>> > > > + //
>> > > > + // ASSERT List not too long
>> > > > + //
>> > > > + ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE));
>> > > > +
>> > > > + Count = 0;
>> > > > + Ptr = List;
>> > > > +
>> > > > + //
>> > > > + // Check to see if Node is a member of List.
>> > > > + // Exit early if the number of nodes in List >=
>> > > > PcdMaximumLinkedListLength
>> > > > + //
>> > > > + do {
>> > > > + Ptr = Ptr->ForwardLink;
>> > > > + if (PcdGet32 (PcdMaximumLinkedListLength) > 0) {
>> > > > + Count++;
>> > > > +
>> > > > + //
>> > > > + // Return if the linked list is too long
>> > > > + //
>> > > > + if (Count == PcdGet32 (PcdMaximumLinkedListLength)) {
>> > > > + return (BOOLEAN)(Ptr == Node);
>> > > > + }
>> > > > + }
>> > > > +
>> > > > + if (Ptr == Node) {
>> > > > + return TRUE;
>> > > > + }
>> > > > + } while (Ptr != List);
>> > > > +
>> > > > + return FALSE;
>> > > > +}
>> > > > +
>> > > > +/**
>> > > > Initializes the head node of a doubly-linked list, and returns
>> > > > the pointer to
>> > > > the head node of the doubly-linked list.
>> > > >
>> > > > diff --git a/MdePkg/Include/Library/BaseLib.h
>> > > > b/MdePkg/Include/Library/BaseLib.h
>> > > > index 791849b80406..4f3f4fd51f3f 100644
>> > > > --- a/MdePkg/Include/Library/BaseLib.h
>> > > > +++ b/MdePkg/Include/Library/BaseLib.h
>> > > > @@ -2869,6 +2869,31 @@ PathCleanUpDirectories(
>> > > >
>> > > >
>> > > > /**
>> > > > + Checks whether Node is part of a doubly-linked list.
>> > > > +
>> > > > + If List is NULL, then ASSERT().
>> > > > + If List->ForwardLink is NULL, then ASSERT().
>> > > > + If List->BackLink is NULL, then ASSERT().
>> > > > + If Node is NULL, then ASSERT(); If PcdMaximumLinkedListLength
>> > > > + is not zero, and List contains
>> > > > more than
>> > > > + PcdMaximumLinkedListLength nodes, then ASSERT().
>> > > > +
>> > > > + @param List A pointer to a node in a linked list.
>> > > > + @param Node A pointer to the node to locate.
>> > > > +
>> > > > + @retval TRUE Node is in List.
>> > > > + @retval FALSE Node isn't in List, or List is invalid.
>> > > > +
>> > > > +**/
>> > > > +BOOLEAN
>> > > > +EFIAPI
>> > > > +IsNodeInList (
>> > > > + IN CONST LIST_ENTRY *List,
>> > > > + IN CONST LIST_ENTRY *Node
>> > > > + );
>> > > > +
>> > > > +
>> > > > +/**
>> > > > Initializes the head node of a doubly linked list, and returns
>> > > > the pointer to
>> > > > the head node of the doubly linked list.
>> > > >
>> > > > diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h
>> > > > b/MdePkg/Library/BaseLib/BaseLibInternals.h
>> > > > index ea387ce37d27..9dca97a0dcc9 100644
>> > > > --- a/MdePkg/Library/BaseLib/BaseLibInternals.h
>> > > > +++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
>> > > > @@ -340,34 +340,6 @@ InternalSwitchStack (
>> > > >
>> > > >
>> > > > /**
>> > > > - Worker function that locates the Node in the List.
>> > > > -
>> > > > - By searching the List, finds the location of the Node in List.
>> > > > At the same time,
>> > > > - verifies the validity of this list.
>> > > > -
>> > > > - If List is NULL, then ASSERT().
>> > > > - If List->ForwardLink is NULL, then ASSERT().
>> > > > - If List->backLink is NULL, then ASSERT().
>> > > > - If Node is NULL, then ASSERT();
>> > > > - If PcdMaximumLinkedListLength is not zero, and prior to
>> > > > insertion the number
>> > > > - of nodes in ListHead, including the ListHead node, is greater
>> > > > than or
>> > > > - equal to PcdMaximumLinkedListLength, then ASSERT().
>> > > > -
>> > > > - @param List A pointer to a node in a linked list.
>> > > > - @param Node A pointer to one nod.
>> > > > -
>> > > > - @retval TRUE Node is in List.
>> > > > - @retval FALSE Node isn't in List, or List is invalid.
>> > > > -
>> > > > -**/
>> > > > -BOOLEAN
>> > > > -EFIAPI
>> > > > -IsNodeInList (
>> > > > - IN CONST LIST_ENTRY *List,
>> > > > - IN CONST LIST_ENTRY *Node
>> > > > - );
>> > > > -
>> > > > -/**
>> > > > Worker function that returns a bit field from Operand.
>> > > >
>> > > > Returns the bitfield specified by the StartBit and the EndBit
>> > > > from Operand.
>> > > > --
>> > > > 2.12.2.windows.2
>> >
>> > _______________________________________________
>> > edk2-devel mailing list
>> > edk2-devel@lists.01.org
>> > https://lists.01.org/mailman/listinfo/edk2-devel
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
2017-08-02 12:12 ` Gao, Liming
@ 2017-08-02 12:23 ` Marvin Häuser
2017-08-02 12:28 ` Gao, Liming
0 siblings, 1 reply; 8+ messages in thread
From: Marvin Häuser @ 2017-08-02 12:23 UTC (permalink / raw)
To: edk2-devel@lists.01.org; +Cc: Gao, Liming, michael.d.kinney@intel.com
Hey Liming,
Thanks for your reply! Sure, that works as well, though usually I prefer to write a static function or a macro instead of repeating.
If you prefer this, I'll get a V2 ready that way. Though shouldn't it be ASSERT ((!FeaturePcdGet (PcdVerifyNodeInList) || IsNodeInList (FirstEntry, SecondEntry)) && InternalBaseLibIsListValid (FirstEntry)); ?
Correct me if I'm wrong, but wouldn't your suggestion not ASSERT if the feature is enabled, the entry is not in the list, but the list is valid? ((TRUE && FALSE) || TRUE) would return TRUE.
Thanks,
Marvin.
> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Wednesday, August 2, 2017 2:12 PM
> To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>
> Marvin:
> How about directly use PCD checker with IsNodeInList()?
>
> ASSERT (IsNodeInList (FirstEntry, SecondEntry)); ==> ASSERT
> (FeaturePcdGet (PcdVerifyNodeInList) && IsNodeInList (FirstEntry,
> SecondEntry))|| InternalBaseLibIsListValid (FirstEntry));
>
> Thanks
> Liming
> >-----Original Message-----
> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >Marvin H?user
> >Sent: Friday, July 28, 2017 12:20 AM
> >To: edk2-devel@lists.01.org
> >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> ><liming.gao@intel.com>
> >Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList()
> function.
> >
> >Hi Liming,
> >
> >Comments are inline. Thanks for your response!
> >
> >Regards,
> >Marvin.
> >
> >> -----Original Message-----
> >> From: Gao, Liming [mailto:liming.gao@intel.com]
> >> Sent: Thursday, July 27, 2017 5:32 PM
> >> To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> >> Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
> >>
> >> Marvin:
> >> Once this API is exposed as the public library API, what code will
> >> you
> >update
> >> to consume this API?
> >
> >The code which gave me the idea for this patch is this:
> >https://github.com/tianocore/edk2-platforms/blob/devel-
> >MinPlatform/Silicon/Intel/KabylakeSiliconPkg/Pch/PchSmiDispatcher/Smm/
> I
> >o
> >Trap.c#L508
> >ForwardLink is obviously checked against EFI_BAD_POINTER to verify
> >whether the entry is still valid. Though, after being freed, the value
> >stored in this location may just be overwritten.
> >The code linked currently doesn't seem to do anything as ForwardLink is
> >never set to EFI_BAD_POINTER, though it can be updated with
> >IsNodeInList() to function (regardless of the entry being freed).
> >It seems to me that this is a leftover from the EDK era as EDK's
> >RemoveEntryList() does set the Links to EFI_BAD_POINTER.
> >
> >>
> >> And, for the patch, I have two comments.
> >> 1) Original logic uses PcdVerifyNodeInList to turn on the
> >> verification of NodeInList. New logic always turns on it. Because
> >> this check takes much overhead, this PCD is FALSE as the default value in
> MdePkg.dec.
> >> 2) In IsListEmpty(), ASSERT() is missing for
> >> InternalBaseLibIsListValid (ListHead).
> >
> >Sorry for these oversights. Would you prefer an internal, static
> >function or a macro to do the PCD check and following the ASSERTin v2?
> >
> >>
> >> Thanks
> >> Liming
> >> > -----Original Message-----
> >> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >> > Of Marvin H?user
> >> > Sent: Tuesday, July 25, 2017 12:53 AM
> >> > To: edk2-devel@lists.01.org
> >> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
> >> > Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList()
> >> function.
> >> >
> >> > Hey Mike,
> >> >
> >> > In contrast to setting 'BackLink' and 'ForwardLink' to dummy values
> >> > such as EFI_BAD_POINTER, verifying that a node is part of a known
> >> > list is a safe way to determine whether a link is still valid
> >> > without needing to keep the memory allocated. If the Node, which
> >> > had its links set to dummys, is freed, the dummy values may be
> >> > overwriten and the check would pass despite the node being invalid.
> >> > This can, for example, be
> >> used in OpenKabylake's PchSmiDispatcher, which does the described
> >> check against EFI_BAD_POINTER (though if I remember correctly, the
> >> value is actually never explicitely set). The issue is that some
> >> function may pass a handle that was unregistered to the function.
> >> IsNodeInList() can be used to determine whether it was previously
> removed.
> >> >
> >> > Thanks for your response!
> >> >
> >> > Regards,
> >> > Marvin.
> >> >
> >> > > -----Original Message-----
> >> > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> >> > > Sent: Monday, July 24, 2017 6:31 PM
> >> > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> >> > > devel@lists.01.org; Kinney, Michael D
> >> > > <michael.d.kinney@intel.com>
> >> > > Cc: Gao, Liming <liming.gao@intel.com>
> >> > > Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList()
> function.
> >> > >
> >> > > Hi Marvin,
> >> > >
> >> > > Can you provide a few more details on why you would like to see
> >> > > tis internal function promoted to a library class API?
> >> > >
> >> > > Thanks,
> >> > >
> >> > > Mike
> >> > >
> >> > > > -----Original Message-----
> >> > > > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com]
> >> > > > Sent: Sunday, July 23, 2017 3:11 AM
> >> > > > To: edk2-devel@lists.01.org
> >> > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> >> > > > <liming.gao@intel.com>
> >> > > > Subject: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
> >> > > >
> >> > > > This patch adds IsNodeInList() to BaseLib, which verifies the
> >> > > > given Node is part of the doubly-linked List provided.
> >> > > >
> >> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > > > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> >> > > > ---
> >> > > > MdePkg/Library/BaseLib/LinkedList.c | 70
> >> > > > +++++++++++++++++++-
> >> > > > MdePkg/Include/Library/BaseLib.h | 25 +++++++
> >> > > > MdePkg/Library/BaseLib/BaseLibInternals.h | 28 --------
> >> > > > 3 files changed, 94 insertions(+), 29 deletions(-)
> >> > > >
> >> > > > diff --git a/MdePkg/Library/BaseLib/LinkedList.c
> >> > > > b/MdePkg/Library/BaseLib/LinkedList.c
> >> > > > index ba373f4b7be3..b364ae41c647 100644
> >> > > > --- a/MdePkg/Library/BaseLib/LinkedList.c
> >> > > > +++ b/MdePkg/Library/BaseLib/LinkedList.c
> >> > > > @@ -1,7 +1,7 @@
> >> > > > /** @file
> >> > > > Linked List Library Functions.
> >> > > >
> >> > > > - Copyright (c) 2006 - 2013, Intel Corporation. All rights
> >> > > > reserved.<BR>
> >> > > > + Copyright (c) 2006 - 2017, Intel Corporation. All rights
> >> > > > reserved.<BR>
> >> > > > This program and the accompanying materials
> >> > > > are licensed and made available under the terms and
> >> > > > conditions of the BSD License
> >> > > > which accompanies this distribution. The full text of the
> >> > > > license may be found at @@ -113,6 +113,74 @@
> >> > > > InternalBaseLibIsNodeInList ( }
> >> > > >
> >> > > > /**
> >> > > > + Checks whether Node is part of a doubly-linked list.
> >> > > > +
> >> > > > + If List is NULL, then ASSERT().
> >> > > > + If List->ForwardLink is NULL, then ASSERT().
> >> > > > + If List->BackLink is NULL, then ASSERT().
> >> > > > + If Node is NULL, then ASSERT(); If
> >> > > > + PcdMaximumLinkedListLength is not zero, and List contains
> >> > > > more than
> >> > > > + PcdMaximumLinkedListLength nodes, then ASSERT().
> >> > > > +
> >> > > > + @param List A pointer to a node in a linked list.
> >> > > > + @param Node A pointer to the node to locate.
> >> > > > +
> >> > > > + @retval TRUE Node is in List.
> >> > > > + @retval FALSE Node isn't in List, or List is invalid.
> >> > > > +
> >> > > > +**/
> >> > > > +BOOLEAN
> >> > > > +EFIAPI
> >> > > > +IsNodeInList (
> >> > > > + IN CONST LIST_ENTRY *List,
> >> > > > + IN CONST LIST_ENTRY *Node
> >> > > > + )
> >> > > > +{
> >> > > > + UINTN Count;
> >> > > > + CONST LIST_ENTRY *Ptr;
> >> > > > +
> >> > > > + //
> >> > > > + // Test the validity of List and Node // ASSERT (List !=
> >> > > > + NULL); ASSERT (List->ForwardLink != NULL); ASSERT
> >> > > > + (List->BackLink != NULL); ASSERT (Node != NULL);
> >> > > > +
> >> > > > + //
> >> > > > + // ASSERT List not too long
> >> > > > + //
> >> > > > + ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry,
> >> > > > + FALSE));
> >> > > > +
> >> > > > + Count = 0;
> >> > > > + Ptr = List;
> >> > > > +
> >> > > > + //
> >> > > > + // Check to see if Node is a member of List.
> >> > > > + // Exit early if the number of nodes in List >=
> >> > > > PcdMaximumLinkedListLength
> >> > > > + //
> >> > > > + do {
> >> > > > + Ptr = Ptr->ForwardLink;
> >> > > > + if (PcdGet32 (PcdMaximumLinkedListLength) > 0) {
> >> > > > + Count++;
> >> > > > +
> >> > > > + //
> >> > > > + // Return if the linked list is too long
> >> > > > + //
> >> > > > + if (Count == PcdGet32 (PcdMaximumLinkedListLength)) {
> >> > > > + return (BOOLEAN)(Ptr == Node);
> >> > > > + }
> >> > > > + }
> >> > > > +
> >> > > > + if (Ptr == Node) {
> >> > > > + return TRUE;
> >> > > > + }
> >> > > > + } while (Ptr != List);
> >> > > > +
> >> > > > + return FALSE;
> >> > > > +}
> >> > > > +
> >> > > > +/**
> >> > > > Initializes the head node of a doubly-linked list, and
> >> > > > returns the pointer to
> >> > > > the head node of the doubly-linked list.
> >> > > >
> >> > > > diff --git a/MdePkg/Include/Library/BaseLib.h
> >> > > > b/MdePkg/Include/Library/BaseLib.h
> >> > > > index 791849b80406..4f3f4fd51f3f 100644
> >> > > > --- a/MdePkg/Include/Library/BaseLib.h
> >> > > > +++ b/MdePkg/Include/Library/BaseLib.h
> >> > > > @@ -2869,6 +2869,31 @@ PathCleanUpDirectories(
> >> > > >
> >> > > >
> >> > > > /**
> >> > > > + Checks whether Node is part of a doubly-linked list.
> >> > > > +
> >> > > > + If List is NULL, then ASSERT().
> >> > > > + If List->ForwardLink is NULL, then ASSERT().
> >> > > > + If List->BackLink is NULL, then ASSERT().
> >> > > > + If Node is NULL, then ASSERT(); If
> >> > > > + PcdMaximumLinkedListLength is not zero, and List contains
> >> > > > more than
> >> > > > + PcdMaximumLinkedListLength nodes, then ASSERT().
> >> > > > +
> >> > > > + @param List A pointer to a node in a linked list.
> >> > > > + @param Node A pointer to the node to locate.
> >> > > > +
> >> > > > + @retval TRUE Node is in List.
> >> > > > + @retval FALSE Node isn't in List, or List is invalid.
> >> > > > +
> >> > > > +**/
> >> > > > +BOOLEAN
> >> > > > +EFIAPI
> >> > > > +IsNodeInList (
> >> > > > + IN CONST LIST_ENTRY *List,
> >> > > > + IN CONST LIST_ENTRY *Node
> >> > > > + );
> >> > > > +
> >> > > > +
> >> > > > +/**
> >> > > > Initializes the head node of a doubly linked list, and
> >> > > > returns the pointer to
> >> > > > the head node of the doubly linked list.
> >> > > >
> >> > > > diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h
> >> > > > b/MdePkg/Library/BaseLib/BaseLibInternals.h
> >> > > > index ea387ce37d27..9dca97a0dcc9 100644
> >> > > > --- a/MdePkg/Library/BaseLib/BaseLibInternals.h
> >> > > > +++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
> >> > > > @@ -340,34 +340,6 @@ InternalSwitchStack (
> >> > > >
> >> > > >
> >> > > > /**
> >> > > > - Worker function that locates the Node in the List.
> >> > > > -
> >> > > > - By searching the List, finds the location of the Node in List.
> >> > > > At the same time,
> >> > > > - verifies the validity of this list.
> >> > > > -
> >> > > > - If List is NULL, then ASSERT().
> >> > > > - If List->ForwardLink is NULL, then ASSERT().
> >> > > > - If List->backLink is NULL, then ASSERT().
> >> > > > - If Node is NULL, then ASSERT();
> >> > > > - If PcdMaximumLinkedListLength is not zero, and prior to
> >> > > > insertion the number
> >> > > > - of nodes in ListHead, including the ListHead node, is
> >> > > > greater than or
> >> > > > - equal to PcdMaximumLinkedListLength, then ASSERT().
> >> > > > -
> >> > > > - @param List A pointer to a node in a linked list.
> >> > > > - @param Node A pointer to one nod.
> >> > > > -
> >> > > > - @retval TRUE Node is in List.
> >> > > > - @retval FALSE Node isn't in List, or List is invalid.
> >> > > > -
> >> > > > -**/
> >> > > > -BOOLEAN
> >> > > > -EFIAPI
> >> > > > -IsNodeInList (
> >> > > > - IN CONST LIST_ENTRY *List,
> >> > > > - IN CONST LIST_ENTRY *Node
> >> > > > - );
> >> > > > -
> >> > > > -/**
> >> > > > Worker function that returns a bit field from Operand.
> >> > > >
> >> > > > Returns the bitfield specified by the StartBit and the
> >> > > > EndBit from Operand.
> >> > > > --
> >> > > > 2.12.2.windows.2
> >> >
> >> > _______________________________________________
> >> > edk2-devel mailing list
> >> > edk2-devel@lists.01.org
> >> > https://lists.01.org/mailman/listinfo/edk2-devel
> >_______________________________________________
> >edk2-devel mailing list
> >edk2-devel@lists.01.org
> >https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
2017-08-02 12:23 ` Marvin Häuser
@ 2017-08-02 12:28 ` Gao, Liming
0 siblings, 0 replies; 8+ messages in thread
From: Gao, Liming @ 2017-08-02 12:28 UTC (permalink / raw)
To: Marvin.Haeuser@outlook.com, edk2-devel@lists.01.org; +Cc: Kinney, Michael D
Thanks for your correctness. I agree MACRO is better.
>-----Original Message-----
>From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com]
>Sent: Wednesday, August 02, 2017 8:24 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
><michael.d.kinney@intel.com>
>Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>
>Hey Liming,
>
>Thanks for your reply! Sure, that works as well, though usually I prefer to
>write a static function or a macro instead of repeating.
>If you prefer this, I'll get a V2 ready that way. Though shouldn't it be ASSERT
>((!FeaturePcdGet (PcdVerifyNodeInList) || IsNodeInList (FirstEntry,
>SecondEntry)) && InternalBaseLibIsListValid (FirstEntry)); ?
>Correct me if I'm wrong, but wouldn't your suggestion not ASSERT if the
>feature is enabled, the entry is not in the list, but the list is valid? ((TRUE &&
>FALSE) || TRUE) would return TRUE.
>
>Thanks,
>Marvin.
>
>> -----Original Message-----
>> From: Gao, Liming [mailto:liming.gao@intel.com]
>> Sent: Wednesday, August 2, 2017 2:12 PM
>> To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
>> Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>>
>> Marvin:
>> How about directly use PCD checker with IsNodeInList()?
>>
>> ASSERT (IsNodeInList (FirstEntry, SecondEntry)); ==> ASSERT
>> (FeaturePcdGet (PcdVerifyNodeInList) && IsNodeInList (FirstEntry,
>> SecondEntry))|| InternalBaseLibIsListValid (FirstEntry));
>>
>> Thanks
>> Liming
>> >-----Original Message-----
>> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> >Marvin H?user
>> >Sent: Friday, July 28, 2017 12:20 AM
>> >To: edk2-devel@lists.01.org
>> >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> ><liming.gao@intel.com>
>> >Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList()
>> function.
>> >
>> >Hi Liming,
>> >
>> >Comments are inline. Thanks for your response!
>> >
>> >Regards,
>> >Marvin.
>> >
>> >> -----Original Message-----
>> >> From: Gao, Liming [mailto:liming.gao@intel.com]
>> >> Sent: Thursday, July 27, 2017 5:32 PM
>> >> To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
>> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>
>> >> Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>> >>
>> >> Marvin:
>> >> Once this API is exposed as the public library API, what code will
>> >> you
>> >update
>> >> to consume this API?
>> >
>> >The code which gave me the idea for this patch is this:
>> >https://github.com/tianocore/edk2-platforms/blob/devel-
>> >MinPlatform/Silicon/Intel/KabylakeSiliconPkg/Pch/PchSmiDispatcher/Smm
>/
>> I
>> >o
>> >Trap.c#L508
>> >ForwardLink is obviously checked against EFI_BAD_POINTER to verify
>> >whether the entry is still valid. Though, after being freed, the value
>> >stored in this location may just be overwritten.
>> >The code linked currently doesn't seem to do anything as ForwardLink is
>> >never set to EFI_BAD_POINTER, though it can be updated with
>> >IsNodeInList() to function (regardless of the entry being freed).
>> >It seems to me that this is a leftover from the EDK era as EDK's
>> >RemoveEntryList() does set the Links to EFI_BAD_POINTER.
>> >
>> >>
>> >> And, for the patch, I have two comments.
>> >> 1) Original logic uses PcdVerifyNodeInList to turn on the
>> >> verification of NodeInList. New logic always turns on it. Because
>> >> this check takes much overhead, this PCD is FALSE as the default value in
>> MdePkg.dec.
>> >> 2) In IsListEmpty(), ASSERT() is missing for
>> >> InternalBaseLibIsListValid (ListHead).
>> >
>> >Sorry for these oversights. Would you prefer an internal, static
>> >function or a macro to do the PCD check and following the ASSERTin v2?
>> >
>> >>
>> >> Thanks
>> >> Liming
>> >> > -----Original Message-----
>> >> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>> >> > Of Marvin H?user
>> >> > Sent: Tuesday, July 25, 2017 12:53 AM
>> >> > To: edk2-devel@lists.01.org
>> >> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>
>> >> > Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList()
>> >> function.
>> >> >
>> >> > Hey Mike,
>> >> >
>> >> > In contrast to setting 'BackLink' and 'ForwardLink' to dummy values
>> >> > such as EFI_BAD_POINTER, verifying that a node is part of a known
>> >> > list is a safe way to determine whether a link is still valid
>> >> > without needing to keep the memory allocated. If the Node, which
>> >> > had its links set to dummys, is freed, the dummy values may be
>> >> > overwriten and the check would pass despite the node being invalid.
>> >> > This can, for example, be
>> >> used in OpenKabylake's PchSmiDispatcher, which does the described
>> >> check against EFI_BAD_POINTER (though if I remember correctly, the
>> >> value is actually never explicitely set). The issue is that some
>> >> function may pass a handle that was unregistered to the function.
>> >> IsNodeInList() can be used to determine whether it was previously
>> removed.
>> >> >
>> >> > Thanks for your response!
>> >> >
>> >> > Regards,
>> >> > Marvin.
>> >> >
>> >> > > -----Original Message-----
>> >> > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
>> >> > > Sent: Monday, July 24, 2017 6:31 PM
>> >> > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
>> >> > > devel@lists.01.org; Kinney, Michael D
>> >> > > <michael.d.kinney@intel.com>
>> >> > > Cc: Gao, Liming <liming.gao@intel.com>
>> >> > > Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList()
>> function.
>> >> > >
>> >> > > Hi Marvin,
>> >> > >
>> >> > > Can you provide a few more details on why you would like to see
>> >> > > tis internal function promoted to a library class API?
>> >> > >
>> >> > > Thanks,
>> >> > >
>> >> > > Mike
>> >> > >
>> >> > > > -----Original Message-----
>> >> > > > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com]
>> >> > > > Sent: Sunday, July 23, 2017 3:11 AM
>> >> > > > To: edk2-devel@lists.01.org
>> >> > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> >> > > > <liming.gao@intel.com>
>> >> > > > Subject: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function.
>> >> > > >
>> >> > > > This patch adds IsNodeInList() to BaseLib, which verifies the
>> >> > > > given Node is part of the doubly-linked List provided.
>> >> > > >
>> >> > > > Contributed-under: TianoCore Contribution Agreement 1.1
>> >> > > > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
>> >> > > > ---
>> >> > > > MdePkg/Library/BaseLib/LinkedList.c | 70
>> >> > > > +++++++++++++++++++-
>> >> > > > MdePkg/Include/Library/BaseLib.h | 25 +++++++
>> >> > > > MdePkg/Library/BaseLib/BaseLibInternals.h | 28 --------
>> >> > > > 3 files changed, 94 insertions(+), 29 deletions(-)
>> >> > > >
>> >> > > > diff --git a/MdePkg/Library/BaseLib/LinkedList.c
>> >> > > > b/MdePkg/Library/BaseLib/LinkedList.c
>> >> > > > index ba373f4b7be3..b364ae41c647 100644
>> >> > > > --- a/MdePkg/Library/BaseLib/LinkedList.c
>> >> > > > +++ b/MdePkg/Library/BaseLib/LinkedList.c
>> >> > > > @@ -1,7 +1,7 @@
>> >> > > > /** @file
>> >> > > > Linked List Library Functions.
>> >> > > >
>> >> > > > - Copyright (c) 2006 - 2013, Intel Corporation. All rights
>> >> > > > reserved.<BR>
>> >> > > > + Copyright (c) 2006 - 2017, Intel Corporation. All rights
>> >> > > > reserved.<BR>
>> >> > > > This program and the accompanying materials
>> >> > > > are licensed and made available under the terms and
>> >> > > > conditions of the BSD License
>> >> > > > which accompanies this distribution. The full text of the
>> >> > > > license may be found at @@ -113,6 +113,74 @@
>> >> > > > InternalBaseLibIsNodeInList ( }
>> >> > > >
>> >> > > > /**
>> >> > > > + Checks whether Node is part of a doubly-linked list.
>> >> > > > +
>> >> > > > + If List is NULL, then ASSERT().
>> >> > > > + If List->ForwardLink is NULL, then ASSERT().
>> >> > > > + If List->BackLink is NULL, then ASSERT().
>> >> > > > + If Node is NULL, then ASSERT(); If
>> >> > > > + PcdMaximumLinkedListLength is not zero, and List contains
>> >> > > > more than
>> >> > > > + PcdMaximumLinkedListLength nodes, then ASSERT().
>> >> > > > +
>> >> > > > + @param List A pointer to a node in a linked list.
>> >> > > > + @param Node A pointer to the node to locate.
>> >> > > > +
>> >> > > > + @retval TRUE Node is in List.
>> >> > > > + @retval FALSE Node isn't in List, or List is invalid.
>> >> > > > +
>> >> > > > +**/
>> >> > > > +BOOLEAN
>> >> > > > +EFIAPI
>> >> > > > +IsNodeInList (
>> >> > > > + IN CONST LIST_ENTRY *List,
>> >> > > > + IN CONST LIST_ENTRY *Node
>> >> > > > + )
>> >> > > > +{
>> >> > > > + UINTN Count;
>> >> > > > + CONST LIST_ENTRY *Ptr;
>> >> > > > +
>> >> > > > + //
>> >> > > > + // Test the validity of List and Node // ASSERT (List !=
>> >> > > > + NULL); ASSERT (List->ForwardLink != NULL); ASSERT
>> >> > > > + (List->BackLink != NULL); ASSERT (Node != NULL);
>> >> > > > +
>> >> > > > + //
>> >> > > > + // ASSERT List not too long
>> >> > > > + //
>> >> > > > + ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry,
>> >> > > > + FALSE));
>> >> > > > +
>> >> > > > + Count = 0;
>> >> > > > + Ptr = List;
>> >> > > > +
>> >> > > > + //
>> >> > > > + // Check to see if Node is a member of List.
>> >> > > > + // Exit early if the number of nodes in List >=
>> >> > > > PcdMaximumLinkedListLength
>> >> > > > + //
>> >> > > > + do {
>> >> > > > + Ptr = Ptr->ForwardLink;
>> >> > > > + if (PcdGet32 (PcdMaximumLinkedListLength) > 0) {
>> >> > > > + Count++;
>> >> > > > +
>> >> > > > + //
>> >> > > > + // Return if the linked list is too long
>> >> > > > + //
>> >> > > > + if (Count == PcdGet32 (PcdMaximumLinkedListLength)) {
>> >> > > > + return (BOOLEAN)(Ptr == Node);
>> >> > > > + }
>> >> > > > + }
>> >> > > > +
>> >> > > > + if (Ptr == Node) {
>> >> > > > + return TRUE;
>> >> > > > + }
>> >> > > > + } while (Ptr != List);
>> >> > > > +
>> >> > > > + return FALSE;
>> >> > > > +}
>> >> > > > +
>> >> > > > +/**
>> >> > > > Initializes the head node of a doubly-linked list, and
>> >> > > > returns the pointer to
>> >> > > > the head node of the doubly-linked list.
>> >> > > >
>> >> > > > diff --git a/MdePkg/Include/Library/BaseLib.h
>> >> > > > b/MdePkg/Include/Library/BaseLib.h
>> >> > > > index 791849b80406..4f3f4fd51f3f 100644
>> >> > > > --- a/MdePkg/Include/Library/BaseLib.h
>> >> > > > +++ b/MdePkg/Include/Library/BaseLib.h
>> >> > > > @@ -2869,6 +2869,31 @@ PathCleanUpDirectories(
>> >> > > >
>> >> > > >
>> >> > > > /**
>> >> > > > + Checks whether Node is part of a doubly-linked list.
>> >> > > > +
>> >> > > > + If List is NULL, then ASSERT().
>> >> > > > + If List->ForwardLink is NULL, then ASSERT().
>> >> > > > + If List->BackLink is NULL, then ASSERT().
>> >> > > > + If Node is NULL, then ASSERT(); If
>> >> > > > + PcdMaximumLinkedListLength is not zero, and List contains
>> >> > > > more than
>> >> > > > + PcdMaximumLinkedListLength nodes, then ASSERT().
>> >> > > > +
>> >> > > > + @param List A pointer to a node in a linked list.
>> >> > > > + @param Node A pointer to the node to locate.
>> >> > > > +
>> >> > > > + @retval TRUE Node is in List.
>> >> > > > + @retval FALSE Node isn't in List, or List is invalid.
>> >> > > > +
>> >> > > > +**/
>> >> > > > +BOOLEAN
>> >> > > > +EFIAPI
>> >> > > > +IsNodeInList (
>> >> > > > + IN CONST LIST_ENTRY *List,
>> >> > > > + IN CONST LIST_ENTRY *Node
>> >> > > > + );
>> >> > > > +
>> >> > > > +
>> >> > > > +/**
>> >> > > > Initializes the head node of a doubly linked list, and
>> >> > > > returns the pointer to
>> >> > > > the head node of the doubly linked list.
>> >> > > >
>> >> > > > diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h
>> >> > > > b/MdePkg/Library/BaseLib/BaseLibInternals.h
>> >> > > > index ea387ce37d27..9dca97a0dcc9 100644
>> >> > > > --- a/MdePkg/Library/BaseLib/BaseLibInternals.h
>> >> > > > +++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
>> >> > > > @@ -340,34 +340,6 @@ InternalSwitchStack (
>> >> > > >
>> >> > > >
>> >> > > > /**
>> >> > > > - Worker function that locates the Node in the List.
>> >> > > > -
>> >> > > > - By searching the List, finds the location of the Node in List.
>> >> > > > At the same time,
>> >> > > > - verifies the validity of this list.
>> >> > > > -
>> >> > > > - If List is NULL, then ASSERT().
>> >> > > > - If List->ForwardLink is NULL, then ASSERT().
>> >> > > > - If List->backLink is NULL, then ASSERT().
>> >> > > > - If Node is NULL, then ASSERT();
>> >> > > > - If PcdMaximumLinkedListLength is not zero, and prior to
>> >> > > > insertion the number
>> >> > > > - of nodes in ListHead, including the ListHead node, is
>> >> > > > greater than or
>> >> > > > - equal to PcdMaximumLinkedListLength, then ASSERT().
>> >> > > > -
>> >> > > > - @param List A pointer to a node in a linked list.
>> >> > > > - @param Node A pointer to one nod.
>> >> > > > -
>> >> > > > - @retval TRUE Node is in List.
>> >> > > > - @retval FALSE Node isn't in List, or List is invalid.
>> >> > > > -
>> >> > > > -**/
>> >> > > > -BOOLEAN
>> >> > > > -EFIAPI
>> >> > > > -IsNodeInList (
>> >> > > > - IN CONST LIST_ENTRY *List,
>> >> > > > - IN CONST LIST_ENTRY *Node
>> >> > > > - );
>> >> > > > -
>> >> > > > -/**
>> >> > > > Worker function that returns a bit field from Operand.
>> >> > > >
>> >> > > > Returns the bitfield specified by the StartBit and the
>> >> > > > EndBit from Operand.
>> >> > > > --
>> >> > > > 2.12.2.windows.2
>> >> >
>> >> > _______________________________________________
>> >> > edk2-devel mailing list
>> >> > edk2-devel@lists.01.org
>> >> > https://lists.01.org/mailman/listinfo/edk2-devel
>> >_______________________________________________
>> >edk2-devel mailing list
>> >edk2-devel@lists.01.org
>> >https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-08-02 12:26 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-23 10:10 [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function Marvin Häuser
2017-07-24 16:31 ` Kinney, Michael D
2017-07-24 16:52 ` Marvin Häuser
2017-07-27 15:31 ` Gao, Liming
2017-07-27 16:19 ` Marvin Häuser
2017-08-02 12:12 ` Gao, Liming
2017-08-02 12:23 ` Marvin Häuser
2017-08-02 12:28 ` Gao, Liming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox