[edk2-devel] 回复: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo bufferoverflow with UINT8 type

[gechao]

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 at greatwall.com.cn; devel at 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 at greatwall.com.cn <gechao at greatwall.com.cn>
> Sent: Tuesday, December 22, 2020 6:19 PM
> To: devel at edk2.groups.io; Gao, Zhichao <zhichao.gao at intel.com>
> Cc: Ni, Ray <ray.ni at intel.com>; gechao <gechao at greatwall.com.cn>
> Subject: [PATCH] MdeModulePkg/TerminalDxe: Fix terminal fifo buffer
> overflow with UINT8 type
> 
> From: gechao <gechao at 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 at 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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70231): https://edk2.groups.io/g/devel/message/70231
Mute This Topic: https://groups.io/mt/79646944/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20210113/d229e694/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 10E2C51438334ED18E547EC29BB3BA5A.png
Type: image/png
Size: 55724 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20210113/d229e694/attachment.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 957235FFBE2B480982A40CAAD82708A1.png
Type: image/png
Size: 79012 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20210113/d229e694/attachment-0001.png>