linux-kernel - Re: [PATCH] vme: Fix integer overflow checking in vme_check

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] [day] [month] [year] [list]

Date:   Sun, 1 Oct 2017 11:19:20 -0400
From:   Dmitry Kalinkin <dmitry.kalinkin@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Martyn Welch <martyn@...chs.me.uk>,
        Manohar Vanga <manohar.vanga@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] vme: Fix integer overflow checking in vme_check_window()


> On 30 Sep 2017, at 07:27, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> 
> The controversial part of this patch is that I've changed it so we now
> prevent integer overflows for VME_USER types and before we didn't.  I
> view it as kernel-hardening.  I looked at a couple places that used
> VME_USER types and they seemed pretty suspicious so I'm pretty sure
> preventing overflows here is a good idea.
> 
> The most common problem which this function is for cases like VME_A16
> where we don't put an upper bound on "size" so you could have "size" set
> to U64_MAX and a valid vme_base would overflow the "vme_base + size"
> into the valid range as well.
> 
> In the VME_A64 case, the integer overflow checking doesn't work because
> "U64_MAX + 1" has an integer overflow and it's just a complicated way of
> saying zero.  That VME_A64 case is sort of interesting as well because
> there is a VME_A64_MAX define which is set to "U64_MAX + 1".  The
> compiler will never let anyone use it since it can't be stored in a u64
> variable...  With my patch it's now limited to just U64_MAX.
> 
> Anyway, I put one integer overflow check at the start of the function
> and deleted all existing checks.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
Acked-by: Dmitry Kalinkin <dmitry.kalinkin@...il.com>
> 
> diff --git a/drivers/vme/vme.c b/drivers/vme/vme.c
> index 6a3ead42aba8..5b4c898d7509 100644
> --- a/drivers/vme/vme.c
> +++ b/drivers/vme/vme.c
> @@ -208,29 +208,27 @@ int vme_check_window(u32 aspace, unsigned long long vme_base,
> {
> 	int retval = 0;
> 
> +	if (vme_base + size < size)
> +		return -EINVAL;
> +
> 	switch (aspace) {
> 	case VME_A16:
> -		if (((vme_base + size) > VME_A16_MAX) ||
> -				(vme_base > VME_A16_MAX))
> +		if (vme_base + size > VME_A16_MAX)
> 			retval = -EFAULT;
> 		break;
> 	case VME_A24:
> -		if (((vme_base + size) > VME_A24_MAX) ||
> -				(vme_base > VME_A24_MAX))
> +		if (vme_base + size > VME_A24_MAX)
> 			retval = -EFAULT;
> 		break;
> 	case VME_A32:
> -		if (((vme_base + size) > VME_A32_MAX) ||
> -				(vme_base > VME_A32_MAX))
> +		if (vme_base + size > VME_A32_MAX)
> 			retval = -EFAULT;
> 		break;
> 	case VME_A64:
> -		if ((size != 0) && (vme_base > U64_MAX + 1 - size))
> -			retval = -EFAULT;
> +		/* The VME_A64_MAX limit is actually U64_MAX + 1 */
> 		break;
> 	case VME_CRCSR:
> -		if (((vme_base + size) > VME_CRCSR_MAX) ||
> -				(vme_base > VME_CRCSR_MAX))
> +		if (vme_base + size > VME_CRCSR_MAX)
> 			retval = -EFAULT;
> 		break;
> 	case VME_USER1: