From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751191AbdJAPTW (ORCPT ); Sun, 1 Oct 2017 11:19:22 -0400 Received: from mail-qk0-f194.google.com ([209.85.220.194]:34442 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750965AbdJAPTU (ORCPT ); Sun, 1 Oct 2017 11:19:20 -0400 X-Google-Smtp-Source: AOwi7QBGg1LfrZuv5ydZAP/4Z+f4TwdYZZV+YsARBtmlwA8htLR32r/pVzHbphFZdQcRC5HDsoXqBA== Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: Re: [PATCH] vme: Fix integer overflow checking in vme_check_window() From: Dmitry Kalinkin In-Reply-To: <20170930112741.6wmswvk25j7o44ui@mwanda> Date: Sun, 1 Oct 2017 11:19:20 -0400 Cc: Martyn Welch , Manohar Vanga , Greg Kroah-Hartman , devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org Message-Id: <234C5902-7086-4941-9895-50A21470A85A@gmail.com> References: <20170930112741.6wmswvk25j7o44ui@mwanda> To: Dan Carpenter X-Mailer: Apple Mail (2.3273) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v91FJTXF024882 > On 30 Sep 2017, at 07:27, Dan Carpenter 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 Acked-by: Dmitry Kalinkin > > 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: