* [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo buffer overflow with UINT8 type @ 2020-12-22 10:18 gechao 2021-01-13 6:58 ` Gao, Zhichao 0 siblings, 1 reply; 5+ messages in thread From: gechao @ 2020-12-22 10:18 UTC (permalink / raw) To: devel, zhichao.gao; +Cc: ray.ni, gechao From: gechao <gechao@greatwall.com.cn> The maximum fifo buffer length is RAW_FIFO_MAX_NUMBER + 1 = 257, but the maximum value of terminal fifo buffer index is sizeof(UINT8) - 1 = 255 with UINT8 type, so check if fifo buffer is empty or full with below expression, ((Tail + 1) % (RAW_FIFO_MAX_NUMBER + 1)) == Head, (Tail + 1) might be sizeof(UINT8) + 1 = 256, for UINT8 type, it does not make any sense. Signed-off-by: gechao <gechao@greatwall.com.cn> --- MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h index 378ace13ce..360e58e847 100644 --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h @@ -37,7 +37,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include <Library/BaseLib.h> -#define RAW_FIFO_MAX_NUMBER 256 +#define RAW_FIFO_MAX_NUMBER 255 #define FIFO_MAX_NUMBER 128 typedef struct { -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo buffer overflow with UINT8 type 2020-12-22 10:18 [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo buffer overflow with UINT8 type gechao @ 2021-01-13 6:58 ` Gao, Zhichao 2021-01-13 9:12 ` 回复: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo bufferoverflow " gechao 0 siblings, 1 reply; 5+ messages in thread From: Gao, Zhichao @ 2021-01-13 6:58 UTC (permalink / raw) To: gechao@greatwall.com.cn, devel@edk2.groups.io; +Cc: Ni, Ray Hi, Sorry, I don't understand the patch. UINT8 type would have the value limitation. But why does it affect the buffer size? Did you observed the overflow with the original value? If yes, can you share the example? Thanks, Zhichao > -----Original Message----- > From: gechao@greatwall.com.cn <gechao@greatwall.com.cn> > Sent: Tuesday, December 22, 2020 6:19 PM > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > Cc: Ni, Ray <ray.ni@intel.com>; gechao <gechao@greatwall.com.cn> > Subject: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo buffer > overflow with UINT8 type > > From: gechao <gechao@greatwall.com.cn> > > The maximum fifo buffer length is RAW_FIFO_MAX_NUMBER + 1 = 257, but > the maximum value of terminal fifo buffer index is sizeof(UINT8) - 1 = 255 with > UINT8 type, so check if fifo buffer is empty or full with below expression, ((Tail > + 1) % (RAW_FIFO_MAX_NUMBER + 1)) == Head, (Tail + 1) might be > sizeof(UINT8) + 1 = 256, for UINT8 type, it does not make any sense. > > Signed-off-by: gechao <gechao@greatwall.com.cn> > --- > MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > index 378ace13ce..360e58e847 100644 > --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > @@ -37,7 +37,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include <Library/BaseLib.h> -#define RAW_FIFO_MAX_NUMBER > 256+#define RAW_FIFO_MAX_NUMBER 255 #define FIFO_MAX_NUMBER > 128 typedef struct {-- > 2.25.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* 回复: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo bufferoverflow with UINT8 type 2021-01-13 6:58 ` Gao, Zhichao @ 2021-01-13 9:12 ` gechao 2021-01-14 2:01 ` Gao, Zhichao 0 siblings, 1 reply; 5+ messages in thread From: gechao @ 2021-01-13 9:12 UTC (permalink / raw) To: Gao, Zhichao, devel@edk2.groups.io; +Cc: Ni, Ray [-- Attachment #1.1: Type: text/plain, Size: 2750 bytes --] Hi Zhigao, Let's take the following code as an example, as we know, The maximum data represented by the UINT8 type is 255, So the equation condition on line 812 will not be true under special circumstances in picture 1, because (RAW_FIFO_MAX_NUMBER + 1) = 257, So when the fifo buffer is full with head = 0 and Tail = 255, the maximum value of (Tail + 1) % (RAW_FIFO_MAX_NUMBER + 1) is 256, This function will return false, This situation will occur in rare cases. This is a classic case we encountered in the project, our program will hang in picture 2 in this situation when the serial port does not respond. File: MdeModulePkg\Universal\Console\TerminalDxe\TerminalConIn.c picture 1: picture 2: 发送自 Windows 10 版邮件应用 发件人: Gao, Zhichao 发送时间: 2021年1月13日 14:57 收件人: gechao@greatwall.com.cn; devel@edk2.groups.io 抄送: Ni, Ray 主题: RE: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo bufferoverflow with UINT8 type Hi, Sorry, I don't understand the patch. UINT8 type would have the value limitation. But why does it affect the buffer size? Did you observed the overflow with the original value? If yes, can you share the example? Thanks, Zhichao > -----Original Message----- > From: gechao@greatwall.com.cn <gechao@greatwall.com.cn> > Sent: Tuesday, December 22, 2020 6:19 PM > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > Cc: Ni, Ray <ray.ni@intel.com>; gechao <gechao@greatwall.com.cn> > Subject: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo buffer > overflow with UINT8 type > > From: gechao <gechao@greatwall.com.cn> > > The maximum fifo buffer length is RAW_FIFO_MAX_NUMBER + 1 = 257, but > the maximum value of terminal fifo buffer index is sizeof(UINT8) - 1 = 255 with > UINT8 type, so check if fifo buffer is empty or full with below expression, ((Tail > + 1) % (RAW_FIFO_MAX_NUMBER + 1)) == Head, (Tail + 1) might be > sizeof(UINT8) + 1 = 256, for UINT8 type, it does not make any sense. > > Signed-off-by: gechao <gechao@greatwall.com.cn> > --- > MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > index 378ace13ce..360e58e847 100644 > --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > @@ -37,7 +37,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include <Library/BaseLib.h> -#define RAW_FIFO_MAX_NUMBER > 256+#define RAW_FIFO_MAX_NUMBER 255 #define FIFO_MAX_NUMBER > 128 typedef struct {-- > 2.25.1 [-- Attachment #1.2: Type: text/html, Size: 9853 bytes --] [-- Attachment #2: 10E2C51438334ED18E547EC29BB3BA5A.png --] [-- Type: image/png, Size: 55724 bytes --] [-- Attachment #3: 957235FFBE2B480982A40CAAD82708A1.png --] [-- Type: image/png, Size: 79012 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo bufferoverflow with UINT8 type 2021-01-13 9:12 ` 回复: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo bufferoverflow " gechao @ 2021-01-14 2:01 ` Gao, Zhichao 2021-01-14 2:13 ` 回复: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifobufferoverflow " gechao 0 siblings, 1 reply; 5+ messages in thread From: Gao, Zhichao @ 2021-01-14 2:01 UTC (permalink / raw) To: gechao, devel@edk2.groups.io; +Cc: Ni, Ray [-- Attachment #1.1: Type: text/plain, Size: 4123 bytes --] Hi, Now I understood. It is a bug fix. But the commit message is not clear. typedef struct { UINT8 Head; UINT8 Tail; UINT8 Data[RAW_FIFO_MAX_NUMBER + 1]; } RAW_DATA_FIFO; RAW_FIFO_MAX_NUMBER is 256. the data buffer size is 257 (Index from 0 to 256), but the max value of the index, Head or Tail (UINT8), is 255. That means the last data of the data buffer would be always empty if we use Head/Tail to output/input the data correctly. And because of the incorrect buffer size the FIFO full check “((Tail + 1) % (RAW_FIFO_MAX_NUMBER + 1)) == Head” would never meet. Thanks, Zhichao From: gechao <gechao@greatwall.com.cn> Sent: Wednesday, January 13, 2021 5:12 PM To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io Cc: Ni, Ray <ray.ni@intel.com> Subject: 回复: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo bufferoverflow with UINT8 type Hi Zhigao, Let's take the following code as an example, as we know, The maximum data represented by the UINT8 type is 255, So the equation condition on line 812 will not be true under special circumstances in picture 1, because (RAW_FIFO_MAX_NUMBER + 1) = 257, So when the fifo buffer is full with head = 0 and Tail = 255, the maximum value of (Tail + 1) % (RAW_FIFO_MAX_NUMBER + 1) is 256, This function will return false, This situation will occur in rare cases. This is a classic case we encountered in the project, our program will hang in picture 2 in this situation when the serial port does not respond. File: MdeModulePkg\Universal\Console\TerminalDxe\TerminalConIn.c picture 1: [cid:image001.png@01D6EA58.3B80D990] picture 2: [cid:image002.png@01D6EA58.3B80D990] 发送自 Windows 10 版邮件<https://go.microsoft.com/fwlink/?LinkId=550986>应用 发件人: Gao, Zhichao<mailto:zhichao.gao@intel.com> 发送时间: 2021年1月13日 14:57 收件人: gechao@greatwall.com.cn<mailto:gechao@greatwall.com.cn>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> 抄送: Ni, Ray<mailto:ray.ni@intel.com> 主题: RE: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo bufferoverflow with UINT8 type Hi, Sorry, I don't understand the patch. UINT8 type would have the value limitation. But why does it affect the buffer size? Did you observed the overflow with the original value? If yes, can you share the example? Thanks, Zhichao > -----Original Message----- > From: gechao@greatwall.com.cn<mailto:gechao@greatwall.com.cn> <gechao@greatwall.com.cn<mailto:gechao@greatwall.com.cn>> > Sent: Tuesday, December 22, 2020 6:19 PM > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Gao, Zhichao <zhichao.gao@intel.com<mailto:zhichao.gao@intel.com>> > Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; gechao <gechao@greatwall.com.cn<mailto:gechao@greatwall.com.cn>> > Subject: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo buffer > overflow with UINT8 type > > From: gechao <gechao@greatwall.com.cn<mailto:gechao@greatwall.com.cn>> > > The maximum fifo buffer length is RAW_FIFO_MAX_NUMBER + 1 = 257, but > the maximum value of terminal fifo buffer index is sizeof(UINT8) - 1 = 255 with > UINT8 type, so check if fifo buffer is empty or full with below expression, ((Tail > + 1) % (RAW_FIFO_MAX_NUMBER + 1)) == Head, (Tail + 1) might be > sizeof(UINT8) + 1 = 256, for UINT8 type, it does not make any sense. > > Signed-off-by: gechao <gechao@greatwall.com.cn<mailto:gechao@greatwall.com.cn>> > --- > MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > index 378ace13ce..360e58e847 100644 > --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > @@ -37,7 +37,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include <Library/BaseLib.h> -#define RAW_FIFO_MAX_NUMBER > 256+#define RAW_FIFO_MAX_NUMBER 255 #define FIFO_MAX_NUMBER > 128 typedef struct {-- > 2.25.1 [-- Attachment #1.2: Type: text/html, Size: 13632 bytes --] [-- Attachment #2: image001.png --] [-- Type: image/png, Size: 55724 bytes --] [-- Attachment #3: image002.png --] [-- Type: image/png, Size: 79012 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* 回复: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifobufferoverflow with UINT8 type 2021-01-14 2:01 ` Gao, Zhichao @ 2021-01-14 2:13 ` gechao 0 siblings, 0 replies; 5+ messages in thread From: gechao @ 2021-01-14 2:13 UTC (permalink / raw) To: Gao, Zhichao, devel@edk2.groups.io; +Cc: Ni, Ray [-- Attachment #1.1: Type: text/plain, Size: 4030 bytes --] Hi Zhichao, I will update the detailed description according to your suggestions，thanks. Chao. 发送自 Windows 10 版邮件应用 发件人: Gao, Zhichao 发送时间: 2021年1月14日 10:06 收件人: gechao; devel@edk2.groups.io 抄送: Ni, Ray 主题: RE: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifobufferoverflow with UINT8 type Hi, Now I understood. It is a bug fix. But the commit message is not clear. typedef struct { UINT8 Head; UINT8 Tail; UINT8 Data[RAW_FIFO_MAX_NUMBER + 1]; } RAW_DATA_FIFO; RAW_FIFO_MAX_NUMBER is 256. the data buffer size is 257 (Index from 0 to 256), but the max value of the index, Head or Tail (UINT8), is 255. That means the last data of the data buffer would be always empty if we use Head/Tail to output/input the data correctly. And because of the incorrect buffer size the FIFO full check “((Tail + 1) % (RAW_FIFO_MAX_NUMBER + 1)) == Head” would never meet. Thanks, Zhichao From: gechao <gechao@greatwall.com.cn> Sent: Wednesday, January 13, 2021 5:12 PM To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io Cc: Ni, Ray <ray.ni@intel.com> Subject: 回复: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo bufferoverflow with UINT8 type Hi Zhigao, Let's take the following code as an example, as we know, The maximum data represented by the UINT8 type is 255, So the equation condition on line 812 will not be true under special circumstances in picture 1, because (RAW_FIFO_MAX_NUMBER + 1) = 257, So when the fifo buffer is full with head = 0 and Tail = 255, the maximum value of (Tail + 1) % (RAW_FIFO_MAX_NUMBER + 1) is 256, This function will return false, This situation will occur in rare cases. This is a classic case we encountered in the project, our program will hang in picture 2 in this situation when the serial port does not respond. File: MdeModulePkg\Universal\Console\TerminalDxe\TerminalConIn.c picture 1: picture 2: 发送自 Windows 10 版邮件应用 发件人: Gao, Zhichao 发送时间: 2021年1月13日 14:57 收件人: gechao@greatwall.com.cn; devel@edk2.groups.io 抄送: Ni, Ray 主题: RE: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo bufferoverflow with UINT8 type Hi, Sorry, I don't understand the patch. UINT8 type would have the value limitation. But why does it affect the buffer size? Did you observed the overflow with the original value? If yes, can you share the example? Thanks, Zhichao > -----Original Message----- > From: gechao@greatwall.com.cn <gechao@greatwall.com.cn> > Sent: Tuesday, December 22, 2020 6:19 PM > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > Cc: Ni, Ray <ray.ni@intel.com>; gechao <gechao@greatwall.com.cn> > Subject: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo buffer > overflow with UINT8 type > > From: gechao <gechao@greatwall.com.cn> > > The maximum fifo buffer length is RAW_FIFO_MAX_NUMBER + 1 = 257, but > the maximum value of terminal fifo buffer index is sizeof(UINT8) - 1 = 255 with > UINT8 type, so check if fifo buffer is empty or full with below expression, ((Tail > + 1) % (RAW_FIFO_MAX_NUMBER + 1)) == Head, (Tail + 1) might be > sizeof(UINT8) + 1 = 256, for UINT8 type, it does not make any sense. > > Signed-off-by: gechao <gechao@greatwall.com.cn> > --- > MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > index 378ace13ce..360e58e847 100644 > --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h > @@ -37,7 +37,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include <Library/BaseLib.h> -#define RAW_FIFO_MAX_NUMBER > 256+#define RAW_FIFO_MAX_NUMBER 255 #define FIFO_MAX_NUMBER > 128 typedef struct {-- > 2.25.1 [-- Attachment #1.2: Type: text/html, Size: 16181 bytes --] [-- Attachment #2: image001.png --] [-- Type: image/png, Size: 55724 bytes --] [-- Attachment #3: image002.png --] [-- Type: image/png, Size: 79012 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-14 2:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-12-22 10:18 [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo buffer overflow with UINT8 type gechao 2021-01-13 6:58 ` Gao, Zhichao 2021-01-13 9:12 ` 回复: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo bufferoverflow " gechao 2021-01-14 2:01 ` Gao, Zhichao 2021-01-14 2:13 ` 回复: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifobufferoverflow " gechao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
![[-- Attachment #2: 10E2C51438334ED18E547EC29BB3BA5A.png --] [-- Type: image/png, Size: 55724 bytes --]](../../5FFEB933.008044.03867@greatwall.com.cn/2-10E2C51438334ED18E547EC29BB3BA5A.png){kind=link}
![[-- Attachment #3: 957235FFBE2B480982A40CAAD82708A1.png --] [-- Type: image/png, Size: 79012 bytes --]](../../5FFEB933.008044.03867@greatwall.com.cn/3-957235FFBE2B480982A40CAAD82708A1.png){kind=link}
![[-- Attachment #2: image001.png --] [-- Type: image/png, Size: 55724 bytes --]](../../MWHPR11MB16470A31C21267EE0043F232F6A80@MWHPR11MB1647.namprd11.prod.outlook.com/2-image001.png){kind=link}
![[-- Attachment #3: image002.png --] [-- Type: image/png, Size: 79012 bytes --]](../../MWHPR11MB16470A31C21267EE0043F232F6A80@MWHPR11MB1647.namprd11.prod.outlook.com/3-image002.png){kind=link}
![[-- Attachment #2: image001.png --] [-- Type: image/png, Size: 55724 bytes --]](../../5FFFA878.008543.03867@greatwall.com.cn/2-image001.png){kind=link}
![[-- Attachment #3: image002.png --] [-- Type: image/png, Size: 79012 bytes --]](../../5FFFA878.008543.03867@greatwall.com.cn/3-image002.png){kind=link}