Aaron, Thanks for your comments. I check the code you provide, I found the input maximum value(200) is bigger than the allowed maximum value(0x7F). So vfrcompiler assume the maximum value is (INT (200) = -38) which is small than minimum value(0), and the error raised. Now I add code to do bound check first. Please review this new patch which add bound check.
Thanks, Eric From: aaron....@congatec.com [mailto:aaron....@congatec.com] Sent: Friday, May 01, 2015 1:33 AM To: edk2-devel@lists.sourceforge.net Subject: Re: [edk2] [Patch 1/2] BaseTools: Enhance the check for numeric opcode with EFI_IFR_DISPLAY_INT_DEC attribute. Hi Eric, I think this patch still needs some work because it isn't enforcing the min/max of signed integers. In the below example, the following error is generated: typedef struct { UINT8 Nothing[1]; } MY_EFI_VARSTORE_DATA; numeric varid = MY_EFI_VARSTORE_DATA.Nothing, prompt = STRING_TOKEN(1), help = STRING_TOKEN(2), flags = DISPLAY_INT_DEC, minimum = 0, maximum = 200, step = 1, endnumeric; ERROR 12288: Maximum can't be less than Minimum : invalid parameter The patch appears to not be bounds checking the values against the maximum value that can be stored in the type. From limits.h: #define _I8_MIN (-127i8 - 1) /* minimum signed 8 bit value */ #define _I8_MAX 127i8 /* maximum signed 8 bit value */ numeric varid = MY_EFI_VARSTORE_DATA.Nothing, prompt = STRING_TOKEN(1), help = STRING_TOKEN(2), flags = DISPLAY_INT_DEC, minimum = -100, maximum = 200, step = 1, endnumeric; And this is allowed to compile successfully even though 200 is above the maximum 127 that can be stored in a signed integer. Best Regards, Aaron From: Eric Dong <eric.d...@intel.com<mailto:eric.d...@intel.com>> To: edk2-devel@lists.sourceforge.net<mailto:edk2-devel@lists.sourceforge.net>, liming....@intel.com<mailto:liming....@intel.com>, Date: 04/29/2015 10:57 PM Subject: [edk2] [Patch 1/2] BaseTools: Enhance the check for numeric opcode with EFI_IFR_DISPLAY_INT_DEC attribute. ________________________________ Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Eric Dong <eric.d...@intel.com<mailto:eric.d...@intel.com>> --- BaseTools/Source/C/VfrCompile/VfrFormPkg.h | 8 ++ BaseTools/Source/C/VfrCompile/VfrSyntax.g | 170 +++++++++++++++++++++++++---- 2 files changed, 156 insertions(+), 22 deletions(-) diff --git a/BaseTools/Source/C/VfrCompile/VfrFormPkg.h b/BaseTools/Source/C/VfrCompile/VfrFormPkg.h index 3d2def8..e64ef1b 100644 --- a/BaseTools/Source/C/VfrCompile/VfrFormPkg.h +++ b/BaseTools/Source/C/VfrCompile/VfrFormPkg.h @@ -308,10 +308,14 @@ public: } VOID UpdateHeader (IN EFI_IFR_OP_HEADER *Header) { mHeader = Header; } + + UINT8 GetOpCode () { + return mHeader->OpCode; + } }; extern UINT8 gScopeCount; /* @@ -1356,10 +1360,14 @@ public: } else { mNumeric->Flags = LFlags; } return VFR_RETURN_SUCCESS; } + + UINT8 GetNumericFlags () { + return mNumeric->Flags; + } }; class CIfrOneOf : public CIfrObj, public CIfrOpHeader, public CIfrQuestionHeader, public CIfrMinMaxStepData { private: EFI_IFR_ONE_OF *mOneOf; diff --git a/BaseTools/Source/C/VfrCompile/VfrSyntax.g b/BaseTools/Source/C/VfrCompile/VfrSyntax.g index 2255d6f..d014aff 100644 --- a/BaseTools/Source/C/VfrCompile/VfrSyntax.g +++ b/BaseTools/Source/C/VfrCompile/VfrSyntax.g @@ -1394,24 +1394,40 @@ vfrQuestionDataFieldName [EFI_QUESTION_ID &QId, UINT32 &Mask, CHAR8 *&VarIdStr, ; vfrConstantValueField[UINT8 Type] > [EFI_IFR_TYPE_VALUE Value] : << EFI_GUID Guid; + BOOLEAN Negative = FALSE; >> + { + "\-" << Negative = TRUE; >> + } N1:Number << switch ($Type) { case EFI_IFR_TYPE_NUM_SIZE_8 : - $Value.u8 = _STOU8(N1->getText(), N1->getLine()); + $Value.u8 = _STOU8(N1->getText(), N1->getLine()); + if (Negative) { + $Value.u8 = ~$Value.u8 + 1; + } break; case EFI_IFR_TYPE_NUM_SIZE_16 : - $Value.u16 = _STOU16(N1->getText(), N1->getLine()); + $Value.u16 = _STOU16(N1->getText(), N1->getLine()); + if (Negative) { + $Value.u16 = ~$Value.u16 + 1; + } break; case EFI_IFR_TYPE_NUM_SIZE_32 : $Value.u32 = _STOU32(N1->getText(), N1->getLine()); + if (Negative) { + $Value.u32 = ~$Value.u32 + 1; + } break; case EFI_IFR_TYPE_NUM_SIZE_64 : $Value.u64 = _STOU64(N1->getText(), N1->getLine()); + if (Negative) { + $Value.u64 = ~$Value.u64 + 1; + } break; case EFI_IFR_TYPE_BOOLEAN : $Value.b = _STOU8(N1->getText(), N1->getLine()); break; case EFI_IFR_TYPE_STRING : @@ -1531,20 +1547,53 @@ vfrStatementDefault : EFI_DEFAULT_ID DefaultId = EFI_HII_DEFAULT_CLASS_STANDARD; CHAR8 *VarStoreName = NULL; EFI_VFR_VARSTORE_TYPE VarStoreType = EFI_VFR_VARSTORE_INVALID; UINT32 Size = 0; EFI_GUID *VarGuid = NULL; + CIfrNumeric *NumericQst = NULL; >> D:Default ( ( "=" vfrConstantValueField[_GET_CURRQEST_DATATYPE()] > [Val] "," << if (gCurrentMinMaxData != NULL && gCurrentMinMaxData->IsNumericOpcode()) { //check default value is valid for Numeric Opcode - if (Val.u64 < gCurrentMinMaxData->GetMinData(_GET_CURRQEST_DATATYPE()) || Val.u64 > gCurrentMinMaxData->GetMaxData(_GET_CURRQEST_DATATYPE())) { - _PCATCH (VFR_RETURN_INVALID_PARAMETER, D->getLine(), "Numeric default value must be between MinValue and MaxValue."); + NumericQst = (CIfrNumeric *) gCurrentQuestion; + if ((NumericQst->GetNumericFlags() & EFI_IFR_DISPLAY) == 0) { + switch (_GET_CURRQEST_DATATYPE()) { + case EFI_IFR_TYPE_NUM_SIZE_8: + if (((INT8) Val.u8 < (INT8) gCurrentMinMaxData->GetMinData(_GET_CURRQEST_DATATYPE())) || + ((INT8) Val.u8 > (INT8) gCurrentMinMaxData->GetMaxData(_GET_CURRQEST_DATATYPE()))) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, D->getLine(), "Numeric default value must be between MinValue and MaxValue."); + } + break; + case EFI_IFR_TYPE_NUM_SIZE_16: + if (((INT16) Val.u16 < (INT16) gCurrentMinMaxData->GetMinData(_GET_CURRQEST_DATATYPE())) || + ((INT16) Val.u16 > (INT16) gCurrentMinMaxData->GetMaxData(_GET_CURRQEST_DATATYPE()))) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, D->getLine(), "Numeric default value must be between MinValue and MaxValue."); + } + break; + case EFI_IFR_TYPE_NUM_SIZE_32: + if (((INT32) Val.u32 < (INT32) gCurrentMinMaxData->GetMinData(_GET_CURRQEST_DATATYPE())) || + ((INT32) Val.u32 > (INT32) gCurrentMinMaxData->GetMaxData(_GET_CURRQEST_DATATYPE()))) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, D->getLine(), "Numeric default value must be between MinValue and MaxValue."); + } + break; + case EFI_IFR_TYPE_NUM_SIZE_64: + if (((INT64) Val.u64 < (INT64) gCurrentMinMaxData->GetMinData(_GET_CURRQEST_DATATYPE())) || + ((INT64) Val.u64 > (INT64) gCurrentMinMaxData->GetMaxData(_GET_CURRQEST_DATATYPE()))) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, D->getLine(), "Numeric default value must be between MinValue and MaxValue."); + } + break; + default: + break; + } + } else { + if (Val.u64 < gCurrentMinMaxData->GetMinData(_GET_CURRQEST_DATATYPE()) || Val.u64 > gCurrentMinMaxData->GetMaxData(_GET_CURRQEST_DATATYPE())) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, D->getLine(), "Numeric default value must be between MinValue and MaxValue."); + } } } if (_GET_CURRQEST_DATATYPE() == EFI_IFR_TYPE_OTHER) { _PCATCH (VFR_RETURN_FATAL_ERROR, D->getLine(), "Default data type error."); Size = sizeof (EFI_IFR_TYPE_VALUE); @@ -2218,45 +2267,122 @@ vfrSetMinMaxStep[CIfrMinMaxStepData & MMSDObj] : << UINT64 MaxU8 = 0, MinU8 = 0, StepU8 = 0; UINT32 MaxU4 = 0, MinU4 = 0, StepU4 = 0; UINT16 MaxU2 = 0, MinU2 = 0, StepU2 = 0; UINT8 MaxU1 = 0, MinU1 = 0, StepU1 = 0; + BOOLEAN IntDecStyle = FALSE; + CIfrNumeric *NObj = (CIfrNumeric *) (&MMSDObj); + if ((NObj->GetOpCode() == EFI_IFR_NUMERIC_OP) && ((NObj->GetNumericFlags() & EFI_IFR_DISPLAY) == 0)) { + IntDecStyle = TRUE; + } + BOOLEAN MinNegative = FALSE; + BOOLEAN MaxNegative = FALSE; >> - Minimum "=" I:Number "," - << + Minimum "=" + { + "\-" << MinNegative = TRUE; >> + } + I:Number "," << + if (!IntDecStyle && MinNegative) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, I->getLine(), "\"-\" can't be used when not in int decimal type. "); + } switch (_GET_CURRQEST_DATATYPE()) { - case EFI_IFR_TYPE_NUM_SIZE_64 : MinU8 = _STOU64(I->getText(), I->getLine()); break; - case EFI_IFR_TYPE_NUM_SIZE_32 : MinU4 = _STOU32(I->getText(), I->getLine()); break; - case EFI_IFR_TYPE_NUM_SIZE_16 : MinU2 = _STOU16(I->getText(), I->getLine()); break; - case EFI_IFR_TYPE_NUM_SIZE_8 : MinU1 = _STOU8(I->getText(), I->getLine()); break; + case EFI_IFR_TYPE_NUM_SIZE_64 : + MinU8 = _STOU64(I->getText(), I->getLine()); + if (MinNegative) { + MinU8 = ~MinU8 + 1; + } + break; + case EFI_IFR_TYPE_NUM_SIZE_32 : + MinU4 = _STOU32(I->getText(), I->getLine()); + if (MinNegative) { + MinU4 = ~MinU4 + 1; + } + break; + case EFI_IFR_TYPE_NUM_SIZE_16 : + MinU2 = _STOU16(I->getText(), I->getLine()); + if (MinNegative) { + MinU2 = ~MinU2 + 1; + } + break; + case EFI_IFR_TYPE_NUM_SIZE_8 : + MinU1 = _STOU8(I->getText(), I->getLine()); + if (MinNegative) { + MinU1 = ~MinU1 + 1; + } + break; } >> - Maximum "=" A:Number "," - << + Maximum "=" + { + "\-" << MaxNegative = TRUE; >> + } + A:Number "," << + if (!IntDecStyle && MaxNegative) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "\"-\" can't be used when not in int decimal type. "); + } + switch (_GET_CURRQEST_DATATYPE()) { case EFI_IFR_TYPE_NUM_SIZE_64 : MaxU8 = _STOU64(A->getText(), A->getLine()); - if (MaxU8 < MinU8) { - _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + if (MaxNegative) { + MaxU8 = ~MaxU8 + 1; + } + if (IntDecStyle) { + if ((INT64) MaxU8 < (INT64) MinU8) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + } + } else { + if (MaxU8 < MinU8) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + } } break; case EFI_IFR_TYPE_NUM_SIZE_32 : - MaxU4 = _STOU32(A->getText(), A->getLine()); - if (MaxU4 < MinU4) { - _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + MaxU4 = _STOU32(A->getText(), A->getLine()); + if (MaxNegative) { + MaxU4 = ~MaxU4 + 1; + } + if (IntDecStyle) { + if ((INT32) MaxU4 < (INT32) MinU4) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + } + } else { + if (MaxU4 < MinU4) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + } } break; case EFI_IFR_TYPE_NUM_SIZE_16 : MaxU2 = _STOU16(A->getText(), A->getLine()); - if (MaxU2 < MinU2) { - _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + if (MaxNegative) { + MaxU2 = ~MaxU2 + 1; + } + + if (IntDecStyle) { + if ((INT16) MaxU2 < (INT16) MinU2) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + } + } else { + if (MaxU2 < MinU2) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + } } break; case EFI_IFR_TYPE_NUM_SIZE_8 : - MaxU1 = _STOU8(A->getText(), A->getLine()); - if (MaxU1 < MinU1) { - _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + MaxU1 = _STOU8(A->getText(), A->getLine()); + if (MaxNegative) { + MaxU1 = ~MaxU1 + 1; + } + if (IntDecStyle) { + if ((INT8) MaxU1 < (INT8) MinU1) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + } + } else { + if (MaxU1 < MinU1) { + _PCATCH (VFR_RETURN_INVALID_PARAMETER, A->getLine(), "Maximum can't be less than Minimum"); + } } break; } >> { -- 1.9.5.msysgit.1 ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net<mailto:edk2-devel@lists.sourceforge.net> https://lists.sourceforge.net/lists/listinfo/edk2-devel
0001-BaseTools-Enhance-the-check-for-numeric-opcode-with-.patch
Description: 0001-BaseTools-Enhance-the-check-for-numeric-opcode-with-.patch
0003-BaseTools-Add-boudary-check-for-minimum-maximum-defa.patch
Description: 0003-BaseTools-Add-boudary-check-for-minimum-maximum-defa.patch
------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
