| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 2 Oct 2021 00:45:42 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Colin King <colin.king@...onical.com>,
Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
David Airlie <airlied@...ux.ie>,
Daniel Vetter <daniel@...ll.ch>, linux-arm-msm@...r.kernel.org,
dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org
Cc: kernel-janitors@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH][V2] drm/msm: Fix potential integer overflow on 32 bit
multiply
On 29/09/2021 14:53, Colin King wrote:
> From: Colin Ian King <colin.king@...onical.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@...onical.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
Powered by blists - more mailing lists