Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCHv5 4/6] drm/omap: fix incorrect union usage
To:     Sebastian Reichel <sebastian.reichel@collabora.com>,
        Sebastian Reichel <sre@kernel.org>,
        Tony Lindgren <tony@atomide.com>, Pavel Machek <pavel@ucw.cz>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>
CC:     "H. Nikolaus Schaller" <hns@goldelico.com>,
        <dri-devel@lists.freedesktop.org>, <linux-omap@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <kernel@collabora.com>
References: <20181121160916.22017-1-sebastian.reichel@collabora.com>
 <20181121160916.22017-5-sebastian.reichel@collabora.com>
From:   Tomi Valkeinen <tomi.valkeinen@ti.com>
Message-ID: <f22070d8-e5ca-a8ef-2833-f6bef6801682@ti.com>
Date:   Mon, 26 Nov 2018 11:32:08 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <20181121160916.22017-5-sebastian.reichel@collabora.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 21/11/18 18:09, Sebastian Reichel wrote:
> The DSI encoder sets dssdev->ops->dsi.set_config, which is stored at the
> same offset as dssdev->ops->hdmi.set_hdmi_mode. The code in omap_encoder
> only checks if dssdev->ops->hdmi.set_hdmi_mode is NULL. Due to the way
> union works, it won't be NULL if dsi.set_config is set. This means
> dsi_set_config will be called with config=hdmi_mode=false=NULL parameter
> resulting in a NULL dereference. Also the dereference happens while
> console is locked, so kernel hangs without any debug output without
> "fb.lockless_register_fb=1" parameter.
> 
> This restructures the code, so that the HDMI mode is only configured
> for HDMI output types. The new function also has a safe-guard directly
> before accessing the union, that can be optimized away by the compiler
> when the function is inlined and HDMI type has already been checked.
> 
> Fixes: 83910ad3f51fb ("drm/omap: Move most omap_dss_driver operations to omap_dss_device_ops")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_encoder.c | 62 +++++++++++++++-----------
>  1 file changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c
> index 32bbe3a80e7d..f356821cd078 100644
> --- a/drivers/gpu/drm/omapdrm/omap_encoder.c
> +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
> @@ -52,17 +52,48 @@ static const struct drm_encoder_funcs omap_encoder_funcs = {
>  	.destroy = omap_encoder_destroy,
>  };
>  
> +static void omap_encoder_hdmi_mode_set(struct drm_encoder *encoder,
> +				       struct drm_display_mode *adjusted_mode)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
> +	struct omap_dss_device *dssdev = omap_encoder->output;
> +	struct drm_connector *connector;
> +	bool hdmi_mode;
> +
> +	hdmi_mode = false;
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +		if (connector->encoder == encoder) {
> +			hdmi_mode = omap_connector_get_hdmi_mode(connector);
> +			break;
> +		}
> +	}
> +
> +	/* safe-guard for accessing dssdev->ops->hdmi union */
> +	if (dssdev->output_type != OMAP_DISPLAY_TYPE_HDMI)
> +		return;

If you safeguard the function, it would make sense to do this at the
very beginning of the function. But to be honest, I think this is just
extra. It's a static function, named "hdmi", so I think we can trust
that we call it correctly.

If you're ok with it, I can pick this patch up and drop the safeguard.
Or you can send a new one.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki