linux-kernel - Re: [PATCH] perf/x86/intel/uncore: fix integer overflow on shift of a u32 integer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191002122357.GA29018@lakrids.cambridge.arm.com>
Date:   Wed, 2 Oct 2019 13:23:57 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Colin King <colin.king@...onical.com>
Cc:     Kan Liang <kan.liang@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        kernel-janitors@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf/x86/intel/uncore: fix integer overflow on shift of
 a u32 integer

On Wed, Oct 02, 2019 at 12:55:45PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@...onical.com>
> 
> Shifting the u32 integer result of (pci_dword & SNR_IMC_MMIO_BASE_MASK)
> will end up with an overflow when pci_dword greater than 0x1ff. Fix this
> by casting pci_dword to a resource_size_t before masking and shifting it.
> 
> Addresses-Coverity: ("Unintentional integer overflow")

I don't see that tag in Documentation/process/submitting-patches.rst ;)

IIUC this is unintented truncation of the upper bits due to missing type
promotion before the shift, rather than overflow (i.e. the value
wrapping across addition/subtraction), so I think the wording is
slightly misleading.

Does coverity call that integer overflow?

It might be better to say:

| [PATCH] perf/x86/intel/uncore: don't truncate upper bits of address
|
| Shifting the u32 integer result of (pci_dword & SNR_IMC_MMIO_BASE_MASK)
| by 23 will throw away the upper 23 bits of the potentially 64-bit
| address. Fix this by casting pci_dword to a resource_size_t before
| masking and shifting it.
|
| Found by coverity ("Unintentional integer overflow").

Otherwise, the patch looks fine to me:

Acked-by: Mark Rutland <mark.rutland@....com>

Thanks,
Mark.

> Fixes: ee49532b38dd ("perf/x86/intel/uncore: Add IMC uncore support for Snow Ridge")
> Signed-off-by: Colin Ian King <colin.king@...onical.com>
> ---
>  arch/x86/events/intel/uncore_snbep.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index b10a5ec79e48..ed69df1340d9 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -4415,7 +4415,7 @@ static void snr_uncore_mmio_init_box(struct intel_uncore_box *box)
>  		return;
>  
>  	pci_read_config_dword(pdev, SNR_IMC_MMIO_BASE_OFFSET, &pci_dword);
> -	addr = (pci_dword & SNR_IMC_MMIO_BASE_MASK) << 23;
> +	addr = ((resource_size_t)pci_dword & SNR_IMC_MMIO_BASE_MASK) << 23;
>  
>  	pci_read_config_dword(pdev, SNR_IMC_MMIO_MEM0_OFFSET, &pci_dword);
>  	addr |= (pci_dword & SNR_IMC_MMIO_MEM0_MASK) << 12;
> -- 
> 2.20.1
>