- TF-A - lists.trustedfirmware.org

Re: [TF-A] fconf: Validating config data

by Louis Mayencourt

Hi Raghu, I do agree with you: case 2 and 3 are similar (wrongly formed DTB) and should lead to the same behavior. A mandatory property miss or a hit with a structurally incorrect node means that the DTB doesn't follow the provided binding document. Such a DTB shouldn't be considered as valid and should trigger a build failure and/or a code panic. With the current implementation, case 2 and 3 are similar. The property_getter() functions expect a specific format of the node. If a node is not found or structurally incorrect, the function will return an error code, which will lead to a panic(). regards, Louis ________________________________ From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> on behalf of Raghu Krishnamurthy via TF-A <tf-a(a)lists.trustedfirmware.org> Sent: 06 April 2020 19:51 To: tf-a(a)lists.trustedfirmware.org <tf-a(a)lists.trustedfirmware.org> Subject: Re: [TF-A] fconf: Validating config data Thanks Louis. This new terminology helps. Let me spell this out again just to make sure we're on the same page. The following are the cases: 1) Mandatory Property hit, nodes are structurally correct - Works normally. 2) Mandatory Property hit, nodes are structurally *incorrect* - Asserts should catch structural issues during development because system integrator expected to not make mistakes with number of nodes etc. 3) Mandatory Property miss - Panic(). Why is this case causing a panic, but not case 2? You are allowing for some one to make the mistake of not having a mandatory property, but assume that if a mandatory property is present, it is always structurally sound. This is the part i have a problem with. In my view case 2 and case 3 should panic and code should not just assert on mandatory properties and must ALWAYS check for structural soundness of an FDT property. Similarly there are 3 cases for optional properties. My question: Why does case 3 panic, but not case 2 ? It sounds like your assumption is that case 2 cannot happen. If case 2 cannot happen, i claim that case 3 cannot happen either. Let me know what you think! Thanks Raghu On 4/6/20 3:57 AM, Louis Mayencourt via TF-A wrote: > Hi Raghu, > > Let me try to clarify/reword my idea: > > We complete the fconf documentation with a binding document, which > defines the nodes that should be present in the config DTB to consider > it as valid/well-formed. The document contains two kinds of node: > > * mandatory (critical): the firmware can't proceed without this > information (example: load address, image UUID, ...). If this node > is not present in the DTS, the build fails and/or the code panic. > > * optional (no-critical): the firmware can assume or assign a default > value and proceed (example: uart config, enable authentication flag, > ...). Such a property is used to influence the default behavior of > the firmware. > > />> This is non-deterministic failure./ > The miss of a mandatory (critical) node will always lead to a build > failure / code panic. > The miss of an optional (no-critical) node should not influence the > default behavior of the firmware. > > />> Is it not confusing to make the assumption that a DTB is "well > formed",i.e expect the build process/integrator to not mess up the >>> structure or number of nodes but allow the same integrator to miss a critical property in the DTB?/ > A DTB with a missing mandatory (critical) property should not be > considered well formed. > > />> If there is a missing critical property, is that not a badly formed > DTB?/ > With the above definition, it is. > > />> And if so, why not check for badly formed DTB's uniformly in code > and why only check for missing "critical" properties?/ > With the rewording of "critical" / "no-critical" to "mandatory" / > "optional", I think the situation is clearer: A DTB with a missing > "optional" node/property is is still considered well-formed. > > I hope I answered your questions and clarify the idea behind the design. > > regards, > Louis > > ------------------------------------------------------------------------ > *From:* Raghu Krishnamurthy <raghu.ncstate(a)icloud.com> > *Sent:* 05 April 2020 00:06 > *To:* Louis Mayencourt <Louis.Mayencourt(a)arm.com> > *Subject:* Re: [TF-A] fconf: Validating config data > Thanks Louis. > >>you can imagine a well-formed DTB which contain a > >>critical set of properties and can contain some optional properties. > > This makes things even more confusing. The assumption we are asking code > to make is that the DTB is always "well formed", ie don't check for > structural issues such as extra nodes etc, but we are still making the > distinctions between critical and non-critical properties, that may or > may not exist in the DTB, which may or may not cause a panic. This is > non-deterministic failure. > Is it not confusing to make the assumption that a DTB is "well > formed",i.e expect the build process/integrator to not mess up the > structure or number of nodes but allow the same integrator to miss a > critical property in the DTB? If there is a missing critical property, > is that not a badly formed DTB ? And if so, why not check for badly > formed DTB's uniformly in code and why only check for missing "critical" > properties? > > -Raghu > > On 4/3/20 3:16 AM, Louis Mayencourt wrote: >> Hi Raghu, >> >> I do agree that we need something similar to a binding document for >> fconf properties. (similar to >> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3694/2/docs/…). > >> At least for the common properties. >> >> The main idea behind the return code of the populator function was to >> allow the code to handle no-critical property misses or to handle >> critical failure by calling a platform hook. >> With this in mind, you can imagine a well-formed DTB which contain a >> critical set of properties and can contain some optional properties. The >> return code and the populator "name" / "config" can be used to handle >> this two cases. >> >> I tried to keep the design of fconf really simple, to leave room for >> improvement according to feedbacks. Thanks for helping improving it! >> >> Regards, >> Louis >> ------------------------------------------------------------------------ >> *From:* TF-A <tf-a-bounces(a)lists.trustedfirmware.org> on behalf of Raghu >> Krishnamurthy via TF-A <tf-a(a)lists.trustedfirmware.org> >> *Sent:* 03 April 2020 10:15 >> *To:* tf-a(a)lists.trustedfirmware.org <tf-a(a)lists.trustedfirmware.org> >> *Subject:* Re: [TF-A] fconf: Validating config data >> A further point is that the fconf populators return an error code and >> panics on error today. But if we are making the assumption that the >> DTB's are well formed, do we really need to fail or even return an error >> code? >> >> -Raghu >> >> On 4/3/20 1:51 AM, Raghu Krishnamurthy via TF-A wrote: >>> Hi All, (Sorry for the long email) >>> >>> The review >>> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3845 >>> attempts to fix bounds check in the fconf populator code for the >>> topology and SP's. During review, Sandrine thoughtfully pointed out that >>> there were discussions around bounds check along the same lines in the >>> review >>> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3492 and >>> it was deemed sufficient to have assertions in code and it was safe to >>> make the assumption that the DTB is always well formed and contains >>> valid values. I think this email mostly echoes Sandrine's concern from >>> review 3492. >>> >>> While i agree with the assumptions, I am generally of the opinion that >>> we should validate/range-check any data, even if it is signed. Being >>> signed does not necessarily mean the data is well formed/valid. If there >>> is a mistake in the build process and it is validly signed, it is >>> possible that we silently corrupt state/data that could later be used to >>> exploit firmware and/or make debugging hard. This is probably far >>> fetched, but the cost of adding the check is trivial to avoid this >>> possibility. >>> >>> I imagine the case where you have secure partitions signed by different >>> entity other than the silicon provider(dual root-of-trust). A silicon >>> provider provides a dev system for the SP provider to test and validate >>> the SP's on silicon. The silicon has production firmware(and hence no >>> assertions), but loads signed data from the SP provider which has some >>> invalid values. There could be silent corruption without any indication >>> whatsoever about what went wrong and it may be hard to debug if/when >>> there are issues. >>> Also, testing does not necessarily catch all invalid values since you >>> will likely not get 100% coverage, given the number of config options >>> available. Moreover, the code today, is not consistent in asserting on >>> every property for valid values and the failure mode is not >>> consistent/deterministic. It seems like every config option should have >>> a list of valid values or a range of acceptable values that must be at a >>> minimum asserted on. >>> I also wouldn't discount platforms such as RPI, where TRUSTED_BOARD_BOOT >>> is likely to be turned off since it really does not provide any >>> security, so assuming we always have signed data might not be valid. >>> >>> Anyway, is this decision worth revisiting? Too paranoid perhaps? :P >>> >>> >>> Thanks >>> Raghu >> -- >> TF-A mailing list >> TF-A(a)lists.trustedfirmware.org >> https://lists.trustedfirmware.org/mailman/listinfo/tf-a >> IMPORTANT NOTICE: The contents of this email and any attachments are >> confidential and may also be privileged. If you are not the intended >> recipient, please notify the sender immediately and do not disclose the >> contents to any other person, use it for any purpose, or store or copy >> the information in any medium. Thank you. > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy > the information in any medium. Thank you. > -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

6 years

1
0
0 0

Re: [TF-A] fconf: Validating config data

by Raghu Krishnamurthy

Thanks Louis. This new terminology helps. Let me spell this out again just to make sure we're on the same page. The following are the cases: 1) Mandatory Property hit, nodes are structurally correct - Works normally. 2) Mandatory Property hit, nodes are structurally *incorrect* - Asserts should catch structural issues during development because system integrator expected to not make mistakes with number of nodes etc. 3) Mandatory Property miss - Panic(). Why is this case causing a panic, but not case 2? You are allowing for some one to make the mistake of not having a mandatory property, but assume that if a mandatory property is present, it is always structurally sound. This is the part i have a problem with. In my view case 2 and case 3 should panic and code should not just assert on mandatory properties and must ALWAYS check for structural soundness of an FDT property. Similarly there are 3 cases for optional properties. My question: Why does case 3 panic, but not case 2 ? It sounds like your assumption is that case 2 cannot happen. If case 2 cannot happen, i claim that case 3 cannot happen either. Let me know what you think! Thanks Raghu On 4/6/20 3:57 AM, Louis Mayencourt via TF-A wrote: > Hi Raghu, > > Let me try to clarify/reword my idea: > > We complete the fconf documentation with a binding document, which > defines the nodes that should be present in the config DTB to consider > it as valid/well-formed. The document contains two kinds of node: > > * mandatory (critical): the firmware can't proceed without this > information (example: load address, image UUID, ...). If this node > is not present in the DTS, the build fails and/or the code panic. > > * optional (no-critical): the firmware can assume or assign a default > value and proceed (example: uart config, enable authentication flag, > ...). Such a property is used to influence the default behavior of > the firmware. > > />> This is non-deterministic failure./ > The miss of a mandatory (critical) node will always lead to a build > failure / code panic. > The miss of an optional (no-critical) node should not influence the > default behavior of the firmware. > > />> Is it not confusing to make the assumption that a DTB is "well > formed",i.e expect the build process/integrator to not mess up the >>> structure or number of nodes but allow the same integrator to miss a critical property in the DTB?/ > A DTB with a missing mandatory (critical) property should not be > considered well formed. > > />> If there is a missing critical property, is that not a badly formed > DTB?/ > With the above definition, it is. > > />> And if so, why not check for badly formed DTB's uniformly in code > and why only check for missing "critical" properties?/ > With the rewording of "critical" / "no-critical" to "mandatory" / > "optional", I think the situation is clearer: A DTB with a missing > "optional" node/property is is still considered well-formed. > > I hope I answered your questions and clarify the idea behind the design. > > regards, > Louis > > ------------------------------------------------------------------------ > *From:* Raghu Krishnamurthy <raghu.ncstate(a)icloud.com> > *Sent:* 05 April 2020 00:06 > *To:* Louis Mayencourt <Louis.Mayencourt(a)arm.com> > *Subject:* Re: [TF-A] fconf: Validating config data > Thanks Louis. > >>you can imagine a well-formed DTB which contain a > >>critical set of properties and can contain some optional properties. > > This makes things even more confusing. The assumption we are asking code > to make is that the DTB is always "well formed", ie don't check for > structural issues such as extra nodes etc, but we are still making the > distinctions between critical and non-critical properties, that may or > may not exist in the DTB, which may or may not cause a panic. This is > non-deterministic failure. > Is it not confusing to make the assumption that a DTB is "well > formed",i.e expect the build process/integrator to not mess up the > structure or number of nodes but allow the same integrator to miss a > critical property in the DTB? If there is a missing critical property, > is that not a badly formed DTB ? And if so, why not check for badly > formed DTB's uniformly in code and why only check for missing "critical" > properties? > > -Raghu > > On 4/3/20 3:16 AM, Louis Mayencourt wrote: >> Hi Raghu, >> >> I do agree that we need something similar to a binding document for >> fconf properties. (similar to >> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3694/2/docs/…). > >> At least for the common properties. >> >> The main idea behind the return code of the populator function was to >> allow the code to handle no-critical property misses or to handle >> critical failure by calling a platform hook. >> With this in mind, you can imagine a well-formed DTB which contain a >> critical set of properties and can contain some optional properties. The >> return code and the populator "name" / "config" can be used to handle >> this two cases. >> >> I tried to keep the design of fconf really simple, to leave room for >> improvement according to feedbacks. Thanks for helping improving it! >> >> Regards, >> Louis >> ------------------------------------------------------------------------ >> *From:* TF-A <tf-a-bounces(a)lists.trustedfirmware.org> on behalf of Raghu >> Krishnamurthy via TF-A <tf-a(a)lists.trustedfirmware.org> >> *Sent:* 03 April 2020 10:15 >> *To:* tf-a(a)lists.trustedfirmware.org <tf-a(a)lists.trustedfirmware.org> >> *Subject:* Re: [TF-A] fconf: Validating config data >> A further point is that the fconf populators return an error code and >> panics on error today. But if we are making the assumption that the >> DTB's are well formed, do we really need to fail or even return an error >> code? >> >> -Raghu >> >> On 4/3/20 1:51 AM, Raghu Krishnamurthy via TF-A wrote: >>> Hi All, (Sorry for the long email) >>> >>> The review >>> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3845 >>> attempts to fix bounds check in the fconf populator code for the >>> topology and SP's. During review, Sandrine thoughtfully pointed out that >>> there were discussions around bounds check along the same lines in the >>> review >>> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3492 and >>> it was deemed sufficient to have assertions in code and it was safe to >>> make the assumption that the DTB is always well formed and contains >>> valid values. I think this email mostly echoes Sandrine's concern from >>> review 3492. >>> >>> While i agree with the assumptions, I am generally of the opinion that >>> we should validate/range-check any data, even if it is signed. Being >>> signed does not necessarily mean the data is well formed/valid. If there >>> is a mistake in the build process and it is validly signed, it is >>> possible that we silently corrupt state/data that could later be used to >>> exploit firmware and/or make debugging hard. This is probably far >>> fetched, but the cost of adding the check is trivial to avoid this >>> possibility. >>> >>> I imagine the case where you have secure partitions signed by different >>> entity other than the silicon provider(dual root-of-trust). A silicon >>> provider provides a dev system for the SP provider to test and validate >>> the SP's on silicon. The silicon has production firmware(and hence no >>> assertions), but loads signed data from the SP provider which has some >>> invalid values. There could be silent corruption without any indication >>> whatsoever about what went wrong and it may be hard to debug if/when >>> there are issues. >>> Also, testing does not necessarily catch all invalid values since you >>> will likely not get 100% coverage, given the number of config options >>> available. Moreover, the code today, is not consistent in asserting on >>> every property for valid values and the failure mode is not >>> consistent/deterministic. It seems like every config option should have >>> a list of valid values or a range of acceptable values that must be at a >>> minimum asserted on. >>> I also wouldn't discount platforms such as RPI, where TRUSTED_BOARD_BOOT >>> is likely to be turned off since it really does not provide any >>> security, so assuming we always have signed data might not be valid. >>> >>> Anyway, is this decision worth revisiting? Too paranoid perhaps? :P >>> >>> >>> Thanks >>> Raghu >> -- >> TF-A mailing list >> TF-A(a)lists.trustedfirmware.org >> https://lists.trustedfirmware.org/mailman/listinfo/tf-a >> IMPORTANT NOTICE: The contents of this email and any attachments are >> confidential and may also be privileged. If you are not the intended >> recipient, please notify the sender immediately and do not disclose the >> contents to any other person, use it for any purpose, or store or copy >> the information in any medium. Thank you. > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy > the information in any medium. Thank you. >

6 years

1
0
0 0

Re: [TF-A] fconf: Validating config data

by Louis Mayencourt

Hi Raghu, Let me try to clarify/reword my idea: We complete the fconf documentation with a binding document, which defines the nodes that should be present in the config DTB to consider it as valid/well-formed. The document contains two kinds of node: * mandatory (critical): the firmware can't proceed without this information (example: load address, image UUID, ...). If this node is not present in the DTS, the build fails and/or the code panic. * optional (no-critical): the firmware can assume or assign a default value and proceed (example: uart config, enable authentication flag, ...). Such a property is used to influence the default behavior of the firmware. >> This is non-deterministic failure. The miss of a mandatory (critical) node will always lead to a build failure / code panic. The miss of an optional (no-critical) node should not influence the default behavior of the firmware. >> Is it not confusing to make the assumption that a DTB is "well formed",i.e expect the build process/integrator to not mess up the >> structure or number of nodes but allow the same integrator to miss a critical property in the DTB? A DTB with a missing mandatory (critical) property should not be considered well formed. >> If there is a missing critical property, is that not a badly formed DTB? With the above definition, it is. >> And if so, why not check for badly formed DTB's uniformly in code and why only check for missing "critical" properties? With the rewording of "critical" / "no-critical" to "mandatory" / "optional", I think the situation is clearer: A DTB with a missing "optional" node/property is is still considered well-formed. I hope I answered your questions and clarify the idea behind the design. regards, Louis ________________________________ From: Raghu Krishnamurthy <raghu.ncstate(a)icloud.com> Sent: 05 April 2020 00:06 To: Louis Mayencourt <Louis.Mayencourt(a)arm.com> Subject: Re: [TF-A] fconf: Validating config data Thanks Louis. >>you can imagine a well-formed DTB which contain a >>critical set of properties and can contain some optional properties. This makes things even more confusing. The assumption we are asking code to make is that the DTB is always "well formed", ie don't check for structural issues such as extra nodes etc, but we are still making the distinctions between critical and non-critical properties, that may or may not exist in the DTB, which may or may not cause a panic. This is non-deterministic failure. Is it not confusing to make the assumption that a DTB is "well formed",i.e expect the build process/integrator to not mess up the structure or number of nodes but allow the same integrator to miss a critical property in the DTB? If there is a missing critical property, is that not a badly formed DTB ? And if so, why not check for badly formed DTB's uniformly in code and why only check for missing "critical" properties? -Raghu On 4/3/20 3:16 AM, Louis Mayencourt wrote: > Hi Raghu, > > I do agree that we need something similar to a binding document for > fconf properties. (similar to > https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3694/2/docs/…). > At least for the common properties. > > The main idea behind the return code of the populator function was to > allow the code to handle no-critical property misses or to handle > critical failure by calling a platform hook. > With this in mind, you can imagine a well-formed DTB which contain a > critical set of properties and can contain some optional properties. The > return code and the populator "name" / "config" can be used to handle > this two cases. > > I tried to keep the design of fconf really simple, to leave room for > improvement according to feedbacks. Thanks for helping improving it! > > Regards, > Louis > ------------------------------------------------------------------------ > *From:* TF-A <tf-a-bounces(a)lists.trustedfirmware.org> on behalf of Raghu > Krishnamurthy via TF-A <tf-a(a)lists.trustedfirmware.org> > *Sent:* 03 April 2020 10:15 > *To:* tf-a(a)lists.trustedfirmware.org <tf-a(a)lists.trustedfirmware.org> > *Subject:* Re: [TF-A] fconf: Validating config data > A further point is that the fconf populators return an error code and > panics on error today. But if we are making the assumption that the > DTB's are well formed, do we really need to fail or even return an error > code? > > -Raghu > > On 4/3/20 1:51 AM, Raghu Krishnamurthy via TF-A wrote: >> Hi All, (Sorry for the long email) >> >> The review >> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3845 >> attempts to fix bounds check in the fconf populator code for the >> topology and SP's. During review, Sandrine thoughtfully pointed out that >> there were discussions around bounds check along the same lines in the >> review >> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3492 and >> it was deemed sufficient to have assertions in code and it was safe to >> make the assumption that the DTB is always well formed and contains >> valid values. I think this email mostly echoes Sandrine's concern from >> review 3492. >> >> While i agree with the assumptions, I am generally of the opinion that >> we should validate/range-check any data, even if it is signed. Being >> signed does not necessarily mean the data is well formed/valid. If there >> is a mistake in the build process and it is validly signed, it is >> possible that we silently corrupt state/data that could later be used to >> exploit firmware and/or make debugging hard. This is probably far >> fetched, but the cost of adding the check is trivial to avoid this >> possibility. >> >> I imagine the case where you have secure partitions signed by different >> entity other than the silicon provider(dual root-of-trust). A silicon >> provider provides a dev system for the SP provider to test and validate >> the SP's on silicon. The silicon has production firmware(and hence no >> assertions), but loads signed data from the SP provider which has some >> invalid values. There could be silent corruption without any indication >> whatsoever about what went wrong and it may be hard to debug if/when >> there are issues. >> Also, testing does not necessarily catch all invalid values since you >> will likely not get 100% coverage, given the number of config options >> available. Moreover, the code today, is not consistent in asserting on >> every property for valid values and the failure mode is not >> consistent/deterministic. It seems like every config option should have >> a list of valid values or a range of acceptable values that must be at a >> minimum asserted on. >> I also wouldn't discount platforms such as RPI, where TRUSTED_BOARD_BOOT >> is likely to be turned off since it really does not provide any >> security, so assuming we always have signed data might not be valid. >> >> Anyway, is this decision worth revisiting? Too paranoid perhaps? :P >> >> >> Thanks >> Raghu > -- > TF-A mailing list > TF-A(a)lists.trustedfirmware.org > https://lists.trustedfirmware.org/mailman/listinfo/tf-a > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy > the information in any medium. Thank you. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

6 years

1
0
0 0

Re: [TF-A] fconf: Validating config data

by Raghu Krishnamurthy

+tf-a list On 4/4/20 4:06 PM, Raghu Krishnamurthy wrote: > Thanks Louis. > >>you can imagine a well-formed DTB which contain a > >>critical set of properties and can contain some optional properties. > > This makes things even more confusing. The assumption we are asking code > to make is that the DTB is always "well formed", ie don't check for > structural issues such as extra nodes etc, but we are still making the > distinctions between critical and non-critical properties, that may or > may not exist in the DTB, which may or may not cause a panic. This is > non-deterministic failure. > Is it not confusing to make the assumption that a DTB is "well > formed",i.e expect the build process/integrator to not mess up the > structure or number of nodes but allow the same integrator to miss a > critical property in the DTB? If there is a missing critical property, > is that not a badly formed DTB ? And if so, why not check for badly > formed DTB's uniformly in code and why only check for missing "critical" > properties? > > -Raghu > > On 4/3/20 3:16 AM, Louis Mayencourt wrote: >> Hi Raghu, >> >> I do agree that we need something similar to a binding document for >> fconf properties. (similar to >> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3694/2/docs/…). >> At least for the common properties. >> >> The main idea behind the return code of the populator function was to >> allow the code to handle no-critical property misses or to handle >> critical failure by calling a platform hook. >> With this in mind, you can imagine a well-formed DTB which contain a >> critical set of properties and can contain some optional properties. >> The return code and the populator "name" / "config" can be used to >> handle this two cases. >> >> I tried to keep the design of fconf really simple, to leave room for >> improvement according to feedbacks. Thanks for helping improving it! >> >> Regards, >> Louis >> ------------------------------------------------------------------------ >> *From:* TF-A <tf-a-bounces(a)lists.trustedfirmware.org> on behalf of >> Raghu Krishnamurthy via TF-A <tf-a(a)lists.trustedfirmware.org> >> *Sent:* 03 April 2020 10:15 >> *To:* tf-a(a)lists.trustedfirmware.org <tf-a(a)lists.trustedfirmware.org> >> *Subject:* Re: [TF-A] fconf: Validating config data >> A further point is that the fconf populators return an error code and >> panics on error today. But if we are making the assumption that the >> DTB's are well formed, do we really need to fail or even return an error >> code? >> >> -Raghu >> >> On 4/3/20 1:51 AM, Raghu Krishnamurthy via TF-A wrote: >>> Hi All, (Sorry for the long email) >>> >>> The review >>> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3845 >>> attempts to fix bounds check in the fconf populator code for the >>> topology and SP's. During review, Sandrine thoughtfully pointed out >>> that there were discussions around bounds check along the same lines >>> in the review >>> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3492 and >>> it was deemed sufficient to have assertions in code and it was safe >>> to make the assumption that the DTB is always well formed and >>> contains valid values. I think this email mostly echoes Sandrine's >>> concern from review 3492. >>> >>> While i agree with the assumptions, I am generally of the opinion >>> that we should validate/range-check any data, even if it is signed. >>> Being signed does not necessarily mean the data is well formed/valid. >>> If there is a mistake in the build process and it is validly signed, >>> it is possible that we silently corrupt state/data that could later >>> be used to exploit firmware and/or make debugging hard. This is >>> probably far fetched, but the cost of adding the check is trivial to >>> avoid this possibility. >>> >>> I imagine the case where you have secure partitions signed by >>> different entity other than the silicon provider(dual root-of-trust). >>> A silicon provider provides a dev system for the SP provider to test >>> and validate the SP's on silicon. The silicon has production >>> firmware(and hence no assertions), but loads signed data from the SP >>> provider which has some invalid values. There could be silent >>> corruption without any indication whatsoever about what went wrong >>> and it may be hard to debug if/when there are issues. >>> Also, testing does not necessarily catch all invalid values since you >>> will likely not get 100% coverage, given the number of config options >>> available. Moreover, the code today, is not consistent in asserting >>> on every property for valid values and the failure mode is not >>> consistent/deterministic. It seems like every config option should >>> have a list of valid values or a range of acceptable values that must >>> be at a minimum asserted on. >>> I also wouldn't discount platforms such as RPI, where >>> TRUSTED_BOARD_BOOT is likely to be turned off since it really does >>> not provide any security, so assuming we always have signed data >>> might not be valid. >>> >>> Anyway, is this decision worth revisiting? Too paranoid perhaps? :P >>> >>> >>> Thanks >>> Raghu >> -- >> TF-A mailing list >> TF-A(a)lists.trustedfirmware.org >> https://lists.trustedfirmware.org/mailman/listinfo/tf-a >> IMPORTANT NOTICE: The contents of this email and any attachments are >> confidential and may also be privileged. If you are not the intended >> recipient, please notify the sender immediately and do not disclose >> the contents to any other person, use it for any purpose, or store or >> copy the information in any medium. Thank you. >

6 years

1
0
0 0

Re: [TF-A] Change in ...trusted-firmware-a[integration]: Check for out-of-bound accesses in the CoT description

by Sandrine Bailleux

Hi Raghu, On 4/3/20 1:38 AM, Raghu Krishnamurthy via TF-A wrote: > Thanks Sandrine. Patches look good. Thanks for the review! > I realized after looking at things a little closer that i had > misunderstood how fconf works for io policies. I thought the image id's > themselves came from the config files and not just the UUID's, which is > why i was worried about bounds check, since the id was coming from an > external source(trusted or untrusted, depending on if it is signed data > or not). > This also made me realize that we are using another table built into > code, to convert from image id to UUID for io policies. Is there a > reason image id's also can't be discovered from the config file? I remember some internal discussions around this topic a few weeks ago. If I recall correctly, the current thinking is that down the line, we would like to move image IDs to DTBs but this looks complicated to achieve today because image IDs are used by several components in TF-A to tie things together. More work would be needed to abstract this properly everywhere. I think other folks in the team (Olivier? Manish? Louis?) might be able to comment further on this. Regards, Sandrine IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

6 years

1
0
0 0

Re: [TF-A] fconf: Validating config data

by Raghu Krishnamurthy

A further point is that the fconf populators return an error code and panics on error today. But if we are making the assumption that the DTB's are well formed, do we really need to fail or even return an error code? -Raghu On 4/3/20 1:51 AM, Raghu Krishnamurthy via TF-A wrote: > Hi All, (Sorry for the long email) > > The review > https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3845 > attempts to fix bounds check in the fconf populator code for the > topology and SP's. During review, Sandrine thoughtfully pointed out that > there were discussions around bounds check along the same lines in the > review > https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3492 and > it was deemed sufficient to have assertions in code and it was safe to > make the assumption that the DTB is always well formed and contains > valid values. I think this email mostly echoes Sandrine's concern from > review 3492. > > While i agree with the assumptions, I am generally of the opinion that > we should validate/range-check any data, even if it is signed. Being > signed does not necessarily mean the data is well formed/valid. If there > is a mistake in the build process and it is validly signed, it is > possible that we silently corrupt state/data that could later be used to > exploit firmware and/or make debugging hard. This is probably far > fetched, but the cost of adding the check is trivial to avoid this > possibility. > > I imagine the case where you have secure partitions signed by different > entity other than the silicon provider(dual root-of-trust). A silicon > provider provides a dev system for the SP provider to test and validate > the SP's on silicon. The silicon has production firmware(and hence no > assertions), but loads signed data from the SP provider which has some > invalid values. There could be silent corruption without any indication > whatsoever about what went wrong and it may be hard to debug if/when > there are issues. > Also, testing does not necessarily catch all invalid values since you > will likely not get 100% coverage, given the number of config options > available. Moreover, the code today, is not consistent in asserting on > every property for valid values and the failure mode is not > consistent/deterministic. It seems like every config option should have > a list of valid values or a range of acceptable values that must be at a > minimum asserted on. > I also wouldn't discount platforms such as RPI, where TRUSTED_BOARD_BOOT > is likely to be turned off since it really does not provide any > security, so assuming we always have signed data might not be valid. > > Anyway, is this decision worth revisiting? Too paranoid perhaps? :P > > > Thanks > Raghu

6 years

1
0
0 0

fconf: Validating config data

by Raghu Krishnamurthy

Hi All, (Sorry for the long email) The review https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3845 attempts to fix bounds check in the fconf populator code for the topology and SP's. During review, Sandrine thoughtfully pointed out that there were discussions around bounds check along the same lines in the review https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3492 and it was deemed sufficient to have assertions in code and it was safe to make the assumption that the DTB is always well formed and contains valid values. I think this email mostly echoes Sandrine's concern from review 3492. While i agree with the assumptions, I am generally of the opinion that we should validate/range-check any data, even if it is signed. Being signed does not necessarily mean the data is well formed/valid. If there is a mistake in the build process and it is validly signed, it is possible that we silently corrupt state/data that could later be used to exploit firmware and/or make debugging hard. This is probably far fetched, but the cost of adding the check is trivial to avoid this possibility. I imagine the case where you have secure partitions signed by different entity other than the silicon provider(dual root-of-trust). A silicon provider provides a dev system for the SP provider to test and validate the SP's on silicon. The silicon has production firmware(and hence no assertions), but loads signed data from the SP provider which has some invalid values. There could be silent corruption without any indication whatsoever about what went wrong and it may be hard to debug if/when there are issues. Also, testing does not necessarily catch all invalid values since you will likely not get 100% coverage, given the number of config options available. Moreover, the code today, is not consistent in asserting on every property for valid values and the failure mode is not consistent/deterministic. It seems like every config option should have a list of valid values or a range of acceptable values that must be at a minimum asserted on. I also wouldn't discount platforms such as RPI, where TRUSTED_BOARD_BOOT is likely to be turned off since it really does not provide any security, so assuming we always have signed data might not be valid. Anyway, is this decision worth revisiting? Too paranoid perhaps? :P Thanks Raghu

6 years

1
0
0 0

Re: [TF-A] Change in ...trusted-firmware-a[integration]: Check for out-of-bound accesses in the CoT description

by Raghu Krishnamurthy

Thanks Sandrine. Patches look good. I realized after looking at things a little closer that i had misunderstood how fconf works for io policies. I thought the image id's themselves came from the config files and not just the UUID's, which is why i was worried about bounds check, since the id was coming from an external source(trusted or untrusted, depending on if it is signed data or not). This also made me realize that we are using another table built into code, to convert from image id to UUID for io policies. Is there a reason image id's also can't be discovered from the config file? -Raghu On 4/2/20 7:17 AM, Sandrine Bailleux (Code Review) wrote: > Hi guys, > > This is the patch I mentioned last Thursday at the TF-A tech call. Sorry > it took me so long to post it. > > View Change > <https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3836> > > To view, visit change 3836 > <https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3836>. > To unsubscribe, or for help writing mail filters, visit settings > <https://review.trustedfirmware.org/settings>. > > Gerrit-Project: TF-A/trusted-firmware-a > Gerrit-Branch: integration > Gerrit-Change-Id: Ic5ea20e43cf8ca959bb7f9b60de7c0839b390add > Gerrit-Change-Number: 3836 > Gerrit-PatchSet: 1 > Gerrit-Owner: Sandrine Bailleux <sandrine.bailleux(a)arm.com> > Gerrit-Reviewer: Louis Mayencourt <louis.mayencourt(a)arm.com> > Gerrit-Reviewer: Raghu K <raghu.ncstate(a)icloud.com> > Gerrit-Reviewer: Sandrine Bailleux <sandrine.bailleux(a)arm.com> > Gerrit-Comment-Date: Thu, 02 Apr 2020 14:17:12 +0000 > Gerrit-HasComments: No > Gerrit-Has-Labels: No > Gerrit-MessageType: comment

6 years

1
0
0 0

New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware

by scan-admin＠coverity.com

Hi, Please find the latest report on new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. 1 new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. New defect(s) Reported-by: Coverity Scan Showing 1 of 1 defect(s) ** CID 355446: (SIZEOF_MISMATCH) /mbedtls/library/x509_crt.c: 1713 in x509_get_other_name() /mbedtls/library/x509_crt.c: 1691 in x509_get_other_name() /mbedtls/library/x509_crt.c: 1728 in x509_get_other_name() ________________________________________________________________________________________________________ *** CID 355446: (SIZEOF_MISMATCH) /mbedtls/library/x509_crt.c: 1713 in x509_get_other_name() 1707 other_name->value.hardware_module_name.oid.tag = MBEDTLS_ASN1_OID; 1708 other_name->value.hardware_module_name.oid.p = p; 1709 other_name->value.hardware_module_name.oid.len = len; 1710 1711 if( p + len >= end ) 1712 { >>> CID 355446: (SIZEOF_MISMATCH) >>> Passing argument "other_name" of type "mbedtls_x509_san_other_name *" and argument "8UL /* sizeof (other_name) */" to function "mbedtls_platform_zeroize" is suspicious. 1713 mbedtls_platform_zeroize( other_name, sizeof( other_name ) ); 1714 return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS + 1715 MBEDTLS_ERR_ASN1_LENGTH_MISMATCH ); 1716 } 1717 p += len; 1718 if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, /mbedtls/library/x509_crt.c: 1691 in x509_get_other_name() 1685 { 1686 return( MBEDTLS_ERR_X509_FEATURE_UNAVAILABLE ); 1687 } 1688 1689 if( p + len >= end ) 1690 { >>> CID 355446: (SIZEOF_MISMATCH) >>> Passing argument "other_name" of type "mbedtls_x509_san_other_name *" and argument "8UL /* sizeof (other_name) */" to function "mbedtls_platform_zeroize" is suspicious. 1691 mbedtls_platform_zeroize( other_name, sizeof( other_name ) ); 1692 return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS + 1693 MBEDTLS_ERR_ASN1_LENGTH_MISMATCH ); 1694 } 1695 p += len; 1696 if( ( ret = mbedtls_asn1_get_tag( &p, end, &len, /mbedtls/library/x509_crt.c: 1728 in x509_get_other_name() 1722 other_name->value.hardware_module_name.val.tag = MBEDTLS_ASN1_OCTET_STRING; 1723 other_name->value.hardware_module_name.val.p = p; 1724 other_name->value.hardware_module_name.val.len = len; 1725 p += len; 1726 if( p != end ) 1727 { >>> CID 355446: (SIZEOF_MISMATCH) >>> Passing argument "other_name" of type "mbedtls_x509_san_other_name *" and argument "8UL /* sizeof (other_name) */" to function "mbedtls_platform_zeroize" is suspicious. 1728 mbedtls_platform_zeroize( other_name, 1729 sizeof( other_name ) ); 1730 return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS + 1731 MBEDTLS_ERR_ASN1_LENGTH_MISMATCH ); 1732 } 1733 return( 0 ); ________________________________________________________________________________________________________ To view the defects in Coverity Scan visit, https://u2389337.ct.sendgrid.net/ls/click?upn=nJaKvJSIH-2FPAfmty-2BK5tYpPkl…

6 years

1
0
0 0

Re: [TF-A] [TF-TSC] [TF-M] Project Maintenance Proposal for tf.org Projects

by Sandrine Bailleux

Hi Joakim, On 4/1/20 10:08 AM, Joakim Bech via TSC wrote: > Hi Christian, Sandrine, all, > > On Thu, Mar 26, 2020 at 10:27:14AM +0100, Sandrine Bailleux wrote: >> Hi Christian, >> >> Thanks a lot for the read and the comments! >> >> On 3/25/20 7:05 PM, Christian Daudt wrote: >>> ï¿½The maintenance proposal looks great ! I have some feedback on >>> specific portions: >>> ï¿½1. maintainer/owner/author patches. " Note that roles can be >>> cumulative, in particular the same individual can be both a code owner >>> and a maintainer. In such a scenario, the individual would be able to >>> self-merge a patch solely affecting his module, having the authority to >>> approve it from both a code owner and maintainer's point of view.": I'm >>> always leery of people self-approving their patches. At a minimum, all >>> self-patches should be published and a minimum wait time provided for >>> feedback. Or preferably that another maintainer does the merge (it does >>> not need to be mandated but should be suggested). >> >> Yes, actually this is something that generated some disagreement inside Arm >> as well and I am glad you're bringing this up here, as I'd like to hear more >> opinions on this. >> >> I too have concerns about allowing self-reviewing. I am not so much >> concerned about people potentially abusing of this situation to silently >> merge patches, as I think we should trust our maintainers. But I am worried >> that a self-review is rarely as good as a peer review, simply because it is >> so easy to miss things when it's your own work. I believe several pair of >> eyes is always better, as different people think differently, have different >> perspectives and backgrounds, and are able to catch different issues. >> >> But to pull this off, we need enough people to do all these reviews. The >> proposal currently allows self-review because some of us feared that >> mandating 2 reviewers for every patch (especially pure platform patches) >> would be impractical and too heavyweight, especially for the TF-M project in >> its current contributors organization, as I understand. It would be great to >> get more feedback from the TF-M community as to whether they think it could >> work in the end. >> >> It's a difficult balance between having the best possible code review >> practices, and realistically getting all the review work done in a timely >> manner, avoiding bottlenecks on specific people and keeping the flow of >> patches smooth. >> >> I like your idea of a minimum wait time provided for feedback. I think it >> could be a good middle ground solution. >> > +1 for that, after silence for X weeks it should be OK to merge the > patch. X would need to be number that is high enough for people to have > a chance to find it and look into it, but shouldn't be too high, since > there is a risk that it'll force the contributor to pile up things that > might be dependent on this patch. To throw something out, I'd say ~2 > weeks sounds like a good number to me. > >> Your other suggestion of having a different maintainer doing the merge would >> work as well IMO but requires more workforce. Again this comes down to >> whether this can realistically be achieved for each project. This solution >> was actually suggested within Arm as well (and even called out at the end of >> the proposal ;) ). >> >> Bottom line is, in an ideal world I would like to condemn self-review >> because I consider this as bad practice > +1 > >> , but I do not know whether this will >> be practical and will work for TF-M as well. >> >>> ï¿½2. 'timely manner': This expectation should be more explicit - when >>> the author can start requesting other maintainers to merge on assumption >>> that silence == approval (or not). Such timeliness expectations are >>> probably best set per project however. >> >> Yes, "timely manner" is definitely too vague and was actually left that way >> on purpose at this stage to avoid touching upon what I think is a sensitive >> subject! I am aware that some patches sometimes spend a long time in review, >> definitely longer than they should and it understandably generates some >> frustration. This is something we absolutely need to improve on IMO and >> hopefully a bigger pool of maintainers will help solve this issue. But I >> agree that the expected review timeline should be clearly established and it >> is probably best to let each project decides theirs. >> >>> ï¿½3. The proposal does not address branching strategies. i.e. will >>> there be separate maintainers for dev/master/stable branches? I don't >>> think it needs to address it yet - keep it simpler for a start. But a >>> todo saying something like "in the future this project maintenance >>> proposal might be expanded to address multi-branch maintainership" would >>> be good. >> >> Good point. A todo sounds good, I will add one in the last section of the >> document. >> >>> ï¿½4. The platform lifecycle state machine has too many transitions. >>> "Fully maintained" <-> "orphan" -> "out" seems sufficient to me. >> >> Hmm OK. There might be too many transitions but I feel we need something >> between fully maintained and out, i.e. the limited support one. >> >> Julius Werner also pointed out on Thursday that orphan might be misplaced, >> as all these other stages deal with some degrees of feature support (what's >> known to work), whereas orphan is an orthogonal topic that is not directly >> related to the level of supported features. For example, a platform could >> have recently become orphan but all features and tests still work for some >> time. >> > At one point in time in the OP-TEE project we tried to keep track of > maintained platforms, by simply saying maintained "Yes" if they are > maintained. However they're not maintained, we indicated that by stating > the last known version where a platform was maintained. People can still > find that information here [1] (not up-to-date). The intention was to > give future users of an old platform a chance to know if it ever has > been supported and what version that was. That could serve as a starting > point in case someone is interested in bring a device/platform back to > life. Yes, I think such information can be very useful. It saves some "git archeology" effort to try and dig this information afterwards. Also, when someone starts looking at a project, I would expect this to be one of the first thing they look up, they would want to know in which shape the project is for the particular platform they are interested in. That's almost as important in my eyes as a "getting started" guide. We could have such a high-level table that just says whether a platform is supported or not (just a yes/no) and have complementary, per-platform documentation that goes into the details of what features are supported exactly. > How that works in practice is that all OP-TEE maintainers are adding > their "Tested-by" (see example [2]) tag for the platform they maintain > when we're doing a release. If there are platforms with no "Tested-by" > tag, then they simply end up with the "last known version". I think that's a very good idea! > However, to keep that up-to-date, it requires some discipline from the > people maintaining such a table ... something that we in the OP-TEE > project haven't been very good at :) Can't this be automated, such that it doesn't need to be manually kept up-to-date? I imagine we could have some tools generating the platform support table out of such a commit message. > So, I'm not proposing something, it's just that I wanted to share what > we've tried and it "works", but not easy to maintain (a release > checklist could fix that). > > [1] https://optee.readthedocs.io/en/latest/general/platforms.html > [2] https://github.com/OP-TEE/optee_os/pull/3309/commits/765b92604459240bed7fcf… >

6 years

2
1
0 0

2026

2025

2024

2023

2022

2021

2020

2019

2018

TF-A