| Message ID | c7d2cd46f06f75f8a1810168c508f05d242f831c.1689329728.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix some issues related to MISRA C:2012 Rule 9.1 | expand |
Hi, On 14/07/2023 12:49, Nicola Vetrini wrote: > This patch aims to fix some occurrences of possibly uninitialized > variables, that may be read before being written. This behaviour would > violate MISRA C:2012 Rule 9.1, besides being generally undesirable. > > In all the analyzed cases, such accesses were actually safe, but it's > quite difficult to prove so by automatic checking, therefore a safer > route is to change the code so as to avoid the behaviour from occurring, > while preserving the semantics. > > An initialization to a safe value is provided to reach this aim. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > Additional input on which values may be 'safe' in each context is > surely welcome, to avoid possibly compromising the correctness of > the function semantics. > --- > xen/arch/arm/cpuerrata.c | 6 +++--- > xen/arch/arm/domctl.c | 8 ++++---- > xen/arch/arm/gic-v3-lpi.c | 17 +++++++++-------- > xen/arch/arm/include/asm/p2m.h | 10 ++++++---- > xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 10 ++-------- > xen/arch/arm/psci.c | 10 +++++----- > xen/drivers/char/pl011.c | 2 +- > 7 files changed, 30 insertions(+), 33 deletions(-) > > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c > index d0658aedb6..14694c6081 100644 > --- a/xen/arch/arm/cpuerrata.c > +++ b/xen/arch/arm/cpuerrata.c > @@ -159,7 +159,7 @@ extern char __mitigate_spectre_bhb_loop_start_32[], > > static int enable_smccc_arch_workaround_1(void *data) > { > - struct arm_smccc_res res; > + struct arm_smccc_res res = {0}; I understand you desire to make happy. But I am not sure that initializing to 0 is the right thing. If the SMCC were not properly setting the register, then we most likely don't want to install the workaround. Instead, we most likely want to warn. So you want (int)res.a0 to be negative. We don't care about the other fields. > const struct arm_cpu_capabilities *entry = data; > > /* > @@ -252,7 +252,7 @@ static int enable_spectre_bhb_workaround(void *data) > > if ( cpus_have_cap(ARM_WORKAROUND_BHB_SMCC_3) ) > { > - struct arm_smccc_res res; > + struct arm_smccc_res res = {0}; Same remark here. > > if ( smccc_ver < SMCCC_VERSION(1, 1) ) > goto warn; > @@ -393,7 +393,7 @@ DEFINE_PER_CPU_READ_MOSTLY(register_t, ssbd_callback_required); > > static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry) > { > - struct arm_smccc_res res; > + struct arm_smccc_res res = {0}; Here you would want (int)res.a0 to be equal to ARM_SMCCC_NOT_SUPPORTED. > bool required; > > if ( smccc_ver < SMCCC_VERSION(1, 1) ) > diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c > index ad56efb0f5..b38fed72be 100644 > --- a/xen/arch/arm/domctl.c > +++ b/xen/arch/arm/domctl.c > @@ -29,10 +29,10 @@ static int handle_vuart_init(struct domain *d, > struct xen_domctl_vuart_op *vuart_op) > { > int rc; > - struct vpl011_init_info info; > - > - info.console_domid = vuart_op->console_domid; > - info.gfn = _gfn(vuart_op->gfn); > + struct vpl011_init_info info = { > + .console_domid = vuart_op->console_domid, > + .gfn = _gfn(vuart_op->gfn) > + }; I am not against, this change. But I don't quite understand how this makes Eclair much happier? Also, if this is the desired way, then I think this should be written down in the CODING_STYLE. > > if ( d->creation_finished ) > return -EPERM; > diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c > index eb0a5535e4..12f2af2e4d 100644 > --- a/xen/arch/arm/gic-v3-lpi.c > +++ b/xen/arch/arm/gic-v3-lpi.c > @@ -210,7 +210,10 @@ out: > void gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id, > uint32_t virt_lpi) > { > - union host_lpi *hlpip, hlpi; > + union host_lpi *hlpip, hlpi = { > + .virt_lpi = virt_lpi, > + .dom_id = domain_id > + }; > > ASSERT(host_lpi >= LPI_OFFSET); > > @@ -218,9 +221,6 @@ void gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id, > > hlpip = &lpi_data.host_lpis[host_lpi / HOST_LPIS_PER_PAGE][host_lpi % HOST_LPIS_PER_PAGE]; > > - hlpi.virt_lpi = virt_lpi; > - hlpi.dom_id = domain_id; > - > write_u64_atomic(&hlpip->data, hlpi.data); > } > > @@ -542,14 +542,15 @@ int gicv3_allocate_host_lpi_block(struct domain *d, uint32_t *first_lpi) > > for ( i = 0; i < LPI_BLOCK; i++ ) > { > - union host_lpi hlpi; > - > /* > * Mark this host LPI as belonging to the domain, but don't assign > * any virtual LPI or a VCPU yet. > */ > - hlpi.virt_lpi = INVALID_LPI; > - hlpi.dom_id = d->domain_id; > + union host_lpi hlpi = { > + .virt_lpi = INVALID_LPI, > + .dom_id = d->domain_id > + }; > + > write_u64_atomic(&lpi_data.host_lpis[chunk][lpi_idx + i].data, > hlpi.data); > > diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h > index 940495d42b..413e2a7add 100644 > --- a/xen/arch/arm/include/asm/p2m.h > +++ b/xen/arch/arm/include/asm/p2m.h > @@ -345,7 +345,7 @@ static inline struct page_info *get_page_from_gfn( > struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q) > { > mfn_t mfn; > - p2m_type_t _t; > + p2m_type_t _t = p2m_invalid; > struct page_info *page; > > /* > @@ -355,10 +355,12 @@ static inline struct page_info *get_page_from_gfn( > if ( likely(d != dom_xen) ) > return p2m_get_page_from_gfn(d, _gfn(gfn), t); > > - if ( !t ) > + /* Allow t to be NULL */ > + if ( t ) > + *t = _t; > + else { > t = &_t; > - > - *t = p2m_invalid; > + } I understand the exist construct is not liked by Eclair. But your new construct is not easily readable by a human. Before I can propose a different approach, can you clarify why Eclair is unhappy this the existing code? > /* > * DOMID_XEN sees 1-1 RAM. The p2m_type is based on the type of the > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c > index 2053ed7ac5..39d9ab4fa9 100644 > --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c > @@ -51,11 +51,11 @@ static inline bool domain_has_reset_access(struct domain *d, uint32_t rst) > > bool zynqmp_eemi(struct cpu_user_regs *regs) > { > - struct arm_smccc_res res; > + struct arm_smccc_res res = {0}; > uint32_t fid = get_user_reg(regs, 0); > uint32_t nodeid = get_user_reg(regs, 1); > unsigned int pm_fn = fid & 0xFFFF; > - enum pm_ret_status ret; > + enum pm_ret_status ret = XST_PM_NO_ACCESS; It is not clear to me why Eclair is unhappy here? > > switch ( fid ) > { > @@ -89,7 +89,6 @@ bool zynqmp_eemi(struct cpu_user_regs *regs) > { > gprintk(XENLOG_WARNING, > "zynqmp-pm: fn=%u No access to node %u\n", pm_fn, nodeid); > - ret = XST_PM_NO_ACCESS; > goto done; I find the new approach less obvious. How about introduce a macro that will set the result and the use replace the two lines with: set_result(regs, XST_PM_NO_ACCESS); return 0; where set_result(regs, ret) is set_user_reg(regs, 0, ret) > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c > index 695d2fa1f1..47e46af608 100644 > --- a/xen/arch/arm/psci.c > +++ b/xen/arch/arm/psci.c > @@ -38,7 +38,7 @@ static uint32_t psci_cpu_on_nr; > > int call_psci_cpu_on(int cpu) > { > - struct arm_smccc_res res; > + struct arm_smccc_res res = {0}; I don't this we should initialize a0 to 0 as this would indicate a success. Instead, we want an invalid value. > > arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), > &res); > @@ -50,7 +50,7 @@ void call_psci_cpu_off(void) > { > if ( psci_ver > PSCI_VERSION(0, 1) ) > { > - struct arm_smccc_res res; > + struct arm_smccc_res res = {0}; Same here. > > /* If successfull the PSCI cpu_off call doesn't return */ > arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF, &res); > @@ -73,7 +73,7 @@ void call_psci_system_reset(void) > > static int __init psci_features(uint32_t psci_func_id) > { > - struct arm_smccc_res res; > + struct arm_smccc_res res = {0}; Same here. > > if ( psci_ver < PSCI_VERSION(1, 0) ) > return PSCI_NOT_SUPPORTED; > @@ -115,7 +115,7 @@ static void __init psci_init_smccc(void) > > if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED ) > { > - struct arm_smccc_res res; > + struct arm_smccc_res res = {0}; Same here. > > arm_smccc_smc(ARM_SMCCC_VERSION_FID, &res); > if ( PSCI_RET(res) != ARM_SMCCC_NOT_SUPPORTED ) > @@ -168,7 +168,7 @@ static int __init psci_init_0_2(void) > { /* sentinel */ }, > }; > int ret; > - struct arm_smccc_res res; > + struct arm_smccc_res res = {0}; Same here. > > if ( acpi_disabled ) > { > diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c > index f7bf3ad117..34ce90be52 100644 > --- a/xen/drivers/char/pl011.c > +++ b/xen/drivers/char/pl011.c > @@ -285,7 +285,7 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev, > const char *config = data; > int res; > paddr_t addr, size; > - uint32_t io_width; > + uint32_t io_width = 0; We have many use of dt_property_read_*() within the code base. I would like us te decide of a policy first. > bool mmio32 = false, sbsa; > > if ( strcmp(config, "") ) Cheers,
Sorry for the late reply. On 14/07/23 15:21, Julien Grall wrote: > Hi, > > On 14/07/2023 12:49, Nicola Vetrini wrote: >> This patch aims to fix some occurrences of possibly uninitialized >> variables, that may be read before being written. This behaviour would >> violate MISRA C:2012 Rule 9.1, besides being generally undesirable. >> >> In all the analyzed cases, such accesses were actually safe, but it's >> quite difficult to prove so by automatic checking, therefore a safer >> route is to change the code so as to avoid the behaviour from occurring, >> while preserving the semantics. >> >> An initialization to a safe value is provided to reach this aim. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> Additional input on which values may be 'safe' in each context is >> surely welcome, to avoid possibly compromising the correctness of >> the function semantics. >> --- >> xen/arch/arm/cpuerrata.c | 6 +++--- >> xen/arch/arm/domctl.c | 8 ++++---- >> xen/arch/arm/gic-v3-lpi.c | 17 +++++++++-------- >> xen/arch/arm/include/asm/p2m.h | 10 ++++++---- >> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 10 ++-------- >> xen/arch/arm/psci.c | 10 +++++----- >> xen/drivers/char/pl011.c | 2 +- >> 7 files changed, 30 insertions(+), 33 deletions(-) >> >> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c >> index d0658aedb6..14694c6081 100644 >> --- a/xen/arch/arm/cpuerrata.c >> +++ b/xen/arch/arm/cpuerrata.c >> @@ -159,7 +159,7 @@ extern char __mitigate_spectre_bhb_loop_start_32[], >> static int enable_smccc_arch_workaround_1(void *data) >> { >> - struct arm_smccc_res res; >> + struct arm_smccc_res res = {0}; > > I understand you desire to make happy. But I am not sure that > initializing to 0 is the right thing. If the SMCC were not properly > setting the register, then we most likely don't want to install the > workaround. Instead, we most likely want to warn. > > So you want (int)res.a0 to be negative. We don't care about the other > fields. > In principle I'm ok with this, but see below. >> const struct arm_cpu_capabilities *entry = data; >> /* >> @@ -252,7 +252,7 @@ static int enable_spectre_bhb_workaround(void *data) >> if ( cpus_have_cap(ARM_WORKAROUND_BHB_SMCC_3) ) >> { >> - struct arm_smccc_res res; >> + struct arm_smccc_res res = {0}; > > Same remark here. > >> if ( smccc_ver < SMCCC_VERSION(1, 1) ) >> goto warn; >> @@ -393,7 +393,7 @@ DEFINE_PER_CPU_READ_MOSTLY(register_t, >> ssbd_callback_required); >> static bool has_ssbd_mitigation(const struct arm_cpu_capabilities >> *entry) >> { >> - struct arm_smccc_res res; >> + struct arm_smccc_res res = {0}; > > Here you would want (int)res.a0 to be equal to ARM_SMCCC_NOT_SUPPORTED. I see that ARM_SMCCC_NOT_SUPPORTED is #define ARM_SMCCC_NOT_SUPPORTED (-1) thus an assignment to res.a0 would violate Rule 10.3: "The value of an expression shall not be assigned to an object with a narrower essential type or of a different essential type category." (signed vs unsigned, and the exception does not apply here). This rule is not yet under discussion, but I would like to avoid knowingly introducing more violations if there's an alternative. Do the fields of struct arm_smccc_res really need to be unsigned? If that's not the case, then I'm happy with the proposed changes. > >> bool required; >> if ( smccc_ver < SMCCC_VERSION(1, 1) ) >> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c >> index ad56efb0f5..b38fed72be 100644 >> --- a/xen/arch/arm/domctl.c >> +++ b/xen/arch/arm/domctl.c >> @@ -29,10 +29,10 @@ static int handle_vuart_init(struct domain *d, >> struct xen_domctl_vuart_op *vuart_op) >> { >> int rc; >> - struct vpl011_init_info info; >> - >> - info.console_domid = vuart_op->console_domid; >> - info.gfn = _gfn(vuart_op->gfn); >> + struct vpl011_init_info info = { >> + .console_domid = vuart_op->console_domid, >> + .gfn = _gfn(vuart_op->gfn) >> + }; > > I am not against, this change. But I don't quite understand how this > makes Eclair much happier? It also zero-initializes the third field: struct vpl011_init_info { domid_t console_domid; gfn_t gfn; evtchn_port_t evtchn; }; > > Also, if this is the desired way, then I think this should be written > down in the CODING_STYLE. This is just a matter of style, I can also set the other field explicitly, if you prefer. Either way, it might be a good idea to settle this in the CODING_STYLE in a different patch. > >> if ( d->creation_finished ) >> return -EPERM; >> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c >> index eb0a5535e4..12f2af2e4d 100644 >> --- a/xen/arch/arm/gic-v3-lpi.c >> +++ b/xen/arch/arm/gic-v3-lpi.c >> @@ -210,7 +210,10 @@ out: >> void gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id, >> uint32_t virt_lpi) >> { >> - union host_lpi *hlpip, hlpi; >> + union host_lpi *hlpip, hlpi = { >> + .virt_lpi = virt_lpi, >> + .dom_id = domain_id >> + }; >> ASSERT(host_lpi >= LPI_OFFSET); >> @@ -218,9 +221,6 @@ void gicv3_lpi_update_host_entry(uint32_t >> host_lpi, int domain_id, >> hlpip = &lpi_data.host_lpis[host_lpi / >> HOST_LPIS_PER_PAGE][host_lpi % HOST_LPIS_PER_PAGE]; >> - hlpi.virt_lpi = virt_lpi; >> - hlpi.dom_id = domain_id; >> - >> write_u64_atomic(&hlpip->data, hlpi.data); >> } >> @@ -542,14 +542,15 @@ int gicv3_allocate_host_lpi_block(struct domain >> *d, uint32_t *first_lpi) >> for ( i = 0; i < LPI_BLOCK; i++ ) >> { >> - union host_lpi hlpi; >> - >> /* >> * Mark this host LPI as belonging to the domain, but don't >> assign >> * any virtual LPI or a VCPU yet. >> */ >> - hlpi.virt_lpi = INVALID_LPI; >> - hlpi.dom_id = d->domain_id; >> + union host_lpi hlpi = { >> + .virt_lpi = INVALID_LPI, >> + .dom_id = d->domain_id >> + }; >> + >> write_u64_atomic(&lpi_data.host_lpis[chunk][lpi_idx + i].data, >> hlpi.data); >> diff --git a/xen/arch/arm/include/asm/p2m.h >> b/xen/arch/arm/include/asm/p2m.h >> index 940495d42b..413e2a7add 100644 >> --- a/xen/arch/arm/include/asm/p2m.h >> +++ b/xen/arch/arm/include/asm/p2m.h >> @@ -345,7 +345,7 @@ static inline struct page_info *get_page_from_gfn( >> struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q) >> { >> mfn_t mfn; >> - p2m_type_t _t; >> + p2m_type_t _t = p2m_invalid; >> struct page_info *page; >> /* >> @@ -355,10 +355,12 @@ static inline struct page_info *get_page_from_gfn( >> if ( likely(d != dom_xen) ) >> return p2m_get_page_from_gfn(d, _gfn(gfn), t); >> - if ( !t ) >> + /* Allow t to be NULL */ >> + if ( t ) >> + *t = _t; >> + else { >> t = &_t; >> - >> - *t = p2m_invalid; >> + } > > I understand the exist construct is not liked by Eclair. But your new > construct is not easily readable by a human. Before I can propose a > different approach, can you clarify why Eclair is unhappy this the > existing code? > This has been addressed in the reply to patch 1/4. >> /* >> * DOMID_XEN sees 1-1 RAM. The p2m_type is based on the type of the >> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >> b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >> index 2053ed7ac5..39d9ab4fa9 100644 >> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >> @@ -51,11 +51,11 @@ static inline bool domain_has_reset_access(struct >> domain *d, uint32_t rst) >> bool zynqmp_eemi(struct cpu_user_regs *regs) >> { >> - struct arm_smccc_res res; >> + struct arm_smccc_res res = {0}; >> uint32_t fid = get_user_reg(regs, 0); >> uint32_t nodeid = get_user_reg(regs, 1); >> unsigned int pm_fn = fid & 0xFFFF; >> - enum pm_ret_status ret; >> + enum pm_ret_status ret = XST_PM_NO_ACCESS; > > It is not clear to me why Eclair is unhappy here? Gotos, as explained in the reply to patch 1/4. Since 'ret' here already sets a value in every branch, I am in favour of initializing it before the switch with that value. The zero-ed 'arm_smccc_res' is ok here? > >> switch ( fid ) >> { >> @@ -89,7 +89,6 @@ bool zynqmp_eemi(struct cpu_user_regs *regs) >> { >> gprintk(XENLOG_WARNING, >> "zynqmp-pm: fn=%u No access to node %u\n", >> pm_fn, nodeid); >> - ret = XST_PM_NO_ACCESS; >> goto done; > > I find the new approach less obvious. How about introduce a macro that > will set the result and the use replace the two lines with: > > set_result(regs, XST_PM_NO_ACCESS); > return 0; > > where set_result(regs, ret) is > > set_user_reg(regs, 0, ret) > >> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c >> index 695d2fa1f1..47e46af608 100644 >> --- a/xen/arch/arm/psci.c >> +++ b/xen/arch/arm/psci.c >> @@ -38,7 +38,7 @@ static uint32_t psci_cpu_on_nr; >> int call_psci_cpu_on(int cpu) >> { >> - struct arm_smccc_res res; >> + struct arm_smccc_res res = {0}; > > I don't this we should initialize a0 to 0 as this would indicate a > success. Instead, we want an invalid value. Same remark as stated above about the invalid value being negative. > >> arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), >> __pa(init_secondary), >> &res); >> @@ -50,7 +50,7 @@ void call_psci_cpu_off(void) >> { >> if ( psci_ver > PSCI_VERSION(0, 1) ) >> { >> - struct arm_smccc_res res; >> + struct arm_smccc_res res = {0}; > > Same here. > >> /* If successfull the PSCI cpu_off call doesn't return */ >> arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF, &res); >> @@ -73,7 +73,7 @@ void call_psci_system_reset(void) >> static int __init psci_features(uint32_t psci_func_id) >> { >> - struct arm_smccc_res res; >> + struct arm_smccc_res res = {0}; > > Same here. > >> if ( psci_ver < PSCI_VERSION(1, 0) ) >> return PSCI_NOT_SUPPORTED; >> @@ -115,7 +115,7 @@ static void __init psci_init_smccc(void) >> if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED ) >> { >> - struct arm_smccc_res res; >> + struct arm_smccc_res res = {0}; > > Same here. > >> arm_smccc_smc(ARM_SMCCC_VERSION_FID, &res); >> if ( PSCI_RET(res) != ARM_SMCCC_NOT_SUPPORTED ) >> @@ -168,7 +168,7 @@ static int __init psci_init_0_2(void) >> { /* sentinel */ }, >> }; >> int ret; >> - struct arm_smccc_res res; >> + struct arm_smccc_res res = {0}; > > Same here. > >> if ( acpi_disabled ) >> { >> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c >> index f7bf3ad117..34ce90be52 100644 >> --- a/xen/drivers/char/pl011.c >> +++ b/xen/drivers/char/pl011.c >> @@ -285,7 +285,7 @@ static int __init pl011_dt_uart_init(struct >> dt_device_node *dev, >> const char *config = data; >> int res; >> paddr_t addr, size; >> - uint32_t io_width; >> + uint32_t io_width = 0; > > We have many use of dt_property_read_*() within the code base. I would > like us te decide of a policy first. > Well, the best outcome for me is if all these functions (i.e., dt_property_*, dt_get_property_*) always write the parameter they are supposed to set. I understand if this is not always feasible, but the alternative is not clean (e.g., deviate or refactor in such a way that the code is less readable).
Hi, On 19/07/2023 14:27, Nicola Vetrini wrote: > On 14/07/23 15:21, Julien Grall wrote: >> Hi, >> >> On 14/07/2023 12:49, Nicola Vetrini wrote: >>> This patch aims to fix some occurrences of possibly uninitialized >>> variables, that may be read before being written. This behaviour would >>> violate MISRA C:2012 Rule 9.1, besides being generally undesirable. >>> >>> In all the analyzed cases, such accesses were actually safe, but it's >>> quite difficult to prove so by automatic checking, therefore a safer >>> route is to change the code so as to avoid the behaviour from occurring, >>> while preserving the semantics. >>> >>> An initialization to a safe value is provided to reach this aim. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> --- >>> Additional input on which values may be 'safe' in each context is >>> surely welcome, to avoid possibly compromising the correctness of >>> the function semantics. >>> --- >>> xen/arch/arm/cpuerrata.c | 6 +++--- >>> xen/arch/arm/domctl.c | 8 ++++---- >>> xen/arch/arm/gic-v3-lpi.c | 17 +++++++++-------- >>> xen/arch/arm/include/asm/p2m.h | 10 ++++++---- >>> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 10 ++-------- >>> xen/arch/arm/psci.c | 10 +++++----- >>> xen/drivers/char/pl011.c | 2 +- >>> 7 files changed, 30 insertions(+), 33 deletions(-) >>> >>> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c >>> index d0658aedb6..14694c6081 100644 >>> --- a/xen/arch/arm/cpuerrata.c >>> +++ b/xen/arch/arm/cpuerrata.c >>> @@ -159,7 +159,7 @@ extern char __mitigate_spectre_bhb_loop_start_32[], >>> static int enable_smccc_arch_workaround_1(void *data) >>> { >>> - struct arm_smccc_res res; >>> + struct arm_smccc_res res = {0}; >> >> I understand you desire to make happy. But I am not sure that >> initializing to 0 is the right thing. If the SMCC were not properly >> setting the register, then we most likely don't want to install the >> workaround. Instead, we most likely want to warn. >> >> So you want (int)res.a0 to be negative. We don't care about the other >> fields. >> > > In principle I'm ok with this, but see below. > >>> const struct arm_cpu_capabilities *entry = data; >>> /* >>> @@ -252,7 +252,7 @@ static int enable_spectre_bhb_workaround(void *data) >>> if ( cpus_have_cap(ARM_WORKAROUND_BHB_SMCC_3) ) >>> { >>> - struct arm_smccc_res res; >>> + struct arm_smccc_res res = {0}; >> >> Same remark here. >> >>> if ( smccc_ver < SMCCC_VERSION(1, 1) ) >>> goto warn; >>> @@ -393,7 +393,7 @@ DEFINE_PER_CPU_READ_MOSTLY(register_t, >>> ssbd_callback_required); >>> static bool has_ssbd_mitigation(const struct arm_cpu_capabilities >>> *entry) >>> { >>> - struct arm_smccc_res res; >>> + struct arm_smccc_res res = {0}; >> >> Here you would want (int)res.a0 to be equal to ARM_SMCCC_NOT_SUPPORTED. > > I see that ARM_SMCCC_NOT_SUPPORTED is > #define ARM_SMCCC_NOT_SUPPORTED (-1) > > thus an assignment to res.a0 would violate Rule 10.3: > "The value of an expression shall not be assigned to an object with a > narrower essential type or of a different essential type category." > (signed vs unsigned, and the exception does not apply here). > > This rule is not yet under discussion, but I would like to avoid > knowingly introducing more violations if there's an alternative. > > Do the fields of struct arm_smccc_res really need to be unsigned? Yes, all the fields represent a register. Also, in this context, only the first 32-bit of the register should be taken into account. That why you will see code using (int)res.a0. >>> bool required; >>> if ( smccc_ver < SMCCC_VERSION(1, 1) ) >>> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c >>> index ad56efb0f5..b38fed72be 100644 >>> --- a/xen/arch/arm/domctl.c >>> +++ b/xen/arch/arm/domctl.c >>> @@ -29,10 +29,10 @@ static int handle_vuart_init(struct domain *d, >>> struct xen_domctl_vuart_op *vuart_op) >>> { >>> int rc; >>> - struct vpl011_init_info info; >>> - >>> - info.console_domid = vuart_op->console_domid; >>> - info.gfn = _gfn(vuart_op->gfn); >>> + struct vpl011_init_info info = { >>> + .console_domid = vuart_op->console_domid, >>> + .gfn = _gfn(vuart_op->gfn) >>> + }; >> >> I am not against, this change. But I don't quite understand how this >> makes Eclair much happier? > > It also zero-initializes the third field: > > struct vpl011_init_info { > domid_t console_domid; > gfn_t gfn; > evtchn_port_t evtchn; > }; > > >> >> Also, if this is the desired way, then I think this should be written >> down in the CODING_STYLE. > > This is just a matter of style, I can also set the other field > explicitly, if you prefer. I am confused. In a previous reply, I thought you said the following would also make ECLAIR unhappy: info.console_domid = <...>; info.gfn = <...>; info.evtchn = <...>; > Either way, it might be a good idea to settle > this in the CODING_STYLE in a different patch. Well... The pattern you complain about is fairly common in Xen and I personally prefer to use in some situation over your proposal. So I think we need to agree first that your proposal is the way to go before modifying the code everywhere. [...] >> >> I understand the exist construct is not liked by Eclair. But your new >> construct is not easily readable by a human. Before I can propose a >> different approach, can you clarify why Eclair is unhappy this the >> existing code? >> > > This has been addressed in the reply to patch 1/4. AFAICT the conversation wasn't settled. We will continue there to avoid duplication. > >>> /* >>> * DOMID_XEN sees 1-1 RAM. The p2m_type is based on the type of >>> the >>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >>> b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >>> index 2053ed7ac5..39d9ab4fa9 100644 >>> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >>> @@ -51,11 +51,11 @@ static inline bool domain_has_reset_access(struct >>> domain *d, uint32_t rst) >>> bool zynqmp_eemi(struct cpu_user_regs *regs) >>> { >>> - struct arm_smccc_res res; >>> + struct arm_smccc_res res = {0}; >>> uint32_t fid = get_user_reg(regs, 0); >>> uint32_t nodeid = get_user_reg(regs, 1); >>> unsigned int pm_fn = fid & 0xFFFF; >>> - enum pm_ret_status ret; >>> + enum pm_ret_status ret = XST_PM_NO_ACCESS; >> >> It is not clear to me why Eclair is unhappy here? > > Gotos, as explained in the reply to patch 1/4. Since 'ret' here already > sets a value in every branch, I am in favour of initializing it before > the switch with that value. The zero-ed 'arm_smccc_res' is ok here? I am not in favor of this approach. Have you considered my proposal... > >> >>> switch ( fid ) >>> { >>> @@ -89,7 +89,6 @@ bool zynqmp_eemi(struct cpu_user_regs *regs) >>> { >>> gprintk(XENLOG_WARNING, >>> "zynqmp-pm: fn=%u No access to node %u\n", >>> pm_fn, nodeid); >>> - ret = XST_PM_NO_ACCESS; >>> goto done; >> >> I find the new approach less obvious. How about introduce a macro that >> will set the result and the use replace the two lines with: >> >> set_result(regs, XST_PM_NO_ACCESS); >> return 0; >> >> where set_result(regs, ret) is >> >> set_user_reg(regs, 0, ret) ... here? [...] >>> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c >>> index f7bf3ad117..34ce90be52 100644 >>> --- a/xen/drivers/char/pl011.c >>> +++ b/xen/drivers/char/pl011.c >>> @@ -285,7 +285,7 @@ static int __init pl011_dt_uart_init(struct >>> dt_device_node *dev, >>> const char *config = data; >>> int res; >>> paddr_t addr, size; >>> - uint32_t io_width; >>> + uint32_t io_width = 0; >> >> We have many use of dt_property_read_*() within the code base. I would >> like us te decide of a policy first. >> > > Well, the best outcome for me is if all these functions (i.e., > dt_property_*, dt_get_property_*) always write the parameter they are > supposed to set. I understand if this is not always feasible, but the > alternative is not clean (e.g., deviate or refactor in such a way that > the code is less readable). I have had a look at the helpers. Here some thoughts * dt_get_property(): Always setting '*lenp' could result to user starting relying on the 'len' rather than the error code. This could potentially be mitigated by forcing the code to check the value (i.e. adding __must_check). * dt_property_read_u*(): The best would be to let the user decide what would a default sane value by adding an extra parameter. * dt_property_read_string(): I would return the string and use ERR_PTR(...) when there is an error. Cheers,
On 19/07/23 16:06, Julien Grall wrote: > Hi, > > On 19/07/2023 14:27, Nicola Vetrini wrote: >> On 14/07/23 15:21, Julien Grall wrote: >>> Hi, >>> >>> On 14/07/2023 12:49, Nicola Vetrini wrote: >>>> This patch aims to fix some occurrences of possibly uninitialized >>>> variables, that may be read before being written. This behaviour would >>>> violate MISRA C:2012 Rule 9.1, besides being generally undesirable. >>>> >>>> In all the analyzed cases, such accesses were actually safe, but it's >>>> quite difficult to prove so by automatic checking, therefore a safer >>>> route is to change the code so as to avoid the behaviour from >>>> occurring, >>>> while preserving the semantics. >>>> >>>> An initialization to a safe value is provided to reach this aim. >>>> >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>> --- >>>> Additional input on which values may be 'safe' in each context is >>>> surely welcome, to avoid possibly compromising the correctness of >>>> the function semantics. >>>> --- >>>> xen/arch/arm/cpuerrata.c | 6 +++--- >>>> xen/arch/arm/domctl.c | 8 ++++---- >>>> xen/arch/arm/gic-v3-lpi.c | 17 +++++++++-------- >>>> xen/arch/arm/include/asm/p2m.h | 10 ++++++---- >>>> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 10 ++-------- >>>> xen/arch/arm/psci.c | 10 +++++----- >>>> xen/drivers/char/pl011.c | 2 +- >>>> 7 files changed, 30 insertions(+), 33 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c >>>> index d0658aedb6..14694c6081 100644 >>>> --- a/xen/arch/arm/cpuerrata.c >>>> +++ b/xen/arch/arm/cpuerrata.c >>>> @@ -159,7 +159,7 @@ extern char __mitigate_spectre_bhb_loop_start_32[], >>>> static int enable_smccc_arch_workaround_1(void *data) >>>> { >>>> - struct arm_smccc_res res; >>>> + struct arm_smccc_res res = {0}; >>> >>> I understand you desire to make happy. But I am not sure that >>> initializing to 0 is the right thing. If the SMCC were not properly >>> setting the register, then we most likely don't want to install the >>> workaround. Instead, we most likely want to warn. >>> >>> So you want (int)res.a0 to be negative. We don't care about the other >>> fields. >>> >> >> In principle I'm ok with this, but see below. >> >>>> const struct arm_cpu_capabilities *entry = data; >>>> /* >>>> @@ -252,7 +252,7 @@ static int enable_spectre_bhb_workaround(void >>>> *data) >>>> if ( cpus_have_cap(ARM_WORKAROUND_BHB_SMCC_3) ) >>>> { >>>> - struct arm_smccc_res res; >>>> + struct arm_smccc_res res = {0}; >>> >>> Same remark here. >>> >>>> if ( smccc_ver < SMCCC_VERSION(1, 1) ) >>>> goto warn; >>>> @@ -393,7 +393,7 @@ DEFINE_PER_CPU_READ_MOSTLY(register_t, >>>> ssbd_callback_required); >>>> static bool has_ssbd_mitigation(const struct arm_cpu_capabilities >>>> *entry) >>>> { >>>> - struct arm_smccc_res res; >>>> + struct arm_smccc_res res = {0}; >>> >>> Here you would want (int)res.a0 to be equal to ARM_SMCCC_NOT_SUPPORTED. >> >> I see that ARM_SMCCC_NOT_SUPPORTED is >> #define ARM_SMCCC_NOT_SUPPORTED (-1) >> >> thus an assignment to res.a0 would violate Rule 10.3: >> "The value of an expression shall not be assigned to an object with a >> narrower essential type or of a different essential type category." >> (signed vs unsigned, and the exception does not apply here). >> >> This rule is not yet under discussion, but I would like to avoid >> knowingly introducing more violations if there's an alternative. >> >> Do the fields of struct arm_smccc_res really need to be unsigned? > > Yes, all the fields represent a register. Also, in this context, only > the first 32-bit of the register should be taken into account. > > That why you will see code using (int)res.a0. > If the purpose of defining it to -1 is to have the value 0xffffffffffffffff, then it could be defined as ~0UL. >>>> bool required; >>>> if ( smccc_ver < SMCCC_VERSION(1, 1) ) >>>> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c >>>> index ad56efb0f5..b38fed72be 100644 >>>> --- a/xen/arch/arm/domctl.c >>>> +++ b/xen/arch/arm/domctl.c >>>> @@ -29,10 +29,10 @@ static int handle_vuart_init(struct domain *d, >>>> struct xen_domctl_vuart_op *vuart_op) >>>> { >>>> int rc; >>>> - struct vpl011_init_info info; >>>> - >>>> - info.console_domid = vuart_op->console_domid; >>>> - info.gfn = _gfn(vuart_op->gfn); >>>> + struct vpl011_init_info info = { >>>> + .console_domid = vuart_op->console_domid, >>>> + .gfn = _gfn(vuart_op->gfn) >>>> + }; >>> >>> I am not against, this change. But I don't quite understand how this >>> makes Eclair much happier? >> >> It also zero-initializes the third field: >> >> struct vpl011_init_info { >> domid_t console_domid; >> gfn_t gfn; >> evtchn_port_t evtchn; >> }; >> >> >>> >>> Also, if this is the desired way, then I think this should be written >>> down in the CODING_STYLE. >> >> This is just a matter of style, I can also set the other field >> explicitly, if you prefer. > > I am confused. In a previous reply, I thought you said the following > would also make ECLAIR unhappy: > > info.console_domid = <...>; > info.gfn = <...>; > info.evtchn = <...>; > If I did (I can't find it right now, but I'll try to dig it up), I'd say that I was either wrong or I forgot to mention some other field that wasn't initialized by the series of statements. >> Either way, it might be a good idea to settle this in the CODING_STYLE >> in a different patch. > > Well... The pattern you complain about is fairly common in Xen and I > personally prefer to use in some situation over your proposal. So I > think we need to agree first that your proposal is the way to go before > modifying the code everywhere. > > [...] > >>> >>> I understand the exist construct is not liked by Eclair. But your new >>> construct is not easily readable by a human. Before I can propose a >>> different approach, can you clarify why Eclair is unhappy this the >>> existing code? >>> >> >> This has been addressed in the reply to patch 1/4. > > AFAICT the conversation wasn't settled. We will continue there to avoid > duplication. > Sure. >> >>>> /* >>>> * DOMID_XEN sees 1-1 RAM. The p2m_type is based on the type >>>> of the >>>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >>>> b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >>>> index 2053ed7ac5..39d9ab4fa9 100644 >>>> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c >>>> @@ -51,11 +51,11 @@ static inline bool >>>> domain_has_reset_access(struct domain *d, uint32_t rst) >>>> bool zynqmp_eemi(struct cpu_user_regs *regs) >>>> { >>>> - struct arm_smccc_res res; >>>> + struct arm_smccc_res res = {0}; >>>> uint32_t fid = get_user_reg(regs, 0); >>>> uint32_t nodeid = get_user_reg(regs, 1); >>>> unsigned int pm_fn = fid & 0xFFFF; >>>> - enum pm_ret_status ret; >>>> + enum pm_ret_status ret = XST_PM_NO_ACCESS; >>> >>> It is not clear to me why Eclair is unhappy here? >> >> Gotos, as explained in the reply to patch 1/4. Since 'ret' here >> already sets a value in every branch, I am in favour of initializing >> it before the switch with that value. The zero-ed 'arm_smccc_res' is >> ok here? > > I am not in favor of this approach. Have you considered my proposal... > >> >>> >>>> switch ( fid ) >>>> { >>>> @@ -89,7 +89,6 @@ bool zynqmp_eemi(struct cpu_user_regs *regs) >>>> { >>>> gprintk(XENLOG_WARNING, >>>> "zynqmp-pm: fn=%u No access to node %u\n", >>>> pm_fn, nodeid); >>>> - ret = XST_PM_NO_ACCESS; >>>> goto done; >>> >>> I find the new approach less obvious. How about introduce a macro >>> that will set the result and the use replace the two lines with: >>> >>> set_result(regs, XST_PM_NO_ACCESS); >>> return 0; >>> >>> where set_result(regs, ret) is >>> >>> set_user_reg(regs, 0, ret) > > ... here? Sorry, I missed it when looking at the reply. I'll look into it, but it seems good. It would remove the need for the local variable, if I understand it correctly. > > [...] > >>>> diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c >>>> index f7bf3ad117..34ce90be52 100644 >>>> --- a/xen/drivers/char/pl011.c >>>> +++ b/xen/drivers/char/pl011.c >>>> @@ -285,7 +285,7 @@ static int __init pl011_dt_uart_init(struct >>>> dt_device_node *dev, >>>> const char *config = data; >>>> int res; >>>> paddr_t addr, size; >>>> - uint32_t io_width; >>>> + uint32_t io_width = 0; >>> >>> We have many use of dt_property_read_*() within the code base. I >>> would like us te decide of a policy first. >>> >> >> Well, the best outcome for me is if all these functions (i.e., >> dt_property_*, dt_get_property_*) always write the parameter they are >> supposed to set. I understand if this is not always feasible, but the >> alternative is not clean (e.g., deviate or refactor in such a way that >> the code is less readable). > > I have had a look at the helpers. Here some thoughts > > * dt_get_property(): Always setting '*lenp' could result to user > starting relying on the 'len' rather than the error code. This could > potentially be mitigated by forcing the code to check the value (i.e. > adding __must_check). > Note that there is a Directive (D4.10) already in docs/misra/rules.rst that prescribes the caller to check return values: "If a function returns error information, then that error information shall be tested" (by checking we mean testing it in some way, but as is the case for many directives, the checking this is not fully automated). > * dt_property_read_u*(): The best would be to let the user decide what > would a default sane value by adding an extra parameter. > > * dt_property_read_string(): I would return the string and use > ERR_PTR(...) when there is an error. > Ok, good. Regards,
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c index d0658aedb6..14694c6081 100644 --- a/xen/arch/arm/cpuerrata.c +++ b/xen/arch/arm/cpuerrata.c @@ -159,7 +159,7 @@ extern char __mitigate_spectre_bhb_loop_start_32[], static int enable_smccc_arch_workaround_1(void *data) { - struct arm_smccc_res res; + struct arm_smccc_res res = {0}; const struct arm_cpu_capabilities *entry = data; /* @@ -252,7 +252,7 @@ static int enable_spectre_bhb_workaround(void *data) if ( cpus_have_cap(ARM_WORKAROUND_BHB_SMCC_3) ) { - struct arm_smccc_res res; + struct arm_smccc_res res = {0}; if ( smccc_ver < SMCCC_VERSION(1, 1) ) goto warn; @@ -393,7 +393,7 @@ DEFINE_PER_CPU_READ_MOSTLY(register_t, ssbd_callback_required); static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry) { - struct arm_smccc_res res; + struct arm_smccc_res res = {0}; bool required; if ( smccc_ver < SMCCC_VERSION(1, 1) ) diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c index ad56efb0f5..b38fed72be 100644 --- a/xen/arch/arm/domctl.c +++ b/xen/arch/arm/domctl.c @@ -29,10 +29,10 @@ static int handle_vuart_init(struct domain *d, struct xen_domctl_vuart_op *vuart_op) { int rc; - struct vpl011_init_info info; - - info.console_domid = vuart_op->console_domid; - info.gfn = _gfn(vuart_op->gfn); + struct vpl011_init_info info = { + .console_domid = vuart_op->console_domid, + .gfn = _gfn(vuart_op->gfn) + }; if ( d->creation_finished ) return -EPERM; diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c index eb0a5535e4..12f2af2e4d 100644 --- a/xen/arch/arm/gic-v3-lpi.c +++ b/xen/arch/arm/gic-v3-lpi.c @@ -210,7 +210,10 @@ out: void gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id, uint32_t virt_lpi) { - union host_lpi *hlpip, hlpi; + union host_lpi *hlpip, hlpi = { + .virt_lpi = virt_lpi, + .dom_id = domain_id + }; ASSERT(host_lpi >= LPI_OFFSET); @@ -218,9 +221,6 @@ void gicv3_lpi_update_host_entry(uint32_t host_lpi, int domain_id, hlpip = &lpi_data.host_lpis[host_lpi / HOST_LPIS_PER_PAGE][host_lpi % HOST_LPIS_PER_PAGE]; - hlpi.virt_lpi = virt_lpi; - hlpi.dom_id = domain_id; - write_u64_atomic(&hlpip->data, hlpi.data); } @@ -542,14 +542,15 @@ int gicv3_allocate_host_lpi_block(struct domain *d, uint32_t *first_lpi) for ( i = 0; i < LPI_BLOCK; i++ ) { - union host_lpi hlpi; - /* * Mark this host LPI as belonging to the domain, but don't assign * any virtual LPI or a VCPU yet. */ - hlpi.virt_lpi = INVALID_LPI; - hlpi.dom_id = d->domain_id; + union host_lpi hlpi = { + .virt_lpi = INVALID_LPI, + .dom_id = d->domain_id + }; + write_u64_atomic(&lpi_data.host_lpis[chunk][lpi_idx + i].data, hlpi.data); diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h index 940495d42b..413e2a7add 100644 --- a/xen/arch/arm/include/asm/p2m.h +++ b/xen/arch/arm/include/asm/p2m.h @@ -345,7 +345,7 @@ static inline struct page_info *get_page_from_gfn( struct domain *d, unsigned long gfn, p2m_type_t *t, p2m_query_t q) { mfn_t mfn; - p2m_type_t _t; + p2m_type_t _t = p2m_invalid; struct page_info *page; /* @@ -355,10 +355,12 @@ static inline struct page_info *get_page_from_gfn( if ( likely(d != dom_xen) ) return p2m_get_page_from_gfn(d, _gfn(gfn), t); - if ( !t ) + /* Allow t to be NULL */ + if ( t ) + *t = _t; + else { t = &_t; - - *t = p2m_invalid; + } /* * DOMID_XEN sees 1-1 RAM. The p2m_type is based on the type of the diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c index 2053ed7ac5..39d9ab4fa9 100644 --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c @@ -51,11 +51,11 @@ static inline bool domain_has_reset_access(struct domain *d, uint32_t rst) bool zynqmp_eemi(struct cpu_user_regs *regs) { - struct arm_smccc_res res; + struct arm_smccc_res res = {0}; uint32_t fid = get_user_reg(regs, 0); uint32_t nodeid = get_user_reg(regs, 1); unsigned int pm_fn = fid & 0xFFFF; - enum pm_ret_status ret; + enum pm_ret_status ret = XST_PM_NO_ACCESS; switch ( fid ) { @@ -89,7 +89,6 @@ bool zynqmp_eemi(struct cpu_user_regs *regs) { gprintk(XENLOG_WARNING, "zynqmp-pm: fn=%u No access to node %u\n", pm_fn, nodeid); - ret = XST_PM_NO_ACCESS; goto done; } goto forward_to_fw; @@ -100,7 +99,6 @@ bool zynqmp_eemi(struct cpu_user_regs *regs) { gprintk(XENLOG_WARNING, "zynqmp-pm: fn=%u No access to reset %u\n", pm_fn, nodeid); - ret = XST_PM_NO_ACCESS; goto done; } goto forward_to_fw; @@ -116,7 +114,6 @@ bool zynqmp_eemi(struct cpu_user_regs *regs) case EEMI_FID(PM_MMIO_READ): gprintk(XENLOG_WARNING, "zynqmp-pm: fn=%u No MMIO access to %u\n", pm_fn, nodeid); - ret = XST_PM_NO_ACCESS; goto done; /* Exclusive to the hardware domain. */ @@ -146,14 +143,12 @@ bool zynqmp_eemi(struct cpu_user_regs *regs) if ( !is_hardware_domain(current->domain) ) { gprintk(XENLOG_WARNING, "eemi: fn=%u No access", pm_fn); - ret = XST_PM_NO_ACCESS; goto done; } goto forward_to_fw; /* These calls are never allowed. */ case EEMI_FID(PM_SYSTEM_SHUTDOWN): - ret = XST_PM_NO_ACCESS; goto done; case IPI_MAILBOX_FID(IPI_MAILBOX_OPEN): @@ -166,7 +161,6 @@ bool zynqmp_eemi(struct cpu_user_regs *regs) if ( !is_hardware_domain(current->domain) ) { gprintk(XENLOG_WARNING, "IPI mailbox: fn=%u No access", pm_fn); - ret = XST_PM_NO_ACCESS; goto done; } goto forward_to_fw; diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c index 695d2fa1f1..47e46af608 100644 --- a/xen/arch/arm/psci.c +++ b/xen/arch/arm/psci.c @@ -38,7 +38,7 @@ static uint32_t psci_cpu_on_nr; int call_psci_cpu_on(int cpu) { - struct arm_smccc_res res; + struct arm_smccc_res res = {0}; arm_smccc_smc(psci_cpu_on_nr, cpu_logical_map(cpu), __pa(init_secondary), &res); @@ -50,7 +50,7 @@ void call_psci_cpu_off(void) { if ( psci_ver > PSCI_VERSION(0, 1) ) { - struct arm_smccc_res res; + struct arm_smccc_res res = {0}; /* If successfull the PSCI cpu_off call doesn't return */ arm_smccc_smc(PSCI_0_2_FN32_CPU_OFF, &res); @@ -73,7 +73,7 @@ void call_psci_system_reset(void) static int __init psci_features(uint32_t psci_func_id) { - struct arm_smccc_res res; + struct arm_smccc_res res = {0}; if ( psci_ver < PSCI_VERSION(1, 0) ) return PSCI_NOT_SUPPORTED; @@ -115,7 +115,7 @@ static void __init psci_init_smccc(void) if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED ) { - struct arm_smccc_res res; + struct arm_smccc_res res = {0}; arm_smccc_smc(ARM_SMCCC_VERSION_FID, &res); if ( PSCI_RET(res) != ARM_SMCCC_NOT_SUPPORTED ) @@ -168,7 +168,7 @@ static int __init psci_init_0_2(void) { /* sentinel */ }, }; int ret; - struct arm_smccc_res res; + struct arm_smccc_res res = {0}; if ( acpi_disabled ) { diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c index f7bf3ad117..34ce90be52 100644 --- a/xen/drivers/char/pl011.c +++ b/xen/drivers/char/pl011.c @@ -285,7 +285,7 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev, const char *config = data; int res; paddr_t addr, size; - uint32_t io_width; + uint32_t io_width = 0; bool mmio32 = false, sbsa; if ( strcmp(config, "") )
This patch aims to fix some occurrences of possibly uninitialized variables, that may be read before being written. This behaviour would violate MISRA C:2012 Rule 9.1, besides being generally undesirable. In all the analyzed cases, such accesses were actually safe, but it's quite difficult to prove so by automatic checking, therefore a safer route is to change the code so as to avoid the behaviour from occurring, while preserving the semantics. An initialization to a safe value is provided to reach this aim. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- Additional input on which values may be 'safe' in each context is surely welcome, to avoid possibly compromising the correctness of the function semantics. --- xen/arch/arm/cpuerrata.c | 6 +++--- xen/arch/arm/domctl.c | 8 ++++---- xen/arch/arm/gic-v3-lpi.c | 17 +++++++++-------- xen/arch/arm/include/asm/p2m.h | 10 ++++++---- xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 10 ++-------- xen/arch/arm/psci.c | 10 +++++----- xen/drivers/char/pl011.c | 2 +- 7 files changed, 30 insertions(+), 33 deletions(-)