kernel-janitors.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Garrit Franke <garritfranke@gmail.com>
To: Liviu Dudau <liviu.dudau@arm.com>
Cc: David Airlie <airlied@linux.ie>,
	Colin Ian King <colin.king@canonical.com>,
	kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/arm: fix unintentional integer overflow on left shift
Date: Thu, 18 Jun 2020 14:36:51 +0000	[thread overview]
Message-ID: <CAD16O86ebsDkwbnuw2G04YZWfukqxJ=_Tex5OT07icEpfPdQNw@mail.gmail.com> (raw)
In-Reply-To: <20200618142106.GK159988@e110455-lin.cambridge.arm.com>

Hi all, newbie here.
Can the BIT macro be safely used on other parts of the kernel as well?
Just using git grep "1 <<" returns a ton of results where bit shifting
is used the old fashioned way.

Am Do., 18. Juni 2020 um 16:23 Uhr schrieb Liviu Dudau <liviu.dudau@arm.com>:
>
> On Thu, Jun 18, 2020 at 01:50:34PM +0100, Colin Ian King wrote:
> > On 18/06/2020 13:14, Liviu Dudau wrote:
> > > On Thu, Jun 18, 2020 at 11:04:00AM +0100, Colin King wrote:
> > >> From: Colin Ian King <colin.king@canonical.com>
> > >
> > > Hi Colin,
> > >
> > >>
> > >> Shifting the integer value 1 is evaluated using 32-bit arithmetic
> > >> and then used in an expression that expects a long value leads to
> > >> a potential integer overflow.
> > >
> > > I'm afraid this explanation makes no sense to me. Do you care to explain better what
> > > you think the issue is? If the shift is done as 32-bit arithmetic and then promoted
> > > to long how does the overflow happen?
> >
> > The shift is performed using 32 bit signed math and then assigned to an
> > unsigned 64 bit long. This if the shift is 31 bits then the signed int
> > conversion of 0x80000000 to unsigned long becomes 0xffffffff80000000.
> > If the shift is more than 32 bits then result overflows and becomes 0x0.
>
> You are right, I've missed the fact that it is signed math. Not very likely that
> we are going to ever have 30 or more CRTCs in the driver, but Coverity has no
> way of knowing that.
>
> Acked-by: Liviu Dudau <liviu.dudau@arm.com>
>
> I will pull this into drm-misc-next today.
>
> Best regards,
> Liviu
>
> >
> > Colin
> >
> > >
> > > Best regards,
> > > Liviu
> > >
> > >> Fix this by using the BIT macro to
> > >> perform the shift to avoid the overflow.
> > >>
> > >> Addresses-Coverity: ("Unintentional integer overflow")
> > >> Fixes: ad49f8602fe8 ("drm/arm: Add support for Mali Display Processors")
> > >> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > >> ---
> > >>  drivers/gpu/drm/arm/malidp_planes.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> > >> index 37715cc6064e..ab45ac445045 100644
> > >> --- a/drivers/gpu/drm/arm/malidp_planes.c
> > >> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> > >> @@ -928,7 +928,7 @@ int malidp_de_planes_init(struct drm_device *drm)
> > >>    const struct malidp_hw_regmap *map = &malidp->dev->hw->map;
> > >>    struct malidp_plane *plane = NULL;
> > >>    enum drm_plane_type plane_type;
> > >> -  unsigned long crtcs = 1 << drm->mode_config.num_crtc;
> > >> +  unsigned long crtcs = BIT(drm->mode_config.num_crtc);
> > >>    unsigned long flags = DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_180 |
> > >>                          DRM_MODE_ROTATE_270 | DRM_MODE_REFLECT_X | DRM_MODE_REFLECT_Y;
> > >>    unsigned int blend_caps = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> > >> --
> > >> 2.27.0.rc0
> > >>
> > >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

  reply	other threads:[~2020-06-18 14:36 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-18 10:04 [PATCH] drm/arm: fix unintentional integer overflow on left shift Colin King
2020-06-18 12:14 ` Liviu Dudau
2020-06-18 12:50   ` Colin Ian King
2020-06-18 14:21     ` Liviu Dudau
2020-06-18 14:36       ` Garrit Franke [this message]
2020-06-18 14:38         ` Colin Ian King
2020-06-18 15:22         ` Dan Carpenter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAD16O86ebsDkwbnuw2G04YZWfukqxJ=_Tex5OT07icEpfPdQNw@mail.gmail.com' \
    --to=garritfranke@gmail.com \
    --cc=airlied@linux.ie \
    --cc=colin.king@canonical.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=kernel-janitors@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=liviu.dudau@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).