Received: by 2002:a05:6a10:1287:0:0:0:0 with SMTP id d7csp6222495pxv; Thu, 29 Jul 2021 09:11:39 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxRH6FbWK2Vi7qasBXPGM3bq7WGduY7NWD6KUD8sDmCjwSE845d02l/qpZ+hqeeqt/mDW1U X-Received: by 2002:aa7:cd96:: with SMTP id x22mr7127636edv.102.1627575099282; Thu, 29 Jul 2021 09:11:39 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1627575099; cv=none; d=google.com; s=arc-20160816; b=orRNyIrsP3u6KcnhrFOohRddd5DO+cXGZujN1UINvA8obW86NxMpPsWaue0s2CI6Kg jR+CiDLV/BTjiwuDh2/TrKZceV8n7rHs4bU+QmiGEyemRIReIs8wbNRNDqSgv7i/jL+m hE/vRgh99/VZPumJxeSMROBXxLzUJOJQnwd8Dyt3PnJZugcypbIsyHCCnQutwyUVclAI kvbMeS/yFH+SiUs2Yx/ffblWuDLvqSOzgBsQ9P2f8lk82JwJoVkQYIiFWOeJm2tn9WGr 58nPwKBHDaeO0r1Av3RoGavnCe688xlgXoV2LCnjsgeWpAOq/Q5kKnBQ9NPAnteaguT5 gIzQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:references:from:subject:cc:to :message-id:date:content-transfer-encoding:mime-version :dkim-signature; bh=crl+m4xpN2rQpodK9IBboDzgVn58oWozCut4cofRiKA=; b=blaX1M9QkcZKjmzaPu6AyTuptSfUI+B9Msq03XSCbCn3/YChs4qdFuUGg6cOEtXdGA uG/R1NkQbQupEp/XGsKBOXJJaroY9ZUe06pFOCSYI22t5s34FeILLfBIrkNjgO2m4hDi lOrcR5T2HiiI/S9bvwqZufjHGS4V4gUg1ZWV82wfzY4T0ubuV0GqXNqLst1rVtwLTk4F dB2BX+5k6/leooHaKDDendXgm6qgEpRxeQR4+2o5LrXF4YIWpaEcz52hwPR8F3ZKKem+ f3cOb1PfF9c3qC3Of5ioNDpFIbkeTer43bFddKnBA1j8U3ETQUtReR1iVUCG9zw1oSXx NH2g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=pLGZ1EOk; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id h1si3007164ede.149.2021.07.29.09.11.12; Thu, 29 Jul 2021 09:11:39 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=pLGZ1EOk; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232676AbhG2QCj (ORCPT + 99 others); Thu, 29 Jul 2021 12:02:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56086 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233234AbhG2QCK (ORCPT ); Thu, 29 Jul 2021 12:02:10 -0400 Received: from mail-qv1-xf32.google.com (mail-qv1-xf32.google.com [IPv6:2607:f8b0:4864:20::f32]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4F3A0C06179E; Thu, 29 Jul 2021 09:02:04 -0700 (PDT) Received: by mail-qv1-xf32.google.com with SMTP id jm13so3639348qvb.5; Thu, 29 Jul 2021 09:02:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:content-transfer-encoding:date:message-id:to:cc :subject:from:references:in-reply-to; bh=crl+m4xpN2rQpodK9IBboDzgVn58oWozCut4cofRiKA=; b=pLGZ1EOkiUZRuQU/Do9j/kxjA/lCZMffc0hvACY1Zge+1zX8hdK1Fx7UsnIGykgq/o /G34St8ykVgFj0QntTTbLoIY4Vzw8fNX4mB/0PkWz8oR2G9hmFsWD+6DVTqHWmyPkGvM /nGhNNRebpfn0Sbc/BPRmUmmV2HZUQt1R4wXyLbzUywSzvmpoyb7faXyEGzXxAeP5HSz SxGU22wVHyxE+q5eqpJbe22P1NWroAS2hd4XGrr2KdVewgaeDbD0chche8p/5O5niCeV XdYfwoTz6B2nFOoo1CUF7EBciyUaPcFzmFTyy8aOI5daH3OcFNS/UbfiyWisUIryIDs8 grLQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:content-transfer-encoding:date :message-id:to:cc:subject:from:references:in-reply-to; bh=crl+m4xpN2rQpodK9IBboDzgVn58oWozCut4cofRiKA=; b=s8FLr1HfcLmWSVLoRZ/wKoEyEzZjUytr0uVQVZ25OQ0q0zXrMjqpWgCFtZ1MdAUjFZ 6j0CJkS/ehb4+sgvuiTTMHBBq+SwcWeocv5IZgbwmL0LPyXB34yn/rjw/uBFq3PUTvog dtutsnQmaQzgevLFQNIb86FqDI41SSCUAHpnYgmSSEbIn1Y0qC/JgXVl++VGnE5wCIM0 u2xCFKs7Pa1NvkOZWyLpgtiuiiOnBdyEKyBTP3QyvyPEfu4WEVuFU8ajhz9pslc0Tcm9 Y9R8gI08WfNuHGuNKsPMb6GRdZnCoEfs7VoiuYoV9tKajrdoIj74l5GKUcdaYnr59c8s qSbg== X-Gm-Message-State: AOAM531KcgJkb+ui1ZBGWDc53oQB8EfZkW0UneIP2XMKnMZCmLU4Yh2z IOXAw7jkKZ6tV2PXaz+Xl3c= X-Received: by 2002:a0c:c3d1:: with SMTP id p17mr5969024qvi.44.1627574523531; Thu, 29 Jul 2021 09:02:03 -0700 (PDT) Received: from localhost (198-48-202-89.cpe.pppoe.ca. [198.48.202.89]) by smtp.gmail.com with ESMTPSA id 6sm1983547qkv.115.2021.07.29.09.02.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 29 Jul 2021 09:02:02 -0700 (PDT) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 29 Jul 2021 12:02:01 -0400 Message-Id: To: "Peter Rosin" , , , Cc: , , , Subject: Re: [PATCH v6 08/13] iio: afe: rescale: reduce risk of integer overflow From: "Liam Beguin" References: <20210721030613.3105327-1-liambeguin@gmail.com> <20210721030613.3105327-9-liambeguin@gmail.com> <9e0e4398-873e-b5c0-0f0c-50a186ed2228@axentia.se> <1a6e4851-9119-f524-76ff-a31ef0db8988@axentia.se> In-Reply-To: <1a6e4851-9119-f524-76ff-a31ef0db8988@axentia.se> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed Jul 28, 2021 at 3:47 AM EDT, Peter Rosin wrote: > On 2021-07-28 02:07, Liam Beguin wrote: > > On Fri Jul 23, 2021 at 5:17 PM EDT, Peter Rosin wrote: > >> On 2021-07-21 05:06, Liam Beguin wrote: > >>> From: Liam Beguin > >>> > >>> Reduce the risk of integer overflow by doing the scale calculation wi= th > >>> 64bit integers and looking for a Greatest Common Divider for both par= ts > >>> of the fractional value when required. > >>> > >>> Signed-off-by: Liam Beguin > >>> --- > >>> drivers/iio/afe/iio-rescale.c | 15 ++++++++++++--- > >>> 1 file changed, 12 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-resc= ale.c > >>> index 6f6a711ae3ae..35fa3b4e53e0 100644 > >>> --- a/drivers/iio/afe/iio-rescale.c > >>> +++ b/drivers/iio/afe/iio-rescale.c > >>> @@ -21,12 +21,21 @@ > >>> int rescale_process_scale(struct rescale *rescale, int scale_type, > >>> int *val, int *val2) > >>> { > >>> - unsigned long long tmp; > >>> + s64 tmp, tmp2; > >>> + u32 factor; > >>> =20 > >>> switch (scale_type) { > >>> case IIO_VAL_FRACTIONAL: > >>> - *val *=3D rescale->numerator; > >>> - *val2 *=3D rescale->denominator; > >>> + if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) || > >>> + check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2))= { > >>> + tmp =3D (s64)*val * rescale->numerator; > >>> + tmp2 =3D (s64)*val2 * rescale->denominator; > >>> + factor =3D gcd(tmp, tmp2); > >=20 > > Hi Peter, > >=20 > >> > >> Hi! > >> > >> Reiterating that gcd() only works for unsigned operands, so this is > >> broken for > >> negative values. > >=20 > > Apologies, I didn't mean to make it seem like I ignored your comments. = I > > should've added a note. After you pointed out that gcd() only works for > > unsigned elements, I added test cases for negative values, and all test= s > > passed. I'll look into it more. > > Maybe I've misread the code and gcd is in fact working for negative > numbers? However, I imagine it might be arch specific, so testing on > a single arch feels insufficient and deeper analysis is required. > > However, looking at lib/math/gcd.c it certainly still looks like > negative values will work very poorly, and there is no macro magic > in include/linux/gcd.h to handle it by wrapping the core C routine. I agree that looking at lib/math/gcd.c odd things might happen with negative values. I'll use the the absolute values to calculate the GCD as it shouldn't affect the value of factor. > > > rescale_voltage_divider_props() seems to also use gcd() with signed > > integers. > > The type of the operands may be s32, but if you look at how those values > are populated, and with what they are populated, I think you will find > that > only positive scale factors are sensible for a voltage divider. Using > resistors with so high resistance that s32 is not enough is simply not > supported. That makes sense! Thanks, Liam > > Cheers, > Peter > > > Thanks, > > Liam > >=20 > >> > >> Cheers, > >> Peter > >> > >>> + tmp =3D div_s64(tmp, factor); > >>> + tmp2 =3D div_s64(tmp2, factor); > >>> + } > >>> + *val =3D tmp; > >>> + *val2 =3D tmp2; > >>> return scale_type; > >>> case IIO_VAL_INT: > >>> *val *=3D rescale->numerator; > >>> > >=20