- TF-A - lists.trustedfirmware.org

by Olivier Deprez

Hi Varun, Please consider Ivy (and Quark) payloads are remnant from older SPCI specs and must be considered deprecated. We did not clean this in deep to remove the related test code but AFAIK it's just not used anywhere in the test suites (although it may still be built). We may have a plan to upgrade this later when working on S-EL0 partitions on top of Hafnium, but that's not an immediate priority. Considering Cactus, that's the bare-metal S-EL1 payload you can use in place of a real TOS on top of Hafnium. The intent is to test FF-A ABIs unitarily at secure virtual FF-A instance. TFTF at EL2 exercises the ABI at non-secure physical FF-A instance to communicate with the secure endpoint. See below the build commands we use for FVP. Hopefully this should help porting to your platform. Notice it needs as well building the SPMC (aka Hafnium in the secure side), which only supports FVP at this moment (and Rpi, qemu...) It's not described here but I can guide you through this as well. I think you can just use a dummy BL32 payload for now, at least to test the build/integration. If TFTF and TF-A reside in the same top level dir: TFTF: this builds TFTF and cactus secure partitions make CROSS_COMPILE=aarch64-none-elf- PLAT=fvp TESTS=spm -j8 TF-A: this uses TFTF, and assembles two cactus secure partitions within the FIP make \ CROSS_COMPILE=aarch64-none-elf- \ SPD=spmd \ CTX_INCLUDE_EL2_REGS=1 \ ARM_ARCH_MINOR=4 \ BL33=../tf-a-tests/build/fvp/debug/tftf.bin \ BL32=<path-to-secure-hafnium-bin>/hafnium.bin \ SP_LAYOUT_FILE=../tf-a-tests/build/fvp/debug/sp_layout.json \ PLAT=fvp \ all fip The tool last FIP tool entries are the two cactus instances: B4B5671E-4A90-4FE1-B81F-FB13DAE1DACB: offset=0x47DA3, size=0xC168, cmdline="--blob" D1582309-F023-47B9-827C-4464F5578FC8: offset=0x53F0B, size=0xC168, cmdline="--blob" Regards, Olivier. ________________________________________ From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> on behalf of Varun Wadekar via TF-A <tf-a(a)lists.trustedfirmware.org> Sent: 27 July 2020 06:46 To: tf-a(a)lists.trustedfirmware.org Subject: [TF-A] cactus and ivy on Tegra194 Hello, In order to test the SPM dispatcher from TF-A, we plan to enable ‘cactus’ and ‘ivy’ on Tegra194 platforms. I was able to muscle my way through all the compilation issues, but the final payload generation part is not that clear. Can someone please help me with the steps to generate the final FIP package with all the payloads – TF-A, Cactus, Ivy? Thanks.

5 years, 3 months

2
3
0 0

TF-A Tech Forum Agenda @ Thr 30th July 2020 16:00-1700 BST

by Joanna Farley

Hi All, The next TF-A Tech Forum is scheduled for Thu 30th July 2020 16:00 – 17:00 (BST). A reoccurring meeting invite has been sent out to the subscribers of this TF-A mailing list. If you don’t have this please let me know. Agenda: * Detailed Investigation for support for TRNG (True Random Number Generator) * Presented by Jimmy Brisson * Initial Investigation for support for GIC600AE * Presented by Jimmy Brisson * Optional TF-A Mailing List Topic Discussions If TF-A contributors have anything they wish to present at any future TF-A tech forum please contact me to have that scheduled. Previous sessions, both recording and presentation material can be found on the trustedfirmware.org TF-A Technical meeting webpage: https://www.trustedfirmware.org/meetings/tf-a-technical-forum/ A scheduling tracking page is also available to help track sessions suggested and being prepared: https://developer.trustedfirmware.org/w/tf_a/tf-a-tech-forum-scheduling/ Final decisions on what will be presented will be shared a few days before the next meeting and shared on the TF-A mailing list. Thanks Joanna

5 years, 3 months

1
0
0 0

Re: [TF-A] New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware

by Sandrine Bailleux

Hello Joanna, Varun, Regarding the idea of looping in Coverity Scan Online in the patch submission process, that is not possible because this free service from Synopsys only allows for a dozen of analyses to be requested per week per open-source project. If we were to request analyses for every patch submission, we would hit the limit very quickly. That is the reason why this is setup to run on the integration branch once per work week day at the moment. I appreciate this is not ideal (as pointed out by both of you) as we get defects reports after patches have been merged but I can't see another way out of this. Regards, Sandrine On 7/27/20 9:08 AM, Joanna Farley via TF-A wrote: > Hi Varun, > > At this time not that I know of which is why we have the solution we have. I'm sure with enough investment of effort something can be done. If we were going to do that though I would personally prefer investigating integrating this tighter into the patch review tooling and report there before patch submitting but there too lies challenges interfacing to the online Coverity server offered as a free service to opensource projects. > > Cheers > > Joanna > > > On 22/07/2020, 18:09, "TF-A on behalf of Varun Wadekar via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: > > Hi, > > Is there a way to create a ticket and assign Coverity flagged issues to the code owner automatically? > > -Varun > > -----Original Message----- > From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of scan-admin--- via TF-A > Sent: Wednesday, July 22, 2020 8:17 AM > To: tf-a(a)lists.trustedfirmware.org > Subject: [TF-A] New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware > > External email: Use caution opening links or attachments > > > Hi, > > Please find the latest report on new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. > > 3 new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. > > > New defect(s) Reported-by: Coverity Scan Showing 3 of 3 defect(s) > > > ** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) > /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() > > > ________________________________________________________________________________________________________ > *** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) > /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() > 267 size_t n; > 268 > 269 ASSERT_SYM_PTR_ALIGN(out); > 270 > 271 for (n = 0U; n < nb_elt; n++) { > 272 out[2 * n] = (uint32_t)rates[n]; > >>> CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) > >>> "(uint64_t)rates[n] >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment. > 273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32); > 274 } > 275 } > 276 > 277 static void scmi_clock_describe_rates(struct scmi_msg *msg) > 278 { > > ** CID 360934: Integer handling issues (BAD_SHIFT) > /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() > > > ________________________________________________________________________________________________________ > *** CID 360934: Integer handling issues (BAD_SHIFT) > /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() > 267 size_t n; > 268 > 269 ASSERT_SYM_PTR_ALIGN(out); > 270 > 271 for (n = 0U; n < nb_elt; n++) { > 272 out[2 * n] = (uint32_t)rates[n]; > >>> CID 360934: Integer handling issues (BAD_SHIFT) > >>> In expression "(uint64_t)rates[n] >> 32", right shifting "rates[n]" by more than 31 bits always yields zero. The shift amount is 32. > 273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32); > 274 } > 275 } > 276 > 277 static void scmi_clock_describe_rates(struct scmi_msg *msg) > 278 { > > ** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) > /drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get() > > > ________________________________________________________________________________________________________ > *** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) > /drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get() > 185 return; > 186 } > 187 > 188 rate = plat_scmi_clock_get_rate(msg->agent_id, clock_id); > 189 > 190 return_values.rate[0] = (uint32_t)rate; > >>> CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) > >>> "(uint64_t)rate >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment. > 191 return_values.rate[1] = (uint32_t)((uint64_t)rate >> 32); > 192 > 193 scmi_write_response(msg, &return_values, sizeof(return_values)); > 194 } > 195 > 196 static void scmi_clock_rate_set(struct scmi_msg *msg) > > > ________________________________________________________________________________________________________ > To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P… > > -- > TF-A mailing list > TF-A(a)lists.trustedfirmware.org > https://lists.trustedfirmware.org/mailman/listinfo/tf-a > -- > TF-A mailing list > TF-A(a)lists.trustedfirmware.org > https://lists.trustedfirmware.org/mailman/listinfo/tf-a >

5 years, 3 months

2
2
0 0

Re: [TF-A] New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware

by Joanna Farley

Hi Varun, At this time not that I know of which is why we have the solution we have. I'm sure with enough investment of effort something can be done. If we were going to do that though I would personally prefer investigating integrating this tighter into the patch review tooling and report there before patch submitting but there too lies challenges interfacing to the online Coverity server offered as a free service to opensource projects. Cheers Joanna On 22/07/2020, 18:09, "TF-A on behalf of Varun Wadekar via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: Hi, Is there a way to create a ticket and assign Coverity flagged issues to the code owner automatically? -Varun -----Original Message----- From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of scan-admin--- via TF-A Sent: Wednesday, July 22, 2020 8:17 AM To: tf-a(a)lists.trustedfirmware.org Subject: [TF-A] New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware External email: Use caution opening links or attachments Hi, Please find the latest report on new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. 3 new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. New defect(s) Reported-by: Coverity Scan Showing 3 of 3 defect(s) ** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() ________________________________________________________________________________________________________ *** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() 267 size_t n; 268 269 ASSERT_SYM_PTR_ALIGN(out); 270 271 for (n = 0U; n < nb_elt; n++) { 272 out[2 * n] = (uint32_t)rates[n]; >>> CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) >>> "(uint64_t)rates[n] >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment. 273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32); 274 } 275 } 276 277 static void scmi_clock_describe_rates(struct scmi_msg *msg) 278 { ** CID 360934: Integer handling issues (BAD_SHIFT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() ________________________________________________________________________________________________________ *** CID 360934: Integer handling issues (BAD_SHIFT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() 267 size_t n; 268 269 ASSERT_SYM_PTR_ALIGN(out); 270 271 for (n = 0U; n < nb_elt; n++) { 272 out[2 * n] = (uint32_t)rates[n]; >>> CID 360934: Integer handling issues (BAD_SHIFT) >>> In expression "(uint64_t)rates[n] >> 32", right shifting "rates[n]" by more than 31 bits always yields zero. The shift amount is 32. 273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32); 274 } 275 } 276 277 static void scmi_clock_describe_rates(struct scmi_msg *msg) 278 { ** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get() ________________________________________________________________________________________________________ *** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get() 185 return; 186 } 187 188 rate = plat_scmi_clock_get_rate(msg->agent_id, clock_id); 189 190 return_values.rate[0] = (uint32_t)rate; >>> CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) >>> "(uint64_t)rate >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment. 191 return_values.rate[1] = (uint32_t)((uint64_t)rate >> 32); 192 193 scmi_write_response(msg, &return_values, sizeof(return_values)); 194 } 195 196 static void scmi_clock_rate_set(struct scmi_msg *msg) ________________________________________________________________________________________________________ To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P… -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a

5 years, 3 months

1
0
0 0

cactus and ivy on Tegra194

by Varun Wadekar

Hello, In order to test the SPM dispatcher from TF-A, we plan to enable 'cactus' and 'ivy' on Tegra194 platforms. I was able to muscle my way through all the compilation issues, but the final payload generation part is not that clear. Can someone please help me with the steps to generate the final FIP package with all the payloads - TF-A, Cactus, Ivy? Thanks.

5 years, 3 months

1
0
0 0

Re: [TF-A] New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware

by Joanna Farley

Hi Varun, I agree things are less than ideal at the moment. We take advantage as an open source project to use the Synopsys online Coverity service and this points to the master branch (actually currently the mirror on github) which means these coverity tests are ran on changes after they have been submitted. Less than ideal I know but better than not being ran at all. Really we would like these to be ran as part of the OpenCI as part of patch reviews but we are not there yet. Not sure if the Synopsys online Coverity service can be setup to support this via Gerrit reviews or if this will have to be done after patch submittals. If it has to be done after then some clever CI scripting would need to be done to identify where the offending change came from. Would welcome other ideas to make this experience better, but for now this is better than nothing. Cheers Joanna On 22/07/2020, 18:09, "TF-A on behalf of Varun Wadekar via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: Hi, Is there a way to create a ticket and assign Coverity flagged issues to the code owner automatically? -Varun -----Original Message----- From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of scan-admin--- via TF-A Sent: Wednesday, July 22, 2020 8:17 AM To: tf-a(a)lists.trustedfirmware.org Subject: [TF-A] New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware External email: Use caution opening links or attachments Hi, Please find the latest report on new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. 3 new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. New defect(s) Reported-by: Coverity Scan Showing 3 of 3 defect(s) ** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() ________________________________________________________________________________________________________ *** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() 267 size_t n; 268 269 ASSERT_SYM_PTR_ALIGN(out); 270 271 for (n = 0U; n < nb_elt; n++) { 272 out[2 * n] = (uint32_t)rates[n]; >>> CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) >>> "(uint64_t)rates[n] >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment. 273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32); 274 } 275 } 276 277 static void scmi_clock_describe_rates(struct scmi_msg *msg) 278 { ** CID 360934: Integer handling issues (BAD_SHIFT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() ________________________________________________________________________________________________________ *** CID 360934: Integer handling issues (BAD_SHIFT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() 267 size_t n; 268 269 ASSERT_SYM_PTR_ALIGN(out); 270 271 for (n = 0U; n < nb_elt; n++) { 272 out[2 * n] = (uint32_t)rates[n]; >>> CID 360934: Integer handling issues (BAD_SHIFT) >>> In expression "(uint64_t)rates[n] >> 32", right shifting "rates[n]" by more than 31 bits always yields zero. The shift amount is 32. 273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32); 274 } 275 } 276 277 static void scmi_clock_describe_rates(struct scmi_msg *msg) 278 { ** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get() ________________________________________________________________________________________________________ *** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get() 185 return; 186 } 187 188 rate = plat_scmi_clock_get_rate(msg->agent_id, clock_id); 189 190 return_values.rate[0] = (uint32_t)rate; >>> CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) >>> "(uint64_t)rate >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment. 191 return_values.rate[1] = (uint32_t)((uint64_t)rate >> 32); 192 193 scmi_write_response(msg, &return_values, sizeof(return_values)); 194 } 195 196 static void scmi_clock_rate_set(struct scmi_msg *msg) ________________________________________________________________________________________________________ To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P… -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a

5 years, 4 months

2
3
0 0

Re: [TF-A] New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware

by Varun Wadekar

Hi, Is there a way to create a ticket and assign Coverity flagged issues to the code owner automatically? -Varun -----Original Message----- From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of scan-admin--- via TF-A Sent: Wednesday, July 22, 2020 8:17 AM To: tf-a(a)lists.trustedfirmware.org Subject: [TF-A] New Defects reported by Coverity Scan for ARM-software/arm-trusted-firmware External email: Use caution opening links or attachments Hi, Please find the latest report on new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. 3 new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. New defect(s) Reported-by: Coverity Scan Showing 3 of 3 defect(s) ** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() ________________________________________________________________________________________________________ *** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() 267 size_t n; 268 269 ASSERT_SYM_PTR_ALIGN(out); 270 271 for (n = 0U; n < nb_elt; n++) { 272 out[2 * n] = (uint32_t)rates[n]; >>> CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) >>> "(uint64_t)rates[n] >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment. 273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32); 274 } 275 } 276 277 static void scmi_clock_describe_rates(struct scmi_msg *msg) 278 { ** CID 360934: Integer handling issues (BAD_SHIFT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() ________________________________________________________________________________________________________ *** CID 360934: Integer handling issues (BAD_SHIFT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() 267 size_t n; 268 269 ASSERT_SYM_PTR_ALIGN(out); 270 271 for (n = 0U; n < nb_elt; n++) { 272 out[2 * n] = (uint32_t)rates[n]; >>> CID 360934: Integer handling issues (BAD_SHIFT) >>> In expression "(uint64_t)rates[n] >> 32", right shifting "rates[n]" by more than 31 bits always yields zero. The shift amount is 32. 273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32); 274 } 275 } 276 277 static void scmi_clock_describe_rates(struct scmi_msg *msg) 278 { ** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get() ________________________________________________________________________________________________________ *** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get() 185 return; 186 } 187 188 rate = plat_scmi_clock_get_rate(msg->agent_id, clock_id); 189 190 return_values.rate[0] = (uint32_t)rate; >>> CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) >>> "(uint64_t)rate >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment. 191 return_values.rate[1] = (uint32_t)((uint64_t)rate >> 32); 192 193 scmi_write_response(msg, &return_values, sizeof(return_values)); 194 } 195 196 static void scmi_clock_rate_set(struct scmi_msg *msg) ________________________________________________________________________________________________________ To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P… -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a

5 years, 4 months

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. 3 new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. New defect(s) Reported-by: Coverity Scan Showing 3 of 3 defect(s) ** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() ________________________________________________________________________________________________________ *** CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() 267 size_t n; 268 269 ASSERT_SYM_PTR_ALIGN(out); 270 271 for (n = 0U; n < nb_elt; n++) { 272 out[2 * n] = (uint32_t)rates[n]; >>> CID 360935: Integer handling issues (CONSTANT_EXPRESSION_RESULT) >>> "(uint64_t)rates[n] >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment. 273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32); 274 } 275 } 276 277 static void scmi_clock_describe_rates(struct scmi_msg *msg) 278 { ** CID 360934: Integer handling issues (BAD_SHIFT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() ________________________________________________________________________________________________________ *** CID 360934: Integer handling issues (BAD_SHIFT) /drivers/st/scmi-msg/clock.c: 273 in write_rate_desc_array_in_buffer() 267 size_t n; 268 269 ASSERT_SYM_PTR_ALIGN(out); 270 271 for (n = 0U; n < nb_elt; n++) { 272 out[2 * n] = (uint32_t)rates[n]; >>> CID 360934: Integer handling issues (BAD_SHIFT) >>> In expression "(uint64_t)rates[n] >> 32", right shifting "rates[n]" by more than 31 bits always yields zero. The shift amount is 32. 273 out[2 * n + 1] = (uint32_t)((uint64_t)rates[n] >> 32); 274 } 275 } 276 277 static void scmi_clock_describe_rates(struct scmi_msg *msg) 278 { ** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get() ________________________________________________________________________________________________________ *** CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /drivers/st/scmi-msg/clock.c: 191 in scmi_clock_rate_get() 185 return; 186 } 187 188 rate = plat_scmi_clock_get_rate(msg->agent_id, clock_id); 189 190 return_values.rate[0] = (uint32_t)rate; >>> CID 360933: Integer handling issues (CONSTANT_EXPRESSION_RESULT) >>> "(uint64_t)rate >> 32" is 0 regardless of the values of its operands. This occurs as the operand of assignment. 191 return_values.rate[1] = (uint32_t)((uint64_t)rate >> 32); 192 193 scmi_write_response(msg, &return_values, sizeof(return_values)); 194 } 195 196 static void scmi_clock_rate_set(struct scmi_msg *msg) ________________________________________________________________________________________________________ To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P…

5 years, 4 months

1
0
0 0

Re: [TF-A] [PATCH] Add support for ARMv8.3-SPE

by Olivier Deprez

Hi Wei Li, Thanks for your change which looks correct. Can you possibly submit it to review.trustedfirmware.org? We can follow up with the review from the gerrit interface. Thanks, Olivier. ________________________________________ From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> on behalf of Wei Li via TF-A <tf-a(a)lists.trustedfirmware.org> Sent: 22 July 2020 14:25 To: tf-a(a)lists.trustedfirmware.org Cc: huawei.libin(a)huawei.com; guohanjun(a)huawei.com Subject: [TF-A] [PATCH] Add support for ARMv8.3-SPE When ARMv8.3-SPE is implemented, ID_AA64DFR0_EL1.PMSVer is read as 0b0010, update the version check to support ARMv8.3-SPE or higher. Signed-off-by: Wei Li <liwei391(a)huawei.com> --- lib/extensions/spe/spe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/extensions/spe/spe.c b/lib/extensions/spe/spe.c index 78876c66b..aa0bcd8ea 100644 --- a/lib/extensions/spe/spe.c +++ b/lib/extensions/spe/spe.c @@ -25,7 +25,7 @@ bool spe_supported(void) uint64_t features; features = read_id_aa64dfr0_el1() >> ID_AA64DFR0_PMS_SHIFT; - return (features & ID_AA64DFR0_PMS_MASK) == 1U; + return (features & ID_AA64DFR0_PMS_MASK) != 0U; } void spe_enable(bool el2_unused) -- 2.17.1 -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a

5 years, 4 months

1
0
0 0

[PATCH] Add support for ARMv8.3-SPE

by Wei Li

When ARMv8.3-SPE is implemented, ID_AA64DFR0_EL1.PMSVer is read as 0b0010, update the version check to support ARMv8.3-SPE or higher. Signed-off-by: Wei Li <liwei391(a)huawei.com> --- lib/extensions/spe/spe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/extensions/spe/spe.c b/lib/extensions/spe/spe.c index 78876c66b..aa0bcd8ea 100644 --- a/lib/extensions/spe/spe.c +++ b/lib/extensions/spe/spe.c @@ -25,7 +25,7 @@ bool spe_supported(void) uint64_t features; features = read_id_aa64dfr0_el1() >> ID_AA64DFR0_PMS_SHIFT; - return (features & ID_AA64DFR0_PMS_MASK) == 1U; + return (features & ID_AA64DFR0_PMS_MASK) != 0U; } void spe_enable(bool el2_unused) -- 2.17.1

5 years, 4 months

1
0
0 0

Re: [TF-A] [Hafnium] Debugging page table creation in Hafnium

by Madhukar Pappireddy

Hi Raghu and Andrew, Thanks for the inputs. I have added the mm_vm_dump() call to the mm_init() function but nothing was dumped to uart log. I guess I am missing something trivial. It looks like mm_root_table_count() returns 0. Any suggestions? diff --git a/src/mm.c b/src/mm.c index 2e352e9..7c56a09 100644 --- a/src/mm.c +++ b/src/mm.c @@ -1041,5 +1041,7 @@ bool mm_init(struct mpool *ppool) mm_identity_map(stage1_locked, layout_data_begin(), layout_data_end(), MM_MODE_R | MM_MODE_W, ppool); + mm_vm_dump(&ptable); + return arch_mm_init(ptable.root); } Thanks, Madhukar -----Original Message----- From: Hafnium <hafnium-bounces(a)lists.trustedfirmware.org> On Behalf Of Andrew Walbran via Hafnium Sent: Friday, July 17, 2020 6:10 AM To: Raghu K <raghu.ncstate(a)icloud.com> Cc: hafnium(a)lists.trustedfirmware.org Subject: Re: [Hafnium] Debugging page table creation in Hafnium Yep, mm_vm_dump sounds like what you're looking for. You can add a call where you like and it will go to the log UART. On Thu, 16 Jul 2020 at 19:14, Raghu K via Hafnium < hafnium(a)lists.trustedfirmware.org> wrote: > Quick search indicates mm_vm_dump() and the functions it calls in > src/mm.c should do what you want. i've not tried it or don't know the > format, but this may be what you are looking for. > > -Raghu > > On 7/16/20 11:03 AM, Madhukar Pappireddy via Hafnium wrote: > > Hi, > > > > I was wondering if there is support in Hafnium to dump page tables > > to a > log file. I am new to the Hafnium project and would appreciate any help. > Below is an example from TF-A which provides such provision. > > > > ......<snip> > > VERBOSE: Translation tables state: > > VERBOSE: Xlat regime: EL3 > > VERBOSE: Max allowed PA: 0xfffffffff > > VERBOSE: Max allowed VA: 0xfffffffff > > VERBOSE: Max mapped PA: 0x2f1fffff > > VERBOSE: Max mapped VA: 0x2f1fffff > > VERBOSE: Initial lookup level: 1 > > VERBOSE: Entries @initial lookup level: 64 > > VERBOSE: Used 3 sub-tables out of 5 (spare: 2) > > [LV1] VA:0x0 size:0x40000000 > > [LV2] VA:0x0 size:0x200000 > > [LV3] VA:0x0 PA:0x0 size:0x1000 MEM-RO-EXEC-S > > [LV3] VA:0x1000 PA:0x1000 size:0x1000 MEM-RO-EXEC-S > > [LV3] VA:0x2000 PA:0x2000 size:0x1000 MEM-RO-EXEC-S > > [LV3] VA:0x3000 PA:0x3000 size:0x1000 MEM-RO-EXEC-S > > [LV3] VA:0x4000 PA:0x4000 size:0x1000 MEM-RO-EXEC-S > > [LV3] VA:0x5000 PA:0x5000 size:0x1000 MEM-RO-EXEC-S > > [LV3] VA:0x6000 PA:0x6000 size:0x1000 MEM-RO-EXEC-S > > [LV3] VA:0x7000 PA:0x7000 size:0x1000 MEM-RO-EXEC-S > > [LV3] VA:0x8000 PA:0x8000 size:0x1000 MEM-RO-XN-S > > [LV3] VA:0x9000 PA:0x9000 size:0x1000 MEM-RO-XN-S > > [LV3] VA:0xa000 PA:0xa000 size:0x1000 MEM-RO-XN-S > > [LV3] VA:0xb000 size:0x1000 > > [LV3] (500 invalid descriptors omitted) > > [LV2] VA:0x200000 size:0x200000 > > [LV2] (30 invalid descriptors omitted) > > [LV2] VA:0x4000000 size:0x200000 ..... <snip> > > > > Thanks, > > Madhukar > > > > -- > Hafnium mailing list > Hafnium(a)lists.trustedfirmware.org > https://lists.trustedfirmware.org/mailman/listinfo/hafnium >

5 years, 4 months

2
3
0 0

Re: [TF-A] Pointer Authentication keys

by Soby Mathew

Hi Varun, AIU, Usually 2 keys are used to protect 2 different software contexts for enforcing the security boundary between them. This is the case for kernel and userspace. In TF-A, every BL image is allowed to choose the IA key to use, hence every BL image can have a separate IA key. Each BL image is executing within a single security domain and hence there is less use create a boundary within the BL image by using the 2nd key registers. The Compiler by default selects either IA or IB instruction key register to use by default. It cannot automatically make use of both key registers and it doesn't support the use of DA or DB yet I think. There are some function attributes which allow to use a different key at a function level, but the functions need to be marked out manually. But the requirement for creating the sub-domain within a BL image by specifying a different key for certain functions has not yet been seen AFAIK. Best Regards Soby Mathew From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of Varun Wadekar via TF-A Sent: 13 July 2020 22:20 To: tf-a(a)lists.trustedfirmware.org Cc: Kalyani Chidambaram Vaidyanathan <kalyanic(a)nvidia.com> Subject: [TF-A] Pointer Authentication keys Hi, >From the initial read of the Arm ARM, there are multiple keys (instruction, data) provided for authenticating pointers. But the current implementation only writes IA key [1]. I would like to understand the thought process behind programming only one key here. AFAIU, we should enable all keys - IA, IB, DA, DB. -Varun [1] https://review.trustedfirmware.org/plugins/gitiles/TF-A/trusted-firmware-a/…

5 years, 4 months

2
1
0 0

Re: [TF-A] Announcing some changes around the code review label in Gerrit

by Joanna Farley

Hi Julius, Sandrine is away on vacation so I thought I would follow up as we had a chat in one of our team meetings about the point below. > On 07/07/2020, 23:05, "TF-A on behalf of Julius Werner via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: >> IOW, you're fine with both options but you'd prefer to have different >> people setting MR+1 and COR+1 (or CR+1 in the special cases mentioned >> above), right? Just want to double-check I understood your position. >Right. So when we had +1/+2 CR before these changes we endeavoured to ensure that these were done by different people and if a maintainer gave a +1 they resisted also giving a +2 as we recognise having multiple eyes is always advantageous. So the general feeling was that this should be the recommended best practice. However, the feeling was also that this specific recommendation should not be strictly enforced by gerrit checking as on occasions it is possible to give both on simple patches and maintainers and code owners in this case can be trusted to make the correct choice. The enforcing of them in Gerrit should be viewed as an aid to try and avoid mistakes rather than strict control for as you say we don't expect people to intentionally break the rules. Perhaps a warning in this case can be made to remind folks what's best practice here. For now though as mentioned as we practice the new rules and recommendations we will use an honour system rather than strict enforcement along with updating the documented rules and recomendations. Thanks Joanna

5 years, 4 months

2
1
0 0

Re: [TF-A] Deadlock in SGI RAS handling

by Soby Mathew

Hi Raghu, The Exception Handling framework (EHF) was designed to provide prioritization between the EL3 interrupts and EA although EA will always have highest priority since it cannot be blocked. In the case you describe, the driver in EL3 which is handling the RAS errors would need to ensure serialization of the events delivered to the S-EL0 payload somehow. This could be either via holding the event in a queue in EL3 till EL0 is done with processing the first event or the EL0 payload is capable of re-entry and can manage a queue internally. In case of MM, I suppose re-entry is not an option and hence a holding queue in EL3 driver needs to be implemented. The current implementation in sgi_ras.c doesn't do this currently as this was a PoC to showcase the RAS flow. Best Regards Soby Mathew > -----Original Message----- > From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of Raghu K > via TF-A > Sent: 13 July 2020 22:28 > To: tf-a(a)lists.trustedfirmware.org > Subject: [TF-A] Deadlock in SGI RAS handling > > Hi All, > > I was going through some code in sgi_ras.c and was wondering if the > situation mentioned below could cause a deadlock or if i'm missing > something. It seems like it is possible to deadlock if we enter MM in S- > EL0(say through an MM_COMMUNICATE SMC or perhaps an initial RAS > interrupt) followed by a SYNC EA or ASYNC EA on the same core. sgi_ras.c > seems like it registers the same handler for both interrupts and aborts. > While interrupts can be blocked/masked, SYNC EA's cannot be blocked(not > that i know of), and i don't see SErrors being blocked on the path to the EA > handler and entry to MM. If this situation does occur, it seems like we could > deadlock when the EA attempts to enter MM again in the interrupt handler. > Is there something that would prevent this situation from happening? > > > Thanks > Raghu > -- > TF-A mailing list > TF-A(a)lists.trustedfirmware.org > https://lists.trustedfirmware.org/mailman/listinfo/tf-a

5 years, 4 months

1
0
0 0

Deadlock in SGI RAS handling

by Raghu K

Hi All, I was going through some code in sgi_ras.c and was wondering if the situation mentioned below could cause a deadlock or if i'm missing something. It seems like it is possible to deadlock if we enter MM in S-EL0(say through an MM_COMMUNICATE SMC or perhaps an initial RAS interrupt) followed by a SYNC EA or ASYNC EA on the same core. sgi_ras.c seems like it registers the same handler for both interrupts and aborts. While interrupts can be blocked/masked, SYNC EA's cannot be blocked(not that i know of), and i don't see SErrors being blocked on the path to the EA handler and entry to MM. If this situation does occur, it seems like we could deadlock when the EA attempts to enter MM again in the interrupt handler. Is there something that would prevent this situation from happening? Thanks Raghu

5 years, 4 months

1
0
0 0

Pointer Authentication keys

by Varun Wadekar

Hi, >From the initial read of the Arm ARM, there are multiple keys (instruction, data) provided for authenticating pointers. But the current implementation only writes IA key [1]. I would like to understand the thought process behind programming only one key here. AFAIU, we should enable all keys - IA, IB, DA, DB. -Varun [1] https://review.trustedfirmware.org/plugins/gitiles/TF-A/trusted-firmware-a/…

5 years, 4 months

1
0
0 0

TF-A Tech Forum Agenda @ Thr 16th July 2020 16:00-1700 BST

by Joanna Farley

Hi All, The next TF-A Tech Forum is scheduled for Thu 16th July 2020 16:00 – 17:00 (BST). A reoccurring meeting invite has been sent out to the subscribers of this TF-A mailing list. If you don’t have this please let me know. Agenda: * Secure EL2 SPM (Secure Partition Manager) Hafnium-based * In this TF-A Tech Forum session we present the status and open roadmap for the Secure Partition Manager firmware development. The TF-A SPM is the reference open source implementation for the PSA FF-A (Platform Security Architecture Firmware Framework for A-class) specification in the Secure world. It leverages the Armv8.4-Secure EL2 extension bringing virtualization technology in the Secure world (S-EL2 exception level). The development derives originally from the Google Hafnium project, which has been recently transitioned to https://www.trustedfirmware.org/ under the BSD 3-Clause license. * Optional TF-A Mailing List Topic Discussions If TF-A contributors have anything they wish to present at any future TF-A tech forum please contact me to have that scheduled. Previous sessions, both recording and presentation material can be found on the trustedfirmware.org TF-A Technical meeting webpage: https://www.trustedfirmware.org/meetings/tf-a-technical-forum/ A scheduling tracking page is also available to help track sessions suggested and being prepared: https://developer.trustedfirmware.org/w/tf_a/tf-a-tech-forum-scheduling/ Final decisions on what will be presented will be shared a few days before the next meeting and shared on the TF-A mailing list. Thanks Joanna

5 years, 4 months

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. 4 new defect(s) introduced to ARM-software/arm-trusted-firmware found with Coverity Scan. New defect(s) Reported-by: Coverity Scan Showing 4 of 4 defect(s) ** CID 360537: Insecure data handling (TAINTED_SCALAR) ________________________________________________________________________________________________________ *** CID 360537: Insecure data handling (TAINTED_SCALAR) /plat/qemu/common/qemu_bl2_setup.c: 72 in update_dt() 66 67 if (dt_add_psci_cpu_enable_methods(fdt)) { 68 ERROR("Failed to add PSCI cpu enable methods in Device Tree\n"); 69 return; 70 } 71 >>> CID 360537: Insecure data handling (TAINTED_SCALAR) >>> Passing tainted variable "fdt->size_dt_struct" to a tainted sink. 72 ret = fdt_pack(fdt); 73 if (ret < 0) 74 ERROR("Failed to pack Device Tree at %p: error %d\n", fdt, ret); 75 } 76 77 void bl2_platform_setup(void) ** CID 360536: (TAINTED_SCALAR) ________________________________________________________________________________________________________ *** CID 360536: (TAINTED_SCALAR) /plat/rpi/rpi4/rpi4_bl31_setup.c: 249 in rpi4_prepare_dtb() 243 gic_int_prop[2] = cpu_to_fdt32(0x0f04); // all cores, level high 244 fdt_setprop(dtb, offs, "interrupts", gic_int_prop, 12); 245 246 offs = fdt_path_offset(dtb, "/chosen"); 247 fdt_setprop_string(dtb, offs, "stdout-path", "serial0"); 248 >>> CID 360536: (TAINTED_SCALAR) >>> Passing tainted variable "dtb->size_dt_struct" to a tainted sink. 249 ret = fdt_pack(dtb); 250 if (ret < 0) 251 ERROR("Failed to pack Device Tree at %p: error %d\n", dtb, ret); 252 253 clean_dcache_range((uintptr_t)dtb, dtb_size(dtb)); 254 INFO("Changed device tree to advertise PSCI.\n"); /plat/rpi/rpi4/rpi4_bl31_setup.c: 220 in rpi4_prepare_dtb() 214 int ret, offs; 215 216 /* Return if no device tree is detected */ 217 if (fdt_check_header(dtb) != 0) 218 return; 219 >>> CID 360536: (TAINTED_SCALAR) >>> Passing tainted variable "dtb->totalsize" to a tainted sink. 220 ret = fdt_open_into(dtb, dtb, 0x100000); 221 if (ret < 0) { 222 ERROR("Invalid Device Tree at %p: error %d\n", dtb, ret); 223 return; 224 } 225 /plat/rpi/rpi4/rpi4_bl31_setup.c: 249 in rpi4_prepare_dtb() 243 gic_int_prop[2] = cpu_to_fdt32(0x0f04); // all cores, level high 244 fdt_setprop(dtb, offs, "interrupts", gic_int_prop, 12); 245 246 offs = fdt_path_offset(dtb, "/chosen"); 247 fdt_setprop_string(dtb, offs, "stdout-path", "serial0"); 248 >>> CID 360536: (TAINTED_SCALAR) >>> Passing tainted variable "dtb->size_dt_struct" to a tainted sink. 249 ret = fdt_pack(dtb); 250 if (ret < 0) 251 ERROR("Failed to pack Device Tree at %p: error %d\n", dtb, ret); 252 253 clean_dcache_range((uintptr_t)dtb, dtb_size(dtb)); 254 INFO("Changed device tree to advertise PSCI.\n"); /plat/rpi/rpi4/rpi4_bl31_setup.c: 249 in rpi4_prepare_dtb() 243 gic_int_prop[2] = cpu_to_fdt32(0x0f04); // all cores, level high 244 fdt_setprop(dtb, offs, "interrupts", gic_int_prop, 12); 245 246 offs = fdt_path_offset(dtb, "/chosen"); 247 fdt_setprop_string(dtb, offs, "stdout-path", "serial0"); 248 >>> CID 360536: (TAINTED_SCALAR) >>> Passing tainted variable "dtb->size_dt_struct" to a tainted sink. 249 ret = fdt_pack(dtb); 250 if (ret < 0) 251 ERROR("Failed to pack Device Tree at %p: error %d\n", dtb, ret); 252 253 clean_dcache_range((uintptr_t)dtb, dtb_size(dtb)); 254 INFO("Changed device tree to advertise PSCI.\n"); /plat/rpi/rpi4/rpi4_bl31_setup.c: 249 in rpi4_prepare_dtb() 243 gic_int_prop[2] = cpu_to_fdt32(0x0f04); // all cores, level high 244 fdt_setprop(dtb, offs, "interrupts", gic_int_prop, 12); 245 246 offs = fdt_path_offset(dtb, "/chosen"); 247 fdt_setprop_string(dtb, offs, "stdout-path", "serial0"); 248 >>> CID 360536: (TAINTED_SCALAR) >>> Passing tainted variable "dtb->size_dt_struct" to a tainted sink. 249 ret = fdt_pack(dtb); 250 if (ret < 0) 251 ERROR("Failed to pack Device Tree at %p: error %d\n", dtb, ret); 252 253 clean_dcache_range((uintptr_t)dtb, dtb_size(dtb)); 254 INFO("Changed device tree to advertise PSCI.\n"); /plat/rpi/rpi4/rpi4_bl31_setup.c: 220 in rpi4_prepare_dtb() 214 int ret, offs; 215 216 /* Return if no device tree is detected */ 217 if (fdt_check_header(dtb) != 0) 218 return; 219 >>> CID 360536: (TAINTED_SCALAR) >>> Passing tainted variable "dtb->size_dt_strings" to a tainted sink. 220 ret = fdt_open_into(dtb, dtb, 0x100000); 221 if (ret < 0) { 222 ERROR("Invalid Device Tree at %p: error %d\n", dtb, ret); 223 return; 224 } 225 ** CID 360535: Insecure data handling (TAINTED_SCALAR) ________________________________________________________________________________________________________ *** CID 360535: Insecure data handling (TAINTED_SCALAR) /plat/renesas/rcar/bl2_plat_setup.c: 1005 in bl2_el3_early_platform_setup() 999 bl2_lossy_setting(1, LOSSY_ST_ADDR1, LOSSY_END_ADDR1, 1000 LOSSY_FMT1, LOSSY_ENA_DIS1, fcnlnode); 1001 bl2_lossy_setting(2, LOSSY_ST_ADDR2, LOSSY_END_ADDR2, 1002 LOSSY_FMT2, LOSSY_ENA_DIS2, fcnlnode); 1003 #endif 1004 >>> CID 360535: Insecure data handling (TAINTED_SCALAR) >>> Passing tainted variable "fdt->size_dt_struct" to a tainted sink. 1005 fdt_pack(fdt); 1006 NOTICE("BL2: FDT at %p\n", fdt); 1007 1008 if (boot_dev == MODEMR_BOOT_DEV_EMMC_25X1 || 1009 boot_dev == MODEMR_BOOT_DEV_EMMC_50X8) 1010 rcar_io_emmc_setup(); ** CID 346762: (TAINTED_SCALAR) ________________________________________________________________________________________________________ *** CID 346762: (TAINTED_SCALAR) /lib/libfdt/fdt_empty_tree.c: 37 in fdt_create_empty_tree() 31 return err; 32 33 err = fdt_finish(buf); 34 if (err) 35 return err; 36 >>> CID 346762: (TAINTED_SCALAR) >>> Passing tainted variable "buf->size_dt_struct" to a tainted sink. 37 return fdt_open_into(buf, buf, bufsize); /lib/libfdt/fdt_empty_tree.c: 33 in fdt_create_empty_tree() 27 return err; 28 29 err = fdt_end_node(buf); 30 if (err) 31 return err; 32 >>> CID 346762: (TAINTED_SCALAR) >>> Passing tainted variable "buf->size_dt_strings" to a tainted sink. 33 err = fdt_finish(buf); 34 if (err) 35 return err; 36 37 return fdt_open_into(buf, buf, bufsize); /lib/libfdt/fdt_empty_tree.c: 37 in fdt_create_empty_tree() 31 return err; 32 33 err = fdt_finish(buf); 34 if (err) 35 return err; 36 >>> CID 346762: (TAINTED_SCALAR) >>> Passing tainted variable "buf->totalsize" to a tainted sink. 37 return fdt_open_into(buf, buf, bufsize); /lib/libfdt/fdt_empty_tree.c: 37 in fdt_create_empty_tree() 31 return err; 32 33 err = fdt_finish(buf); 34 if (err) 35 return err; 36 >>> CID 346762: (TAINTED_SCALAR) >>> Passing tainted variable "buf->size_dt_strings" to a tainted sink. 37 return fdt_open_into(buf, buf, bufsize); ________________________________________________________________________________________________________ To view the defects in Coverity Scan visit, https://u2389337.ct.sendgrid.net/ls/click?upn=nJaKvJSIH-2FPAfmty-2BK5tYpPkl…

5 years, 4 months

1
0
0 0

Re: [TF-A] Announcing some changes around the code review label in Gerrit

by Joanna Farley

+1 for re-introduction of 'Code-Review' label in addition to 'Code-Owner-Review' label. I would like to see the enforcement of authors of patches not being allowed to self review eventually for all labels. Hopefully we can enable maintainers who are not the author of deadlocked patches to allow the setting of all required +1's to labels is situations where this is necessary for admin purposes after due consideration. I agree though while this process is settling down an honour system is appropriate. Cheers Joanna On 01/07/2020, 08:56, "TF-A on behalf of Sandrine Bailleux via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: Hi Julius, On 7/1/20 2:13 AM, Julius Werner wrote: > Hi Sandrine, > > Sounds like good changes in general! I'm curious what the ACLs are for > Code-Owner-Review? Is it tied to docs/about/maintainers.rst or just > based on the honor system? (I notice that I seem to be able to give a > +1 for code I'm not owning, but maybe that's because I am a > maintainer?) Right now, the ACLs are not tied to docs/about/maintainers.rst (any registered user could vote on this label) and it is just based on the honor system. However, I'd like this to be enforced in the future, we just haven't had the time to put the right tooling in place for that. Also we did not want to spend time on developing such scripts before we tried out these process changes in practice. If they proved too heavy or inconvenient, part of this work would have gone to waste. BTW, any help for the tooling is welcome! If you've got plugin configuration files or scripts we could reuse (with an appropriate license) or even tips on how to best set this up, please feel free to share these. > Also, are code owners allowed to +1 themselves (I think > we said we didn't want maintainers to do that, but for code owners I > could see how we might want to allow it since there are usually not > that many)? What do we do when someone uploads the first patch for a > new platform, do they just COR+1 themselves (since there is no code > owner yet)? That is indeed a grey area. As you say, many modules have a single code owner and we need to agree on how we would like such code reviews to be conducted. Thanks for starting this discussion! One option as you say would be to allow code owners to self-review their patches but I am not convinced we would gain anything out of this. It sounds like a tick-box exercise to me, an admin overhead just to get the patch through the system and I would like to avoid that as much as possible. It is likely that a patch submitter has already self-reviewed his code before posting a patch anyway. The alternative we've been discussing in the team is to call out another reviewer in these situations. I think that there is still value in having a second fresh pair of eyes on a patch. Even if the reviewer has no particular expertise on this specific module, they can still catch potential logic problems or structural issues in the code. The latter would be my preference. What do others think? > I think it might still be useful to retain the existing Code-Review as > a +1/-1 label next to the two new ones, just to allow other interested > parties to record their opinion (without it having any enforcing > effect on the merge). In other Gerrit instances I have used people > often like to give CR+1 as a "I'm not the one who needs to approve > this but I have looked at it and think it's fine" or a CR-1 as a "I > can't stop you from doing this but I still want to be clear that I > don't think it's a good idea". It allows people outside the official > approval process a clearer way to participate and can aid the official > approvers in their decisions (e.g. when I am reviewing a patch as a > maintainer that already has a CR-1 from someone else I know to pay > extra attention to their concerns, and it's more visible than just > some random comment further up in the list). What do you think? That's a very good idea, thanks! It also aligns with Javier's concerns, which sound perfectly valid to me. I think your proposal of having 3 distinct labels is better than having code owners and non-code owners voting on the same generic 'Code-Review' label, which is what Javier was suggesting. It clarifies further the intent of each label and as you say allows us to configure different rules for each (i.e. make the generic 'Code-Review' label informative/optional, while making the 'Code-Owner-Review' label mandatory for patch submission). I am gonna wait for others' opinions on this before changing the configuration in Gerrit again. I see Raghu agrees with this approach. If nobody disagrees by the end of the week, I'll do these changes on Monday. In the meantime, as discussed above, any registered user can vote on the new 'Code-Owner-Review' label so let's continue to use that for the rest of this week. Regards, Sandrine -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a

5 years, 4 months

3
5
0 0

Re: [TF-A] Announcing some changes around the code review label in Gerrit

by Varun Wadekar

Hello Sandrine, IMO, the code review process has now evolved from the previous version. I am not too clear on the final outcome. Does it make sense to add a "lifecycle of a review" section to the initial proposal? -Varun -----Original Message----- From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of Sandrine Bailleux via TF-A Sent: Monday, July 6, 2020 1:53 AM To: Julius Werner <jwerner(a)chromium.org> Cc: tf-a <tf-a(a)lists.trustedfirmware.org> Subject: Re: [TF-A] Announcing some changes around the code review label in Gerrit External email: Use caution opening links or attachments Hi Julius and all, As agreed and announced last week, I've now reintroduced the Code-Review label in addition to the Code-Owner-Review and Maintainer-Review ones. The Code-Review label is purely informational and won't influence whether a patch is submittable in Gerrit. Anyone should be able to vote on this label, if you face any issue please let me know. On 7/1/20 11:12 PM, Julius Werner wrote: >> BTW, any help for the tooling is welcome! If you've got plugin >> configuration files or scripts we could reuse (with an appropriate >> license) or even tips on how to best set this up, please feel free to >> share these. > > The Chromium Gerrit has an owners-enforcement system but I'm not > familiar with how exactly they set it up. I think they're using this > plugin > https://gerrit.googlesource.com/plugins/find-owners/+/refs/heads/maste > r and this is where it's integrated into Gerrit submission rules: > https://chromium.googlesource.com/chromiumos/+/refs/meta/config/rules. > pl . It works by having a file called OWNERS in certain directories > which sets the owners for the subtree below it, so we would have to > rewrite the current code owner list in that format if we wanted to use > it. This "find-owners" plugin sounds promising! Thanks for the pointers. I notice the "reviewers" plugin [1] is installed on review.tf.org but it does not sound as configurable as "find-owners". Rewriting the current code owners list to use the OWNERS format does not sound like a big task to me (just a dull one... could be scripted, though) so this approach definitely sounds worth investigating to me. [1] https://review.trustedfirmware.org/plugins/reviewers/Documentation/index.ht… > There also seems to be this completely separate plugin that claims to > do roughly the same thing, I have no idea where the difference is > between the two: > https://gerrit.googlesource.com/plugins/owners/+/refs/heads/master I notice the "find-owners" plugin documentation mentions that "this plugin works with Gerrit projects that use Android or Chromium compatible OWNERS files" so maybe they started off from the generic "owners" plugin and customized it for Android/Chromium's needs? Just a guess. Anyway, worth a look as well, thanks! [2] https://gerrit.googlesource.com/plugins/find-owners/+/refs/heads/master/src… >> One option as you say would be to allow code owners to self-review their >> patches but I am not convinced we would gain anything out of this. It >> sounds like a tick-box exercise to me, an admin overhead just to get the >> patch through the system and I would like to avoid that as much as >> possible. It is likely that a patch submitter has already self-reviewed >> his code before posting a patch anyway. >> >> The alternative we've been discussing in the team is to call out another >> reviewer in these situations. I think that there is still value in >> having a second fresh pair of eyes on a patch. Even if the reviewer has >> no particular expertise on this specific module, they can still catch >> potential logic problems or structural issues in the code. > > I definitely did not mean to suggest that these patches should not be > reviewed at all -- I was just talking about the Code-Owner-Review > label. There would still be a maintainer review, of course, and it > seems like a good idea to get additional reviews from other people > too. They just couldn't set the COR+1 bit once we start ACLing it. Right, I see (and agree with you). > Basically, each of these bits have their own purpose, and I would see > the purpose of the COR+1 bit to be that the person most familiar with > that particular piece of code has made sure that it fits in and > doesn't cause unexpected problems with certain quirky configurations > or something like that. That's important when, say, I make a generic > refactoring that touches a platform I'm unfamiliar with, but if the > code owner adds to their own platform that's probably already a given > and they are likely still the best person to judge that, even for > their own code. Yes, that makes sense to me. > That all code gets reviewed by people other than the author I would > see as a somewhat orthogonal concern that should be checked > independently. So maybe the rule could be that if the code owner sets > COR+1 for themselves, there needs to be at least one Code-Review+1 (if > we reintroduce that label) from another person (who is also not the > one setting Maintainer-Review+1) to make sure the minimum amount of > reviews stays the same. Or maybe these should all be completely > independent checks so every patch needs a Code-Owner-Review+1 (can be > from the author), a Maintainer-Review+1 (not from the author) and at > least two Code-Review+1 from people other than the author -- with the > normal expectation that when a maintainer or code owner reviews a > patch, they would also set Code-Review+1 (as a general sign that they > reviewed the patch) in addition to their special meaning flag. I still don't see the benefit of code owners setting COR+1 for themselves. I would think that a patch owner already reviews his own patch before posting it for review so a self COR+1 is just reinstating the obvious in my eyes. Your first suggestion (i.e. having at least one CR+1 from another person when a patch author cannot find another code owner to review their patches) sounds aligned with what I proposed earlier. Your second suggestion sounds a bit too heavy to me. The fact that people would have to replay their vote on several labels sounds like an admin overhead to me. I guess this would simplify the ACL configuration but I would prefer to avoid this if we can. So I suggest the following. We could have 2 ways to get a patch approved. 1) For patches that have multiple code owners: * COR+1 (not from patch owner) * MR+1 (not from patch owner, not from the person setting COR+1) 2) For patches that a have single code owner: * CR+1 (not from patch owner) * MR+1 (not from patch owner, not from the person setting CR+1) In 2), I like your suggestion of mandating 2 CR+1 but I fear this might be difficult to achieve in practice (simply because we might not have enough reviewers for this). Note that in some cases, we would need to mix both policies. If a patch modifies several modules, some of them having multiple code owners and others having a single code owner, we would apply one or the other policy for each module. How does that sound? Also, 1) assumes we always want different individuals doing the code-owner review and the maintainer review. I personally don't feel strongly about this split and I would be fine with the same person doing both types of review, as they are typically looking at different criteria. What do others think? Regards, Sandrine -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a

5 years, 4 months

2
1
0 0

Re: [TF-A] Erroneous speculative AT workaround

by Soby Mathew

Thanks Andrew for raising the issue and Marc and Raghu for your inputs. The effect of stage 2 when Stage 1 is disabled is something that was overlooked in the workaround implementation. After some discussions this is what we currently understand regarding the problem. 1. EL3 does not modify the EL2 stage 1 translations and hence any speculative execution of AT execution in EL3 and resulting PTW and TLB caching should not be a problem as we always return to the same context in EL2. TF-A in EL3 need not worry about this. Also, we expect that future CPUs having v8.4 extensions for S-EL2 will have already corrected this errata. 2. Whenever the EL1/0 Stage 1 translations are switched in EL3 as part of switching the worlds, it should take Stage 2 into account. Hence disabling the Stage 1 MMU is not effective, as Marc pointed out, because speculative PTW can still take place using Stage 2 identity mapping. After discussion with James, we also realized that flipping the SCR_EL3.NS bit could take Stage 2 out of context (Stage 2 is not valid in secure world) leaving Stage 1 pointing to invalid translations. As discussed in previous emails, on Entry to EL3, if TCR_EL1.EPDx = 1 and SCTLR_EL3.M = 1, any speculative AT PTWs for EL1 context can be prevented (including Stage 2). So the fix as we currently understand would involve the following sequence : a. On Entry to EL3, save the incoming SCTLR_EL1.M and TCR_EL1.EPDx bits and set them (ensure TCR_EL1.EPDx =1 prior to SCTLR_EL1.M =1 using isb()) b. Prior to Exit from EL3, after the target context is restored, restore the SCTLR_EL1.M and TCR_EL1.EPDx bits. The above sequence now means that any use of AT instruction targeted at lower EL from EL3 that require PTW will fault. So prior to use of AT, ensure the PTW are re-enabled and disabled back again after the AT instructions. If the above sequence is agreed upon to resolve the errata, then we can work on a patch for the same. I suspect current el1 register save and restore sequence in TF-A is a bit unwieldy and we may need to analyze all the entry points to EL3 to ensure we cover everything. Best Regards Soby Mathew > -----Original Message----- > From: TF-A <tf-a-bounces(a)lists.trustedfirmware.org> On Behalf Of Andrew > Scull via TF-A > Sent: 02 July 2020 09:20 > To: Raghu K <raghu.ncstate(a)icloud.com> > Cc: android-kvm(a)google.com; willdeacon(a)google.com; Marc Zyngier > <mzyngier(a)google.com>; tf-a(a)lists.trustedfirmware.org > Subject: Re: [TF-A] Erroneous speculative AT workaround > > On Wed, Jul 01, 2020 at 05:47:00PM -0700, Raghu K wrote: > > This is interesting. It appears that there is no way on entry to EL3 > > to guarantee that the out-of-context(el2 and el1) translation regimes > > are in a consistent state and on every entry into EL3, we have to > > conservatively assume that it is in an inconsistent state. This is > > because of the situation Andrew mentioned(interrupts to EL3 can occur at any > time). > > > > If this is the case, on EL3 entry: > > 1) For EL1, we will need to save SCTLR_EL1, set SCTLR_EL1.M = 1,.EPDx > > = 0 > > This would still be racing against any potential speculative execution of an AT > instruction upon the switch to EL3, IIUC. The window would be much smaller > but not entirely eliminated. > > For KVM, this would be enough as KVM will have already applied this > workaround (with Marc's corrections) whenever it is going to enter an > inconsistent state. However, other EL2 software may choose to handle the > errata differently, possibly going to the lengths of ensuring that no AT > instruction is ever mapped executable. > -- > TF-A mailing list > TF-A(a)lists.trustedfirmware.org > https://lists.trustedfirmware.org/mailman/listinfo/tf-a

5 years, 4 months

4
4
0 0

Re: [TF-A] Erroneous speculative AT workaround

by Raghu K

5 years, 4 months

1
0
0 0

Re: [TF-A] Erroneous speculative AT workaround

by Raghu K

5 years, 4 months

1
0
0 0

Re: [TF-A] Erroneous speculative AT workaround

by Raghu K

This is interesting. It appears that there is no way on entry to EL3 to guarantee that the out-of-context(el2 and el1) translation regimes are in a consistent state and on every entry into EL3, we have to conservatively assume that it is in an inconsistent state. This is because of the situation Andrew mentioned(interrupts to EL3 can occur at any time). If this is the case, on EL3 entry: 1) For EL1, we will need to save SCTLR_EL1, set SCTLR_EL1.M = 1,.EPDx = 0 2) Set whatever bits we need to for EL2 and S2 translations to not succeed(What are these?) 3) DSB, to ensure no speculative AT can be issued until completion of DSB, so any AT that occurs will not fill the TLB with bad translations. On exit(right before ERET), we need to restore the registers saved on entry, and have the ERET followed by a DSB so that there can be no speculative execution of AT instructions. Thanks Raghu On 7/1/20 5:54 AM, Marc Zyngier via TF-A wrote: > Hi Manish, > > On Wed, 1 Jul 2020 at 13:14, Manish Badarkhe <Manish.Badarkhe(a)arm.com> wrote: >> Hi Andrew, >> >> As per current implementation, in “el1_sysregs_context_restore” routine do below things: >> >> 1. TCR_EL1.EPD0 = 1 >> 2. TCR_EL1.EPD1 = 1 >> 3. SCTR_EL1.M = 0 >> 4. Isb >> Code snippet: >> mrs x9, tcr_el1 >> orr x9, x9, #TCR_EPD0_BIT >> orr x9, x9, #TCR_EPD1_BIT >> msr tcr_el1, x9 >> mrs x9, sctlr_el1 >> bic x9, x9, #SCTLR_M_BIT >> msr sctlr_el1, x9 >> isb >> This is to avoid PTW through while updating system registers at step 5 > Unfortunately, this doesn't prevent anything. > > If SCTLR_EL1.M is clear, TCR_EL1.EPDx don't mean much (S1 MMU is > disabled, no S1 page table walk), and you can still have S2 PTWs > (using an idmap for S1) and creating TLB corruption if these entry > alias with any S1 mapping that exists at EL1. > > Which is why KVM does *set* SCTLR_EL1.M, which prevents the use of a > 1:1 mapping at S1, and at which point the TCR_EL1.EPDx bits are > actually useful in preventing a PTW. > >> 5. Restore all system registers for El1 except SCTLR_EL1 and TCR_EL1 >> 6. isb() >> 7. restore SCTLR_EL1 and TCR_EL1 >> Code Snippet: >> ldr x9, [x0, #CTX_SCTLR_EL1] -> saved value from "el2_sysregs_context_save" >> msr sctlr_el1, x9 >> ldr x9, [x0, #CTX_TCR_EL1] >> msr tcr_el1, x9 >> >> As per above steps. SCTLR_EL1 get restored back with actual settings at step 7. >> Similar flow is present for “el2_sysregs_context_restore” to restore SCTLR_EL1 register. >> >> In conclusion, this routine temporarily clear M bit of SCTLR_EL1 to avoid speculation but restored it back >> to its original setting while leaving back to its caller. Please let us know whether this align with KVM >> workaround for speculative AT erratum. > It doesn't, unfortunately. I believe this code actively creates > problems on a system that is affected by speculative AT execution. > > I don't understand your rationale for touching SCTLR_EL2.M either if > you are not context-switching the EL2 S1 state: as far as I understand > no affected cores have S-EL2, so no switch should happen at this > stage. > > Thanks, > > M.

5 years, 4 months

3
2
0 0

Re: [TF-A] Erroneous speculative AT workaround

by Manish Badarkhe

Hi Andrew, As per current implementation, in “el1_sysregs_context_restore” routine do below things: 1. TCR_EL1.EPD0 = 1 2. TCR_EL1.EPD1 = 1 3. SCTR_EL1.M = 0 4. Isb Code snippet: mrs x9, tcr_el1 orr x9, x9, #TCR_EPD0_BIT orr x9, x9, #TCR_EPD1_BIT msr tcr_el1, x9 mrs x9, sctlr_el1 bic x9, x9, #SCTLR_M_BIT msr sctlr_el1, x9 isb This is to avoid PTW through while updating system registers at step 5 5. Restore all system registers for El1 except SCTLR_EL1 and TCR_EL1 6. isb() 7. restore SCTLR_EL1 and TCR_EL1 Code Snippet: ldr x9, [x0, #CTX_SCTLR_EL1] -> saved value from "el2_sysregs_context_save" msr sctlr_el1, x9 ldr x9, [x0, #CTX_TCR_EL1] msr tcr_el1, x9 As per above steps. SCTLR_EL1 get restored back with actual settings at step 7. Similar flow is present for “el2_sysregs_context_restore” to restore SCTLR_EL1 register. In conclusion, this routine temporarily clear M bit of SCTLR_EL1 to avoid speculation but restored it back to its original setting while leaving back to its caller. Please let us know whether this align with KVM workaround for speculative AT erratum. Thanks Manish Badarkhe On 01/07/2020, 17:02, "TF-A on behalf of Joanna Farley via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: Thanks Andrew for the notification. We will look into it and get back to this thread. Thanks Joanna On 01/07/2020, 10:16, "TF-A on behalf of Andrew Scull via TF-A" <tf-a-bounces(a)lists.trustedfirmware.org on behalf of tf-a(a)lists.trustedfirmware.org> wrote: Whilst looking at the speculative AT workaround in KVM, I compared it against the workaround in TF-A and noticed an inconsistency whereby TF-A **breaks** KVM's workaround. In `el1_sysregs_context_restore`, the M bit of SCTRL_EL1 is cleared however Linux requires this to be set for its workaround to be correct. If an exception is taken to EL3 partway through a VM context switch, e.g. a secure interrupt, causing a switch to the secure world, TF-A will reintroduce the possibility of TLB corruption. The above explains how it is broken for Linux's chosen workaround however TF-A will also have to be compatible with whatever workaround the EL2 software is using. Starting this thread with the issue identified and we can add more details as needed. Thanks Andrew -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a -- TF-A mailing list TF-A(a)lists.trustedfirmware.org https://lists.trustedfirmware.org/mailman/listinfo/tf-a

5 years, 4 months

2
1
0 0

2025

2024

2023

2022

2021

2020

2019

2018

TF-A