From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752316AbdK1OMH (ORCPT ); Tue, 28 Nov 2017 09:12:07 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:42928 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751501AbdK1OMG (ORCPT ); Tue, 28 Nov 2017 09:12:06 -0500 Date: Tue, 28 Nov 2017 17:11:49 +0300 From: Dan Carpenter To: David Kershner Cc: gregkh@linuxfoundation.org, jes.sorensen@gmail.com, linux-kernel@vger.kernel.org, driverdev-devel@linuxdriverproject.org, sparmaintainer@unisys.com, erik.arfvidson@gmail.com, wadgaonkarsam@gmail.com, Tim Sell Subject: Re: [PATCH 1/2] staging: unisys: visorbus: address theoretical int overflows Message-ID: <20171128141148.3pkz6pwteuzoo2aa@mwanda> References: <1510939659-15039-1-git-send-email-david.kershner@unisys.com> <1510939659-15039-2-git-send-email-david.kershner@unisys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1510939659-15039-2-git-send-email-david.kershner@unisys.com> User-Agent: NeoMutt/20170609 (1.8.3) X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > --- a/drivers/staging/unisys/visorbus/visorchipset.c > +++ b/drivers/staging/unisys/visorbus/visorchipset.c > @@ -581,7 +581,8 @@ static void *parser_name_get(struct parser_context *ctx) > struct visor_controlvm_parameters_header *phdr; > > phdr = &ctx->data; > - if (phdr->name_offset + phdr->name_length > ctx->param_bytes) > + if ((unsigned long)phdr->name_offset + > + (unsigned long)phdr->name_length > ctx->param_bytes) > return NULL; > ctx->curr = (char *)&phdr + phdr->name_offset; > ctx->bytes_remaining = phdr->name_length; I haven't reviewed this code very thouroughly. This should fix the issue on 64 bit systems, but it's a no-op on 32 bit systems. Which might be fine? I would be more comfortable if we just checked for integer overflow explicitly. There are bunch of ways to do that: if (phdr->name_offset > ctx->param_bytes || phdr->name_length > ctx->param_bytes || phdr->name_offset + phdr->name_length > ctx->param_bytes) Or: if (phdr->name_offset + phdr->name_length < phdr->name_offset || phdr->name_offset + phdr->name_length > ctx->param_bytes) return NULL; regards, dan carpenter