linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Colin King <colin.king@canonical.com>,
	Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
	freedreno@lists.freedesktop.org
Cc: kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH][V2] drm/msm: Fix potential integer overflow on 32 bit multiply
Date: Sat, 2 Oct 2021 00:45:42 +0300	[thread overview]
Message-ID: <e1d66d58-7bfa-ec21-9c19-5c81c071932a@linaro.org> (raw)
In-Reply-To: <20210929115352.212849-1-colin.king@canonical.com>

On 29/09/2021 14:53, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> In the case where clock is 2147485 or greater the 32 bit multiplication
> by 1000 will cause an integer overflow. Fix this by making the constant
> 1000 an unsigned long to ensure a long multiply occurs to avoid the

You are talking about 'unsigned long' here, however in the patch you've 
used just 'unsigned' suffix. So, which one should be used?

I suspect that wanted to use UL here, since mode->clock is int, so it is 
int * unsigned.

Also I'd suggest to define a helper function macro in the drm_modes.h(?) 
that would take struct drm_display_mode pointer and return proper clock. 
See icc_units_to_bps() for the inspiration.


> overflow before assigning the result to the long result in variable
> requested.  Most probably a theoretical overflow issue, but worth fixing
> to clear up static analysis warnings.
> 
> Addresses-Coverity: ("Unintentional integer overflow")
> Fixes: c8afe684c95c ("drm/msm: basic KMS driver for snapdragon")
> Fixes: 3e87599b68e7 ("drm/msm/mdp4: add LVDS panel support")
> Fixes: 937f941ca06f ("drm/msm/dp: Use qmp phy for DP PLL and PHY")
> Fixes: ab5b0107ccf3 ("drm/msm: Initial add eDP support in msm drm driver (v5)")
> Fixes: a3376e3ec81c ("drm/msm: convert to drm_bridge")
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> V2: Find and fix all unintentional integer overflows that match this
>      overflow pattern.
> ---
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c    | 2 +-
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c   | 2 +-
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 2 +-
>   drivers/gpu/drm/msm/dp/dp_ctrl.c                    | 4 ++--
>   drivers/gpu/drm/msm/edp/edp_connector.c             | 2 +-
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c              | 2 +-
>   drivers/gpu/drm/msm/hdmi/hdmi_connector.c           | 2 +-
>   7 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c
> index 88645dbc3785..83140066441e 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c
> @@ -50,7 +50,7 @@ static void mdp4_dtv_encoder_mode_set(struct drm_encoder *encoder,
>   
>   	DBG("set mode: " DRM_MODE_FMT, DRM_MODE_ARG(mode));
>   
> -	mdp4_dtv_encoder->pixclock = mode->clock * 1000;
> +	mdp4_dtv_encoder->pixclock = mode->clock * 1000U;
>   
>   	DBG("pixclock=%lu", mdp4_dtv_encoder->pixclock);
>   
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> index 10eb3e5b218e..d90dc0a39855 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c
> @@ -225,7 +225,7 @@ static void mdp4_lcdc_encoder_mode_set(struct drm_encoder *encoder,
>   
>   	DBG("set mode: " DRM_MODE_FMT, DRM_MODE_ARG(mode));
>   
> -	mdp4_lcdc_encoder->pixclock = mode->clock * 1000;
> +	mdp4_lcdc_encoder->pixclock = mode->clock * 1000U;
>   
>   	DBG("pixclock=%lu", mdp4_lcdc_encoder->pixclock);
>   
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> index 7288041dd86a..a965e7962a7f 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
> @@ -64,7 +64,7 @@ static int mdp4_lvds_connector_mode_valid(struct drm_connector *connector,
>   	struct drm_encoder *encoder = mdp4_lvds_connector->encoder;
>   	long actual, requested;
>   
> -	requested = 1000 * mode->clock;
> +	requested = 1000U * mode->clock;
>   	actual = mdp4_lcdc_round_pixclk(encoder, requested);
>   
>   	DBG("requested=%ld, actual=%ld", requested, actual);
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 62e75dc8afc6..6babeb79aeb0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -1316,7 +1316,7 @@ static int dp_ctrl_enable_mainlink_clocks(struct dp_ctrl_private *ctrl)
>   	opts_dp->lanes = ctrl->link->link_params.num_lanes;
>   	opts_dp->link_rate = ctrl->link->link_params.rate / 100;
>   	dp_ctrl_set_clock_rate(ctrl, DP_CTRL_PM, "ctrl_link",
> -					ctrl->link->link_params.rate * 1000);
> +					ctrl->link->link_params.rate * 1000U);
>   
>   	phy_configure(phy, &dp_io->phy_opts);
>   	phy_power_on(phy);
> @@ -1336,7 +1336,7 @@ static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl)
>   	int ret = 0;
>   
>   	dp_ctrl_set_clock_rate(ctrl, DP_STREAM_PM, "stream_pixel",
> -					ctrl->dp_ctrl.pixel_rate * 1000);
> +					ctrl->dp_ctrl.pixel_rate * 1000U);
>   
>   	ret = dp_power_clk_enable(ctrl->power, DP_STREAM_PM, true);
>   	if (ret)
> diff --git a/drivers/gpu/drm/msm/edp/edp_connector.c b/drivers/gpu/drm/msm/edp/edp_connector.c
> index 73cb5fd97a5a..837e7873141f 100644
> --- a/drivers/gpu/drm/msm/edp/edp_connector.c
> +++ b/drivers/gpu/drm/msm/edp/edp_connector.c
> @@ -64,7 +64,7 @@ static int edp_connector_mode_valid(struct drm_connector *connector,
>   	struct msm_kms *kms = priv->kms;
>   	long actual, requested;
>   
> -	requested = 1000 * mode->clock;
> +	requested = 1000L * mode->clock;
>   	actual = kms->funcs->round_pixclk(kms,
>   			requested, edp_connector->edp->encoder);
>   
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 6e380db9287b..e4c68a59772a 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -209,7 +209,7 @@ static void msm_hdmi_bridge_mode_set(struct drm_bridge *bridge,
>   
>   	mode = adjusted_mode;
>   
> -	hdmi->pixclock = mode->clock * 1000;
> +	hdmi->pixclock = mode->clock * 1000U;
>   
>   	hstart = mode->htotal - mode->hsync_start;
>   	hend   = mode->htotal - mode->hsync_start + mode->hdisplay;
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> index 58707a1f3878..ce116a7b1bba 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> @@ -385,7 +385,7 @@ static int msm_hdmi_connector_mode_valid(struct drm_connector *connector,
>   	struct msm_kms *kms = priv->kms;
>   	long actual, requested;
>   
> -	requested = 1000 * mode->clock;
> +	requested = 1000U * mode->clock;
>   	actual = kms->funcs->round_pixclk(kms,
>   			requested, hdmi_connector->hdmi->encoder);
>   
> 


-- 
With best wishes
Dmitry

      reply	other threads:[~2021-10-01 21:45 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-29 11:53 [PATCH][V2] drm/msm: Fix potential integer overflow on 32 bit multiply Colin King
2021-10-01 21:45 ` Dmitry Baryshkov [this message]

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=e1d66d58-7bfa-ec21-9c19-5c81c071932a@linaro.org \
    --to=dmitry.baryshkov@linaro.org \
    --cc=airlied@linux.ie \
    --cc=colin.king@canonical.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=kernel-janitors@vger.kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robdclark@gmail.com \
    --cc=sean@poorly.run \
    /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).