diff mbox

[edk2,Linaro-uefi] MdePkg: ensure SafeString length functions don't access beyond MaxSize

Message ID CAF7UmSzZ9hkGD2N9R0Xd1jCninQ==Xw4szmXQD1jt6oWb8WEdw@mail.gmail.com
State New
Headers show

Commit Message

Leif Lindholm July 13, 2015, 10:29 a.m. UTC 
Sure - please find attached.

Regards,

Leif

On 13 July 2015 at 07:33, Gao, Liming <liming.gao@intel.com> wrote:
> Reviewed-by: Liming Gao <liming.gao@intel.com>
>
> Besides, if you expect me to commit this patch, please send the patch as attachment to me, I would like to help do it.
>
> Thanks
> Liming
> -----Original Message-----
> From: Yao, Jiewen
> Sent: Monday, July 13, 2015 12:11 PM
> To: Leif Lindholm; edk2-devel@lists.sourceforge.net
> Cc: Kinney, Michael D; Gao, Liming; linaro-uefi@lists.linaro.org
> Subject: RE: [Linaro-uefi] [PATCH] MdePkg: ensure SafeString length functions don't access beyond MaxSize
>
> Thank to catch this.
>
> Reviewed by: Yao, Jiewen <Jiewen.Yao@intel.com>
>
>
> -----Original Message-----
> From: Linaro-uefi [mailto:linaro-uefi-bounces@lists.linaro.org] On Behalf Of Leif Lindholm
> Sent: Friday, July 10, 2015 9:21 PM
> To: edk2-devel@lists.sourceforge.net
> Cc: Kinney, Michael D; Gao, Liming; linaro-uefi@lists.linaro.org
> Subject: [Linaro-uefi] [PATCH] MdePkg: ensure SafeString length functions don't access beyond MaxSize
>
> The StrnLenS and AsciiStrnLenS functions, when presented with a string with no terminating NULL in the first MaxSize characters will check the character at String[MaxSize] before checking if Length < MaxSize.
> (They return the correct value, but have accessed beyond the stated limit in the process.)
>
> Flip the order of the tests to prevent this behaviour.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  MdePkg/Library/BaseLib/SafeString.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c
> index 7c1b075..b0e1ce7 100644
> --- a/MdePkg/Library/BaseLib/SafeString.c
> +++ b/MdePkg/Library/BaseLib/SafeString.c
> @@ -141,7 +141,7 @@ StrnLenS (
>    // String then StrnLenS returns MaxSize. At most the first MaxSize characters of String shall
>    // be accessed by StrnLenS.
>    //
> -  for (Length = 0; (*String != 0) && (Length < MaxSize); String++, Length++) {
> +  for (Length = 0; (Length < MaxSize) && (*String != 0); String++,
> + Length++) {
>      ;
>    }
>    return Length;
> @@ -551,7 +551,7 @@ AsciiStrnLenS (
>    // String then AsciiStrnLenS returns MaxSize. At most the first MaxSize characters of String shall
>    // be accessed by AsciiStrnLenS.
>    //
> -  for (Length = 0; (*String != 0) && (Length < MaxSize); String++, Length++) {
> +  for (Length = 0; (Length < MaxSize) && (*String != 0); String++,
> + Length++) {
>      ;
>    }
>    return Length;
> --
> 2.1.4
>
> _______________________________________________
> Linaro-uefi mailing list
> Linaro-uefi@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/linaro-uefi
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
diff mbox

Patch

From 98dc45e7de665deef2e9e2bea3c3ac63ff07380a Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Fri, 10 Jul 2015 13:59:30 +0100
Subject: [PATCH] MdePkg: ensure SafeString length functions don't access
 beyond MaxSize

The StrnLenS and AsciiStrnLenS functions, when presented with a string
with no terminating NULL in the first MaxSize characters will check
the character at String[MaxSize] before checking if Length < MaxSize.
(They return the correct value, but have accessed beyond the stated
limit in the process.)

Flip the order of the tests to prevent this behaviour.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 MdePkg/Library/BaseLib/SafeString.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c
index 7c1b075..b0e1ce7 100644
--- a/MdePkg/Library/BaseLib/SafeString.c
+++ b/MdePkg/Library/BaseLib/SafeString.c
@@ -141,7 +141,7 @@  StrnLenS (
   // String then StrnLenS returns MaxSize. At most the first MaxSize characters of String shall
   // be accessed by StrnLenS.
   //
-  for (Length = 0; (*String != 0) && (Length < MaxSize); String++, Length++) {
+  for (Length = 0; (Length < MaxSize) && (*String != 0); String++, Length++) {
     ;
   }
   return Length;
@@ -551,7 +551,7 @@  AsciiStrnLenS (
   // String then AsciiStrnLenS returns MaxSize. At most the first MaxSize characters of String shall
   // be accessed by AsciiStrnLenS.
   //
-  for (Length = 0; (*String != 0) && (Length < MaxSize); String++, Length++) {
+  for (Length = 0; (Length < MaxSize) && (*String != 0); String++, Length++) {
     ;
   }
   return Length;
-- 
2.1.4