| Message ID | 1644518851-16847-1-git-send-email-victor.erminpour@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ACPI/IORT: Fix GCC 12 warning | expand |
On Fri, 2022-02-11 at 16:37 -0800, Kees Cook wrote: [] > Well, I'm for #1, though perhaps with a more narrow view: some semantics > are just weird/surprising. ;) Until I first encountered this warning a > few years ago when working on GCC_PLUGIN_STRUCTLEAK_BYREF_ALL, I didn't > even know putting declarations there was valid C. ;) > > Whack-a-mole is part of the work to make these kinds of treewide > changes, but the hope is to find as much of it ahead of time as > possible. And, no, I have no interest in security theater. (Not > everything has equal levels of effectiveness, of course, but I don't > think that's what you're saying.) > > > In fairness I'd have no objection to that patch if it came with a convincing > > justification, but that is so far very much lacking. My aim here is not to > > be a change-averse Luddite, but to try to find a compromise where I can > > actually have some confidence in such changes being made. Let's not start > > pretending that 3 100ml bottles of shampoo are somehow "safer" than a 300ml > > bottle of shampoo... > > Sure. I think I am trying to take a pragmatic approach here, which is > that gaining auto-var-init is a big deal (killing entire classes of > vulnerabilities), but it comes with an annoying compiler bug (that we do > get a warning about) for an uncommon code pattern that is easy to fix. > So rather than delaying the defense until the sharp edge on the compiler > gets fixed, I'd like to get the rest rolling while the edge is filed. coccinelle would probably find most all of them. $ cat switch_define.cocci @@ expression e; type t; identifier i; @@ switch (e) { * t i; } $ spatch --very-quiet -sp-file switch_define.cocci . 13831 files match diff -u -p ./arch/arc/kernel/unwind.c /tmp/nothing/arch/arc/kernel/unwind.c --- ./arch/arc/kernel/unwind.c +++ /tmp/nothing/arch/arc/kernel/unwind.c @@ -718,7 +718,6 @@ static int processCFI(const u8 *start, c } for (ptr.p8 = start; result && ptr.p8 < end;) { switch (*ptr.p8 >> 6) { - uleb128_t value; case 0: opcode = *ptr.p8++; diff -u -p ./arch/arm/kernel/module-plts.c /tmp/nothing/arch/arm/kernel/module-plts.c --- ./arch/arm/kernel/module-plts.c +++ /tmp/nothing/arch/arm/kernel/module-plts.c @@ -124,7 +124,6 @@ static bool is_zero_addend_relocation(El * PC bias into account, i.e., -8 for ARM and -4 for Thumb2. */ switch (ELF32_R_TYPE(rel->r_info)) { - u16 upper, lower; case R_ARM_THM_CALL: case R_ARM_THM_JUMP24: diff -u -p ./arch/mips/mti-malta/malta-init.c /tmp/nothing/arch/mips/mti-malta/malta-init.c --- ./arch/mips/mti-malta/malta-init.c +++ /tmp/nothing/arch/mips/mti-malta/malta-init.c @@ -168,7 +168,6 @@ void __init prom_init(void) } switch (mips_revision_sconid) { - u32 start, map, mask, data; case MIPS_REVISION_SCON_GT64120: /* diff -u -p ./arch/parisc/kernel/inventory.c /tmp/nothing/arch/parisc/kernel/inventory.c --- ./arch/parisc/kernel/inventory.c +++ /tmp/nothing/arch/parisc/kernel/inventory.c @@ -235,7 +235,6 @@ pat_query_module(ulong pcell_loc, ulong #ifdef DEBUG_PAT /* dump what we see so far... */ switch (PAT_GET_ENTITY(dev->mod_info)) { - pdc_pat_cell_mod_maddr_block_t io_pdc_cell; unsigned long i; case PAT_ENTITY_PROC: diff -u -p ./arch/powerpc/xmon/xmon.c /tmp/nothing/arch/powerpc/xmon/xmon.c --- ./arch/powerpc/xmon/xmon.c +++ /tmp/nothing/arch/powerpc/xmon/xmon.c @@ -1528,7 +1528,6 @@ bpt_cmds(void) cmd = inchar(); switch (cmd) { - static const char badaddr[] = "Only kernel addresses are permitted for breakpoints\n"; int mode; case 'd': /* bd - hardware data breakpoint */ if (xmon_is_ro) { diff -u -p ./arch/um/drivers/ubd_kern.c /tmp/nothing/arch/um/drivers/ubd_kern.c --- ./arch/um/drivers/ubd_kern.c +++ /tmp/nothing/arch/um/drivers/ubd_kern.c @@ -1407,7 +1407,6 @@ static int ubd_ioctl(struct block_device u16 ubd_id[ATA_ID_WORDS]; switch (cmd) { - struct cdrom_volctrl volume; case HDIO_GET_IDENTITY: memset(&ubd_id, 0, ATA_ID_WORDS * 2); ubd_id[ATA_ID_CYLS] = ubd_dev->size / (128 * 32 * 512); diff -u -p ./arch/x86/kernel/cpu/cyrix.c /tmp/nothing/arch/x86/kernel/cpu/cyrix.c --- ./arch/x86/kernel/cpu/cyrix.c +++ /tmp/nothing/arch/x86/kernel/cpu/cyrix.c @@ -224,7 +224,6 @@ static void init_cyrix(struct cpuinfo_x8 */ switch (dir0_msn) { - unsigned char tmp; case 0: /* Cx486SLC/DLC/SRx/DRx */ p = Cx486_name[dir0_lsn & 7]; diff -u -p ./arch/x86/pci/irq.c /tmp/nothing/arch/x86/pci/irq.c --- ./arch/x86/pci/irq.c +++ /tmp/nothing/arch/x86/pci/irq.c @@ -959,7 +959,6 @@ static __init int intel_router_probe(str return 0; switch (device) { - u8 rid; case PCI_DEVICE_ID_INTEL_82375: r->name = "PCEB/ESC"; r->get = pirq_esc_get; diff -u -p ./drivers/acpi/arm64/iort.c /tmp/nothing/drivers/acpi/arm64/iort.c --- ./drivers/acpi/arm64/iort.c +++ /tmp/nothing/drivers/acpi/arm64/iort.c @@ -1667,7 +1667,6 @@ phys_addr_t __init acpi_iort_dma_get_max break; switch (node->type) { - struct acpi_iort_named_component *ncomp; struct acpi_iort_root_complex *rc; phys_addr_t local_limit; diff -u -p ./drivers/infiniband/hw/irdma/hw.c /tmp/nothing/drivers/infiniband/hw/irdma/hw.c --- ./drivers/infiniband/hw/irdma/hw.c +++ /tmp/nothing/drivers/infiniband/hw/irdma/hw.c @@ -267,7 +267,6 @@ static void irdma_process_aeq(struct ird } switch (info->ae_id) { - struct irdma_cm_node *cm_node; case IRDMA_AE_LLP_CONNECTION_ESTABLISHED: cm_node = iwqp->cm_node; if (cm_node->accept_pend) { diff -u -p ./drivers/infiniband/hw/irdma/utils.c /tmp/nothing/drivers/infiniband/hw/irdma/utils.c --- ./drivers/infiniband/hw/irdma/utils.c +++ /tmp/nothing/drivers/infiniband/hw/irdma/utils.c @@ -1212,7 +1212,6 @@ enum irdma_status_code irdma_hw_modify_q return status; switch (m_info->next_iwarp_state) { - struct irdma_gen_ae_info ae_info; case IRDMA_QP_STATE_RTS: case IRDMA_QP_STATE_IDLE: diff -u -p ./drivers/input/serio/hil_mlc.c /tmp/nothing/drivers/input/serio/hil_mlc.c --- ./drivers/input/serio/hil_mlc.c +++ /tmp/nothing/drivers/input/serio/hil_mlc.c @@ -633,7 +633,6 @@ static int hilse_donode(hil_mlc *mlc) node = hil_mlc_se + mlc->seidx; switch (node->act) { - int rc; hil_packet pack; case HILSE_FUNC: diff -u -p ./drivers/rtc/rtc-pcf8523.c /tmp/nothing/drivers/rtc/rtc-pcf8523.c --- ./drivers/rtc/rtc-pcf8523.c +++ /tmp/nothing/drivers/rtc/rtc-pcf8523.c @@ -242,7 +242,6 @@ static int pcf8523_param_get(struct devi int ret; switch(param->param) { - u32 value; case RTC_PARAM_BACKUP_SWITCH_MODE: ret = regmap_read(pcf8523->regmap, PCF8523_REG_CONTROL3, &value); @@ -281,7 +280,6 @@ static int pcf8523_param_set(struct devi struct pcf8523 *pcf8523 = dev_get_drvdata(dev); switch(param->param) { - u8 mode; case RTC_PARAM_BACKUP_SWITCH_MODE: switch (param->uvalue) { case RTC_BSM_DISABLED: diff -u -p ./drivers/rtc/rtc-rv3028.c /tmp/nothing/drivers/rtc/rtc-rv3028.c --- ./drivers/rtc/rtc-rv3028.c +++ /tmp/nothing/drivers/rtc/rtc-rv3028.c @@ -523,7 +523,6 @@ static int rv3028_param_get(struct devic int ret; switch(param->param) { - u32 value; case RTC_PARAM_BACKUP_SWITCH_MODE: ret = regmap_read(rv3028->regmap, RV3028_BACKUP, &value); @@ -556,7 +555,6 @@ static int rv3028_param_set(struct devic struct rv3028_data *rv3028 = dev_get_drvdata(dev); switch(param->param) { - u8 mode; case RTC_PARAM_BACKUP_SWITCH_MODE: switch (param->uvalue) { case RTC_BSM_DISABLED: diff -u -p ./drivers/rtc/rtc-rv3032.c /tmp/nothing/drivers/rtc/rtc-rv3032.c --- ./drivers/rtc/rtc-rv3032.c +++ /tmp/nothing/drivers/rtc/rtc-rv3032.c @@ -401,7 +401,6 @@ static int rv3032_param_get(struct devic int ret; switch(param->param) { - u32 value; case RTC_PARAM_BACKUP_SWITCH_MODE: ret = regmap_read(rv3032->regmap, RV3032_PMU, &value); @@ -435,7 +434,6 @@ static int rv3032_param_set(struct devic struct rv3032_data *rv3032 = dev_get_drvdata(dev); switch(param->param) { - u8 mode; case RTC_PARAM_BACKUP_SWITCH_MODE: if (rv3032->trickle_charger_set) return -EINVAL; diff -u -p ./drivers/s390/net/ctcm_fsms.c /tmp/nothing/drivers/s390/net/ctcm_fsms.c --- ./drivers/s390/net/ctcm_fsms.c +++ /tmp/nothing/drivers/s390/net/ctcm_fsms.c @@ -1432,7 +1432,6 @@ static void ctcmpc_chx_rx(fsm_instance * again: switch (fsm_getstate(grp->fsm)) { - int rc, dolock; case MPCG_STATE_FLOWC: case MPCG_STATE_READY: if (ctcm_checkalloc_buffer(ch)) diff -u -p ./lib/test_stackinit.c /tmp/nothing/lib/test_stackinit.c --- ./lib/test_stackinit.c +++ /tmp/nothing/lib/test_stackinit.c @@ -398,7 +398,6 @@ static int noinline __leaf_switch_none(i * This is intentionally unreachable. To silence the * warning, build with -Wno-switch-unreachable */ - uint64_t var; case 1: target_start = &var; diff -u -p ./net/mpls/af_mpls.c /tmp/nothing/net/mpls/af_mpls.c --- ./net/mpls/af_mpls.c +++ /tmp/nothing/net/mpls/af_mpls.c @@ -1621,7 +1621,6 @@ static int mpls_dev_notify(struct notifi return NOTIFY_OK; switch (event) { - int err; case NETDEV_DOWN: err = mpls_ifdown(dev, event); diff -u -p ./scripts/recordmcount.c /tmp/nothing/scripts/recordmcount.c --- ./scripts/recordmcount.c +++ /tmp/nothing/scripts/recordmcount.c @@ -488,7 +488,6 @@ static int do_file(char const *const fna w2 = w2nat; w8 = w8nat; switch (ehdr->e_ident[EI_DATA]) { - static unsigned int const endian = 1; default: fprintf(stderr, "unrecognized ELF data encoding %d: %s\n", ehdr->e_ident[EI_DATA], fname); diff -u -p ./tools/testing/selftests/powerpc/nx-gzip/gunz_test.c /tmp/nothing/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c --- ./tools/testing/selftests/powerpc/nx-gzip/gunz_test.c +++ /tmp/nothing/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c @@ -805,7 +805,6 @@ ok_cc3: */ switch (sfbt) { - int dhtlen; case 0x0: /* Deflate final EOB received */ diff -u -p ./tools/testing/selftests/proc/read.c /tmp/nothing/tools/testing/selftests/proc/read.c --- ./tools/testing/selftests/proc/read.c +++ /tmp/nothing/tools/testing/selftests/proc/read.c @@ -91,7 +91,6 @@ static void f(DIR *d, unsigned int level assert(!streq(de->d_name, "..")); switch (de->d_type) { - DIR *dd; int fd; case DT_REG: $
From: Kees Cook > Sent: 12 February 2022 00:38 .... > > Sure. I think I am trying to take a pragmatic approach here, which is > that gaining auto-var-init is a big deal (killing entire classes of > vulnerabilities), but it comes with an annoying compiler bug (that we do > get a warning about) for an uncommon code pattern that is easy to fix. My worry about the compiler setting variables to zero is that people will get lazy and assume it happens. Then some code will get compiled without that enabled (possibly because it slows things down too much) and then really horrid bugs start to appear. If the intent is to detect code that is failing to initialise locals then setting to a non-zero value (that is likely to cause grief) is much better than setting to zero. I'm particularly worried if the compiler starts to zero on-stack arrays. There is plenty of userspace code that will use char buff[MAX_PATH] to build a filename or char errmsg[1024] for an error message. At the moment the size only needs to be 'big enough' but if the compiler zeros it then there are performance issues and oversize buffers make a difference. At which point you start making the arrays 'just big enough' and then fall foul of truncations and overruns because the calculation was wrong. Definitely double edged. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index 3b23fb775ac4..65395f0decf9 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -1645,7 +1645,7 @@ void __init acpi_iort_init(void) */ phys_addr_t __init acpi_iort_dma_get_max_cpu_address(void) { - phys_addr_t limit = PHYS_ADDR_MAX; + phys_addr_t local_limit, limit = PHYS_ADDR_MAX; struct acpi_iort_node *node, *end; struct acpi_table_iort *iort; acpi_status status; @@ -1667,17 +1667,16 @@ phys_addr_t __init acpi_iort_dma_get_max_cpu_address(void) break; switch (node->type) { + case ACPI_IORT_NODE_NAMED_COMPONENT: { struct acpi_iort_named_component *ncomp; - struct acpi_iort_root_complex *rc; - phys_addr_t local_limit; - - case ACPI_IORT_NODE_NAMED_COMPONENT: ncomp = (struct acpi_iort_named_component *)node->node_data; local_limit = DMA_BIT_MASK(ncomp->memory_address_limit); limit = min_not_zero(limit, local_limit); break; - case ACPI_IORT_NODE_PCI_ROOT_COMPLEX: + } + case ACPI_IORT_NODE_PCI_ROOT_COMPLEX: { + struct acpi_iort_root_complex *rc; if (node->revision < 1) break; @@ -1686,6 +1685,7 @@ phys_addr_t __init acpi_iort_dma_get_max_cpu_address(void) limit = min_not_zero(limit, local_limit); break; } + } node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->length); } acpi_put_table(&iort->header);
When building with automatic stack variable initialization, GCC 12 complains about variables defined outside of switch case statements. Move the variable into the case that uses it, which silences the warning: ./drivers/acpi/arm64/iort.c:1670:59: error: statement will never be executed [-Werror=switch-unreachable] 1670 | struct acpi_iort_named_component *ncomp; | ^~~~~ Signed-off-by: Victor Erminpour <victor.erminpour@oracle.com> --- drivers/acpi/arm64/iort.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)