Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()
> -Original Message- > From: Krzysztof Koch [mailto:krzysztof.k...@arm.com] > Sent: Friday, July 19, 2019 6:41 PM > To: Gao, Zhichao ; devel@edk2.groups.io > Cc: nd > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to > DumpGasStruct() > > Hi Zhichao, > > Sorry about private emails, I just mistakenly hit the wrong button. I forgot > that the emails do not get sent to the mailing list unless I hit 'Reply all' > (silly > mistake). > > I see your point. I will submit v2 of this patchset with: > > DumpGasStruct (DataPtr, 4, *DbgDevInfoLen); > Replaced with > > DumpGasStruct (DataPtr, 4, GAS_LENGTH); > > Is that ok? Yes. Thanks for taking my advice. Thanks, Zhichao > > Kind regards, > > Krzysztof > > > -Original Message- > From: Gao, Zhichao > Sent: Friday, July 19, 2019 9:09 > To: Krzysztof Koch > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to > DumpGasStruct() > > HI Krzysztof, > > Why do you send the email to me only? I think it is fine to send to the > community. It would let more people who are interest with this component > join the discuss. Maybe not . :( > > Then for your comments: > Sorry, I missed the commit message before. But I don't think keeping an > inappropriate code with a commit massage to explain that is OK. If you have a > plan to fix that, it should combine into one patch. > Back to the code, I don't find out the possibility of overrun of buffer when > use 'GAS_LENGTH'. And DataPtr + * DbgDevInfoLen definitely overflow. > DevInfoPtr DataPtr >EndDevInfoPtr > ---|---|--| > |<-- DbgDevInfoLen > -> | > > Thanks, > Zhichao > > > -Original Message- > > From: Krzysztof Koch [mailto:krzysztof.k...@arm.com] > > Sent: Friday, July 19, 2019 3:39 PM > > To: Gao, Zhichao > > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer > > length to > > DumpGasStruct() > > > > Hi Zhichao, > > > > Please see my comments inline marked as [Krzysztof] > > > > Kind regards, > > > > Krzysztof > > > > -Original Message- > > From: Gao, Zhichao > > Sent: Friday, July 19, 2019 2:15 > > To: Krzysztof Koch ; devel@edk2.groups.io > > Cc: Carsey, Jaben ; Ni, Ray > > ; Sami Mujawar ; Matteo > > Carlini ; nd > > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer > > length to > > DumpGasStruct() > > > > > > > > > -Original Message- > > > From: Krzysztof Koch [mailto:krzysztof.k...@arm.com] > > > Sent: Thursday, July 18, 2019 8:32 PM > > > To: devel@edk2.groups.io > > > Cc: Carsey, Jaben ; Ni, Ray > > > ; Gao, Zhichao ; > > > sami.muja...@arm.com; matteo.carl...@arm.com; n...@arm.com > > > Subject: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer > > > length to > > > DumpGasStruct() > > > > > > Modify the signature of the DumpGasStruct() function to include the > > > buffer length parameter and to return the number of bytes parsed by > > > the > > function. > > > > > > This way it becomes possible to prevent buffer overruns when dumping > > > Generic Address Structure's (GAS) fields in the acpiview table parsers. > > > > > > Update all existing DumpGasStruct() calls in acpiview to add the > > > length argument. > > > > > > Signed-off-by: Krzysztof Koch > > > --- > > > > > > Notes: > > > v1: > > > - Modify DumpGasStruct() signature [Krzysztof] > > > > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | > > > 26 > > > +++- > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | > > > 8 > > > -- > > > > > > > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c > > > | 2 +- > > > 3 files changed, 22 insertions(+), 14 deletions(-) > > > > > > diff --git > > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > > index > > > > > > 8b3153516d2b7d9b920ab2de0344c17798ac572c..2d6ff80e299eebe7853061d3 > > > db89332197c0dc0e 100644 > > > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > > @@ -589,23 +589,27 @@ STATIC CONST ACPI_PARSER GasParser[] = { > > > > > >@param [in] Ptr Pointer to the start of the buffer. > > >@param [in] Indent Number of spaces to indent the output. > > > + @param [in] Length Length of the GAS structure buffer. > > > + > > > + @retval Number of bytes parsed. > > > **/ > > > -VOID > > > +UINT32 > > > EFIAPI > > > DumpGasStruct ( > > >IN UINT8*Ptr, > > > - IN UINT32Indent > > > + IN UINT32Indent, > > > + IN UINT32Length > > >) > > > { > > >Print (L"\n"); > > > - ParseAcpi ( > > > -TRUE, > > > -Indent, > > > -NULL, > > > -Ptr,
Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()
Hi Zhichao, Sorry about private emails, I just mistakenly hit the wrong button. I forgot that the emails do not get sent to the mailing list unless I hit 'Reply all' (silly mistake). I see your point. I will submit v2 of this patchset with: > DumpGasStruct (DataPtr, 4, *DbgDevInfoLen); Replaced with > DumpGasStruct (DataPtr, 4, GAS_LENGTH); Is that ok? Kind regards, Krzysztof -Original Message- From: Gao, Zhichao Sent: Friday, July 19, 2019 9:09 To: Krzysztof Koch Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct() HI Krzysztof, Why do you send the email to me only? I think it is fine to send to the community. It would let more people who are interest with this component join the discuss. Maybe not . :( Then for your comments: Sorry, I missed the commit message before. But I don't think keeping an inappropriate code with a commit massage to explain that is OK. If you have a plan to fix that, it should combine into one patch. Back to the code, I don't find out the possibility of overrun of buffer when use 'GAS_LENGTH'. And DataPtr + * DbgDevInfoLen definitely overflow. DevInfoPtr DataPtr EndDevInfoPtr ---|---|--| |<-- DbgDevInfoLen -> | Thanks, Zhichao > -Original Message- > From: Krzysztof Koch [mailto:krzysztof.k...@arm.com] > Sent: Friday, July 19, 2019 3:39 PM > To: Gao, Zhichao > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer > length to > DumpGasStruct() > > Hi Zhichao, > > Please see my comments inline marked as [Krzysztof] > > Kind regards, > > Krzysztof > > -Original Message- > From: Gao, Zhichao > Sent: Friday, July 19, 2019 2:15 > To: Krzysztof Koch ; devel@edk2.groups.io > Cc: Carsey, Jaben ; Ni, Ray > ; Sami Mujawar ; Matteo > Carlini ; nd > Subject: RE: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer > length to > DumpGasStruct() > > > > > -Original Message- > > From: Krzysztof Koch [mailto:krzysztof.k...@arm.com] > > Sent: Thursday, July 18, 2019 8:32 PM > > To: devel@edk2.groups.io > > Cc: Carsey, Jaben ; Ni, Ray > > ; Gao, Zhichao ; > > sami.muja...@arm.com; matteo.carl...@arm.com; n...@arm.com > > Subject: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer > > length to > > DumpGasStruct() > > > > Modify the signature of the DumpGasStruct() function to include the > > buffer length parameter and to return the number of bytes parsed by > > the > function. > > > > This way it becomes possible to prevent buffer overruns when dumping > > Generic Address Structure's (GAS) fields in the acpiview table parsers. > > > > Update all existing DumpGasStruct() calls in acpiview to add the > > length argument. > > > > Signed-off-by: Krzysztof Koch > > --- > > > > Notes: > > v1: > > - Modify DumpGasStruct() signature [Krzysztof] > > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 26 > > +++- > > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 8 > > -- > > > > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c > > | 2 +- > > 3 files changed, 22 insertions(+), 14 deletions(-) > > > > diff --git > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > index > > > 8b3153516d2b7d9b920ab2de0344c17798ac572c..2d6ff80e299eebe7853061d3 > > db89332197c0dc0e 100644 > > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > > @@ -589,23 +589,27 @@ STATIC CONST ACPI_PARSER GasParser[] = { > > > >@param [in] Ptr Pointer to the start of the buffer. > >@param [in] Indent Number of spaces to indent the output. > > + @param [in] Length Length of the GAS structure buffer. > > + > > + @retval Number of bytes parsed. > > **/ > > -VOID > > +UINT32 > > EFIAPI > > DumpGasStruct ( > >IN UINT8*Ptr, > > - IN UINT32Indent > > + IN UINT32Indent, > > + IN UINT32Length > >) > > { > >Print (L"\n"); > > - ParseAcpi ( > > -TRUE, > > -Indent, > > -NULL, > > -Ptr, > > -GAS_LENGTH, > > -PARSER_PARAMS (GasParser) > > -); > > + return ParseAcpi ( > > + TRUE, > > + Indent, > > + NULL, > > + Ptr, > > + Length, > > + PARSER_PARAMS (GasParser) > > + ); > > } > > > > /** > > @@ -621,7 +625,7 @@ DumpGas ( > >IN UINT8*Ptr > >) > > { > > - DumpGasStruct (Ptr, 2); > > + DumpGasStruct (Ptr, 2, GAS_LENGTH); > > } > > > > /** > > diff --git > > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > > index > > >
Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()
> -Original Message- > From: Krzysztof Koch [mailto:krzysztof.k...@arm.com] > Sent: Thursday, July 18, 2019 8:32 PM > To: devel@edk2.groups.io > Cc: Carsey, Jaben ; Ni, Ray ; > Gao, Zhichao ; sami.muja...@arm.com; > matteo.carl...@arm.com; n...@arm.com > Subject: [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to > DumpGasStruct() > > Modify the signature of the DumpGasStruct() function to include the buffer > length parameter and to return the number of bytes parsed by the function. > > This way it becomes possible to prevent buffer overruns when dumping > Generic Address Structure's (GAS) fields in the acpiview table parsers. > > Update all existing DumpGasStruct() calls in acpiview to add the length > argument. > > Signed-off-by: Krzysztof Koch > --- > > Notes: > v1: > - Modify DumpGasStruct() signature [Krzysztof] > > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c | 26 > +++- > ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h | 8 > -- > > ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parser.c > | 2 +- > 3 files changed, 22 insertions(+), 14 deletions(-) > > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > index > 8b3153516d2b7d9b920ab2de0344c17798ac572c..2d6ff80e299eebe7853061d3 > db89332197c0dc0e 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.c > @@ -589,23 +589,27 @@ STATIC CONST ACPI_PARSER GasParser[] = { > >@param [in] Ptr Pointer to the start of the buffer. >@param [in] Indent Number of spaces to indent the output. > + @param [in] Length Length of the GAS structure buffer. > + > + @retval Number of bytes parsed. > **/ > -VOID > +UINT32 > EFIAPI > DumpGasStruct ( >IN UINT8*Ptr, > - IN UINT32Indent > + IN UINT32Indent, > + IN UINT32Length >) > { >Print (L"\n"); > - ParseAcpi ( > -TRUE, > -Indent, > -NULL, > -Ptr, > -GAS_LENGTH, > -PARSER_PARAMS (GasParser) > -); > + return ParseAcpi ( > + TRUE, > + Indent, > + NULL, > + Ptr, > + Length, > + PARSER_PARAMS (GasParser) > + ); > } > > /** > @@ -621,7 +625,7 @@ DumpGas ( >IN UINT8*Ptr >) > { > - DumpGasStruct (Ptr, 2); > + DumpGasStruct (Ptr, 2, GAS_LENGTH); > } > > /** > diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > index > 7657892d9fd2e2e14c6578611ff0cf1b6f6cd750..20ca358bddfa5953bfb1d1beba > ebbf3079eaba01 100644 > --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h > @@ -405,12 +405,16 @@ ParseAcpi ( > >@param [in] Ptr Pointer to the start of the buffer. >@param [in] Indent Number of spaces to indent the output. > + @param [in] Length Length of the GAS structure buffer. > + > + @retval Number of bytes parsed. > **/ > -VOID > +UINT32 > EFIAPI > DumpGasStruct ( >IN UINT8*Ptr, > - IN UINT32Indent > + IN UINT32Indent, > + IN UINT32Length >); > > /** > diff --git > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse > r.c > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse > r.c > index > 8de5ebf74775bab8e765849cba6ef4eb6f659a5a..2c47a3f848aa2dd512c53343e > cf1c3c285173dd6 100644 > --- > a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Parse > r.c > +++ > b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Dbg2/Dbg2Pars > +++ er.c > @@ -164,7 +164,7 @@ DumpDbgDeviceInfo ( >AddrSize = (UINT32*)(Ptr + (*AddrSizeOffset)); >while (Index < (*GasCount)) { > PrintFieldName (4, L"BaseAddressRegister"); > -DumpGasStruct (DataPtr, 4); > +DumpGasStruct (DataPtr, 4, *DbgDevInfoLen); This input length should be GAS_LENGTH. *DbgDevInfoLen is the length of the whole structure and the DataPtr is increased during the loop. Inputing such a length would give the ParseAcpi function a chance to overrun the DataPtr. Thanks, Zhichao > PrintFieldName (4, L"Address Size"); > Print (L"0x%x\n", AddrSize[Index]); > DataPtr += GAS_LENGTH; > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43991): https://edk2.groups.io/g/devel/message/43991 Mute This Topic: https://groups.io/mt/32514379/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/6] ShellPkg: acpiview: Allow passing buffer length to DumpGasStruct()
Reviewed-by: Alexei Fedorov -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43953): https://edk2.groups.io/g/devel/message/43953 Mute This Topic: https://groups.io/mt/32514379/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-