* [PATCH] MdePkg/SafeString: Directly return when length of source string is 0 @ 2018-02-02 10:47 Ruiyu Ni 2018-02-02 13:42 ` Laszlo Ersek 2018-02-05 1:23 ` Wang, Jian J 0 siblings, 2 replies; 5+ messages in thread From: Ruiyu Ni @ 2018-02-02 10:47 UTC (permalink / raw) To: edk2-devel; +Cc: Jiewen Yao, Liming Gao, Jian J Wang Today's implementation of [Ascii]StrnCpyS/[Ascii]StrnCatS doesn't directly return the the length of source string is 0. When length of source string is 0, it means the Source points to a memory that shouldn't be deferenced at all. So it's not proper to call StrnLenS() in such situation. In a pool guard enabled environment, when using shell to edit an existing file which contains empty line, the page fault is met. The patch fixes the four library functions to align to the behavior of non-safe version: directly return when length of source string is 0. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Liming Gao <liming.gao@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> --- MdePkg/Library/BaseLib/SafeString.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c index 68c33e9b7b..fed818ef33 100644 --- a/MdePkg/Library/BaseLib/SafeString.c +++ b/MdePkg/Library/BaseLib/SafeString.c @@ -1,7 +1,7 @@ /** @file Safe String functions. - Copyright (c) 2014 - 2017, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2014 - 2018, 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 @@ -317,6 +317,10 @@ StrnCpyS ( { UINTN SourceLen; + if (Length == 0) { + return RETURN_SUCCESS; + } + ASSERT (((UINTN) Destination & BIT0) == 0); ASSERT (((UINTN) Source & BIT0) == 0); @@ -515,6 +519,10 @@ StrnCatS ( UINTN CopyLen; UINTN SourceLen; + if (Length == 0) { + return RETURN_SUCCESS; + } + ASSERT (((UINTN) Destination & BIT0) == 0); ASSERT (((UINTN) Source & BIT0) == 0); @@ -1894,6 +1902,10 @@ AsciiStrnCpyS ( { UINTN SourceLen; + if (Length == 0) { + return RETURN_SUCCESS; + } + // // 1. Neither Destination nor Source shall be a null pointer. // @@ -2082,6 +2094,10 @@ AsciiStrnCatS ( UINTN CopyLen; UINTN SourceLen; + if (Length == 0) { + return RETURN_SUCCESS; + } + // // Let CopyLen denote the value DestMax - AsciiStrnLenS(Destination, DestMax) upon entry to AsciiStrnCatS. // -- 2.16.1.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] MdePkg/SafeString: Directly return when length of source string is 0 2018-02-02 10:47 [PATCH] MdePkg/SafeString: Directly return when length of source string is 0 Ruiyu Ni @ 2018-02-02 13:42 ` Laszlo Ersek 2018-02-05 3:55 ` Yao, Jiewen 2018-02-05 1:23 ` Wang, Jian J 1 sibling, 1 reply; 5+ messages in thread From: Laszlo Ersek @ 2018-02-02 13:42 UTC (permalink / raw) To: Ruiyu Ni, edk2-devel; +Cc: Jiewen Yao, Liming Gao On 02/02/18 11:47, Ruiyu Ni wrote: > Today's implementation of [Ascii]StrnCpyS/[Ascii]StrnCatS doesn't > directly return the the length of source string is 0. > > When length of source string is 0, it means the Source points to > a memory that shouldn't be deferenced at all. > So it's not proper to call StrnLenS() in such situation. > In a pool guard enabled environment, when using shell to edit an > existing file which contains empty line, the page fault is met. > > The patch fixes the four library functions to align to the behavior > of non-safe version: directly return when length of source string > is 0. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > --- > MdePkg/Library/BaseLib/SafeString.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c > index 68c33e9b7b..fed818ef33 100644 > --- a/MdePkg/Library/BaseLib/SafeString.c > +++ b/MdePkg/Library/BaseLib/SafeString.c > @@ -1,7 +1,7 @@ > /** @file > Safe String functions. > > - Copyright (c) 2014 - 2017, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2014 - 2018, 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 > @@ -317,6 +317,10 @@ StrnCpyS ( > { > UINTN SourceLen; > > + if (Length == 0) { > + return RETURN_SUCCESS; > + } > + > ASSERT (((UINTN) Destination & BIT0) == 0); > ASSERT (((UINTN) Source & BIT0) == 0); > > @@ -515,6 +519,10 @@ StrnCatS ( > UINTN CopyLen; > UINTN SourceLen; > > + if (Length == 0) { > + return RETURN_SUCCESS; > + } > + > ASSERT (((UINTN) Destination & BIT0) == 0); > ASSERT (((UINTN) Source & BIT0) == 0); > > @@ -1894,6 +1902,10 @@ AsciiStrnCpyS ( > { > UINTN SourceLen; > > + if (Length == 0) { > + return RETURN_SUCCESS; > + } > + > // > // 1. Neither Destination nor Source shall be a null pointer. > // > @@ -2082,6 +2094,10 @@ AsciiStrnCatS ( > UINTN CopyLen; > UINTN SourceLen; > > + if (Length == 0) { > + return RETURN_SUCCESS; > + } > + > // > // Let CopyLen denote the value DestMax - AsciiStrnLenS(Destination, DestMax) upon entry to AsciiStrnCatS. > // > Reviewed-by: Laszlo Ersek <lersek@redhat.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MdePkg/SafeString: Directly return when length of source string is 0 2018-02-02 13:42 ` Laszlo Ersek @ 2018-02-05 3:55 ` Yao, Jiewen 2018-02-05 8:04 ` Ni, Ruiyu 0 siblings, 1 reply; 5+ messages in thread From: Yao, Jiewen @ 2018-02-05 3:55 UTC (permalink / raw) To: Laszlo Ersek, Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Gao, Liming Thanks to catch this. The root-cause of the failure is below line: SourceLen = StrnLenS (Source, DestMax); We should only limit the Source string access within Length, not DestMax, if Length is smaller than DestMax. 0 is just one special case. Length might be 1, 2, or 3 and it triggers same failure. So only checking 0 is not enough. Reviewed the code with Ruiyu. We think using below check seems better way to handle all cases. SourceLen = StrnLenS (Source, MIN(DestMax, Length)); The check for 0 is not needed. Thank you Yao Jiewen > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Friday, February 2, 2018 9:43 PM > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com> > Subject: Re: [edk2] [PATCH] MdePkg/SafeString: Directly return when length of > source string is 0 > > On 02/02/18 11:47, Ruiyu Ni wrote: > > Today's implementation of [Ascii]StrnCpyS/[Ascii]StrnCatS doesn't > > directly return the the length of source string is 0. > > > > When length of source string is 0, it means the Source points to > > a memory that shouldn't be deferenced at all. > > So it's not proper to call StrnLenS() in such situation. > > In a pool guard enabled environment, when using shell to edit an > > existing file which contains empty line, the page fault is met. > > > > The patch fixes the four library functions to align to the behavior > > of non-safe version: directly return when length of source string > > is 0. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com> > > Cc: Jiewen Yao <jiewen.yao@intel.com> > > Cc: Liming Gao <liming.gao@intel.com> > > Cc: Jian J Wang <jian.j.wang@intel.com> > > --- > > MdePkg/Library/BaseLib/SafeString.c | 18 +++++++++++++++++- > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/MdePkg/Library/BaseLib/SafeString.c > b/MdePkg/Library/BaseLib/SafeString.c > > index 68c33e9b7b..fed818ef33 100644 > > --- a/MdePkg/Library/BaseLib/SafeString.c > > +++ b/MdePkg/Library/BaseLib/SafeString.c > > @@ -1,7 +1,7 @@ > > /** @file > > Safe String functions. > > > > - Copyright (c) 2014 - 2017, Intel Corporation. All rights reserved.<BR> > > + Copyright (c) 2014 - 2018, 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 > > @@ -317,6 +317,10 @@ StrnCpyS ( > > { > > UINTN SourceLen; > > > > + if (Length == 0) { > > + return RETURN_SUCCESS; > > + } > > + > > ASSERT (((UINTN) Destination & BIT0) == 0); > > ASSERT (((UINTN) Source & BIT0) == 0); > > > > @@ -515,6 +519,10 @@ StrnCatS ( > > UINTN CopyLen; > > UINTN SourceLen; > > > > + if (Length == 0) { > > + return RETURN_SUCCESS; > > + } > > + > > ASSERT (((UINTN) Destination & BIT0) == 0); > > ASSERT (((UINTN) Source & BIT0) == 0); > > > > @@ -1894,6 +1902,10 @@ AsciiStrnCpyS ( > > { > > UINTN SourceLen; > > > > + if (Length == 0) { > > + return RETURN_SUCCESS; > > + } > > + > > // > > // 1. Neither Destination nor Source shall be a null pointer. > > // > > @@ -2082,6 +2094,10 @@ AsciiStrnCatS ( > > UINTN CopyLen; > > UINTN SourceLen; > > > > + if (Length == 0) { > > + return RETURN_SUCCESS; > > + } > > + > > // > > // Let CopyLen denote the value DestMax - AsciiStrnLenS(Destination, > DestMax) upon entry to AsciiStrnCatS. > > // > > > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MdePkg/SafeString: Directly return when length of source string is 0 2018-02-05 3:55 ` Yao, Jiewen @ 2018-02-05 8:04 ` Ni, Ruiyu 0 siblings, 0 replies; 5+ messages in thread From: Ni, Ruiyu @ 2018-02-05 8:04 UTC (permalink / raw) To: Yao, Jiewen, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Gao, Liming On 2/5/2018 11:55 AM, Yao, Jiewen wrote: > Thanks to catch this. > > The root-cause of the failure is below line: > > SourceLen = StrnLenS (Source, DestMax); > > We should only limit the Source string access within Length, not DestMax, if Length is smaller than DestMax. > > 0 is just one special case. Length might be 1, 2, or 3 and it triggers same failure. > > So only checking 0 is not enough. > > Reviewed the code with Ruiyu. We think using below check seems better way to handle all cases. > > SourceLen = StrnLenS (Source, MIN(DestMax, Length)); > > The check for 0 is not needed. Patch v2 is sent out. > > Thank you > Yao Jiewen > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Friday, February 2, 2018 9:43 PM >> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com> >> Subject: Re: [edk2] [PATCH] MdePkg/SafeString: Directly return when length of >> source string is 0 >> >> On 02/02/18 11:47, Ruiyu Ni wrote: >>> Today's implementation of [Ascii]StrnCpyS/[Ascii]StrnCatS doesn't >>> directly return the the length of source string is 0. >>> >>> When length of source string is 0, it means the Source points to >>> a memory that shouldn't be deferenced at all. >>> So it's not proper to call StrnLenS() in such situation. >>> In a pool guard enabled environment, when using shell to edit an >>> existing file which contains empty line, the page fault is met. >>> >>> The patch fixes the four library functions to align to the behavior >>> of non-safe version: directly return when length of source string >>> is 0. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com> >>> Cc: Jiewen Yao <jiewen.yao@intel.com> >>> Cc: Liming Gao <liming.gao@intel.com> >>> Cc: Jian J Wang <jian.j.wang@intel.com> >>> --- >>> MdePkg/Library/BaseLib/SafeString.c | 18 +++++++++++++++++- >>> 1 file changed, 17 insertions(+), 1 deletion(-) >>> >>> diff --git a/MdePkg/Library/BaseLib/SafeString.c >> b/MdePkg/Library/BaseLib/SafeString.c >>> index 68c33e9b7b..fed818ef33 100644 >>> --- a/MdePkg/Library/BaseLib/SafeString.c >>> +++ b/MdePkg/Library/BaseLib/SafeString.c >>> @@ -1,7 +1,7 @@ >>> /** @file >>> Safe String functions. >>> >>> - Copyright (c) 2014 - 2017, Intel Corporation. All rights reserved.<BR> >>> + Copyright (c) 2014 - 2018, 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 >>> @@ -317,6 +317,10 @@ StrnCpyS ( >>> { >>> UINTN SourceLen; >>> >>> + if (Length == 0) { >>> + return RETURN_SUCCESS; >>> + } >>> + >>> ASSERT (((UINTN) Destination & BIT0) == 0); >>> ASSERT (((UINTN) Source & BIT0) == 0); >>> >>> @@ -515,6 +519,10 @@ StrnCatS ( >>> UINTN CopyLen; >>> UINTN SourceLen; >>> >>> + if (Length == 0) { >>> + return RETURN_SUCCESS; >>> + } >>> + >>> ASSERT (((UINTN) Destination & BIT0) == 0); >>> ASSERT (((UINTN) Source & BIT0) == 0); >>> >>> @@ -1894,6 +1902,10 @@ AsciiStrnCpyS ( >>> { >>> UINTN SourceLen; >>> >>> + if (Length == 0) { >>> + return RETURN_SUCCESS; >>> + } >>> + >>> // >>> // 1. Neither Destination nor Source shall be a null pointer. >>> // >>> @@ -2082,6 +2094,10 @@ AsciiStrnCatS ( >>> UINTN CopyLen; >>> UINTN SourceLen; >>> >>> + if (Length == 0) { >>> + return RETURN_SUCCESS; >>> + } >>> + >>> // >>> // Let CopyLen denote the value DestMax - AsciiStrnLenS(Destination, >> DestMax) upon entry to AsciiStrnCatS. >>> // >>> >> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> -- Thanks, Ray ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MdePkg/SafeString: Directly return when length of source string is 0 2018-02-02 10:47 [PATCH] MdePkg/SafeString: Directly return when length of source string is 0 Ruiyu Ni 2018-02-02 13:42 ` Laszlo Ersek @ 2018-02-05 1:23 ` Wang, Jian J 1 sibling, 0 replies; 5+ messages in thread From: Wang, Jian J @ 2018-02-05 1:23 UTC (permalink / raw) To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Gao, Liming Reviewed-by: Jian J Wang <jian.j.wang@intel.com> > -----Original Message----- > From: Ni, Ruiyu > Sent: Friday, February 02, 2018 6:48 PM > To: edk2-devel@lists.01.org > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; > Wang, Jian J <jian.j.wang@intel.com> > Subject: [PATCH] MdePkg/SafeString: Directly return when length of source > string is 0 > > Today's implementation of [Ascii]StrnCpyS/[Ascii]StrnCatS doesn't > directly return the the length of source string is 0. > > When length of source string is 0, it means the Source points to > a memory that shouldn't be deferenced at all. > So it's not proper to call StrnLenS() in such situation. > In a pool guard enabled environment, when using shell to edit an > existing file which contains empty line, the page fault is met. > > The patch fixes the four library functions to align to the behavior > of non-safe version: directly return when length of source string > is 0. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Liming Gao <liming.gao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > --- > MdePkg/Library/BaseLib/SafeString.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/MdePkg/Library/BaseLib/SafeString.c > b/MdePkg/Library/BaseLib/SafeString.c > index 68c33e9b7b..fed818ef33 100644 > --- a/MdePkg/Library/BaseLib/SafeString.c > +++ b/MdePkg/Library/BaseLib/SafeString.c > @@ -1,7 +1,7 @@ > /** @file > Safe String functions. > > - Copyright (c) 2014 - 2017, Intel Corporation. All rights reserved.<BR> > + Copyright (c) 2014 - 2018, 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 > @@ -317,6 +317,10 @@ StrnCpyS ( > { > UINTN SourceLen; > > + if (Length == 0) { > + return RETURN_SUCCESS; > + } > + > ASSERT (((UINTN) Destination & BIT0) == 0); > ASSERT (((UINTN) Source & BIT0) == 0); > > @@ -515,6 +519,10 @@ StrnCatS ( > UINTN CopyLen; > UINTN SourceLen; > > + if (Length == 0) { > + return RETURN_SUCCESS; > + } > + > ASSERT (((UINTN) Destination & BIT0) == 0); > ASSERT (((UINTN) Source & BIT0) == 0); > > @@ -1894,6 +1902,10 @@ AsciiStrnCpyS ( > { > UINTN SourceLen; > > + if (Length == 0) { > + return RETURN_SUCCESS; > + } > + > // > // 1. Neither Destination nor Source shall be a null pointer. > // > @@ -2082,6 +2094,10 @@ AsciiStrnCatS ( > UINTN CopyLen; > UINTN SourceLen; > > + if (Length == 0) { > + return RETURN_SUCCESS; > + } > + > // > // Let CopyLen denote the value DestMax - AsciiStrnLenS(Destination, > DestMax) upon entry to AsciiStrnCatS. > // > -- > 2.16.1.windows.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-05 7:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-02-02 10:47 [PATCH] MdePkg/SafeString: Directly return when length of source string is 0 Ruiyu Ni 2018-02-02 13:42 ` Laszlo Ersek 2018-02-05 3:55 ` Yao, Jiewen 2018-02-05 8:04 ` Ni, Ruiyu 2018-02-05 1:23 ` Wang, Jian J
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox