From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.bootlin.com ([62.4.15.54]:53817 "EHLO mail.bootlin.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727363AbeJRRte (ORCPT ); Thu, 18 Oct 2018 13:49:34 -0400 Date: Thu, 18 Oct 2018 11:49:19 +0200 From: Maxime Ripard To: Boris Brezillon Cc: Chen-Yu Tsai , dri-devel@lists.freedesktop.org, stable@vger.kernel.org Subject: Re: [PATCH] drm/sun4i: Fix an ulong overflow in the dotclock driver Message-ID: <20181018094919.oej363mftxqz7tqg@flea> References: <20181018092546.23078-1-boris.brezillon@bootlin.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ripabaipah35cryc" Content-Disposition: inline In-Reply-To: <20181018092546.23078-1-boris.brezillon@bootlin.com> Sender: stable-owner@vger.kernel.org List-ID: --ripabaipah35cryc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi! On Thu, Oct 18, 2018 at 11:25:46AM +0200, Boris Brezillon wrote: > The calculated ideal rate can easily overflow an unsigned long, thus > making the best div selection buggy as soon as no ideal match is found > before the overflow occurs. >=20 > Fixes: 4731a72df273 ("drm/sun4i: request exact rates to our parents") > Cc: > Signed-off-by: Boris Brezillon > --- > drivers/gpu/drm/sun4i/sun4i_dotclock.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) >=20 > diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun= 4i/sun4i_dotclock.c > index e36004fbe453..82132a9bd1d5 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c > +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c > @@ -81,9 +81,12 @@ static long sun4i_dclk_round_rate(struct clk_hw *hw, u= nsigned long rate, > int i; > =20 > for (i =3D tcon->dclk_min_div; i <=3D tcon->dclk_max_div; i++) { > - unsigned long ideal =3D rate * i; > + u64 ideal =3D (u64)rate * i; > unsigned long rounded; > =20 > + if (ideal > ULONG_MAX) > + break; > + So I guess the rationale is that since we've overflown already, every divider above that limit will make us overflow more and therefore there's no point in going forward? I'd be fine with that approach, but I'd like a comment explaining why you have that test, and a goto out instead of the break for consistency. with that fixed: Acked-by: Maxime Ripard Thanks! Maxime --=20 Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com --ripabaipah35cryc Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIyBAABCAAdFiEE0VqZU19dR2zEVaqr0rTAlCFNr3QFAlvIVx4ACgkQ0rTAlCFN r3SrSA/4p4Mcagvh9zd3/qGgt6vCXpF0+TnIvWPPPbLL2d5iS/V4zoWaoJ+OK2XD 5y6vK96W0mSDflMowAf1VZe+QsIulBplvNCLB8ZXuMJHzZpufE4wTTPeiSr3hX3V envzvHDZh2i0cmcOQ1wVMCIkQWrk0/3sqCQgGv0S/3UpL8KMQCRNxcXLJdfHBcom R5hefNanmHxV9Yj+S1l4H51IhJmOZ40FyhpyJcCzu1cl7MzozkBINES6R9LPyxEJ iIO66N0MlU0eSCmr43JOPhtUawDv5+hMJktkRI1DpS55Coy85M2VltLsn2G685gu p4d3tSbIv4vE4Xm5pVnBgViqz8Si111fFjYODgAR01zf848gSqRLKj7pH4AJCExr sVVM2STrNea1l4Vq8SEinKD1BLSRcizSiFT5N/bRVMRYYCEFzvTiVHW/IjNyoUIa G0f4vM0lOXtO6mwJ7A4pVifNoGXVDRFa0yaESJOVk8ne8l170dULuMBkMy5wccQz PgTXYJszGfqMoc5rg8IyznAC0mFpLrbYEbJN1YdCX9ySLPmq1odXw5+oVRYKDoqR mVXvOYc5qqU25R/4aM2t6LLwu32qA/sL3iDBKZm0MCjdf5CanN8JDrreWfm9fNKr w5KnWbOCMCfvSa2FUFemcx61V2KkL6G2INTgfk2JOLUjTJCC9Q== =4yDw -----END PGP SIGNATURE----- --ripabaipah35cryc-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Ripard Subject: Re: [PATCH] drm/sun4i: Fix an ulong overflow in the dotclock driver Date: Thu, 18 Oct 2018 11:49:19 +0200 Message-ID: <20181018094919.oej363mftxqz7tqg@flea> References: <20181018092546.23078-1-boris.brezillon@bootlin.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0222559186==" Return-path: Received: from mail.bootlin.com (mail.bootlin.com [62.4.15.54]) by gabe.freedesktop.org (Postfix) with ESMTP id C9E986E46F for ; Thu, 18 Oct 2018 09:49:20 +0000 (UTC) In-Reply-To: <20181018092546.23078-1-boris.brezillon@bootlin.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Boris Brezillon Cc: Chen-Yu Tsai , stable@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0222559186== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ripabaipah35cryc" Content-Disposition: inline --ripabaipah35cryc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi! On Thu, Oct 18, 2018 at 11:25:46AM +0200, Boris Brezillon wrote: > The calculated ideal rate can easily overflow an unsigned long, thus > making the best div selection buggy as soon as no ideal match is found > before the overflow occurs. >=20 > Fixes: 4731a72df273 ("drm/sun4i: request exact rates to our parents") > Cc: > Signed-off-by: Boris Brezillon > --- > drivers/gpu/drm/sun4i/sun4i_dotclock.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) >=20 > diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun= 4i/sun4i_dotclock.c > index e36004fbe453..82132a9bd1d5 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c > +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c > @@ -81,9 +81,12 @@ static long sun4i_dclk_round_rate(struct clk_hw *hw, u= nsigned long rate, > int i; > =20 > for (i =3D tcon->dclk_min_div; i <=3D tcon->dclk_max_div; i++) { > - unsigned long ideal =3D rate * i; > + u64 ideal =3D (u64)rate * i; > unsigned long rounded; > =20 > + if (ideal > ULONG_MAX) > + break; > + So I guess the rationale is that since we've overflown already, every divider above that limit will make us overflow more and therefore there's no point in going forward? I'd be fine with that approach, but I'd like a comment explaining why you have that test, and a goto out instead of the break for consistency. with that fixed: Acked-by: Maxime Ripard Thanks! Maxime --=20 Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com --ripabaipah35cryc Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIyBAABCAAdFiEE0VqZU19dR2zEVaqr0rTAlCFNr3QFAlvIVx4ACgkQ0rTAlCFN r3SrSA/4p4Mcagvh9zd3/qGgt6vCXpF0+TnIvWPPPbLL2d5iS/V4zoWaoJ+OK2XD 5y6vK96W0mSDflMowAf1VZe+QsIulBplvNCLB8ZXuMJHzZpufE4wTTPeiSr3hX3V envzvHDZh2i0cmcOQ1wVMCIkQWrk0/3sqCQgGv0S/3UpL8KMQCRNxcXLJdfHBcom R5hefNanmHxV9Yj+S1l4H51IhJmOZ40FyhpyJcCzu1cl7MzozkBINES6R9LPyxEJ iIO66N0MlU0eSCmr43JOPhtUawDv5+hMJktkRI1DpS55Coy85M2VltLsn2G685gu p4d3tSbIv4vE4Xm5pVnBgViqz8Si111fFjYODgAR01zf848gSqRLKj7pH4AJCExr sVVM2STrNea1l4Vq8SEinKD1BLSRcizSiFT5N/bRVMRYYCEFzvTiVHW/IjNyoUIa G0f4vM0lOXtO6mwJ7A4pVifNoGXVDRFa0yaESJOVk8ne8l170dULuMBkMy5wccQz PgTXYJszGfqMoc5rg8IyznAC0mFpLrbYEbJN1YdCX9ySLPmq1odXw5+oVRYKDoqR mVXvOYc5qqU25R/4aM2t6LLwu32qA/sL3iDBKZm0MCjdf5CanN8JDrreWfm9fNKr w5KnWbOCMCfvSa2FUFemcx61V2KkL6G2INTgfk2JOLUjTJCC9Q== =4yDw -----END PGP SIGNATURE----- --ripabaipah35cryc-- --===============0222559186== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0222559186==--