| Message ID | 1ad20473a031eca75db4007bdc169616b512ef44.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 Nicola, 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. > > To achieve this goal, I adopted the following strategies: Please let's at least one patch per strategy. I would also consider some of the rework separate so they can go in regardless the decision for the SAF-*. > > - Add a suitably formatted local deviation comment > (as indicated in 'docs/misra/documenting-violations.rst') > to exempt the following line from checking. > > - Provide an initialization for the variable at the declaration. > > - Substitute a goto breaking out of control flow logic with a semantically > equivalent do { .. } while(0). As I already mentioned in private, it is unclear to me how you decided which strategy to use. I still think we need to define our policy before changing the code. Otherwise, it is going to be difficult to decide for new code. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > docs/misra/safe.json | 8 +++++++ > xen/arch/arm/arm64/lib/find_next_bit.c | 1 + > xen/arch/arm/bootfdt.c | 6 +++++ > xen/arch/arm/decode.c | 2 ++ > xen/arch/arm/domain_build.c | 29 ++++++++++++++++++---- > xen/arch/arm/efi/efi-boot.h | 6 +++-- > xen/arch/arm/gic-v3-its.c | 9 ++++--- > xen/arch/arm/mm.c | 1 + > xen/arch/arm/p2m.c | 33 +++++++++++++++----------- > 9 files changed, 69 insertions(+), 26 deletions(-) > > diff --git a/docs/misra/safe.json b/docs/misra/safe.json > index e3c8a1d8eb..244001f5be 100644 > --- a/docs/misra/safe.json > +++ b/docs/misra/safe.json > @@ -12,6 +12,14 @@ > }, > { > "id": "SAF-1-safe", > + "analyser": { > + "eclair": "MC3R1.R9.1" > + }, > + "name": "Rule 9.1: initializer not needed", > + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." I am bit concerned which such statement because the code instance was today with the current code. This could change in the future and invalide the reasoning. It is not clear to me if we have any mechanism to prevent that. If we don't, then I think we need to drastically reduce the number of time this is used (there are a bit too much for my taste). > + }, > + { > + "id": "SAF-2-safe", > "analyser": {}, > "name": "Sentinel", > "text": "Next ID to be used" > diff --git a/xen/arch/arm/arm64/lib/find_next_bit.c b/xen/arch/arm/arm64/lib/find_next_bit.c > index ca6f82277e..51b852c595 100644 > --- a/xen/arch/arm/arm64/lib/find_next_bit.c > +++ b/xen/arch/arm/arm64/lib/find_next_bit.c > @@ -67,6 +67,7 @@ unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size, > { > const unsigned long *p = addr + BIT_WORD(offset); > unsigned long result = offset & ~(BITS_PER_LONG-1); > + /* SAF-1-safe MC3R1.R9.1 */ > unsigned long tmp; This is a file that was copied as-is from Linux. I thought we exclude them. Anyway, can you provide some details why Eclair thinks this is may be used unitialized? > > if (offset >= size) > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 2673ad17a1..1292a64e8d 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -34,6 +34,7 @@ static bool __init device_tree_node_matches(const void *fdt, int node, > static bool __init device_tree_node_compatible(const void *fdt, int node, > const char *match) > { > + /* SAF-1-safe MC3R1.R9.1 */ > int len, l; It is unclear to me whether the SAF-* applies to 'len' or 'l'. > const void *prop; > > @@ -169,7 +170,9 @@ int __init device_tree_for_each_node(const void *fdt, int node, > */ > int depth = 0; > const int first_node = node; > + /* SAF-1-safe MC3R1.R9.1 */ > u32 address_cells[DEVICE_TREE_MAX_DEPTH]; > + /* SAF-1-safe MC3R1.R9.1 */ > u32 size_cells[DEVICE_TREE_MAX_DEPTH]; I have never sent formally, but in the past I proposed to rework the code so {address, size}_cells[0] would be initialized before the loop. See https://lore.kernel.org/all/ea06f2ac-4ac1-4a6f-bda6-e775a7e68699@xen.org/. Do you think this would help? > int ret; > > @@ -249,8 +252,10 @@ static void __init process_multiboot_node(const void *fdt, int node, > const __be32 *cell; > bootmodule_kind kind; > paddr_t start, size; > + /* SAF-1-safe MC3R1.R9.1 */ > int len; > /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + '/0' => 92 */ > + /* SAF-1-safe MC3R1.R9.1*/ > char path[92]; So the two above, is one category of issue. The variables are passed as argument of function which will fill them. Can Eclair look at the callers, if so, can we consider to always initialize the values in the callee? This would reduce the number of SAF-*. There are a few other examples like that below. So I will skip them for now. [...] > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index d0d6be922d..d43f86c2f0 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -62,7 +62,7 @@ custom_param("dom0_mem", parse_dom0_mem); > > int __init parse_arch_dom0_param(const char *s, const char *e) > { > - long long val; > + long long val = LLONG_MAX; Can you explain why you decided to initialize rather than SAF-? For clarity, I am not asking to switch to SAF-, I am mainly interested what were your though process. > > if ( !parse_signed_integer("sve", s, e, &val) ) > { > @@ -1077,6 +1077,7 @@ static void __init assign_static_memory_11(struct domain *d, > static int __init handle_linux_pci_domain(struct kernel_info *kinfo, > const struct dt_device_node *node) > { > + /* SAF-1-safe MC3R1.R9.1 */ > uint16_t segment; > int res; > > @@ -1351,6 +1352,7 @@ static int __init make_memory_node(const struct domain *d, > unsigned int i; > int res, reg_size = addrcells + sizecells; > int nr_cells = 0; > + /* SAF-1-safe MC3R1.R9.1*/ > __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */]; > __be32 *cells; > > @@ -1578,6 +1580,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > struct rangeset *unalloc_mem; > paddr_t start, end; > unsigned int i; > + /* SAF-1-safe MC3R1.R9.1 */ > int res; > > dt_dprintk("Find unallocated memory for extended regions\n"); > @@ -1727,6 +1730,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, > dt_for_each_device_node( dt_host, np ) > { > unsigned int naddr; > + /* SAF-1-safe MC3R1.R9.1 */ > paddr_t addr, size; > > naddr = dt_number_of_address(np); > @@ -1976,9 +1980,11 @@ static int __init make_cpus_node(const struct domain *d, void *fdt) > const struct dt_device_node *npcpu; > unsigned int cpu; > const void *compatible = NULL; > + /* SAF-1-safe MC3R1.R9.1 */ > u32 len; > /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */ > char buf[13]; > + /* SAF-1-safe MC3R1.R9.1 */ > u32 clock_frequency; > /* Keep the compiler happy with -Og */ > bool clock_valid = false; > @@ -2104,6 +2110,7 @@ static int __init make_gic_node(const struct domain *d, void *fdt, > const struct dt_device_node *gic = dt_interrupt_controller; > int res = 0; > const void *addrcells, *sizecells; > + /* SAF-1-safe MC3R1.R9.1 */ > u32 addrcells_len, sizecells_len; > > /* > @@ -2179,6 +2186,7 @@ static int __init make_timer_node(const struct kernel_info *kinfo) > int res; > unsigned int irq[MAX_TIMER_PPI]; > gic_interrupt_t intrs[3]; > + /* SAF-1-safe MC3R1.R9.1 */ > u32 clock_frequency; > bool clock_valid; > > @@ -2511,6 +2519,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev, > unsigned int naddr; > unsigned int i; > int res; > + /* SAF-1-safe MC3R1.R9.1 */ > paddr_t addr, size; > bool own_device = !dt_device_for_passthrough(dev); > /* > @@ -2779,6 +2788,7 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) > { > void *fdt = kinfo->fdt; > int res = 0; > + /* SAF-1-safe MC3R1.R9.1*/ > __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; > __be32 *cells; > const struct domain *d = kinfo->d; > @@ -2914,6 +2924,7 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo) > void *fdt = kinfo->fdt; > int res; > gic_interrupt_t intr; > + /* SAF-1-safe MC3R1.R9.1*/ > __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; > __be32 *cells; > struct domain *d = kinfo->d; > @@ -3435,6 +3446,7 @@ static void __init initrd_load(struct kernel_info *kinfo) > paddr_t paddr, len; > int node; > int res; > + /* SAF-1-safe MC3R1.R9.1 */ > __be32 val[2]; > __be32 *cellp; > void __iomem *initrd; > @@ -3514,6 +3526,7 @@ static int __init get_evtchn_dt_property(const struct dt_device_node *np, > uint32_t *port, uint32_t *phandle) > { > const __be32 *prop = NULL; > + /* SAF-1-safe MC3R1.R9.1 */ > uint32_t len; > > prop = dt_get_property(np, "xen,evtchn", &len); > @@ -3538,10 +3551,13 @@ static int __init get_evtchn_dt_property(const struct dt_device_node *np, > static int __init alloc_domain_evtchn(struct dt_device_node *node) > { > int rc; > + /* SAF-1-safe MC3R1.R9.1 */ > uint32_t domU1_port, domU2_port, remote_phandle; > struct dt_device_node *remote_node; > const struct dt_device_node *p1_node, *p2_node; > + /* SAF-1-safe MC3R1.R9.1 */ > struct evtchn_alloc_unbound alloc_unbound; > + /* SAF-1-safe MC3R1.R9.1 */ > struct evtchn_bind_interdomain bind_interdomain; > struct domain *d1 = NULL, *d2 = NULL; > > @@ -3789,11 +3805,12 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo) > > static int __init alloc_xenstore_evtchn(struct domain *d) > { > - evtchn_alloc_unbound_t alloc; > + evtchn_alloc_unbound_t alloc = { > + .dom = d->domain_id, > + .remote_dom = hardware_domain->domain_id > + }; > int rc; > > - alloc.dom = d->domain_id; > - alloc.remote_dom = hardware_domain->domain_id; It is not clear why this is modified. There are only two fields in 'alloc' and they are both initialized. > rc = evtchn_alloc_unbound(&alloc, 0); > if ( rc ) > { > @@ -3810,8 +3827,9 @@ static int __init construct_domU(struct domain *d, > const struct dt_device_node *node) > { > struct kernel_info kinfo = {}; > - const char *dom0less_enhanced; > + const char *dom0less_enhanced = NULL; If you look at the user below, all the callers assume dom0less_enhanced will be non-NULL. So it is unclear to me how this value is safer. Looking at the code, I wonder whether we should convert dt_property_read_string() to use ERR_PTR(). So we could remove the last argument and return it instead. > int rc; > + /* SAF-1-safe MC3R1.R9.1 */ > u64 mem; > u32 p2m_mem_mb; > unsigned long p2m_pages; > @@ -3939,6 +3957,7 @@ void __init create_domUs(void) > .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), > }; > unsigned int flags = 0U; > + /* SAF-1-safe MC3R1.R9.1 */ > uint32_t val; > int rc; > > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h > index bb64925d70..25f39364d1 100644 > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -117,6 +117,7 @@ static int __init setup_chosen_node(void *fdt, int *addr_cells, int *size_cells) > static int __init fdt_set_reg(void *fdt, int node, int addr_cells, > int size_cells, uint64_t addr, uint64_t len) > { > + /* SAF-1-safe MC3R1.R9.1 */ > __be32 val[4]; /* At most 2 64 bit values to be stored */ > __be32 *cellp; > > @@ -308,7 +309,7 @@ fdt_set_fail: > static void __init *fdt_increase_size(struct file *fdtfile, int add_size) > { > EFI_STATUS status; > - EFI_PHYSICAL_ADDRESS fdt_addr; > + EFI_PHYSICAL_ADDRESS fdt_addr = 0; > int fdt_size; > int pages; > void *new_fdt; > @@ -433,7 +434,7 @@ static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image, > > static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) > { > - void *ptr; > + void *ptr = NULL; > EFI_STATUS status; > > status = efi_bs->AllocatePool(EfiLoaderData, map_size, &ptr); > @@ -538,6 +539,7 @@ static void __init efi_arch_handle_module(const struct file *file, > { > int node; > int chosen; > + /* SAF-1-safe MC3R1.R9.1 */ > int addr_len, size_len; > > if ( file == &dtbfile ) > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > index 3aa4edda10..aa0180ab5b 100644 > --- a/xen/arch/arm/gic-v3-its.c > +++ b/xen/arch/arm/gic-v3-its.c > @@ -192,8 +192,7 @@ static int its_send_cmd_mapc(struct host_its *its, uint32_t collection_id, > > cmd[0] = GITS_CMD_MAPC; > cmd[1] = 0x00; > - cmd[2] = encode_rdbase(its, cpu, collection_id); > - cmd[2] |= GITS_VALID_BIT; > + cmd[2] = encode_rdbase(its, cpu, collection_id) | GITS_VALID_BIT; Hmmm... How is this even considered as unitialized variable? > cmd[3] = 0x00; > > return its_send_command(its, cmd); > @@ -215,9 +214,7 @@ static int its_send_cmd_mapd(struct host_its *its, uint32_t deviceid, > } > cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32); > cmd[1] = size_bits; > - cmd[2] = itt_addr; > - if ( valid ) > - cmd[2] |= GITS_VALID_BIT; > + cmd[2] = itt_addr | (valid ? GITS_VALID_BIT : 0x00); Same here. > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index c688227abd..a36068b2d8 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -935,6 +935,7 @@ static int xen_pt_update_entry(mfn_t root, unsigned long virt, > mfn_t mfn, unsigned int target, > unsigned int flags) > { > + /* SAF-1-safe MC3R1.R9.1 */ > int rc; > unsigned int level; > lpae_t *table; > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index de32a2d638..83c56cf1cb 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -496,16 +496,18 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn, > lpae_t entry, *table; > int rc; > mfn_t mfn = INVALID_MFN; > - p2m_type_t _t; > + p2m_type_t _t = p2m_invalid; > DECLARE_OFFSETS(offsets, addr); > > ASSERT(p2m_is_locked(p2m)); > BUILD_BUG_ON(THIRD_MASK != PAGE_MASK); > > /* Allow t to be NULL */ > - t = t ?: &_t; > - > - *t = p2m_invalid; > + if( t ) { > + *t = _t; > + } else { > + t = &_t; > + } What was the problem with the previous code? This is also not conformant to Xen coding style. > > if ( valid ) > *valid = false; > @@ -1031,6 +1033,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, > unsigned int level = 0; > unsigned int target = 3 - (page_order / XEN_PT_LPAE_SHIFT); > lpae_t *entry, *table, orig_pte; > + /* SAF-1-safe MC3R1.R9.1 */ > int rc; Can you provide some details why Eclair thinks it is unitialized? > /* A mapping is removed if the MFN is invalid. */ > bool removing_mapping = mfn_eq(smfn, INVALID_MFN); > @@ -1483,6 +1486,7 @@ static inline int p2m_remove_mapping(struct domain *d, > { > struct p2m_domain *p2m = p2m_get_hostp2m(d); > unsigned long i; > + /* SAF-1-safe MC3R1.R9.1 */ > int rc; Can you provide some details why Eclair thinks it is unitialized? > > p2m_write_lock(p2m); > @@ -1685,20 +1689,21 @@ static int p2m_alloc_vmid(struct domain *d) > > ASSERT(nr != INVALID_VMID); > > - if ( nr == MAX_VMID ) > - { > - rc = -EBUSY; > - printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", d->domain_id); > - goto out; > - } > + do { I don't understand this change. How is this making better for Eclair? > + if ( nr == MAX_VMID ) > + { > + rc = -EBUSY; > + printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", d->domain_id); > + break; > + } > > - set_bit(nr, vmid_mask); > + set_bit(nr, vmid_mask); > > - p2m->vmid = nr; > + p2m->vmid = nr; > > - rc = 0; > + rc = 0; > + } while ( 0 ); > > -out: > spin_unlock(&vmid_alloc_lock); > return rc; > }
On 14/07/23 15:00, Julien Grall wrote: > Hi Nicola, > > 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. >> >> To achieve this goal, I adopted the following strategies: > > Please let's at least one patch per strategy. I would also consider some > of the rework separate so they can go in regardless the decision for the > SAF-*. > >> >> - Add a suitably formatted local deviation comment >> (as indicated in 'docs/misra/documenting-violations.rst') >> to exempt the following line from checking. >> >> - Provide an initialization for the variable at the declaration. >> >> - Substitute a goto breaking out of control flow logic with a >> semantically >> equivalent do { .. } while(0). > > As I already mentioned in private, it is unclear to me how you decided > which strategy to use. I still think we need to define our policy before > changing the code. Otherwise, it is going to be difficult to decide for > new code. > The main point of this RFC is doing so. From what I gathered, it's not an easy task: sometimes there are no 'safe' values to initialize variables to and sometimes there is no easy way to prove that indeed something is always initialized or not accessed at all. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> docs/misra/safe.json | 8 +++++++ >> xen/arch/arm/arm64/lib/find_next_bit.c | 1 + >> xen/arch/arm/bootfdt.c | 6 +++++ >> xen/arch/arm/decode.c | 2 ++ >> xen/arch/arm/domain_build.c | 29 ++++++++++++++++++---- >> xen/arch/arm/efi/efi-boot.h | 6 +++-- >> xen/arch/arm/gic-v3-its.c | 9 ++++--- >> xen/arch/arm/mm.c | 1 + >> xen/arch/arm/p2m.c | 33 +++++++++++++++----------- >> 9 files changed, 69 insertions(+), 26 deletions(-) >> >> diff --git a/docs/misra/safe.json b/docs/misra/safe.json >> index e3c8a1d8eb..244001f5be 100644 >> --- a/docs/misra/safe.json >> +++ b/docs/misra/safe.json >> @@ -12,6 +12,14 @@ >> }, >> { >> "id": "SAF-1-safe", >> + "analyser": { >> + "eclair": "MC3R1.R9.1" >> + }, >> + "name": "Rule 9.1: initializer not needed", >> + "text": "The following local variables are possibly >> subject to being read before being written, but code inspection >> ensured that the control flow in the construct where they appear >> ensures that no such event may happen." > I am bit concerned which such statement because the code instance was > today with the current code. This could change in the future and > invalide the reasoning. > > It is not clear to me if we have any mechanism to prevent that. If we > don't, then I think we need to drastically reduce the number of time > this is used (there are a bit too much for my taste). > Indeed, the purpose of such a deviation is that the sound overapproximation computed by the tool requires a human to look at the code and think twice before modifying it (i.e., if ever that code is touched, the reviewer ought to assess whether that justification still holds or some other thing should be done about it. >> + }, >> + { >> + "id": "SAF-2-safe", >> "analyser": {}, >> "name": "Sentinel", >> "text": "Next ID to be used" >> diff --git a/xen/arch/arm/arm64/lib/find_next_bit.c >> b/xen/arch/arm/arm64/lib/find_next_bit.c >> index ca6f82277e..51b852c595 100644 >> --- a/xen/arch/arm/arm64/lib/find_next_bit.c >> +++ b/xen/arch/arm/arm64/lib/find_next_bit.c >> @@ -67,6 +67,7 @@ unsigned long find_next_zero_bit(const unsigned long >> *addr, unsigned long size, >> { >> const unsigned long *p = addr + BIT_WORD(offset); >> unsigned long result = offset & ~(BITS_PER_LONG-1); >> + /* SAF-1-safe MC3R1.R9.1 */ >> unsigned long tmp; > > This is a file that was copied as-is from Linux. I thought we exclude them. There was an expansion location in the report that caused that file to appear in the output. Thanks for reporting it. > > Anyway, can you provide some details why Eclair thinks this is may be > used unitialized? The problem lies in the goto skipping out of control flow structures. It can't be refactored with a "do { tmp=...; break; } while(0)", as done elsewhere, because there are two distinct labels in there, so the semantics would be altered. The same comment applies in other instances as well. > >> if (offset >= size) >> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c >> index 2673ad17a1..1292a64e8d 100644 >> --- a/xen/arch/arm/bootfdt.c >> +++ b/xen/arch/arm/bootfdt.c >> @@ -34,6 +34,7 @@ static bool __init device_tree_node_matches(const >> void *fdt, int node, >> static bool __init device_tree_node_compatible(const void *fdt, int >> node, >> const char *match) >> { >> + /* SAF-1-safe MC3R1.R9.1 */ >> int len, l; > > It is unclear to me whether the SAF-* applies to 'len' or 'l'. Just len, they need to be two separate declarations if it is agreed not to initialize it. Good catch. > >> const void *prop; >> @@ -169,7 +170,9 @@ int __init device_tree_for_each_node(const void >> *fdt, int node, >> */ >> int depth = 0; >> const int first_node = node; >> + /* SAF-1-safe MC3R1.R9.1 */ >> u32 address_cells[DEVICE_TREE_MAX_DEPTH]; >> + /* SAF-1-safe MC3R1.R9.1 */ >> u32 size_cells[DEVICE_TREE_MAX_DEPTH]; > > I have never sent formally, but in the past I proposed to rework the > code so {address, size}_cells[0] would be initialized before the loop. > See > https://lore.kernel.org/all/ea06f2ac-4ac1-4a6f-bda6-e775a7e68699@xen.org/. > > Do you think this would help? If that initialization is applied to all the array elements yes, because the Amplification of R9.1 says: "For the purposes of this rule, an array element or structure member shall be considered as a discrete object". Otherwise, initializing just the first element does not improve the situation in any way, and this is currently not within of the analysis capabilities of ECLAIR. > >> int ret; >> @@ -249,8 +252,10 @@ static void __init process_multiboot_node(const >> void *fdt, int node, >> const __be32 *cell; >> bootmodule_kind kind; >> paddr_t start, size; >> + /* SAF-1-safe MC3R1.R9.1 */ >> int len; >> /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + '/0' >> => 92 */ >> + /* SAF-1-safe MC3R1.R9.1*/ >> char path[92]; > > So the two above, is one category of issue. The variables are passed as > argument of function which will fill them. > > Can Eclair look at the callers, if so, can we consider to always > initialize the values in the callee? > > This would reduce the number of SAF-*. There are a few other examples > like that below. So I will skip them for now. > > [...] > If the value is always initialized in the callee, then there's no problem configuring ECLAIR so that it knows that this parameter is always written, and therefore any subsequent use in the caller is ok. Another possibility is stating that a function never reads the pointee before writing to it (it may or may not write it, but if it doesn't, then the pointee is not read either). The 'strncmp' after 'fdt_get_path' does get in the way, though, because this property is not strong enough to ensure that we can use 'path' after returning from the function. >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index d0d6be922d..d43f86c2f0 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -62,7 +62,7 @@ custom_param("dom0_mem", parse_dom0_mem); >> int __init parse_arch_dom0_param(const char *s, const char *e) >> { >> - long long val; >> + long long val = LLONG_MAX; > > Can you explain why you decided to initialize rather than SAF-? For > clarity, I am not asking to switch to SAF-, I am mainly interested what > were your though process. > I saw that putting a large enough value here would be out of range and trigger the printk even if the conditional leads to somehow enter the if branch with an invalid value. >> if ( !parse_signed_integer("sve", s, e, &val) ) >> { >> @@ -1077,6 +1077,7 @@ static void __init >> assign_static_memory_11(struct domain *d, >> static int __init handle_linux_pci_domain(struct kernel_info *kinfo, >> const struct >> dt_device_node *node) >> { >> + /* SAF-1-safe MC3R1.R9.1 */ >> uint16_t segment; >> int res; >> @@ -1351,6 +1352,7 @@ static int __init make_memory_node(const struct >> domain *d, >> unsigned int i; >> int res, reg_size = addrcells + sizecells; >> int nr_cells = 0; >> + /* SAF-1-safe MC3R1.R9.1*/ >> __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells >> */]; >> __be32 *cells; >> @@ -1578,6 +1580,7 @@ static int __init find_unallocated_memory(const >> struct kernel_info *kinfo, >> struct rangeset *unalloc_mem; >> paddr_t start, end; >> unsigned int i; >> + /* SAF-1-safe MC3R1.R9.1 */ >> int res; >> dt_dprintk("Find unallocated memory for extended regions\n"); >> @@ -1727,6 +1730,7 @@ static int __init find_memory_holes(const struct >> kernel_info *kinfo, >> dt_for_each_device_node( dt_host, np ) >> { >> unsigned int naddr; >> + /* SAF-1-safe MC3R1.R9.1 */ >> paddr_t addr, size; >> naddr = dt_number_of_address(np); >> @@ -1976,9 +1980,11 @@ static int __init make_cpus_node(const struct >> domain *d, void *fdt) >> const struct dt_device_node *npcpu; >> unsigned int cpu; >> const void *compatible = NULL; >> + /* SAF-1-safe MC3R1.R9.1 */ >> u32 len; >> /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */ >> char buf[13]; >> + /* SAF-1-safe MC3R1.R9.1 */ >> u32 clock_frequency; >> /* Keep the compiler happy with -Og */ >> bool clock_valid = false; >> @@ -2104,6 +2110,7 @@ static int __init make_gic_node(const struct >> domain *d, void *fdt, >> const struct dt_device_node *gic = dt_interrupt_controller; >> int res = 0; >> const void *addrcells, *sizecells; >> + /* SAF-1-safe MC3R1.R9.1 */ >> u32 addrcells_len, sizecells_len; >> /* >> @@ -2179,6 +2186,7 @@ static int __init make_timer_node(const struct >> kernel_info *kinfo) >> int res; >> unsigned int irq[MAX_TIMER_PPI]; >> gic_interrupt_t intrs[3]; >> + /* SAF-1-safe MC3R1.R9.1 */ >> u32 clock_frequency; >> bool clock_valid; >> @@ -2511,6 +2519,7 @@ static int __init handle_device(struct domain >> *d, struct dt_device_node *dev, >> unsigned int naddr; >> unsigned int i; >> int res; >> + /* SAF-1-safe MC3R1.R9.1 */ >> paddr_t addr, size; >> bool own_device = !dt_device_for_passthrough(dev); >> /* >> @@ -2779,6 +2788,7 @@ static int __init make_gicv2_domU_node(struct >> kernel_info *kinfo) >> { >> void *fdt = kinfo->fdt; >> int res = 0; >> + /* SAF-1-safe MC3R1.R9.1*/ >> __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; >> __be32 *cells; >> const struct domain *d = kinfo->d; >> @@ -2914,6 +2924,7 @@ static int __init make_vpl011_uart_node(struct >> kernel_info *kinfo) >> void *fdt = kinfo->fdt; >> int res; >> gic_interrupt_t intr; >> + /* SAF-1-safe MC3R1.R9.1*/ >> __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; >> __be32 *cells; >> struct domain *d = kinfo->d; >> @@ -3435,6 +3446,7 @@ static void __init initrd_load(struct >> kernel_info *kinfo) >> paddr_t paddr, len; >> int node; >> int res; >> + /* SAF-1-safe MC3R1.R9.1 */ >> __be32 val[2]; >> __be32 *cellp; >> void __iomem *initrd; >> @@ -3514,6 +3526,7 @@ static int __init get_evtchn_dt_property(const >> struct dt_device_node *np, >> uint32_t *port, uint32_t >> *phandle) >> { >> const __be32 *prop = NULL; >> + /* SAF-1-safe MC3R1.R9.1 */ >> uint32_t len; >> prop = dt_get_property(np, "xen,evtchn", &len); >> @@ -3538,10 +3551,13 @@ static int __init get_evtchn_dt_property(const >> struct dt_device_node *np, >> static int __init alloc_domain_evtchn(struct dt_device_node *node) >> { >> int rc; >> + /* SAF-1-safe MC3R1.R9.1 */ >> uint32_t domU1_port, domU2_port, remote_phandle; >> struct dt_device_node *remote_node; >> const struct dt_device_node *p1_node, *p2_node; >> + /* SAF-1-safe MC3R1.R9.1 */ >> struct evtchn_alloc_unbound alloc_unbound; >> + /* SAF-1-safe MC3R1.R9.1 */ >> struct evtchn_bind_interdomain bind_interdomain; >> struct domain *d1 = NULL, *d2 = NULL; >> @@ -3789,11 +3805,12 @@ static int __init construct_domain(struct >> domain *d, struct kernel_info *kinfo) >> static int __init alloc_xenstore_evtchn(struct domain *d) >> { >> - evtchn_alloc_unbound_t alloc; >> + evtchn_alloc_unbound_t alloc = { >> + .dom = d->domain_id, >> + .remote_dom = hardware_domain->domain_id >> + }; >> int rc; >> - alloc.dom = d->domain_id; >> - alloc.remote_dom = hardware_domain->domain_id; > > It is not clear why this is modified. There are only two fields in > 'alloc' and they are both initialized. > There are three fields in the struct, and the third is clearly to be set by an initalization function. If, as above with 'path', the init function ensures that the third field is always written then there's no problem, otherwise this initialization prevents any use of an uninitialized port. struct evtchn_alloc_unbound { /* IN parameters */ domid_t dom, remote_dom; /* OUT parameters */ evtchn_port_t port; }; typedef struct evtchn_alloc_unbound evtchn_alloc_unbound_t; >> rc = evtchn_alloc_unbound(&alloc, 0); >> if ( rc ) >> { >> @@ -3810,8 +3827,9 @@ static int __init construct_domU(struct domain *d, >> const struct dt_device_node *node) >> { >> struct kernel_info kinfo = {}; >> - const char *dom0less_enhanced; >> + const char *dom0less_enhanced = NULL; > > If you look at the user below, all the callers assume dom0less_enhanced > will be non-NULL. So it is unclear to me how this value is safer. > > Looking at the code, I wonder whether we should convert > dt_property_read_string() to use ERR_PTR(). So we could remove the last > argument and return it instead. Is relying on that assumption somehow safer? The suggestion to remove the last parameter seems better. > >> int rc; >> + /* SAF-1-safe MC3R1.R9.1 */ >> u64 mem; >> u32 p2m_mem_mb; >> unsigned long p2m_pages; >> @@ -3939,6 +3957,7 @@ void __init create_domUs(void) >> .grant_opts = >> XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), >> }; >> unsigned int flags = 0U; >> + /* SAF-1-safe MC3R1.R9.1 */ >> uint32_t val; >> int rc; >> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h >> index bb64925d70..25f39364d1 100644 >> --- a/xen/arch/arm/efi/efi-boot.h >> +++ b/xen/arch/arm/efi/efi-boot.h >> @@ -117,6 +117,7 @@ static int __init setup_chosen_node(void *fdt, int >> *addr_cells, int *size_cells) >> static int __init fdt_set_reg(void *fdt, int node, int addr_cells, >> int size_cells, uint64_t addr, >> uint64_t len) >> { >> + /* SAF-1-safe MC3R1.R9.1 */ >> __be32 val[4]; /* At most 2 64 bit values to be stored */ >> __be32 *cellp; >> @@ -308,7 +309,7 @@ fdt_set_fail: >> static void __init *fdt_increase_size(struct file *fdtfile, int >> add_size) >> { >> EFI_STATUS status; >> - EFI_PHYSICAL_ADDRESS fdt_addr; >> + EFI_PHYSICAL_ADDRESS fdt_addr = 0; >> int fdt_size; >> int pages; >> void *new_fdt; >> @@ -433,7 +434,7 @@ static void __init efi_arch_cfg_file_late(const >> EFI_LOADED_IMAGE *image, >> static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) >> { >> - void *ptr; >> + void *ptr = NULL; >> EFI_STATUS status; >> status = efi_bs->AllocatePool(EfiLoaderData, map_size, &ptr); >> @@ -538,6 +539,7 @@ static void __init efi_arch_handle_module(const >> struct file *file, >> { >> int node; >> int chosen; >> + /* SAF-1-safe MC3R1.R9.1 */ >> int addr_len, size_len; >> if ( file == &dtbfile ) >> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >> index 3aa4edda10..aa0180ab5b 100644 >> --- a/xen/arch/arm/gic-v3-its.c >> +++ b/xen/arch/arm/gic-v3-its.c >> @@ -192,8 +192,7 @@ static int its_send_cmd_mapc(struct host_its *its, >> uint32_t collection_id, >> cmd[0] = GITS_CMD_MAPC; >> cmd[1] = 0x00; >> - cmd[2] = encode_rdbase(its, cpu, collection_id); >> - cmd[2] |= GITS_VALID_BIT; >> + cmd[2] = encode_rdbase(its, cpu, collection_id) | GITS_VALID_BIT; > > Hmmm... How is this even considered as unitialized variable? > The analysis here could use some more precision, but the modified construct is entirely equivalent. >> cmd[3] = 0x00; >> return its_send_command(its, cmd); >> @@ -215,9 +214,7 @@ static int its_send_cmd_mapd(struct host_its *its, >> uint32_t deviceid, >> } >> cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32); >> cmd[1] = size_bits; >> - cmd[2] = itt_addr; >> - if ( valid ) >> - cmd[2] |= GITS_VALID_BIT; >> + cmd[2] = itt_addr | (valid ? GITS_VALID_BIT : 0x00); > > Same here. > >> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >> index c688227abd..a36068b2d8 100644 >> --- a/xen/arch/arm/mm.c >> +++ b/xen/arch/arm/mm.c >> @@ -935,6 +935,7 @@ static int xen_pt_update_entry(mfn_t root, >> unsigned long virt, >> mfn_t mfn, unsigned int target, >> unsigned int flags) >> { >> + /* SAF-1-safe MC3R1.R9.1 */ >> int rc; >> unsigned int level; >> lpae_t *table; >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index de32a2d638..83c56cf1cb 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -496,16 +496,18 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, >> gfn_t gfn, >> lpae_t entry, *table; >> int rc; >> mfn_t mfn = INVALID_MFN; >> - p2m_type_t _t; >> + p2m_type_t _t = p2m_invalid; >> DECLARE_OFFSETS(offsets, addr); >> ASSERT(p2m_is_locked(p2m)); >> BUILD_BUG_ON(THIRD_MASK != PAGE_MASK); >> /* Allow t to be NULL */ >> - t = t ?: &_t; >> - >> - *t = p2m_invalid; >> + if( t ) { >> + *t = _t; >> + } else { >> + t = &_t; >> + } > What was the problem with the previous code? > > This is also not conformant to Xen coding style. > The problem is that _t may be uninitialized, hence assigning its address to t could be problematic. Another way to address this is to initialize _t to a bad value and use this variable in the body, then assign to t based on the value just before returning. Point taken on the coding style. >> if ( valid ) >> *valid = false; >> @@ -1031,6 +1033,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, >> unsigned int level = 0; >> unsigned int target = 3 - (page_order / XEN_PT_LPAE_SHIFT); >> lpae_t *entry, *table, orig_pte; >> + /* SAF-1-safe MC3R1.R9.1 */ >> int rc; > > Can you provide some details why Eclair thinks it is unitialized? Same issue with gotos explained above, can't be refactored because of the for enclosing the goto. > >> /* A mapping is removed if the MFN is invalid. */ >> bool removing_mapping = mfn_eq(smfn, INVALID_MFN); >> @@ -1483,6 +1486,7 @@ static inline int p2m_remove_mapping(struct >> domain *d, >> { >> struct p2m_domain *p2m = p2m_get_hostp2m(d); >> unsigned long i; >> + /* SAF-1-safe MC3R1.R9.1 */ >> int rc; > > > Can you provide some details why Eclair thinks it is unitialized? > Same as above. >> p2m_write_lock(p2m); >> @@ -1685,20 +1689,21 @@ static int p2m_alloc_vmid(struct domain *d) >> ASSERT(nr != INVALID_VMID); >> - if ( nr == MAX_VMID ) >> - { >> - rc = -EBUSY; >> - printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", >> d->domain_id); >> - goto out; >> - } >> + do { > > I don't understand this change. How is this making better for Eclair? > This is an example where the goto can be eliminated, which in turn allows to automatically check the correctness. >> + if ( nr == MAX_VMID ) >> + { >> + rc = -EBUSY; >> + printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", >> d->domain_id); >> + break; >> + } >> - set_bit(nr, vmid_mask); >> + set_bit(nr, vmid_mask); >> - p2m->vmid = nr; >> + p2m->vmid = nr; >> - rc = 0; >> + rc = 0; >> + } while ( 0 ); >> -out: >> spin_unlock(&vmid_alloc_lock); >> return rc; >> } > Considering all of the replies above, a first draft of a strategy/policy I can think of is having: - Initializer functions that always write their parameter, so that the strongest "pointee always written" property can be stated. This causes all further uses to be marked safe. - Initialize the variable when there exists a known safe value that does not alter the semantics of the function. The initialization does not need to be at the declaration, but doing so simplifies the code. - Deviate all cases where any of the previous does not apply, with a comment deviation that refers to a justification reporting that the code has been checked to respect the rule (keep in mind that _violations_ to a Mandatory rule such as R9.1 are not allowed to claim MISRA compliance).
Hi Nicola, On 17/07/2023 13:08, Nicola Vetrini wrote: > On 14/07/23 15:00, Julien Grall wrote: >> Hi Nicola, >> >> 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. >>> >>> To achieve this goal, I adopted the following strategies: >> >> Please let's at least one patch per strategy. I would also consider >> some of the rework separate so they can go in regardless the decision >> for the SAF-*. >> >>> >>> - Add a suitably formatted local deviation comment >>> (as indicated in 'docs/misra/documenting-violations.rst') >>> to exempt the following line from checking. >>> >>> - Provide an initialization for the variable at the declaration. >>> >>> - Substitute a goto breaking out of control flow logic with a >>> semantically >>> equivalent do { .. } while(0). >> >> As I already mentioned in private, it is unclear to me how you decided >> which strategy to use. I still think we need to define our policy >> before changing the code. Otherwise, it is going to be difficult to >> decide for new code. >> > > The main point of this RFC is doing so. From what I gathered, it's not > an easy task: sometimes there are no 'safe' values to initialize > variables to and sometimes there is no easy way to prove that indeed > something is always initialized or not accessed at all. But you wrote the code. So you should be able to explain how you took the decision between one and the others. Also, even if this is an RFC, it would have been good to summarize any discussion that happened in private and if there were concern try to come up with ideas or at least listing the concerns after '---. > >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> --- >>> docs/misra/safe.json | 8 +++++++ >>> xen/arch/arm/arm64/lib/find_next_bit.c | 1 + >>> xen/arch/arm/bootfdt.c | 6 +++++ >>> xen/arch/arm/decode.c | 2 ++ >>> xen/arch/arm/domain_build.c | 29 ++++++++++++++++++---- >>> xen/arch/arm/efi/efi-boot.h | 6 +++-- >>> xen/arch/arm/gic-v3-its.c | 9 ++++--- >>> xen/arch/arm/mm.c | 1 + >>> xen/arch/arm/p2m.c | 33 +++++++++++++++----------- >>> 9 files changed, 69 insertions(+), 26 deletions(-) >>> >>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json >>> index e3c8a1d8eb..244001f5be 100644 >>> --- a/docs/misra/safe.json >>> +++ b/docs/misra/safe.json >>> @@ -12,6 +12,14 @@ >>> }, >>> { >>> "id": "SAF-1-safe", >>> + "analyser": { >>> + "eclair": "MC3R1.R9.1" >>> + }, >>> + "name": "Rule 9.1: initializer not needed", >>> + "text": "The following local variables are possibly >>> subject to being read before being written, but code inspection >>> ensured that the control flow in the construct where they appear >>> ensures that no such event may happen." >> I am bit concerned which such statement because the code instance was >> today with the current code. This could change in the future and >> invalide the reasoning. >> >> It is not clear to me if we have any mechanism to prevent that. If we >> don't, then I think we need to drastically reduce the number of time >> this is used (there are a bit too much for my taste). >> > > Indeed, the purpose of such a deviation is that the sound > overapproximation computed by the tool requires a human to look at the > code and think twice before modifying it (i.e., if ever that code is > touched, the reviewer ought to assess whether that justification still > holds or some other thing should be done about it. Your assumption is the reviewer will notice there is an existing devitation and be able to assess it has changed. I view this assumption as risky in the long term. Have you investigate to improve the automatic tooling? > >>> + }, >>> + { >>> + "id": "SAF-2-safe", >>> "analyser": {}, >>> "name": "Sentinel", >>> "text": "Next ID to be used" >>> diff --git a/xen/arch/arm/arm64/lib/find_next_bit.c >>> b/xen/arch/arm/arm64/lib/find_next_bit.c >>> index ca6f82277e..51b852c595 100644 >>> --- a/xen/arch/arm/arm64/lib/find_next_bit.c >>> +++ b/xen/arch/arm/arm64/lib/find_next_bit.c >>> @@ -67,6 +67,7 @@ unsigned long find_next_zero_bit(const unsigned >>> long *addr, unsigned long size, >>> { >>> const unsigned long *p = addr + BIT_WORD(offset); >>> unsigned long result = offset & ~(BITS_PER_LONG-1); >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> unsigned long tmp; >> >> This is a file that was copied as-is from Linux. I thought we exclude >> them. > > There was an expansion location in the report that caused that file to > appear in the output. Thanks for reporting it. > >> >> Anyway, can you provide some details why Eclair thinks this is may be >> used unitialized? > > The problem lies in the goto skipping out of control flow structures. It > can't be refactored with a "do { tmp=...; break; } while(0)", as done > elsewhere, because there are two distinct labels in there, so the > semantics would be altered. The same comment applies in other instances > as well. > >> >>> if (offset >= size) >>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c >>> index 2673ad17a1..1292a64e8d 100644 >>> --- a/xen/arch/arm/bootfdt.c >>> +++ b/xen/arch/arm/bootfdt.c >>> @@ -34,6 +34,7 @@ static bool __init device_tree_node_matches(const >>> void *fdt, int node, >>> static bool __init device_tree_node_compatible(const void *fdt, int >>> node, >>> const char *match) >>> { >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> int len, l; >> >> It is unclear to me whether the SAF-* applies to 'len' or 'l'. > > Just len, they need to be two separate declarations if it is agreed not > to initialize it. Good catch. > >> >>> const void *prop; >>> @@ -169,7 +170,9 @@ int __init device_tree_for_each_node(const void >>> *fdt, int node, >>> */ >>> int depth = 0; >>> const int first_node = node; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> u32 address_cells[DEVICE_TREE_MAX_DEPTH]; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> u32 size_cells[DEVICE_TREE_MAX_DEPTH]; >> >> I have never sent formally, but in the past I proposed to rework the >> code so {address, size}_cells[0] would be initialized before the loop. >> See >> https://lore.kernel.org/all/ea06f2ac-4ac1-4a6f-bda6-e775a7e68699@xen.org/. >> >> Do you think this would help? > > If that initialization is applied to all the array elements yes, because > the Amplification of R9.1 says: > "For the purposes of this rule, an array element or structure member > shall be considered as a discrete > object". > Otherwise, initializing just the first element does not improve the > situation in any way, and this is currently not within of the analysis > capabilities of ECLAIR. Any plan to improve ECLAIR? > >> >>> int ret; >>> @@ -249,8 +252,10 @@ static void __init process_multiboot_node(const >>> void *fdt, int node, >>> const __be32 *cell; >>> bootmodule_kind kind; >>> paddr_t start, size; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> int len; >>> /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + '/0' >>> => 92 */ >>> + /* SAF-1-safe MC3R1.R9.1*/ >>> char path[92]; >> >> So the two above, is one category of issue. The variables are passed >> as argument of function which will fill them. >> >> Can Eclair look at the callers, if so, can we consider to always >> initialize the values in the callee? >> >> This would reduce the number of SAF-*. There are a few other examples >> like that below. So I will skip them for now. >> >> [...] >> > > If the value is always initialized in the callee, then there's no > problem configuring ECLAIR so that it knows that this parameter is > always written, and therefore any subsequent use in the caller is ok. > > Another possibility is stating that a function never reads the pointee > before writing to it (it may or may not write it, but if it doesn't, > then the pointee is not read either). The 'strncmp' after 'fdt_get_path' > does get in the way, though, because this property is not strong enough > to ensure that we can use 'path' after returning from the function. I am not sure I fully understand what you wrote. Can you provide a C example? > >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index d0d6be922d..d43f86c2f0 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -62,7 +62,7 @@ custom_param("dom0_mem", parse_dom0_mem); >>> int __init parse_arch_dom0_param(const char *s, const char *e) >>> { >>> - long long val; >>> + long long val = LLONG_MAX; >> >> Can you explain why you decided to initialize rather than SAF-? For >> clarity, I am not asking to switch to SAF-, I am mainly interested >> what were your though process. >> > > I saw that putting a large enough value here would be out of range and > trigger the printk even if the conditional leads to somehow enter the if > branch with an invalid value. Ok. This wants to be documented on top of the assignment. > >>> if ( !parse_signed_integer("sve", s, e, &val) ) >>> { >>> @@ -1077,6 +1077,7 @@ static void __init >>> assign_static_memory_11(struct domain *d, >>> static int __init handle_linux_pci_domain(struct kernel_info *kinfo, >>> const struct >>> dt_device_node *node) >>> { >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> uint16_t segment; >>> int res; >>> @@ -1351,6 +1352,7 @@ static int __init make_memory_node(const struct >>> domain *d, >>> unsigned int i; >>> int res, reg_size = addrcells + sizecells; >>> int nr_cells = 0; >>> + /* SAF-1-safe MC3R1.R9.1*/ >>> __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells >>> */]; >>> __be32 *cells; >>> @@ -1578,6 +1580,7 @@ static int __init find_unallocated_memory(const >>> struct kernel_info *kinfo, >>> struct rangeset *unalloc_mem; >>> paddr_t start, end; >>> unsigned int i; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> int res; >>> dt_dprintk("Find unallocated memory for extended regions\n"); >>> @@ -1727,6 +1730,7 @@ static int __init find_memory_holes(const >>> struct kernel_info *kinfo, >>> dt_for_each_device_node( dt_host, np ) >>> { >>> unsigned int naddr; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> paddr_t addr, size; >>> naddr = dt_number_of_address(np); >>> @@ -1976,9 +1980,11 @@ static int __init make_cpus_node(const struct >>> domain *d, void *fdt) >>> const struct dt_device_node *npcpu; >>> unsigned int cpu; >>> const void *compatible = NULL; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> u32 len; >>> /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */ >>> char buf[13]; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> u32 clock_frequency; >>> /* Keep the compiler happy with -Og */ >>> bool clock_valid = false; >>> @@ -2104,6 +2110,7 @@ static int __init make_gic_node(const struct >>> domain *d, void *fdt, >>> const struct dt_device_node *gic = dt_interrupt_controller; >>> int res = 0; >>> const void *addrcells, *sizecells; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> u32 addrcells_len, sizecells_len; >>> /* >>> @@ -2179,6 +2186,7 @@ static int __init make_timer_node(const struct >>> kernel_info *kinfo) >>> int res; >>> unsigned int irq[MAX_TIMER_PPI]; >>> gic_interrupt_t intrs[3]; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> u32 clock_frequency; >>> bool clock_valid; >>> @@ -2511,6 +2519,7 @@ static int __init handle_device(struct domain >>> *d, struct dt_device_node *dev, >>> unsigned int naddr; >>> unsigned int i; >>> int res; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> paddr_t addr, size; >>> bool own_device = !dt_device_for_passthrough(dev); >>> /* >>> @@ -2779,6 +2788,7 @@ static int __init make_gicv2_domU_node(struct >>> kernel_info *kinfo) >>> { >>> void *fdt = kinfo->fdt; >>> int res = 0; >>> + /* SAF-1-safe MC3R1.R9.1*/ >>> __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * >>> 2]; >>> __be32 *cells; >>> const struct domain *d = kinfo->d; >>> @@ -2914,6 +2924,7 @@ static int __init make_vpl011_uart_node(struct >>> kernel_info *kinfo) >>> void *fdt = kinfo->fdt; >>> int res; >>> gic_interrupt_t intr; >>> + /* SAF-1-safe MC3R1.R9.1*/ >>> __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; >>> __be32 *cells; >>> struct domain *d = kinfo->d; >>> @@ -3435,6 +3446,7 @@ static void __init initrd_load(struct >>> kernel_info *kinfo) >>> paddr_t paddr, len; >>> int node; >>> int res; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> __be32 val[2]; >>> __be32 *cellp; >>> void __iomem *initrd; >>> @@ -3514,6 +3526,7 @@ static int __init get_evtchn_dt_property(const >>> struct dt_device_node *np, >>> uint32_t *port, uint32_t >>> *phandle) >>> { >>> const __be32 *prop = NULL; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> uint32_t len; >>> prop = dt_get_property(np, "xen,evtchn", &len); >>> @@ -3538,10 +3551,13 @@ static int __init >>> get_evtchn_dt_property(const struct dt_device_node *np, >>> static int __init alloc_domain_evtchn(struct dt_device_node *node) >>> { >>> int rc; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> uint32_t domU1_port, domU2_port, remote_phandle; >>> struct dt_device_node *remote_node; >>> const struct dt_device_node *p1_node, *p2_node; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> struct evtchn_alloc_unbound alloc_unbound; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> struct evtchn_bind_interdomain bind_interdomain; >>> struct domain *d1 = NULL, *d2 = NULL; >>> @@ -3789,11 +3805,12 @@ static int __init construct_domain(struct >>> domain *d, struct kernel_info *kinfo) >>> static int __init alloc_xenstore_evtchn(struct domain *d) >>> { >>> - evtchn_alloc_unbound_t alloc; >>> + evtchn_alloc_unbound_t alloc = { >>> + .dom = d->domain_id, >>> + .remote_dom = hardware_domain->domain_id >>> + }; >>> int rc; >>> - alloc.dom = d->domain_id; >>> - alloc.remote_dom = hardware_domain->domain_id; >> >> It is not clear why this is modified. There are only two fields in >> 'alloc' and they are both initialized. >> > > There are three fields in the struct, and the third is clearly to be set > by an initalization function. If, as above with 'path', the init > function ensures that the third field is always written then there's no > problem, otherwise this initialization prevents any use of an > uninitialized port. > > struct evtchn_alloc_unbound { > /* IN parameters */ > domid_t dom, remote_dom; > /* OUT parameters */ > evtchn_port_t port; > }; > typedef struct evtchn_alloc_unbound evtchn_alloc_unbound_t; Ah, I didn't spot the third field because it was defined on the same line. I would prefer if the value is explicitely initialized to 0. Also, I think this ought to be in a separate patch (with other similar pattern). >>> rc = evtchn_alloc_unbound(&alloc, 0); >>> if ( rc ) >>> { >>> @@ -3810,8 +3827,9 @@ static int __init construct_domU(struct domain *d, >>> const struct dt_device_node *node) >>> { >>> struct kernel_info kinfo = {}; >>> - const char *dom0less_enhanced; >>> + const char *dom0less_enhanced = NULL; >> >> If you look at the user below, all the callers assume >> dom0less_enhanced will be non-NULL. So it is unclear to me how this >> value is safer. >> > Looking at the code, I wonder whether we should convert >> dt_property_read_string() to use ERR_PTR(). So we could remove the >> last argument and return it instead. > > Is relying on that assumption somehow safer? I am assuming you are referring to "If you look at the user below, all the callers assume dom0less_enhanced will be non-NULL". Note that I didn't suggest it is safer. I am only pointed out that you didn't specify how this was better in the context of the code. > The suggestion to remove > the last parameter seems better. > >> >>> int rc; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> u64 mem; >>> u32 p2m_mem_mb; >>> unsigned long p2m_pages; >>> @@ -3939,6 +3957,7 @@ void __init create_domUs(void) >>> .grant_opts = >>> XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), >>> }; >>> unsigned int flags = 0U; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> uint32_t val; >>> int rc; >>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h >>> index bb64925d70..25f39364d1 100644 >>> --- a/xen/arch/arm/efi/efi-boot.h >>> +++ b/xen/arch/arm/efi/efi-boot.h >>> @@ -117,6 +117,7 @@ static int __init setup_chosen_node(void *fdt, >>> int *addr_cells, int *size_cells) >>> static int __init fdt_set_reg(void *fdt, int node, int addr_cells, >>> int size_cells, uint64_t addr, >>> uint64_t len) >>> { >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> __be32 val[4]; /* At most 2 64 bit values to be stored */ >>> __be32 *cellp; >>> @@ -308,7 +309,7 @@ fdt_set_fail: >>> static void __init *fdt_increase_size(struct file *fdtfile, int >>> add_size) >>> { >>> EFI_STATUS status; >>> - EFI_PHYSICAL_ADDRESS fdt_addr; >>> + EFI_PHYSICAL_ADDRESS fdt_addr = 0; >>> int fdt_size; >>> int pages; >>> void *new_fdt; >>> @@ -433,7 +434,7 @@ static void __init efi_arch_cfg_file_late(const >>> EFI_LOADED_IMAGE *image, >>> static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) >>> { >>> - void *ptr; >>> + void *ptr = NULL; >>> EFI_STATUS status; >>> status = efi_bs->AllocatePool(EfiLoaderData, map_size, &ptr); >>> @@ -538,6 +539,7 @@ static void __init efi_arch_handle_module(const >>> struct file *file, >>> { >>> int node; >>> int chosen; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> int addr_len, size_len; >>> if ( file == &dtbfile ) >>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >>> index 3aa4edda10..aa0180ab5b 100644 >>> --- a/xen/arch/arm/gic-v3-its.c >>> +++ b/xen/arch/arm/gic-v3-its.c >>> @@ -192,8 +192,7 @@ static int its_send_cmd_mapc(struct host_its >>> *its, uint32_t collection_id, >>> cmd[0] = GITS_CMD_MAPC; >>> cmd[1] = 0x00; >>> - cmd[2] = encode_rdbase(its, cpu, collection_id); >>> - cmd[2] |= GITS_VALID_BIT; >>> + cmd[2] = encode_rdbase(its, cpu, collection_id) | GITS_VALID_BIT; >> >> Hmmm... How is this even considered as unitialized variable? >> > > The analysis here could use some more precision, but the modified > construct is entirely equivalent. I agree that they are equivalent. But in general, we don't change the style of the construct without explaining why. In this case, the first step would be to improve Eclair. > >>> cmd[3] = 0x00; >>> return its_send_command(its, cmd); >>> @@ -215,9 +214,7 @@ static int its_send_cmd_mapd(struct host_its >>> *its, uint32_t deviceid, >>> } >>> cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32); >>> cmd[1] = size_bits; >>> - cmd[2] = itt_addr; >>> - if ( valid ) >>> - cmd[2] |= GITS_VALID_BIT; >>> + cmd[2] = itt_addr | (valid ? GITS_VALID_BIT : 0x00); >> >> Same here. >> >>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >>> index c688227abd..a36068b2d8 100644 >>> --- a/xen/arch/arm/mm.c >>> +++ b/xen/arch/arm/mm.c >>> @@ -935,6 +935,7 @@ static int xen_pt_update_entry(mfn_t root, >>> unsigned long virt, >>> mfn_t mfn, unsigned int target, >>> unsigned int flags) >>> { >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> int rc; >>> unsigned int level; >>> lpae_t *table; >>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>> index de32a2d638..83c56cf1cb 100644 >>> --- a/xen/arch/arm/p2m.c >>> +++ b/xen/arch/arm/p2m.c >>> @@ -496,16 +496,18 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, >>> gfn_t gfn, >>> lpae_t entry, *table; >>> int rc; >>> mfn_t mfn = INVALID_MFN; >>> - p2m_type_t _t; >>> + p2m_type_t _t = p2m_invalid; >>> DECLARE_OFFSETS(offsets, addr); >>> ASSERT(p2m_is_locked(p2m)); >>> BUILD_BUG_ON(THIRD_MASK != PAGE_MASK); >>> /* Allow t to be NULL */ >>> - t = t ?: &_t; >>> - >>> - *t = p2m_invalid; >>> + if( t ) { >>> + *t = _t; >>> + } else { >>> + t = &_t; >>> + } >> What was the problem with the previous code? >> >> This is also not conformant to Xen coding style. >> > > The problem is that _t may be uninitialized, hence assigning its address > to t could be problematic. But the value is set right after. IOW, there is no read between. So how is this prob > Another way to address this is to initialize > _t to a bad value and use this variable in the body, then assign to t > based on the value just before returning. IHMO, neither solution are ideal. I think we should investigate whether Eclair can be improved. [...] >>> if ( valid ) >>> *valid = false; >>> @@ -1031,6 +1033,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, >>> unsigned int level = 0; >>> unsigned int target = 3 - (page_order / XEN_PT_LPAE_SHIFT); >>> lpae_t *entry, *table, orig_pte; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> int rc; >> >> Can you provide some details why Eclair thinks it is unitialized? > > Same issue with gotos explained above, can't be refactored because of > the for enclosing the goto. > >> >>> /* A mapping is removed if the MFN is invalid. */ >>> bool removing_mapping = mfn_eq(smfn, INVALID_MFN); >>> @@ -1483,6 +1486,7 @@ static inline int p2m_remove_mapping(struct >>> domain *d, >>> { >>> struct p2m_domain *p2m = p2m_get_hostp2m(d); >>> unsigned long i; >>> + /* SAF-1-safe MC3R1.R9.1 */ >>> int rc; >> >> >> Can you provide some details why Eclair thinks it is unitialized? >> > > Same as above. > >>> p2m_write_lock(p2m); >>> @@ -1685,20 +1689,21 @@ static int p2m_alloc_vmid(struct domain *d) >>> ASSERT(nr != INVALID_VMID); >>> - if ( nr == MAX_VMID ) >>> - { >>> - rc = -EBUSY; >>> - printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", >>> d->domain_id); >>> - goto out; >>> - } >>> + do { >> >> I don't understand this change. How is this making better for Eclair? >> > > This is an example where the goto can be eliminated, which in turn > allows to automatically check the correctness. If you want to eliminate the 'goto' then they are better way to do it. Like: diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index bc9c3ae25693..8771679dd5fc 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -186,16 +186,14 @@ int p2m_alloc_vmid(struct domain *d) { rc = -EBUSY; printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", d->domain_id); - goto out; + } + else + { + set_bit(nr, vmid_mask); + p2m->vmid = nr; + rc = 0; } - set_bit(nr, vmid_mask); - - p2m->vmid = nr; - - rc = 0; - -out: spin_unlock(&vmid_alloc_lock); return rc; } I have a slight preference with the goto version, but I could accept it if Eclair can't cope with the construct. In any case, this is the sort of change that deserve its own patch as you want to explain why Eclair can't cope with such construct (I don't view it as complex). > >>> + if ( nr == MAX_VMID ) >>> + { >>> + rc = -EBUSY; >>> + printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", >>> d->domain_id); >>> + break; >>> + } >>> - set_bit(nr, vmid_mask); >>> + set_bit(nr, vmid_mask); >>> - p2m->vmid = nr; >>> + p2m->vmid = nr; >>> - rc = 0; >>> + rc = 0; >>> + } while ( 0 ); >>> -out: >>> spin_unlock(&vmid_alloc_lock); >>> return rc; >>> } >> > > > Considering all of the replies above, a first draft of a strategy/policy > I can think of is having: > > - Initializer functions that always write their parameter, so that the > strongest "pointee always written" property can be stated. This causes > all further uses to be marked safe. > > - Initialize the variable when there exists a known safe value that does > not alter the semantics of the function. The initialization does not > need to be at the declaration, but doing so simplifies the code. As I mentionned in private there are two risks with that: 1. You silence compiler to spot other issues 2. You may now get warning from Coverity if it spots you set a value that get overwritten before its first use. So I think such approach should be used with parcimony. Instead, we should look at reworking the code when possible. > > - Deviate all cases where any of the previous does not apply, with a > comment deviation that refers to a justification reporting that the code > has been checked to respect the rule (keep in mind that _violations_ to > a Mandatory rule such as R9.1 are not allowed to claim MISRA compliance). See above for my concern about adding so many deviations. But I am confused with what you wrote. If the rule is mandatory, then why are you trying to add deviation in Xen? Who is going to solve them to make Xen MISRA compliant? Cheers,
Hi Nicola, I am currently looking at the code to check if we can avoid some SAF-*. But I need some clarification on the usage. On 14/07/2023 12:49, Nicola Vetrini wrote: > @@ -2914,6 +2924,7 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo) > void *fdt = kinfo->fdt; > int res; > gic_interrupt_t intr; This value will be passed to set_interrupt() which contains the following code: __be32 *cells = interrupt; [...] dt_set_cells(&cells, ....); Where gic_interrupt_t is: typedef __be32 gic_interrupt[3]; This is very similar to the pattern in ... > + /* SAF-1-safe MC3R1.R9.1*/ > __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; > __be32 *cells; > struct domain *d = kinfo->d; > @@ -3435,6 +3446,7 @@ static void __init initrd_load(struct kernel_info *kinfo) > paddr_t paddr, len; > int node; > int res; > + /* SAF-1-safe MC3R1.R9.1 */ > __be32 val[2]; ... here. cellp = (__be32 *)val; dt_set_cell(&cellp, ..., load_addr); Would you be able to explain why Eclair is complaining for this one but not the previous one? Cheers,
On 17/07/23 23:15, Julien Grall wrote: > Hi Nicola, > > I am currently looking at the code to check if we can avoid some SAF-*. > But I need some clarification on the usage. > > On 14/07/2023 12:49, Nicola Vetrini wrote: >> @@ -2914,6 +2924,7 @@ static int __init make_vpl011_uart_node(struct >> kernel_info *kinfo) >> void *fdt = kinfo->fdt; >> int res; >> gic_interrupt_t intr; > > This value will be passed to set_interrupt() which contains the > following code: > > __be32 *cells = interrupt; > [...] > dt_set_cells(&cells, ....); > > Where gic_interrupt_t is: > > typedef __be32 gic_interrupt[3]; > > This is very similar to the pattern in ... > >> + /* SAF-1-safe MC3R1.R9.1*/ >> __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; >> __be32 *cells; >> struct domain *d = kinfo->d; >> @@ -3435,6 +3446,7 @@ static void __init initrd_load(struct >> kernel_info *kinfo) >> paddr_t paddr, len; >> int node; >> int res; >> + /* SAF-1-safe MC3R1.R9.1 */ >> __be32 val[2]; > > ... here. > > cellp = (__be32 *)val; > dt_set_cell(&cellp, ..., load_addr); > > Would you be able to explain why Eclair is complaining for this one but > not the previous one? > > Cheers, > It did complain, but I specified an always_write property on the first parameter of set_interrupt, because dt_set_cell does always write it trough the argument cells. (there are three calls to dt_set_cell, which means that the array is fully initialized). The difference is that initrd_load calls dt_set_cell directly. dt_set_cell does indeed initialize val[2], but only because it's called with the appropriate size. If the latter is wrapped in a function or macro, then I can add the same property as above and avoid the deviation. Regards,
On 17/07/23 15:40, Julien Grall wrote: > Hi Nicola, > > On 17/07/2023 13:08, Nicola Vetrini wrote: >> On 14/07/23 15:00, Julien Grall wrote: >>> Hi Nicola, >>> >>> 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. >>>> >>>> To achieve this goal, I adopted the following strategies: >>> >>> Please let's at least one patch per strategy. I would also consider >>> some of the rework separate so they can go in regardless the decision >>> for the SAF-*. >>> >>>> >>>> - Add a suitably formatted local deviation comment >>>> (as indicated in 'docs/misra/documenting-violations.rst') >>>> to exempt the following line from checking. >>>> >>>> - Provide an initialization for the variable at the declaration. >>>> >>>> - Substitute a goto breaking out of control flow logic with a >>>> semantically >>>> equivalent do { .. } while(0). >>> >>> As I already mentioned in private, it is unclear to me how you >>> decided which strategy to use. I still think we need to define our >>> policy before changing the code. Otherwise, it is going to be >>> difficult to decide for new code. >>> >> >> The main point of this RFC is doing so. From what I gathered, it's not >> an easy task: sometimes there are no 'safe' values to initialize >> variables to and sometimes there is no easy way to prove that indeed >> something is always initialized or not accessed at all. > > But you wrote the code. So you should be able to explain how you took > the decision between one and the others. > > Also, even if this is an RFC, it would have been good to summarize any > discussion that happened in private and if there were concern try to > come up with ideas or at least listing the concerns after '---. > I'll keep this if the need arises in the future. >> >>>> >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>> --- >>>> docs/misra/safe.json | 8 +++++++ >>>> xen/arch/arm/arm64/lib/find_next_bit.c | 1 + >>>> xen/arch/arm/bootfdt.c | 6 +++++ >>>> xen/arch/arm/decode.c | 2 ++ >>>> xen/arch/arm/domain_build.c | 29 ++++++++++++++++++---- >>>> xen/arch/arm/efi/efi-boot.h | 6 +++-- >>>> xen/arch/arm/gic-v3-its.c | 9 ++++--- >>>> xen/arch/arm/mm.c | 1 + >>>> xen/arch/arm/p2m.c | 33 >>>> +++++++++++++++----------- >>>> 9 files changed, 69 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json >>>> index e3c8a1d8eb..244001f5be 100644 >>>> --- a/docs/misra/safe.json >>>> +++ b/docs/misra/safe.json >>>> @@ -12,6 +12,14 @@ >>>> }, >>>> { >>>> "id": "SAF-1-safe", >>>> + "analyser": { >>>> + "eclair": "MC3R1.R9.1" >>>> + }, >>>> + "name": "Rule 9.1: initializer not needed", >>>> + "text": "The following local variables are possibly >>>> subject to being read before being written, but code inspection >>>> ensured that the control flow in the construct where they appear >>>> ensures that no such event may happen." >>> I am bit concerned which such statement because the code instance was >>> today with the current code. This could change in the future and >>> invalide the reasoning. >>> >>> It is not clear to me if we have any mechanism to prevent that. If we >>> don't, then I think we need to drastically reduce the number of time >>> this is used (there are a bit too much for my taste). >>> >> >> Indeed, the purpose of such a deviation is that the sound >> overapproximation computed by the tool requires a human to look at the >> code and think twice before modifying it (i.e., if ever that code is >> touched, the reviewer ought to assess whether that justification still >> holds or some other thing should be done about it. > > Your assumption is the reviewer will notice there is an existing > devitation and be able to assess it has changed. I view this assumption > as risky in the long term. > > Have you investigate to improve the automatic tooling? > Well, as discussed elsewhere in the thread, a slightly modified version of this deviation comment can list the specific reason why such a thing was deviated directly at the declaration or where the caution is, if you think this is better. Example: // <- SAF-x here int var; [...] // <- or HERE f(&var); An alternative approach to justification, partly discussed with Stefano in private is a macro that looks like an attribute to signal that the variable is intentionally uninitialized. This does not have the benefit of a written justification with a proper comment or an entry in the json file, but is less intrusive and the justification for all occurrences of __uninit w.r.t R9.1 would be included in the static analysis tool configuration, which would be part of the MISRA compliance documentation. This does imply a coarse justification like the one above, but if further clarification is needed it can be provided locally in the code, as guidance for contributors. Example: #define __uninit __uninit int x; >> >>>> + }, >>>> + { >>>> + "id": "SAF-2-safe", >>>> "analyser": {}, >>>> "name": "Sentinel", >>>> "text": "Next ID to be used" >>>> diff --git a/xen/arch/arm/arm64/lib/find_next_bit.c >>>> b/xen/arch/arm/arm64/lib/find_next_bit.c >>>> index ca6f82277e..51b852c595 100644 >>>> --- a/xen/arch/arm/arm64/lib/find_next_bit.c >>>> +++ b/xen/arch/arm/arm64/lib/find_next_bit.c >>>> @@ -67,6 +67,7 @@ unsigned long find_next_zero_bit(const unsigned >>>> long *addr, unsigned long size, >>>> { >>>> const unsigned long *p = addr + BIT_WORD(offset); >>>> unsigned long result = offset & ~(BITS_PER_LONG-1); >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> unsigned long tmp; >>> >>> This is a file that was copied as-is from Linux. I thought we exclude >>> them. >> >> There was an expansion location in the report that caused that file to >> appear in the output. Thanks for reporting it. >> >>> >>> Anyway, can you provide some details why Eclair thinks this is may be >>> used unitialized? >> >> The problem lies in the goto skipping out of control flow structures. >> It can't be refactored with a "do { tmp=...; break; } while(0)", as >> done elsewhere, because there are two distinct labels in there, so the >> semantics would be altered. The same comment applies in other >> instances as well. >> >>> >>>> if (offset >= size) >>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c >>>> index 2673ad17a1..1292a64e8d 100644 >>>> --- a/xen/arch/arm/bootfdt.c >>>> +++ b/xen/arch/arm/bootfdt.c >>>> @@ -34,6 +34,7 @@ static bool __init device_tree_node_matches(const >>>> void *fdt, int node, >>>> static bool __init device_tree_node_compatible(const void *fdt, >>>> int node, >>>> const char *match) >>>> { >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> int len, l; >>> >>> It is unclear to me whether the SAF-* applies to 'len' or 'l'. >> >> Just len, they need to be two separate declarations if it is agreed >> not to initialize it. Good catch. >> >>> >>>> const void *prop; >>>> @@ -169,7 +170,9 @@ int __init device_tree_for_each_node(const void >>>> *fdt, int node, >>>> */ >>>> int depth = 0; >>>> const int first_node = node; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> u32 address_cells[DEVICE_TREE_MAX_DEPTH]; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> u32 size_cells[DEVICE_TREE_MAX_DEPTH]; >>> >>> I have never sent formally, but in the past I proposed to rework the >>> code so {address, size}_cells[0] would be initialized before the >>> loop. See >>> https://lore.kernel.org/all/ea06f2ac-4ac1-4a6f-bda6-e775a7e68699@xen.org/. >>> >>> Do you think this would help? >> >> If that initialization is applied to all the array elements yes, >> because the Amplification of R9.1 says: >> "For the purposes of this rule, an array element or structure member >> shall be considered as a discrete >> object". >> Otherwise, initializing just the first element does not improve the >> situation in any way, and this is currently not within of the analysis >> capabilities of ECLAIR. > > Any plan to improve ECLAIR? > Not in the short term on this. See further below. >> >>> >>>> int ret; >>>> @@ -249,8 +252,10 @@ static void __init process_multiboot_node(const >>>> void *fdt, int node, >>>> const __be32 *cell; >>>> bootmodule_kind kind; >>>> paddr_t start, size; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> int len; >>>> /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + '/0' >>>> => 92 */ >>>> + /* SAF-1-safe MC3R1.R9.1*/ >>>> char path[92]; >>> >>> So the two above, is one category of issue. The variables are passed >>> as argument of function which will fill them. >>> >>> Can Eclair look at the callers, if so, can we consider to always >>> initialize the values in the callee? >>> >>> This would reduce the number of SAF-*. There are a few other examples >>> like that below. So I will skip them for now. >>> >>> [...] >>> >> >> If the value is always initialized in the callee, then there's no >> problem configuring ECLAIR so that it knows that this parameter is >> always written, and therefore any subsequent use in the caller is ok. >> >> Another possibility is stating that a function never reads the pointee >> before writing to it (it may or may not write it, but if it doesn't, >> then the pointee is not read either). The 'strncmp' after >> 'fdt_get_path' does get in the way, though, because this property is >> not strong enough to ensure that we can use 'path' after returning >> from the function. > > I am not sure I fully understand what you wrote. Can you provide a C > example? > void f(int *x) { if(x) { *x = 10; int y =*x; // read the pointee after it's initialized } else { int z; // in this branch the pointee is not read nor written } // we can say that f never reads *x before (possibly) writing to it. } >> >>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>> index d0d6be922d..d43f86c2f0 100644 >>>> --- a/xen/arch/arm/domain_build.c >>>> +++ b/xen/arch/arm/domain_build.c >>>> @@ -62,7 +62,7 @@ custom_param("dom0_mem", parse_dom0_mem); >>>> int __init parse_arch_dom0_param(const char *s, const char *e) >>>> { >>>> - long long val; >>>> + long long val = LLONG_MAX; >>> >>> Can you explain why you decided to initialize rather than SAF-? For >>> clarity, I am not asking to switch to SAF-, I am mainly interested >>> what were your though process. >>> >> >> I saw that putting a large enough value here would be out of range and >> trigger the printk even if the conditional leads to somehow enter the >> if branch with an invalid value. > > Ok. This wants to be documented on top of the assignment. > Ok. While testing this I also switched to LONG_MAX, whose definition is already available in that file, and does not alter the semantics anyway. >> >>>> if ( !parse_signed_integer("sve", s, e, &val) ) >>>> { >>>> @@ -1077,6 +1077,7 @@ static void __init >>>> assign_static_memory_11(struct domain *d, >>>> static int __init handle_linux_pci_domain(struct kernel_info *kinfo, >>>> const struct >>>> dt_device_node *node) >>>> { >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> uint16_t segment; >>>> int res; >>>> @@ -1351,6 +1352,7 @@ static int __init make_memory_node(const >>>> struct domain *d, >>>> unsigned int i; >>>> int res, reg_size = addrcells + sizecells; >>>> int nr_cells = 0; >>>> + /* SAF-1-safe MC3R1.R9.1*/ >>>> __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + >>>> sizecells */]; >>>> __be32 *cells; >>>> @@ -1578,6 +1580,7 @@ static int __init >>>> find_unallocated_memory(const struct kernel_info *kinfo, >>>> struct rangeset *unalloc_mem; >>>> paddr_t start, end; >>>> unsigned int i; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> int res; >>>> dt_dprintk("Find unallocated memory for extended regions\n"); >>>> @@ -1727,6 +1730,7 @@ static int __init find_memory_holes(const >>>> struct kernel_info *kinfo, >>>> dt_for_each_device_node( dt_host, np ) >>>> { >>>> unsigned int naddr; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> paddr_t addr, size; >>>> naddr = dt_number_of_address(np); >>>> @@ -1976,9 +1980,11 @@ static int __init make_cpus_node(const struct >>>> domain *d, void *fdt) >>>> const struct dt_device_node *npcpu; >>>> unsigned int cpu; >>>> const void *compatible = NULL; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> u32 len; >>>> /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */ >>>> char buf[13]; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> u32 clock_frequency; >>>> /* Keep the compiler happy with -Og */ >>>> bool clock_valid = false; >>>> @@ -2104,6 +2110,7 @@ static int __init make_gic_node(const struct >>>> domain *d, void *fdt, >>>> const struct dt_device_node *gic = dt_interrupt_controller; >>>> int res = 0; >>>> const void *addrcells, *sizecells; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> u32 addrcells_len, sizecells_len; >>>> /* >>>> @@ -2179,6 +2186,7 @@ static int __init make_timer_node(const struct >>>> kernel_info *kinfo) >>>> int res; >>>> unsigned int irq[MAX_TIMER_PPI]; >>>> gic_interrupt_t intrs[3]; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> u32 clock_frequency; >>>> bool clock_valid; >>>> @@ -2511,6 +2519,7 @@ static int __init handle_device(struct domain >>>> *d, struct dt_device_node *dev, >>>> unsigned int naddr; >>>> unsigned int i; >>>> int res; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> paddr_t addr, size; >>>> bool own_device = !dt_device_for_passthrough(dev); >>>> /* >>>> @@ -2779,6 +2788,7 @@ static int __init make_gicv2_domU_node(struct >>>> kernel_info *kinfo) >>>> { >>>> void *fdt = kinfo->fdt; >>>> int res = 0; >>>> + /* SAF-1-safe MC3R1.R9.1*/ >>>> __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) >>>> * 2]; >>>> __be32 *cells; >>>> const struct domain *d = kinfo->d; >>>> @@ -2914,6 +2924,7 @@ static int __init make_vpl011_uart_node(struct >>>> kernel_info *kinfo) >>>> void *fdt = kinfo->fdt; >>>> int res; >>>> gic_interrupt_t intr; >>>> + /* SAF-1-safe MC3R1.R9.1*/ >>>> __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; >>>> __be32 *cells; >>>> struct domain *d = kinfo->d; >>>> @@ -3435,6 +3446,7 @@ static void __init initrd_load(struct >>>> kernel_info *kinfo) >>>> paddr_t paddr, len; >>>> int node; >>>> int res; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> __be32 val[2]; >>>> __be32 *cellp; >>>> void __iomem *initrd; >>>> @@ -3514,6 +3526,7 @@ static int __init get_evtchn_dt_property(const >>>> struct dt_device_node *np, >>>> uint32_t *port, uint32_t >>>> *phandle) >>>> { >>>> const __be32 *prop = NULL; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> uint32_t len; >>>> prop = dt_get_property(np, "xen,evtchn", &len); >>>> @@ -3538,10 +3551,13 @@ static int __init >>>> get_evtchn_dt_property(const struct dt_device_node *np, >>>> static int __init alloc_domain_evtchn(struct dt_device_node *node) >>>> { >>>> int rc; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> uint32_t domU1_port, domU2_port, remote_phandle; >>>> struct dt_device_node *remote_node; >>>> const struct dt_device_node *p1_node, *p2_node; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> struct evtchn_alloc_unbound alloc_unbound; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> struct evtchn_bind_interdomain bind_interdomain; >>>> struct domain *d1 = NULL, *d2 = NULL; >>>> @@ -3789,11 +3805,12 @@ static int __init construct_domain(struct >>>> domain *d, struct kernel_info *kinfo) >>>> static int __init alloc_xenstore_evtchn(struct domain *d) >>>> { >>>> - evtchn_alloc_unbound_t alloc; >>>> + evtchn_alloc_unbound_t alloc = { >>>> + .dom = d->domain_id, >>>> + .remote_dom = hardware_domain->domain_id >>>> + }; >>>> int rc; >>>> - alloc.dom = d->domain_id; >>>> - alloc.remote_dom = hardware_domain->domain_id; >>> >>> It is not clear why this is modified. There are only two fields in >>> 'alloc' and they are both initialized. >>> >> >> There are three fields in the struct, and the third is clearly to be >> set by an initalization function. If, as above with 'path', the init >> function ensures that the third field is always written then there's >> no problem, otherwise this initialization prevents any use of an >> uninitialized port. >> > struct evtchn_alloc_unbound { >> /* IN parameters */ >> domid_t dom, remote_dom; >> /* OUT parameters */ >> evtchn_port_t port; >> }; >> typedef struct evtchn_alloc_unbound evtchn_alloc_unbound_t; > > Ah, I didn't spot the third field because it was defined on the same > line. I would prefer if the value is explicitely initialized to 0. > > Also, I think this ought to be in a separate patch (with other similar > pattern). > Ok. >>>> rc = evtchn_alloc_unbound(&alloc, 0); >>>> if ( rc ) >>>> { >>>> @@ -3810,8 +3827,9 @@ static int __init construct_domU(struct domain >>>> *d, >>>> const struct dt_device_node *node) >>>> { >>>> struct kernel_info kinfo = {}; >>>> - const char *dom0less_enhanced; >>>> + const char *dom0less_enhanced = NULL; >>> >>> If you look at the user below, all the callers assume >>> dom0less_enhanced will be non-NULL. So it is unclear to me how this >>> value is safer. >>> > Looking at the code, I wonder whether we should convert >>> dt_property_read_string() to use ERR_PTR(). So we could remove the >>> last argument and return it instead. >> >> Is relying on that assumption somehow safer? > > I am assuming you are referring to "If you look at the user below, all > the callers assume dom0less_enhanced will be non-NULL". Note that I > didn't suggest it is safer. I am only pointed out that you didn't > specify how this was better in the context of the code. > This should be probably discussed after deciding on the refactoring 'dt_property_read_string' >> The suggestion to remove the last parameter seems better. >> >>> >>>> int rc; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> u64 mem; >>>> u32 p2m_mem_mb; >>>> unsigned long p2m_pages; >>>> @@ -3939,6 +3957,7 @@ void __init create_domUs(void) >>>> .grant_opts = >>>> XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), >>>> }; >>>> unsigned int flags = 0U; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> uint32_t val; >>>> int rc; >>>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h >>>> index bb64925d70..25f39364d1 100644 >>>> --- a/xen/arch/arm/efi/efi-boot.h >>>> +++ b/xen/arch/arm/efi/efi-boot.h >>>> @@ -117,6 +117,7 @@ static int __init setup_chosen_node(void *fdt, >>>> int *addr_cells, int *size_cells) >>>> static int __init fdt_set_reg(void *fdt, int node, int addr_cells, >>>> int size_cells, uint64_t addr, >>>> uint64_t len) >>>> { >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> __be32 val[4]; /* At most 2 64 bit values to be stored */ >>>> __be32 *cellp; >>>> @@ -308,7 +309,7 @@ fdt_set_fail: >>>> static void __init *fdt_increase_size(struct file *fdtfile, int >>>> add_size) >>>> { >>>> EFI_STATUS status; >>>> - EFI_PHYSICAL_ADDRESS fdt_addr; >>>> + EFI_PHYSICAL_ADDRESS fdt_addr = 0; >>>> int fdt_size; >>>> int pages; >>>> void *new_fdt; >>>> @@ -433,7 +434,7 @@ static void __init efi_arch_cfg_file_late(const >>>> EFI_LOADED_IMAGE *image, >>>> static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) >>>> { >>>> - void *ptr; >>>> + void *ptr = NULL; >>>> EFI_STATUS status; >>>> status = efi_bs->AllocatePool(EfiLoaderData, map_size, &ptr); >>>> @@ -538,6 +539,7 @@ static void __init efi_arch_handle_module(const >>>> struct file *file, >>>> { >>>> int node; >>>> int chosen; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> int addr_len, size_len; >>>> if ( file == &dtbfile ) >>>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >>>> index 3aa4edda10..aa0180ab5b 100644 >>>> --- a/xen/arch/arm/gic-v3-its.c >>>> +++ b/xen/arch/arm/gic-v3-its.c >>>> @@ -192,8 +192,7 @@ static int its_send_cmd_mapc(struct host_its >>>> *its, uint32_t collection_id, >>>> cmd[0] = GITS_CMD_MAPC; >>>> cmd[1] = 0x00; >>>> - cmd[2] = encode_rdbase(its, cpu, collection_id); >>>> - cmd[2] |= GITS_VALID_BIT; >>>> + cmd[2] = encode_rdbase(its, cpu, collection_id) | GITS_VALID_BIT; >>> >>> Hmmm... How is this even considered as unitialized variable? >>> >> >> The analysis here could use some more precision, but the modified >> construct is entirely equivalent. > > I agree that they are equivalent. But in general, we don't change the > style of the construct without explaining why. > > In this case, the first step would be to improve Eclair. > The changes needed for this kind of analysis are not trivial: we've looked into this, but there's no easy way to support this in a timely manner. I understand that this is an estabilished pattern, but what would you think of an initializer using designators? uint64_t cmd[4] = { .[0] = GITS_CMD_MAPC; .[1] = 0x00; .[2] = encode_rdbase(its, cpu, collection_id) | GITS_VALID_BIT; .[3] = 0x00; } >> >>>> cmd[3] = 0x00; >>>> return its_send_command(its, cmd); >>>> @@ -215,9 +214,7 @@ static int its_send_cmd_mapd(struct host_its >>>> *its, uint32_t deviceid, >>>> } >>>> cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32); >>>> cmd[1] = size_bits; >>>> - cmd[2] = itt_addr; >>>> - if ( valid ) >>>> - cmd[2] |= GITS_VALID_BIT; >>>> + cmd[2] = itt_addr | (valid ? GITS_VALID_BIT : 0x00); >>> >>> Same here. >>> >>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >>>> index c688227abd..a36068b2d8 100644 >>>> --- a/xen/arch/arm/mm.c >>>> +++ b/xen/arch/arm/mm.c >>>> @@ -935,6 +935,7 @@ static int xen_pt_update_entry(mfn_t root, >>>> unsigned long virt, >>>> mfn_t mfn, unsigned int target, >>>> unsigned int flags) >>>> { >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> int rc; >>>> unsigned int level; >>>> lpae_t *table; >>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>>> index de32a2d638..83c56cf1cb 100644 >>>> --- a/xen/arch/arm/p2m.c >>>> +++ b/xen/arch/arm/p2m.c >>>> @@ -496,16 +496,18 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, >>>> gfn_t gfn, >>>> lpae_t entry, *table; >>>> int rc; >>>> mfn_t mfn = INVALID_MFN; >>>> - p2m_type_t _t; >>>> + p2m_type_t _t = p2m_invalid; >>>> DECLARE_OFFSETS(offsets, addr); >>>> ASSERT(p2m_is_locked(p2m)); >>>> BUILD_BUG_ON(THIRD_MASK != PAGE_MASK); >>>> /* Allow t to be NULL */ >>>> - t = t ?: &_t; >>>> - >>>> - *t = p2m_invalid; >>>> + if( t ) { >>>> + *t = _t; >>>> + } else { >>>> + t = &_t; >>>> + } >>> What was the problem with the previous code? >>> >>> This is also not conformant to Xen coding style. >>> >> >> The problem is that _t may be uninitialized, hence assigning its >> address to t could be problematic. > > But the value is set right after. IOW, there is no read between. So how > is this prob > >> Another way to address this is to initialize _t to a bad value and use >> this variable in the body, then assign to t based on the value just >> before returning. > > IHMO, neither solution are ideal. I think we should investigate whether > Eclair can be improved. > > [...] > I'll see what can be done about it, I'll reply when I have an answer. >>>> if ( valid ) >>>> *valid = false; >>>> @@ -1031,6 +1033,7 @@ static int __p2m_set_entry(struct p2m_domain >>>> *p2m, >>>> unsigned int level = 0; >>>> unsigned int target = 3 - (page_order / XEN_PT_LPAE_SHIFT); >>>> lpae_t *entry, *table, orig_pte; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> int rc; >>> >>> Can you provide some details why Eclair thinks it is unitialized? >> >> Same issue with gotos explained above, can't be refactored because of >> the for enclosing the goto. >> >>> >>>> /* A mapping is removed if the MFN is invalid. */ >>>> bool removing_mapping = mfn_eq(smfn, INVALID_MFN); >>>> @@ -1483,6 +1486,7 @@ static inline int p2m_remove_mapping(struct >>>> domain *d, >>>> { >>>> struct p2m_domain *p2m = p2m_get_hostp2m(d); >>>> unsigned long i; >>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>> int rc; >>> >>> >>> Can you provide some details why Eclair thinks it is unitialized? >>> >> >> Same as above. >> >>>> p2m_write_lock(p2m); >>>> @@ -1685,20 +1689,21 @@ static int p2m_alloc_vmid(struct domain *d) >>>> ASSERT(nr != INVALID_VMID); >>>> - if ( nr == MAX_VMID ) >>>> - { >>>> - rc = -EBUSY; >>>> - printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", >>>> d->domain_id); >>>> - goto out; >>>> - } >>>> + do { >>> >>> I don't understand this change. How is this making better for Eclair? >>> >> >> This is an example where the goto can be eliminated, which in turn >> allows to automatically check the correctness. > If you want to eliminate the 'goto' then they are better way to do it. > Like: > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index bc9c3ae25693..8771679dd5fc 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -186,16 +186,14 @@ int p2m_alloc_vmid(struct domain *d) > { > rc = -EBUSY; > printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", > d->domain_id); > - goto out; > + } > + else > + { > + set_bit(nr, vmid_mask); > + p2m->vmid = nr; > + rc = 0; > } > > - set_bit(nr, vmid_mask); > - > - p2m->vmid = nr; > - > - rc = 0; > - > -out: > spin_unlock(&vmid_alloc_lock); > return rc; > } > > I have a slight preference with the goto version, but I could accept it > if Eclair can't cope with the construct. In any case, this is the sort > of change that deserve its own patch as you want to explain why Eclair > can't cope with such construct (I don't view it as complex). > ok >> >>>> + if ( nr == MAX_VMID ) >>>> + { >>>> + rc = -EBUSY; >>>> + printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", >>>> d->domain_id); >>>> + break; >>>> + } >>>> - set_bit(nr, vmid_mask); >>>> + set_bit(nr, vmid_mask); >>>> - p2m->vmid = nr; >>>> + p2m->vmid = nr; >>>> - rc = 0; >>>> + rc = 0; >>>> + } while ( 0 ); >>>> -out: >>>> spin_unlock(&vmid_alloc_lock); >>>> return rc; >>>> } >>> >> >> >> Considering all of the replies above, a first draft of a >> strategy/policy I can think of is having: >> >> - Initializer functions that always write their parameter, so that the >> strongest "pointee always written" property can be stated. This causes >> all further uses to be marked safe. >> >> - Initialize the variable when there exists a known safe value that >> does not alter the semantics of the function. The initialization does >> not need to be at the declaration, but doing so simplifies the code. > > As I mentionned in private there are two risks with that: > 1. You silence compiler to spot other issues > 2. You may now get warning from Coverity if it spots you set a value > that get overwritten before its first use. > > So I think such approach should be used with parcimony. Instead, we > should look at reworking the code when possible. > Do you think it would help if you look directly at actual cautions to spot possible functions that can be refactored? >> >> - Deviate all cases where any of the previous does not apply, with a >> comment deviation that refers to a justification reporting that the >> code has been checked to respect the rule (keep in mind that >> _violations_ to a Mandatory rule such as R9.1 are not allowed to claim >> MISRA compliance). > > See above for my concern about adding so many deviations. But I am > confused with what you wrote. If the rule is mandatory, then why are you > trying to add deviation in Xen? Who is going to solve them to make Xen > MISRA compliant? > Because only cautions were found, not violations (which cannot be deviated). In the former case Xen can say that the code does not violate the rule, but it can't be proven by the tool. Regards,
On 20/07/23 12:14, Nicola Vetrini wrote: > > > On 17/07/23 15:40, Julien Grall wrote: >> Hi Nicola, >> >> On 17/07/2023 13:08, Nicola Vetrini wrote: >>> On 14/07/23 15:00, Julien Grall wrote: >>>> Hi Nicola, >>>> >>>> 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. >>>>> >>>>> To achieve this goal, I adopted the following strategies: >>>> >>>> Please let's at least one patch per strategy. I would also consider >>>> some of the rework separate so they can go in regardless the >>>> decision for the SAF-*. >>>> >>>>> >>>>> - Add a suitably formatted local deviation comment >>>>> (as indicated in 'docs/misra/documenting-violations.rst') >>>>> to exempt the following line from checking. >>>>> >>>>> - Provide an initialization for the variable at the declaration. >>>>> >>>>> - Substitute a goto breaking out of control flow logic with a >>>>> semantically >>>>> equivalent do { .. } while(0). >>>> >>>> As I already mentioned in private, it is unclear to me how you >>>> decided which strategy to use. I still think we need to define our >>>> policy before changing the code. Otherwise, it is going to be >>>> difficult to decide for new code. >>>> >>> >>> The main point of this RFC is doing so. From what I gathered, it's >>> not an easy task: sometimes there are no 'safe' values to initialize >>> variables to and sometimes there is no easy way to prove that indeed >>> something is always initialized or not accessed at all. >> >> But you wrote the code. So you should be able to explain how you took >> the decision between one and the others. >> >> Also, even if this is an RFC, it would have been good to summarize any >> discussion that happened in private and if there were concern try to >> come up with ideas or at least listing the concerns after '---. >> > > I'll keep this if the need arises in the future. > >>> >>>>> >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>> --- >>>>> docs/misra/safe.json | 8 +++++++ >>>>> xen/arch/arm/arm64/lib/find_next_bit.c | 1 + >>>>> xen/arch/arm/bootfdt.c | 6 +++++ >>>>> xen/arch/arm/decode.c | 2 ++ >>>>> xen/arch/arm/domain_build.c | 29 ++++++++++++++++++---- >>>>> xen/arch/arm/efi/efi-boot.h | 6 +++-- >>>>> xen/arch/arm/gic-v3-its.c | 9 ++++--- >>>>> xen/arch/arm/mm.c | 1 + >>>>> xen/arch/arm/p2m.c | 33 >>>>> +++++++++++++++----------- >>>>> 9 files changed, 69 insertions(+), 26 deletions(-) >>>>> >>>>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json >>>>> index e3c8a1d8eb..244001f5be 100644 >>>>> --- a/docs/misra/safe.json >>>>> +++ b/docs/misra/safe.json >>>>> @@ -12,6 +12,14 @@ >>>>> }, >>>>> { >>>>> "id": "SAF-1-safe", >>>>> + "analyser": { >>>>> + "eclair": "MC3R1.R9.1" >>>>> + }, >>>>> + "name": "Rule 9.1: initializer not needed", >>>>> + "text": "The following local variables are possibly >>>>> subject to being read before being written, but code inspection >>>>> ensured that the control flow in the construct where they appear >>>>> ensures that no such event may happen." >>>> I am bit concerned which such statement because the code instance >>>> was today with the current code. This could change in the future and >>>> invalide the reasoning. >>>> >>>> It is not clear to me if we have any mechanism to prevent that. If >>>> we don't, then I think we need to drastically reduce the number of >>>> time this is used (there are a bit too much for my taste). >>>> >>> >>> Indeed, the purpose of such a deviation is that the sound >>> overapproximation computed by the tool requires a human to look at >>> the code and think twice before modifying it (i.e., if ever that code >>> is touched, the reviewer ought to assess whether that justification >>> still holds or some other thing should be done about it. >> >> Your assumption is the reviewer will notice there is an existing >> devitation and be able to assess it has changed. I view this >> assumption as risky in the long term. >> >> Have you investigate to improve the automatic tooling? >> > > Well, as discussed elsewhere in the thread, a slightly modified version > of this deviation comment can list the specific reason why such a thing > was deviated directly at the declaration or where the caution is, if you > think this is better. > > Example: > > // <- SAF-x here > int var; > > [...] > > // <- or HERE > f(&var); > > An alternative approach to justification, partly discussed with Stefano > in private is a macro that looks like an attribute to signal that the > variable is intentionally uninitialized. This does not have the benefit > of a written justification with a proper comment or an entry in the json > file, but is less intrusive and the justification for all occurrences of > __uninit w.r.t R9.1 would be included in the static analysis tool > configuration, which would be part of the MISRA compliance > documentation. This does imply a coarse justification like the one > above, but if further clarification is needed it can be provided locally > in the code, as guidance for contributors. > > Example: > #define __uninit > > __uninit int x; > >>> >>>>> + }, >>>>> + { >>>>> + "id": "SAF-2-safe", >>>>> "analyser": {}, >>>>> "name": "Sentinel", >>>>> "text": "Next ID to be used" >>>>> diff --git a/xen/arch/arm/arm64/lib/find_next_bit.c >>>>> b/xen/arch/arm/arm64/lib/find_next_bit.c >>>>> index ca6f82277e..51b852c595 100644 >>>>> --- a/xen/arch/arm/arm64/lib/find_next_bit.c >>>>> +++ b/xen/arch/arm/arm64/lib/find_next_bit.c >>>>> @@ -67,6 +67,7 @@ unsigned long find_next_zero_bit(const unsigned >>>>> long *addr, unsigned long size, >>>>> { >>>>> const unsigned long *p = addr + BIT_WORD(offset); >>>>> unsigned long result = offset & ~(BITS_PER_LONG-1); >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> unsigned long tmp; >>>> >>>> This is a file that was copied as-is from Linux. I thought we >>>> exclude them. >>> >>> There was an expansion location in the report that caused that file >>> to appear in the output. Thanks for reporting it. >>> >>>> >>>> Anyway, can you provide some details why Eclair thinks this is may >>>> be used unitialized? >>> >>> The problem lies in the goto skipping out of control flow structures. >>> It can't be refactored with a "do { tmp=...; break; } while(0)", as >>> done elsewhere, because there are two distinct labels in there, so >>> the semantics would be altered. The same comment applies in other >>> instances as well. >>> >>>> >>>>> if (offset >= size) >>>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c >>>>> index 2673ad17a1..1292a64e8d 100644 >>>>> --- a/xen/arch/arm/bootfdt.c >>>>> +++ b/xen/arch/arm/bootfdt.c >>>>> @@ -34,6 +34,7 @@ static bool __init device_tree_node_matches(const >>>>> void *fdt, int node, >>>>> static bool __init device_tree_node_compatible(const void *fdt, >>>>> int node, >>>>> const char *match) >>>>> { >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> int len, l; >>>> >>>> It is unclear to me whether the SAF-* applies to 'len' or 'l'. >>> >>> Just len, they need to be two separate declarations if it is agreed >>> not to initialize it. Good catch. >>> >>>> >>>>> const void *prop; >>>>> @@ -169,7 +170,9 @@ int __init device_tree_for_each_node(const void >>>>> *fdt, int node, >>>>> */ >>>>> int depth = 0; >>>>> const int first_node = node; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> u32 address_cells[DEVICE_TREE_MAX_DEPTH]; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> u32 size_cells[DEVICE_TREE_MAX_DEPTH]; >>>> >>>> I have never sent formally, but in the past I proposed to rework the >>>> code so {address, size}_cells[0] would be initialized before the >>>> loop. See >>>> https://lore.kernel.org/all/ea06f2ac-4ac1-4a6f-bda6-e775a7e68699@xen.org/. >>>> >>>> Do you think this would help? >>> >>> If that initialization is applied to all the array elements yes, >>> because the Amplification of R9.1 says: >>> "For the purposes of this rule, an array element or structure member >>> shall be considered as a discrete >>> object". >>> Otherwise, initializing just the first element does not improve the >>> situation in any way, and this is currently not within of the >>> analysis capabilities of ECLAIR. >> >> Any plan to improve ECLAIR? >> > > Not in the short term on this. See further below. > >>> >>>> >>>>> int ret; >>>>> @@ -249,8 +252,10 @@ static void __init >>>>> process_multiboot_node(const void *fdt, int node, >>>>> const __be32 *cell; >>>>> bootmodule_kind kind; >>>>> paddr_t start, size; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> int len; >>>>> /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + >>>>> '/0' => 92 */ >>>>> + /* SAF-1-safe MC3R1.R9.1*/ >>>>> char path[92]; >>>> >>>> So the two above, is one category of issue. The variables are passed >>>> as argument of function which will fill them. >>>> >>>> Can Eclair look at the callers, if so, can we consider to always >>>> initialize the values in the callee? >>>> >>>> This would reduce the number of SAF-*. There are a few other >>>> examples like that below. So I will skip them for now. >>>> >>>> [...] >>>> >>> >>> If the value is always initialized in the callee, then there's no >>> problem configuring ECLAIR so that it knows that this parameter is >>> always written, and therefore any subsequent use in the caller is ok. >>> >>> Another possibility is stating that a function never reads the >>> pointee before writing to it (it may or may not write it, but if it >>> doesn't, then the pointee is not read either). The 'strncmp' after >>> 'fdt_get_path' does get in the way, though, because this property is >>> not strong enough to ensure that we can use 'path' after returning >>> from the function. >> >> I am not sure I fully understand what you wrote. Can you provide a C >> example? >> > > void f(int *x) { > if(x) { > *x = 10; > int y =*x; // read the pointee after it's initialized > } else { > int z; // in this branch the pointee is not read nor written > } > // we can say that f never reads *x before (possibly) writing to it. > } > >>> >>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>>>> index d0d6be922d..d43f86c2f0 100644 >>>>> --- a/xen/arch/arm/domain_build.c >>>>> +++ b/xen/arch/arm/domain_build.c >>>>> @@ -62,7 +62,7 @@ custom_param("dom0_mem", parse_dom0_mem); >>>>> int __init parse_arch_dom0_param(const char *s, const char *e) >>>>> { >>>>> - long long val; >>>>> + long long val = LLONG_MAX; >>>> >>>> Can you explain why you decided to initialize rather than SAF-? For >>>> clarity, I am not asking to switch to SAF-, I am mainly interested >>>> what were your though process. >>>> >>> >>> I saw that putting a large enough value here would be out of range >>> and trigger the printk even if the conditional leads to somehow enter >>> the if branch with an invalid value. >> >> Ok. This wants to be documented on top of the assignment. >> > > Ok. While testing this I also switched to LONG_MAX, whose definition is > already available in that file, and does not alter the semantics anyway. > >>> >>>>> if ( !parse_signed_integer("sve", s, e, &val) ) >>>>> { >>>>> @@ -1077,6 +1077,7 @@ static void __init >>>>> assign_static_memory_11(struct domain *d, >>>>> static int __init handle_linux_pci_domain(struct kernel_info *kinfo, >>>>> const struct >>>>> dt_device_node *node) >>>>> { >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> uint16_t segment; >>>>> int res; >>>>> @@ -1351,6 +1352,7 @@ static int __init make_memory_node(const >>>>> struct domain *d, >>>>> unsigned int i; >>>>> int res, reg_size = addrcells + sizecells; >>>>> int nr_cells = 0; >>>>> + /* SAF-1-safe MC3R1.R9.1*/ >>>>> __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + >>>>> sizecells */]; >>>>> __be32 *cells; >>>>> @@ -1578,6 +1580,7 @@ static int __init >>>>> find_unallocated_memory(const struct kernel_info *kinfo, >>>>> struct rangeset *unalloc_mem; >>>>> paddr_t start, end; >>>>> unsigned int i; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> int res; >>>>> dt_dprintk("Find unallocated memory for extended regions\n"); >>>>> @@ -1727,6 +1730,7 @@ static int __init find_memory_holes(const >>>>> struct kernel_info *kinfo, >>>>> dt_for_each_device_node( dt_host, np ) >>>>> { >>>>> unsigned int naddr; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> paddr_t addr, size; >>>>> naddr = dt_number_of_address(np); >>>>> @@ -1976,9 +1980,11 @@ static int __init make_cpus_node(const >>>>> struct domain *d, void *fdt) >>>>> const struct dt_device_node *npcpu; >>>>> unsigned int cpu; >>>>> const void *compatible = NULL; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> u32 len; >>>>> /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */ >>>>> char buf[13]; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> u32 clock_frequency; >>>>> /* Keep the compiler happy with -Og */ >>>>> bool clock_valid = false; >>>>> @@ -2104,6 +2110,7 @@ static int __init make_gic_node(const struct >>>>> domain *d, void *fdt, >>>>> const struct dt_device_node *gic = dt_interrupt_controller; >>>>> int res = 0; >>>>> const void *addrcells, *sizecells; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> u32 addrcells_len, sizecells_len; >>>>> /* >>>>> @@ -2179,6 +2186,7 @@ static int __init make_timer_node(const >>>>> struct kernel_info *kinfo) >>>>> int res; >>>>> unsigned int irq[MAX_TIMER_PPI]; >>>>> gic_interrupt_t intrs[3]; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> u32 clock_frequency; >>>>> bool clock_valid; >>>>> @@ -2511,6 +2519,7 @@ static int __init handle_device(struct domain >>>>> *d, struct dt_device_node *dev, >>>>> unsigned int naddr; >>>>> unsigned int i; >>>>> int res; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> paddr_t addr, size; >>>>> bool own_device = !dt_device_for_passthrough(dev); >>>>> /* >>>>> @@ -2779,6 +2788,7 @@ static int __init make_gicv2_domU_node(struct >>>>> kernel_info *kinfo) >>>>> { >>>>> void *fdt = kinfo->fdt; >>>>> int res = 0; >>>>> + /* SAF-1-safe MC3R1.R9.1*/ >>>>> __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) >>>>> * 2]; >>>>> __be32 *cells; >>>>> const struct domain *d = kinfo->d; >>>>> @@ -2914,6 +2924,7 @@ static int __init >>>>> make_vpl011_uart_node(struct kernel_info *kinfo) >>>>> void *fdt = kinfo->fdt; >>>>> int res; >>>>> gic_interrupt_t intr; >>>>> + /* SAF-1-safe MC3R1.R9.1*/ >>>>> __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; >>>>> __be32 *cells; >>>>> struct domain *d = kinfo->d; >>>>> @@ -3435,6 +3446,7 @@ static void __init initrd_load(struct >>>>> kernel_info *kinfo) >>>>> paddr_t paddr, len; >>>>> int node; >>>>> int res; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> __be32 val[2]; >>>>> __be32 *cellp; >>>>> void __iomem *initrd; >>>>> @@ -3514,6 +3526,7 @@ static int __init >>>>> get_evtchn_dt_property(const struct dt_device_node *np, >>>>> uint32_t *port, uint32_t >>>>> *phandle) >>>>> { >>>>> const __be32 *prop = NULL; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> uint32_t len; >>>>> prop = dt_get_property(np, "xen,evtchn", &len); >>>>> @@ -3538,10 +3551,13 @@ static int __init >>>>> get_evtchn_dt_property(const struct dt_device_node *np, >>>>> static int __init alloc_domain_evtchn(struct dt_device_node *node) >>>>> { >>>>> int rc; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> uint32_t domU1_port, domU2_port, remote_phandle; >>>>> struct dt_device_node *remote_node; >>>>> const struct dt_device_node *p1_node, *p2_node; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> struct evtchn_alloc_unbound alloc_unbound; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> struct evtchn_bind_interdomain bind_interdomain; >>>>> struct domain *d1 = NULL, *d2 = NULL; >>>>> @@ -3789,11 +3805,12 @@ static int __init construct_domain(struct >>>>> domain *d, struct kernel_info *kinfo) >>>>> static int __init alloc_xenstore_evtchn(struct domain *d) >>>>> { >>>>> - evtchn_alloc_unbound_t alloc; >>>>> + evtchn_alloc_unbound_t alloc = { >>>>> + .dom = d->domain_id, >>>>> + .remote_dom = hardware_domain->domain_id >>>>> + }; >>>>> int rc; >>>>> - alloc.dom = d->domain_id; >>>>> - alloc.remote_dom = hardware_domain->domain_id; >>>> >>>> It is not clear why this is modified. There are only two fields in >>>> 'alloc' and they are both initialized. >>>> >>> >>> There are three fields in the struct, and the third is clearly to be >>> set by an initalization function. If, as above with 'path', the init >>> function ensures that the third field is always written then there's >>> no problem, otherwise this initialization prevents any use of an >>> uninitialized port. >>> > struct evtchn_alloc_unbound { >>> /* IN parameters */ >>> domid_t dom, remote_dom; >>> /* OUT parameters */ >>> evtchn_port_t port; >>> }; >>> typedef struct evtchn_alloc_unbound evtchn_alloc_unbound_t; >> >> Ah, I didn't spot the third field because it was defined on the same >> line. I would prefer if the value is explicitely initialized to 0. >> >> Also, I think this ought to be in a separate patch (with other similar >> pattern). >> > > Ok. > >>>>> rc = evtchn_alloc_unbound(&alloc, 0); >>>>> if ( rc ) >>>>> { >>>>> @@ -3810,8 +3827,9 @@ static int __init construct_domU(struct >>>>> domain *d, >>>>> const struct dt_device_node *node) >>>>> { >>>>> struct kernel_info kinfo = {}; >>>>> - const char *dom0less_enhanced; >>>>> + const char *dom0less_enhanced = NULL; >>>> >>>> If you look at the user below, all the callers assume >>>> dom0less_enhanced will be non-NULL. So it is unclear to me how this >>>> value is safer. >>>> > Looking at the code, I wonder whether we should convert >>>> dt_property_read_string() to use ERR_PTR(). So we could remove the >>>> last argument and return it instead. >>> >>> Is relying on that assumption somehow safer? >> >> I am assuming you are referring to "If you look at the user below, all >> the callers assume dom0less_enhanced will be non-NULL". Note that I >> didn't suggest it is safer. I am only pointed out that you didn't >> specify how this was better in the context of the code. >> > > This should be probably discussed after deciding on the refactoring > 'dt_property_read_string' > >>> The suggestion to remove the last parameter seems better. >>> >>>> >>>>> int rc; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> u64 mem; >>>>> u32 p2m_mem_mb; >>>>> unsigned long p2m_pages; >>>>> @@ -3939,6 +3957,7 @@ void __init create_domUs(void) >>>>> .grant_opts = >>>>> XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), >>>>> }; >>>>> unsigned int flags = 0U; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> uint32_t val; >>>>> int rc; >>>>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h >>>>> index bb64925d70..25f39364d1 100644 >>>>> --- a/xen/arch/arm/efi/efi-boot.h >>>>> +++ b/xen/arch/arm/efi/efi-boot.h >>>>> @@ -117,6 +117,7 @@ static int __init setup_chosen_node(void *fdt, >>>>> int *addr_cells, int *size_cells) >>>>> static int __init fdt_set_reg(void *fdt, int node, int addr_cells, >>>>> int size_cells, uint64_t addr, >>>>> uint64_t len) >>>>> { >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> __be32 val[4]; /* At most 2 64 bit values to be stored */ >>>>> __be32 *cellp; >>>>> @@ -308,7 +309,7 @@ fdt_set_fail: >>>>> static void __init *fdt_increase_size(struct file *fdtfile, int >>>>> add_size) >>>>> { >>>>> EFI_STATUS status; >>>>> - EFI_PHYSICAL_ADDRESS fdt_addr; >>>>> + EFI_PHYSICAL_ADDRESS fdt_addr = 0; >>>>> int fdt_size; >>>>> int pages; >>>>> void *new_fdt; >>>>> @@ -433,7 +434,7 @@ static void __init efi_arch_cfg_file_late(const >>>>> EFI_LOADED_IMAGE *image, >>>>> static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) >>>>> { >>>>> - void *ptr; >>>>> + void *ptr = NULL; >>>>> EFI_STATUS status; >>>>> status = efi_bs->AllocatePool(EfiLoaderData, map_size, &ptr); >>>>> @@ -538,6 +539,7 @@ static void __init efi_arch_handle_module(const >>>>> struct file *file, >>>>> { >>>>> int node; >>>>> int chosen; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> int addr_len, size_len; >>>>> if ( file == &dtbfile ) >>>>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >>>>> index 3aa4edda10..aa0180ab5b 100644 >>>>> --- a/xen/arch/arm/gic-v3-its.c >>>>> +++ b/xen/arch/arm/gic-v3-its.c >>>>> @@ -192,8 +192,7 @@ static int its_send_cmd_mapc(struct host_its >>>>> *its, uint32_t collection_id, >>>>> cmd[0] = GITS_CMD_MAPC; >>>>> cmd[1] = 0x00; >>>>> - cmd[2] = encode_rdbase(its, cpu, collection_id); >>>>> - cmd[2] |= GITS_VALID_BIT; >>>>> + cmd[2] = encode_rdbase(its, cpu, collection_id) | GITS_VALID_BIT; >>>> >>>> Hmmm... How is this even considered as unitialized variable? >>>> >>> >>> The analysis here could use some more precision, but the modified >>> construct is entirely equivalent. >> >> I agree that they are equivalent. But in general, we don't change the >> style of the construct without explaining why. >> >> In this case, the first step would be to improve Eclair. >> > > The changes needed for this kind of analysis are not trivial: we've > looked into this, but there's no easy way to support this in a timely > manner. I understand that this is an estabilished pattern, but what > would you think of an initializer using designators? > > uint64_t cmd[4] = { > .[0] = GITS_CMD_MAPC; > .[1] = 0x00; > .[2] = encode_rdbase(its, cpu, collection_id) | GITS_VALID_BIT; > .[3] = 0x00; > } > >>> >>>>> cmd[3] = 0x00; >>>>> return its_send_command(its, cmd); >>>>> @@ -215,9 +214,7 @@ static int its_send_cmd_mapd(struct host_its >>>>> *its, uint32_t deviceid, >>>>> } >>>>> cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32); >>>>> cmd[1] = size_bits; >>>>> - cmd[2] = itt_addr; >>>>> - if ( valid ) >>>>> - cmd[2] |= GITS_VALID_BIT; >>>>> + cmd[2] = itt_addr | (valid ? GITS_VALID_BIT : 0x00); >>>> >>>> Same here. >>>> >>>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >>>>> index c688227abd..a36068b2d8 100644 >>>>> --- a/xen/arch/arm/mm.c >>>>> +++ b/xen/arch/arm/mm.c >>>>> @@ -935,6 +935,7 @@ static int xen_pt_update_entry(mfn_t root, >>>>> unsigned long virt, >>>>> mfn_t mfn, unsigned int target, >>>>> unsigned int flags) >>>>> { >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> int rc; >>>>> unsigned int level; >>>>> lpae_t *table; >>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>>>> index de32a2d638..83c56cf1cb 100644 >>>>> --- a/xen/arch/arm/p2m.c >>>>> +++ b/xen/arch/arm/p2m.c >>>>> @@ -496,16 +496,18 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, >>>>> gfn_t gfn, >>>>> lpae_t entry, *table; >>>>> int rc; >>>>> mfn_t mfn = INVALID_MFN; >>>>> - p2m_type_t _t; >>>>> + p2m_type_t _t = p2m_invalid; >>>>> DECLARE_OFFSETS(offsets, addr); >>>>> ASSERT(p2m_is_locked(p2m)); >>>>> BUILD_BUG_ON(THIRD_MASK != PAGE_MASK); >>>>> /* Allow t to be NULL */ >>>>> - t = t ?: &_t; >>>>> - >>>>> - *t = p2m_invalid; >>>>> + if( t ) { >>>>> + *t = _t; >>>>> + } else { >>>>> + t = &_t; >>>>> + } >>>> What was the problem with the previous code? >>>> >>>> This is also not conformant to Xen coding style. >>>> >>> >>> The problem is that _t may be uninitialized, hence assigning its >>> address to t could be problematic. >> >> But the value is set right after. IOW, there is no read between. So >> how is this prob >> >>> Another way to address this is to initialize _t to a bad value and >>> use this variable in the body, then assign to t based on the value >>> just before returning. >> >> IHMO, neither solution are ideal. I think we should investigate >> whether Eclair can be improved. >> >> [...] >> > > I'll see what can be done about it, I'll reply when I have an answer. > What about this: - p2m_type_t _t; + p2m_type_t _t = p2m_invalid; [...] t = t ?: &_t; - *t = p2m_invalid; + *t = _t; >>>>> if ( valid ) >>>>> *valid = false; >>>>> @@ -1031,6 +1033,7 @@ static int __p2m_set_entry(struct p2m_domain >>>>> *p2m, >>>>> unsigned int level = 0; >>>>> unsigned int target = 3 - (page_order / XEN_PT_LPAE_SHIFT); >>>>> lpae_t *entry, *table, orig_pte; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> int rc; >>>> >>>> Can you provide some details why Eclair thinks it is unitialized? >>> >>> Same issue with gotos explained above, can't be refactored because of >>> the for enclosing the goto. >>> >>>> >>>>> /* A mapping is removed if the MFN is invalid. */ >>>>> bool removing_mapping = mfn_eq(smfn, INVALID_MFN); >>>>> @@ -1483,6 +1486,7 @@ static inline int p2m_remove_mapping(struct >>>>> domain *d, >>>>> { >>>>> struct p2m_domain *p2m = p2m_get_hostp2m(d); >>>>> unsigned long i; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> int rc; >>>> >>>> >>>> Can you provide some details why Eclair thinks it is unitialized? >>>> >>> >>> Same as above. >>> >>>>> p2m_write_lock(p2m); >>>>> @@ -1685,20 +1689,21 @@ static int p2m_alloc_vmid(struct domain *d) >>>>> ASSERT(nr != INVALID_VMID); >>>>> - if ( nr == MAX_VMID ) >>>>> - { >>>>> - rc = -EBUSY; >>>>> - printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", >>>>> d->domain_id); >>>>> - goto out; >>>>> - } >>>>> + do { >>>> >>>> I don't understand this change. How is this making better for Eclair? >>>> >>> >>> This is an example where the goto can be eliminated, which in turn >>> allows to automatically check the correctness. >> If you want to eliminate the 'goto' then they are better way to do it. >> Like: >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index bc9c3ae25693..8771679dd5fc 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -186,16 +186,14 @@ int p2m_alloc_vmid(struct domain *d) >> { >> rc = -EBUSY; >> printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", >> d->domain_id); >> - goto out; >> + } >> + else >> + { >> + set_bit(nr, vmid_mask); >> + p2m->vmid = nr; >> + rc = 0; >> } >> >> - set_bit(nr, vmid_mask); >> - >> - p2m->vmid = nr; >> - >> - rc = 0; >> - >> -out: >> spin_unlock(&vmid_alloc_lock); >> return rc; >> } >> >> I have a slight preference with the goto version, but I could accept >> it if Eclair can't cope with the construct. In any case, this is the >> sort of change that deserve its own patch as you want to explain why >> Eclair can't cope with such construct (I don't view it as complex). >> > > ok > >>> >>>>> + if ( nr == MAX_VMID ) >>>>> + { >>>>> + rc = -EBUSY; >>>>> + printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", >>>>> d->domain_id); >>>>> + break; >>>>> + } >>>>> - set_bit(nr, vmid_mask); >>>>> + set_bit(nr, vmid_mask); >>>>> - p2m->vmid = nr; >>>>> + p2m->vmid = nr; >>>>> - rc = 0; >>>>> + rc = 0; >>>>> + } while ( 0 ); >>>>> -out: >>>>> spin_unlock(&vmid_alloc_lock); >>>>> return rc; >>>>> } >>>> >>> >>> >>> Considering all of the replies above, a first draft of a >>> strategy/policy I can think of is having: >>> >>> - Initializer functions that always write their parameter, so that >>> the strongest "pointee always written" property can be stated. This >>> causes all further uses to be marked safe. >>> >>> - Initialize the variable when there exists a known safe value that >>> does not alter the semantics of the function. The initialization does >>> not need to be at the declaration, but doing so simplifies the code. >> >> As I mentionned in private there are two risks with that: >> 1. You silence compiler to spot other issues >> 2. You may now get warning from Coverity if it spots you set a value >> that get overwritten before its first use. >> >> So I think such approach should be used with parcimony. Instead, we >> should look at reworking the code when possible. >> > > Do you think it would help if you look directly at actual cautions to > spot possible functions that can be refactored? > >>> >>> - Deviate all cases where any of the previous does not apply, with a >>> comment deviation that refers to a justification reporting that the >>> code has been checked to respect the rule (keep in mind that >>> _violations_ to a Mandatory rule such as R9.1 are not allowed to >>> claim MISRA compliance). >> >> See above for my concern about adding so many deviations. But I am >> confused with what you wrote. If the rule is mandatory, then why are >> you trying to add deviation in Xen? Who is going to solve them to make >> Xen MISRA compliant? >> > > Because only cautions were found, not violations (which cannot be > deviated). In the former case Xen can say that the code does not violate > the rule, but it can't be proven by the tool. > > Regards, >
Hi Nicola, On 20/07/2023 11:14, Nicola Vetrini wrote: > > > On 17/07/23 15:40, Julien Grall wrote: >> Hi Nicola, >> >> On 17/07/2023 13:08, Nicola Vetrini wrote: >>> On 14/07/23 15:00, Julien Grall wrote: >>>> Hi Nicola, >>>> >>>> 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. >>>>> >>>>> To achieve this goal, I adopted the following strategies: >>>> >>>> Please let's at least one patch per strategy. I would also consider >>>> some of the rework separate so they can go in regardless the >>>> decision for the SAF-*. >>>> >>>>> >>>>> - Add a suitably formatted local deviation comment >>>>> (as indicated in 'docs/misra/documenting-violations.rst') >>>>> to exempt the following line from checking. >>>>> >>>>> - Provide an initialization for the variable at the declaration. >>>>> >>>>> - Substitute a goto breaking out of control flow logic with a >>>>> semantically >>>>> equivalent do { .. } while(0). >>>> >>>> As I already mentioned in private, it is unclear to me how you >>>> decided which strategy to use. I still think we need to define our >>>> policy before changing the code. Otherwise, it is going to be >>>> difficult to decide for new code. >>>> >>> >>> The main point of this RFC is doing so. From what I gathered, it's >>> not an easy task: sometimes there are no 'safe' values to initialize >>> variables to and sometimes there is no easy way to prove that indeed >>> something is always initialized or not accessed at all. >> >> But you wrote the code. So you should be able to explain how you took >> the decision between one and the others. >> >> Also, even if this is an RFC, it would have been good to summarize any >> discussion that happened in private and if there were concern try to >> come up with ideas or at least listing the concerns after '---. >> > > I'll keep this if the need arises in the future. > >>> >>>>> >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>> --- >>>>> docs/misra/safe.json | 8 +++++++ >>>>> xen/arch/arm/arm64/lib/find_next_bit.c | 1 + >>>>> xen/arch/arm/bootfdt.c | 6 +++++ >>>>> xen/arch/arm/decode.c | 2 ++ >>>>> xen/arch/arm/domain_build.c | 29 ++++++++++++++++++---- >>>>> xen/arch/arm/efi/efi-boot.h | 6 +++-- >>>>> xen/arch/arm/gic-v3-its.c | 9 ++++--- >>>>> xen/arch/arm/mm.c | 1 + >>>>> xen/arch/arm/p2m.c | 33 >>>>> +++++++++++++++----------- >>>>> 9 files changed, 69 insertions(+), 26 deletions(-) >>>>> >>>>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json >>>>> index e3c8a1d8eb..244001f5be 100644 >>>>> --- a/docs/misra/safe.json >>>>> +++ b/docs/misra/safe.json >>>>> @@ -12,6 +12,14 @@ >>>>> }, >>>>> { >>>>> "id": "SAF-1-safe", >>>>> + "analyser": { >>>>> + "eclair": "MC3R1.R9.1" >>>>> + }, >>>>> + "name": "Rule 9.1: initializer not needed", >>>>> + "text": "The following local variables are possibly >>>>> subject to being read before being written, but code inspection >>>>> ensured that the control flow in the construct where they appear >>>>> ensures that no such event may happen." >>>> I am bit concerned which such statement because the code instance >>>> was today with the current code. This could change in the future and >>>> invalide the reasoning. >>>> >>>> It is not clear to me if we have any mechanism to prevent that. If >>>> we don't, then I think we need to drastically reduce the number of >>>> time this is used (there are a bit too much for my taste). >>>> >>> >>> Indeed, the purpose of such a deviation is that the sound >>> overapproximation computed by the tool requires a human to look at >>> the code and think twice before modifying it (i.e., if ever that code >>> is touched, the reviewer ought to assess whether that justification >>> still holds or some other thing should be done about it. >> >> Your assumption is the reviewer will notice there is an existing >> devitation and be able to assess it has changed. I view this >> assumption as risky in the long term. >> >> Have you investigate to improve the automatic tooling? >> > > Well, as discussed elsewhere in the thread, a slightly modified version > of this deviation comment can list the specific reason why such a thing > was deviated directly at the declaration or where the caution is, if you > think this is better. > > Example: > > // <- SAF-x here > int var; > > [...] > > // <- or HERE > f(&var); > > An alternative approach to justification, partly discussed with Stefano > in private is a macro that looks like an attribute to signal that the > variable is intentionally uninitialized. This does not have the benefit > of a written justification with a proper comment or an entry in the json > file, but is less intrusive and the justification for all occurrences of > __uninit w.r.t R9.1 would be included in the static analysis tool > configuration, which would be part of the MISRA compliance > documentation. This does imply a coarse justification like the one > above, but if further clarification is needed it can be provided locally > in the code, as guidance for contributors. > > Example: > #define __uninit > > __uninit int x; IHMO none of the example really help. You are still expecting the reviewer/developper to drop __uninit or any comment if the behavior has changed. I don't have a good idea how we can mitigate it other than reworking the code in a way that makes both ECLAIR and the maintainers happy. Maybe the others will. [...] >>>> >>>>> int ret; >>>>> @@ -249,8 +252,10 @@ static void __init >>>>> process_multiboot_node(const void *fdt, int node, >>>>> const __be32 *cell; >>>>> bootmodule_kind kind; >>>>> paddr_t start, size; >>>>> + /* SAF-1-safe MC3R1.R9.1 */ >>>>> int len; >>>>> /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + >>>>> '/0' => 92 */ >>>>> + /* SAF-1-safe MC3R1.R9.1*/ >>>>> char path[92]; >>>> >>>> So the two above, is one category of issue. The variables are passed >>>> as argument of function which will fill them. >>>> >>>> Can Eclair look at the callers, if so, can we consider to always >>>> initialize the values in the callee? >>>> >>>> This would reduce the number of SAF-*. There are a few other >>>> examples like that below. So I will skip them for now. >>>> >>>> [...] >>>> >>> >>> If the value is always initialized in the callee, then there's no >>> problem configuring ECLAIR so that it knows that this parameter is >>> always written, and therefore any subsequent use in the caller is ok. >>> >>> Another possibility is stating that a function never reads the >>> pointee before writing to it (it may or may not write it, but if it >>> doesn't, then the pointee is not read either). The 'strncmp' after >>> 'fdt_get_path' does get in the way, though, because this property is >>> not strong enough to ensure that we can use 'path' after returning >>> from the function. >> >> I am not sure I fully understand what you wrote. Can you provide a C >> example? >> > > void f(int *x) { > if(x) { > *x = 10; > int y =*x; // read the pointee after it's initialized > } else { > int z; // in this branch the pointee is not read nor written > } > // we can say that f never reads *x before (possibly) writing to it. > } I am having trouble to understand it in the context of fdt_get_path(). Is 'f' meant to be fdt_get_path()? [...] >>>>> rc = evtchn_alloc_unbound(&alloc, 0); >>>>> if ( rc ) >>>>> { >>>>> @@ -3810,8 +3827,9 @@ static int __init construct_domU(struct >>>>> domain *d, >>>>> const struct dt_device_node *node) >>>>> { >>>>> struct kernel_info kinfo = {}; >>>>> - const char *dom0less_enhanced; >>>>> + const char *dom0less_enhanced = NULL; >>>> >>>> If you look at the user below, all the callers assume >>>> dom0less_enhanced will be non-NULL. So it is unclear to me how this >>>> value is safer. >>>> > Looking at the code, I wonder whether we should convert >>>> dt_property_read_string() to use ERR_PTR(). So we could remove the >>>> last argument and return it instead. >>> >>> Is relying on that assumption somehow safer? >> >> I am assuming you are referring to "If you look at the user below, all >> the callers assume dom0less_enhanced will be non-NULL". Note that I >> didn't suggest it is safer. I am only pointed out that you didn't >> specify how this was better in the context of the code. >> > > This should be probably discussed after deciding on the refactoring > 'dt_property_read_string' FAOD, I think we should refactor dt_property_read_string(). I am happy to write a patch if you want. [...] >>> The analysis here could use some more precision, but the modified >>> construct is entirely equivalent. >> >> I agree that they are equivalent. But in general, we don't change the >> style of the construct without explaining why. >> >> In this case, the first step would be to improve Eclair. >> > > The changes needed for this kind of analysis are not trivial: we've > looked into this, but there's no easy way to support this in a timely > manner. I understand that this is an estabilished pattern, but what > would you think of an initializer using designators? > > uint64_t cmd[4] = { > .[0] = GITS_CMD_MAPC; > .[1] = 0x00; > .[2] = encode_rdbase(its, cpu, collection_id) | GITS_VALID_BIT; > .[3] = 0x00; > } The reability is Ok here. But this may not be the case here. In particular... > >>> >>>>> cmd[3] = 0x00; >>>>> return its_send_command(its, cmd); >>>>> @@ -215,9 +214,7 @@ static int its_send_cmd_mapd(struct host_its >>>>> *its, uint32_t deviceid, >>>>> } >>>>> cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32); >>>>> cmd[1] = size_bits; >>>>> - cmd[2] = itt_addr; >>>>> - if ( valid ) >>>>> - cmd[2] |= GITS_VALID_BIT; >>>>> + cmd[2] = itt_addr | (valid ? GITS_VALID_BIT : 0x00); >>>> >>>> Same here. here... I much prefer the existing version. [...] >>>>> + if ( nr == MAX_VMID ) >>>>> + { >>>>> + rc = -EBUSY; >>>>> + printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", >>>>> d->domain_id); >>>>> + break; >>>>> + } >>>>> - set_bit(nr, vmid_mask); >>>>> + set_bit(nr, vmid_mask); >>>>> - p2m->vmid = nr; >>>>> + p2m->vmid = nr; >>>>> - rc = 0; >>>>> + rc = 0; >>>>> + } while ( 0 ); >>>>> -out: >>>>> spin_unlock(&vmid_alloc_lock); >>>>> return rc; >>>>> } >>>> >>> >>> >>> Considering all of the replies above, a first draft of a >>> strategy/policy I can think of is having: >>> >>> - Initializer functions that always write their parameter, so that >>> the strongest "pointee always written" property can be stated. This >>> causes all further uses to be marked safe. >>> >>> - Initialize the variable when there exists a known safe value that >>> does not alter the semantics of the function. The initialization does >>> not need to be at the declaration, but doing so simplifies the code. >> >> As I mentionned in private there are two risks with that: >> 1. You silence compiler to spot other issues >> 2. You may now get warning from Coverity if it spots you set a value >> that get overwritten before its first use. >> >> So I think such approach should be used with parcimony. Instead, we >> should look at reworking the code when possible. >> > > Do you think it would help if you look directly at actual cautions to > spot possible functions that can be refactored? I have already looked at some. Can we focus on them and see how much it helps? Cheers,
Hi, The e-mail is getting quite long. Can you trim the unnecessary bits when replying? On 20/07/2023 15:23, Nicola Vetrini wrote: >>>> The problem is that _t may be uninitialized, hence assigning its >>>> address to t could be problematic. >>> >>> But the value is set right after. IOW, there is no read between. So >>> how is this prob >>> >>>> Another way to address this is to initialize _t to a bad value and >>>> use this variable in the body, then assign to t based on the value >>>> just before returning. >>> >>> IHMO, neither solution are ideal. I think we should investigate >>> whether Eclair can be improved. >>> >>> [...] >>> >> >> I'll see what can be done about it, I'll reply when I have an answer. >> > > What about this: > > - p2m_type_t _t; > + p2m_type_t _t = p2m_invalid; > [...] > t = t ?: &_t; > - *t = p2m_invalid; > + *t = _t; The resulting code is still quite confusing. I am still not quite sure why ECLAIR can't understand the construct. Apologies if this was already said, but this thread is getting quite long with many different issues. So it is a bit difficult to navigate (which is why I suggested to split and have a commit message explaining the rationale for each). Anyway, if we can't improve Eclair, then my preference would be the following: diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index de32a2d638ba..05d65db01b0c 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -496,16 +496,13 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn, lpae_t entry, *table; int rc; mfn_t mfn = INVALID_MFN; - p2m_type_t _t; DECLARE_OFFSETS(offsets, addr); ASSERT(p2m_is_locked(p2m)); BUILD_BUG_ON(THIRD_MASK != PAGE_MASK); - /* Allow t to be NULL */ - t = t ?: &_t; - - *t = p2m_invalid; + if ( t ) + *t = p2m_invalid; if ( valid ) *valid = false; @@ -549,7 +546,8 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn, if ( p2m_is_valid(entry) ) { - *t = entry.p2m.type; + if ( t ) + *t = entry.p2m.type; if ( a ) *a = p2m_mem_access_radix_get(p2m, gfn); Cheers,
On 20/07/23 17:39, Julien Grall wrote: > Hi, > > The e-mail is getting quite long. Can you trim the unnecessary bits when > replying? > Ok. > On 20/07/2023 15:23, Nicola Vetrini wrote: >>>>> The problem is that _t may be uninitialized, hence assigning its >>>>> address to t could be problematic. >>>> >>>> But the value is set right after. IOW, there is no read between. So >>>> how is this prob >>>> >>>>> Another way to address this is to initialize _t to a bad value and >>>>> use this variable in the body, then assign to t based on the value >>>>> just before returning. >>>> >>>> IHMO, neither solution are ideal. I think we should investigate >>>> whether Eclair can be improved. >>>> >>>> [...] >>>> >>> >>> I'll see what can be done about it, I'll reply when I have an answer. >>> >> >> What about this: >> >> - p2m_type_t _t; >> + p2m_type_t _t = p2m_invalid; >> [...] >> t = t ?: &_t; >> - *t = p2m_invalid; >> + *t = _t; > > The resulting code is still quite confusing. I am still not quite sure > why ECLAIR can't understand the construct. Apologies if this was already > said, but this thread is getting quite long with many different issues. > So it is a bit difficult to navigate (which is why I suggested to split > and have a commit message explaining the rationale for each). > > Anyway, if we can't improve Eclair, then my preference would be the > following: > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index de32a2d638ba..05d65db01b0c 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -496,16 +496,13 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t > gfn, > lpae_t entry, *table; > int rc; > mfn_t mfn = INVALID_MFN; > - p2m_type_t _t; > DECLARE_OFFSETS(offsets, addr); > > ASSERT(p2m_is_locked(p2m)); > BUILD_BUG_ON(THIRD_MASK != PAGE_MASK); > > - /* Allow t to be NULL */ > - t = t ?: &_t; > - > - *t = p2m_invalid; > + if ( t ) > + *t = p2m_invalid; > > if ( valid ) > *valid = false; > @@ -549,7 +546,8 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn, > > if ( p2m_is_valid(entry) ) > { > - *t = entry.p2m.type; > + if ( t ) > + *t = entry.p2m.type; > > if ( a ) > *a = p2m_mem_access_radix_get(p2m, gfn); > Ok, I'll make a separate patch.
>>>> If the value is always initialized in the callee, then there's no >>>> problem configuring ECLAIR so that it knows that this parameter is >>>> always written, and therefore any subsequent use in the caller is ok. >>>> >>>> Another possibility is stating that a function never reads the >>>> pointee before writing to it (it may or may not write it, but if it >>>> doesn't, then the pointee is not read either). The 'strncmp' after >>>> 'fdt_get_path' does get in the way, though, because this property is >>>> not strong enough to ensure that we can use 'path' after returning >>>> from the function. >>> >>> I am not sure I fully understand what you wrote. Can you provide a C >>> example? >>> >> >> void f(int *x) { >> if(x) { >> *x = 10; >> int y =*x; // read the pointee after it's initialized >> } else { >> int z; // in this branch the pointee is not read nor written >> } >> // we can say that f never reads *x before (possibly) writing to it. >> } > > I am having trouble to understand it in the context of fdt_get_path(). > Is 'f' meant to be fdt_get_path()? > Yes, exactly. The point is that 'fdt_get_path' surely ensures not to read uninitialized addresses from the path array, therefore if the strcmp can be somehow incorporated in a function or macro e.g. int fdt_compare_path(fdt, node, path, str) { /* Check that the node is under "/chosen" (first 7 chars of path) */ ret = fdt_get_path(fdt, node, path, sizeof (path)); if ( ret != 0 || strncmp(path, "/chosen", 7) ) return ret; return 0; } called in bootfdt as fdt_compare_path(fdt, node, path, "/chosen"); then 'fdt_compare_path' has the, shall we say, "no read before write" property and because path isn't used anywhere else in 'process_multiboot_node' that is enough to make ECLAIR happy. >> >> This should be probably discussed after deciding on the refactoring >> 'dt_property_read_string' > > FAOD, I think we should refactor dt_property_read_string(). I am happy > to write a patch if you want. > That would be perfect, I'll ll test it when I see it, so that I can give you feedback on that patch directly. > >>>> The analysis here could use some more precision, but the modified >>>> construct is entirely equivalent. >>> >>> I agree that they are equivalent. But in general, we don't change the >>> style of the construct without explaining why. >>> >>> In this case, the first step would be to improve Eclair. >>> >> >> The changes needed for this kind of analysis are not trivial: we've >> looked into this, but there's no easy way to support this in a timely >> manner. I understand that this is an estabilished pattern, but what >> would you think of an initializer using designators? >> >> uint64_t cmd[4] = { >> .[0] = GITS_CMD_MAPC; >> .[1] = 0x00; >> .[2] = encode_rdbase(its, cpu, collection_id) | GITS_VALID_BIT; >> .[3] = 0x00; >> } > > The reability is Ok here. But this may not be the case here. In > particular... > >> >>>> >>>>>> cmd[3] = 0x00; >>>>>> return its_send_command(its, cmd); >>>>>> @@ -215,9 +214,7 @@ static int its_send_cmd_mapd(struct host_its >>>>>> *its, uint32_t deviceid, >>>>>> } >>>>>> cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32); >>>>>> cmd[1] = size_bits; >>>>>> - cmd[2] = itt_addr; >>>>>> - if ( valid ) >>>>>> - cmd[2] |= GITS_VALID_BIT; >>>>>> + cmd[2] = itt_addr | (valid ? GITS_VALID_BIT : 0x00); >>>>> >>>>> Same here. > > here... I much prefer the existing version. > Well, that if can be kept as well. Like this: cmd = { .[0] = .., .[2] = itt_addr, ... }; if(valid) cmd[2] |= GITS_VALID_BIT; >>>> >>>> Considering all of the replies above, a first draft of a >>>> strategy/policy I can think of is having: >>>> >>>> - Initializer functions that always write their parameter, so that >>>> the strongest "pointee always written" property can be stated. This >>>> causes all further uses to be marked safe. >>>> >>>> - Initialize the variable when there exists a known safe value that >>>> does not alter the semantics of the function. The initialization >>>> does not need to be at the declaration, but doing so simplifies the >>>> code. >>> >>> As I mentionned in private there are two risks with that: >>> 1. You silence compiler to spot other issues >>> 2. You may now get warning from Coverity if it spots you set a >>> value that get overwritten before its first use. >>> >>> So I think such approach should be used with parcimony. Instead, we >>> should look at reworking the code when possible. >>> >> >> Do you think it would help if you look directly at actual cautions to >> spot possible functions that can be refactored? > > I have already looked at some. Can we focus on them and see how much it > helps? > Yes. It would reduce the noise for me too Regards,
diff --git a/docs/misra/safe.json b/docs/misra/safe.json index e3c8a1d8eb..244001f5be 100644 --- a/docs/misra/safe.json +++ b/docs/misra/safe.json @@ -12,6 +12,14 @@ }, { "id": "SAF-1-safe", + "analyser": { + "eclair": "MC3R1.R9.1" + }, + "name": "Rule 9.1: initializer not needed", + "text": "The following local variables are possibly subject to being read before being written, but code inspection ensured that the control flow in the construct where they appear ensures that no such event may happen." + }, + { + "id": "SAF-2-safe", "analyser": {}, "name": "Sentinel", "text": "Next ID to be used" diff --git a/xen/arch/arm/arm64/lib/find_next_bit.c b/xen/arch/arm/arm64/lib/find_next_bit.c index ca6f82277e..51b852c595 100644 --- a/xen/arch/arm/arm64/lib/find_next_bit.c +++ b/xen/arch/arm/arm64/lib/find_next_bit.c @@ -67,6 +67,7 @@ unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size, { const unsigned long *p = addr + BIT_WORD(offset); unsigned long result = offset & ~(BITS_PER_LONG-1); + /* SAF-1-safe MC3R1.R9.1 */ unsigned long tmp; if (offset >= size) diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 2673ad17a1..1292a64e8d 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -34,6 +34,7 @@ static bool __init device_tree_node_matches(const void *fdt, int node, static bool __init device_tree_node_compatible(const void *fdt, int node, const char *match) { + /* SAF-1-safe MC3R1.R9.1 */ int len, l; const void *prop; @@ -169,7 +170,9 @@ int __init device_tree_for_each_node(const void *fdt, int node, */ int depth = 0; const int first_node = node; + /* SAF-1-safe MC3R1.R9.1 */ u32 address_cells[DEVICE_TREE_MAX_DEPTH]; + /* SAF-1-safe MC3R1.R9.1 */ u32 size_cells[DEVICE_TREE_MAX_DEPTH]; int ret; @@ -249,8 +252,10 @@ static void __init process_multiboot_node(const void *fdt, int node, const __be32 *cell; bootmodule_kind kind; paddr_t start, size; + /* SAF-1-safe MC3R1.R9.1 */ int len; /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + '/0' => 92 */ + /* SAF-1-safe MC3R1.R9.1*/ char path[92]; int parent_node, ret; bool domU; @@ -326,6 +331,7 @@ static int __init process_chosen_node(const void *fdt, int node, { const struct fdt_property *prop; paddr_t start, end; + /* SAF-1-safe MC3R1.R9.1 */ int len; if ( fdt_get_property(fdt, node, "xen,static-heap", NULL) ) diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c index 2537dbebc1..b3ea504356 100644 --- a/xen/arch/arm/decode.c +++ b/xen/arch/arm/decode.c @@ -27,6 +27,7 @@ static void update_dabt(struct hsr_dabt *dabt, int reg, static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1) { + /* SAF-1-safe MC3R1.R9.1 */ uint16_t hw2; uint16_t rt; @@ -151,6 +152,7 @@ static int decode_arm64(register_t pc, mmio_info_t *info) static int decode_thumb(register_t pc, struct hsr_dabt *dabt) { + /* SAF-1-safe MC3R1.R9.1 */ uint16_t instr; if ( raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr)) ) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index d0d6be922d..d43f86c2f0 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -62,7 +62,7 @@ custom_param("dom0_mem", parse_dom0_mem); int __init parse_arch_dom0_param(const char *s, const char *e) { - long long val; + long long val = LLONG_MAX; if ( !parse_signed_integer("sve", s, e, &val) ) { @@ -1077,6 +1077,7 @@ static void __init assign_static_memory_11(struct domain *d, static int __init handle_linux_pci_domain(struct kernel_info *kinfo, const struct dt_device_node *node) { + /* SAF-1-safe MC3R1.R9.1 */ uint16_t segment; int res; @@ -1351,6 +1352,7 @@ static int __init make_memory_node(const struct domain *d, unsigned int i; int res, reg_size = addrcells + sizecells; int nr_cells = 0; + /* SAF-1-safe MC3R1.R9.1*/ __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */]; __be32 *cells; @@ -1578,6 +1580,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, struct rangeset *unalloc_mem; paddr_t start, end; unsigned int i; + /* SAF-1-safe MC3R1.R9.1 */ int res; dt_dprintk("Find unallocated memory for extended regions\n"); @@ -1727,6 +1730,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, dt_for_each_device_node( dt_host, np ) { unsigned int naddr; + /* SAF-1-safe MC3R1.R9.1 */ paddr_t addr, size; naddr = dt_number_of_address(np); @@ -1976,9 +1980,11 @@ static int __init make_cpus_node(const struct domain *d, void *fdt) const struct dt_device_node *npcpu; unsigned int cpu; const void *compatible = NULL; + /* SAF-1-safe MC3R1.R9.1 */ u32 len; /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */ char buf[13]; + /* SAF-1-safe MC3R1.R9.1 */ u32 clock_frequency; /* Keep the compiler happy with -Og */ bool clock_valid = false; @@ -2104,6 +2110,7 @@ static int __init make_gic_node(const struct domain *d, void *fdt, const struct dt_device_node *gic = dt_interrupt_controller; int res = 0; const void *addrcells, *sizecells; + /* SAF-1-safe MC3R1.R9.1 */ u32 addrcells_len, sizecells_len; /* @@ -2179,6 +2186,7 @@ static int __init make_timer_node(const struct kernel_info *kinfo) int res; unsigned int irq[MAX_TIMER_PPI]; gic_interrupt_t intrs[3]; + /* SAF-1-safe MC3R1.R9.1 */ u32 clock_frequency; bool clock_valid; @@ -2511,6 +2519,7 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev, unsigned int naddr; unsigned int i; int res; + /* SAF-1-safe MC3R1.R9.1 */ paddr_t addr, size; bool own_device = !dt_device_for_passthrough(dev); /* @@ -2779,6 +2788,7 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) { void *fdt = kinfo->fdt; int res = 0; + /* SAF-1-safe MC3R1.R9.1*/ __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; __be32 *cells; const struct domain *d = kinfo->d; @@ -2914,6 +2924,7 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo) void *fdt = kinfo->fdt; int res; gic_interrupt_t intr; + /* SAF-1-safe MC3R1.R9.1*/ __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; __be32 *cells; struct domain *d = kinfo->d; @@ -3435,6 +3446,7 @@ static void __init initrd_load(struct kernel_info *kinfo) paddr_t paddr, len; int node; int res; + /* SAF-1-safe MC3R1.R9.1 */ __be32 val[2]; __be32 *cellp; void __iomem *initrd; @@ -3514,6 +3526,7 @@ static int __init get_evtchn_dt_property(const struct dt_device_node *np, uint32_t *port, uint32_t *phandle) { const __be32 *prop = NULL; + /* SAF-1-safe MC3R1.R9.1 */ uint32_t len; prop = dt_get_property(np, "xen,evtchn", &len); @@ -3538,10 +3551,13 @@ static int __init get_evtchn_dt_property(const struct dt_device_node *np, static int __init alloc_domain_evtchn(struct dt_device_node *node) { int rc; + /* SAF-1-safe MC3R1.R9.1 */ uint32_t domU1_port, domU2_port, remote_phandle; struct dt_device_node *remote_node; const struct dt_device_node *p1_node, *p2_node; + /* SAF-1-safe MC3R1.R9.1 */ struct evtchn_alloc_unbound alloc_unbound; + /* SAF-1-safe MC3R1.R9.1 */ struct evtchn_bind_interdomain bind_interdomain; struct domain *d1 = NULL, *d2 = NULL; @@ -3789,11 +3805,12 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo) static int __init alloc_xenstore_evtchn(struct domain *d) { - evtchn_alloc_unbound_t alloc; + evtchn_alloc_unbound_t alloc = { + .dom = d->domain_id, + .remote_dom = hardware_domain->domain_id + }; int rc; - alloc.dom = d->domain_id; - alloc.remote_dom = hardware_domain->domain_id; rc = evtchn_alloc_unbound(&alloc, 0); if ( rc ) { @@ -3810,8 +3827,9 @@ static int __init construct_domU(struct domain *d, const struct dt_device_node *node) { struct kernel_info kinfo = {}; - const char *dom0less_enhanced; + const char *dom0less_enhanced = NULL; int rc; + /* SAF-1-safe MC3R1.R9.1 */ u64 mem; u32 p2m_mem_mb; unsigned long p2m_pages; @@ -3939,6 +3957,7 @@ void __init create_domUs(void) .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version), }; unsigned int flags = 0U; + /* SAF-1-safe MC3R1.R9.1 */ uint32_t val; int rc; diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index bb64925d70..25f39364d1 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -117,6 +117,7 @@ static int __init setup_chosen_node(void *fdt, int *addr_cells, int *size_cells) static int __init fdt_set_reg(void *fdt, int node, int addr_cells, int size_cells, uint64_t addr, uint64_t len) { + /* SAF-1-safe MC3R1.R9.1 */ __be32 val[4]; /* At most 2 64 bit values to be stored */ __be32 *cellp; @@ -308,7 +309,7 @@ fdt_set_fail: static void __init *fdt_increase_size(struct file *fdtfile, int add_size) { EFI_STATUS status; - EFI_PHYSICAL_ADDRESS fdt_addr; + EFI_PHYSICAL_ADDRESS fdt_addr = 0; int fdt_size; int pages; void *new_fdt; @@ -433,7 +434,7 @@ static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image, static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size) { - void *ptr; + void *ptr = NULL; EFI_STATUS status; status = efi_bs->AllocatePool(EfiLoaderData, map_size, &ptr); @@ -538,6 +539,7 @@ static void __init efi_arch_handle_module(const struct file *file, { int node; int chosen; + /* SAF-1-safe MC3R1.R9.1 */ int addr_len, size_len; if ( file == &dtbfile ) diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c index 3aa4edda10..aa0180ab5b 100644 --- a/xen/arch/arm/gic-v3-its.c +++ b/xen/arch/arm/gic-v3-its.c @@ -192,8 +192,7 @@ static int its_send_cmd_mapc(struct host_its *its, uint32_t collection_id, cmd[0] = GITS_CMD_MAPC; cmd[1] = 0x00; - cmd[2] = encode_rdbase(its, cpu, collection_id); - cmd[2] |= GITS_VALID_BIT; + cmd[2] = encode_rdbase(its, cpu, collection_id) | GITS_VALID_BIT; cmd[3] = 0x00; return its_send_command(its, cmd); @@ -215,9 +214,7 @@ static int its_send_cmd_mapd(struct host_its *its, uint32_t deviceid, } cmd[0] = GITS_CMD_MAPD | ((uint64_t)deviceid << 32); cmd[1] = size_bits; - cmd[2] = itt_addr; - if ( valid ) - cmd[2] |= GITS_VALID_BIT; + cmd[2] = itt_addr | (valid ? GITS_VALID_BIT : 0x00); cmd[3] = 0x00; return its_send_command(its, cmd); @@ -911,6 +908,7 @@ int gicv3_its_make_hwdom_dt_nodes(const struct domain *d, const struct dt_device_node *gic, void *fdt) { + /* SAF-1-safe MC3R1.R9.1 */ uint32_t len; int res; const void *prop = NULL; @@ -1004,6 +1002,7 @@ static void gicv3_its_dt_init(const struct dt_device_node *node) */ dt_for_each_child_node(node, its) { + /* SAF-1-safe MC3R1.R9.1 */ paddr_t addr, size; if ( !dt_device_is_compatible(its, "arm,gic-v3-its") ) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index c688227abd..a36068b2d8 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -935,6 +935,7 @@ static int xen_pt_update_entry(mfn_t root, unsigned long virt, mfn_t mfn, unsigned int target, unsigned int flags) { + /* SAF-1-safe MC3R1.R9.1 */ int rc; unsigned int level; lpae_t *table; diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index de32a2d638..83c56cf1cb 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -496,16 +496,18 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn, lpae_t entry, *table; int rc; mfn_t mfn = INVALID_MFN; - p2m_type_t _t; + p2m_type_t _t = p2m_invalid; DECLARE_OFFSETS(offsets, addr); ASSERT(p2m_is_locked(p2m)); BUILD_BUG_ON(THIRD_MASK != PAGE_MASK); /* Allow t to be NULL */ - t = t ?: &_t; - - *t = p2m_invalid; + if( t ) { + *t = _t; + } else { + t = &_t; + } if ( valid ) *valid = false; @@ -1031,6 +1033,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m, unsigned int level = 0; unsigned int target = 3 - (page_order / XEN_PT_LPAE_SHIFT); lpae_t *entry, *table, orig_pte; + /* SAF-1-safe MC3R1.R9.1 */ int rc; /* A mapping is removed if the MFN is invalid. */ bool removing_mapping = mfn_eq(smfn, INVALID_MFN); @@ -1483,6 +1486,7 @@ static inline int p2m_remove_mapping(struct domain *d, { struct p2m_domain *p2m = p2m_get_hostp2m(d); unsigned long i; + /* SAF-1-safe MC3R1.R9.1 */ int rc; p2m_write_lock(p2m); @@ -1685,20 +1689,21 @@ static int p2m_alloc_vmid(struct domain *d) ASSERT(nr != INVALID_VMID); - if ( nr == MAX_VMID ) - { - rc = -EBUSY; - printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", d->domain_id); - goto out; - } + do { + if ( nr == MAX_VMID ) + { + rc = -EBUSY; + printk(XENLOG_ERR "p2m.c: dom%d: VMID pool exhausted\n", d->domain_id); + break; + } - set_bit(nr, vmid_mask); + set_bit(nr, vmid_mask); - p2m->vmid = nr; + p2m->vmid = nr; - rc = 0; + rc = 0; + } while ( 0 ); -out: spin_unlock(&vmid_alloc_lock); return rc; }
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. To achieve this goal, I adopted the following strategies: - Add a suitably formatted local deviation comment (as indicated in 'docs/misra/documenting-violations.rst') to exempt the following line from checking. - Provide an initialization for the variable at the declaration. - Substitute a goto breaking out of control flow logic with a semantically equivalent do { .. } while(0). Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- docs/misra/safe.json | 8 +++++++ xen/arch/arm/arm64/lib/find_next_bit.c | 1 + xen/arch/arm/bootfdt.c | 6 +++++ xen/arch/arm/decode.c | 2 ++ xen/arch/arm/domain_build.c | 29 ++++++++++++++++++---- xen/arch/arm/efi/efi-boot.h | 6 +++-- xen/arch/arm/gic-v3-its.c | 9 ++++--- xen/arch/arm/mm.c | 1 + xen/arch/arm/p2m.c | 33 +++++++++++++++----------- 9 files changed, 69 insertions(+), 26 deletions(-)