linux-kernel - Re: [Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 6 Sep 2016 11:21:10 +0100
From:   Dave Gordon <david.s.gordon@...el.com>
To:     Nicolas Iooss <nicolas.iooss_linux@....org>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about
 uninitialized return value

On 04/09/16 19:58, Nicolas Iooss wrote:
> When building the kernel with clang and some warning flags, the compiler
> reports that the return value of dcs_get_backlight() may be
> uninitialized:
>
>     drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:53:2: error: variable
>     'data' is used uninitialized whenever 'for' loop exits because its
>     condition is false [-Werror,-Wsometimes-uninitialized]
>             for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     drivers/gpu/drm/i915/intel_dsi.h:126:49: note: expanded from macro
>     'for_each_dsi_port'
>     #define for_each_dsi_port(__port, __ports_mask)
>                                 for_each_port_masked(__port, __ports_mask)
>                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     drivers/gpu/drm/i915/i915_drv.h:322:26: note: expanded from macro
>     'for_each_port_masked'
>         for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++)  \
>                                 ^~~~~~~~~~~~~~~~~~~~~~~~~
>     drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:60:9: note:
>     uninitialized use occurs here
>             return data;
>                    ^~~~
>
> As intel_dsi->dcs_backlight_ports seems to be always initialized to a
> non-null value, the content of the for loop is always executed and there
> is no bug in the current code. Nevertheless the compiler has no way of
> knowing that assumption, so initialize variable 'data' to silence the
> warning here.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@....org>

Interesting ... there are two things that could lead to this (possibly) 
incorrect analysis. Either it thinks the loop could be executed zero 
times, which would be a deficiency in the compiler, as the initialiser 
and loop bound are both known (different) constants:

enum port {
         PORT_A = 0,
         PORT_B,
         PORT_C,
         PORT_D,
         PORT_E,
         I915_MAX_PORTS
};

or, it doesn't understand that because we've passed &data to another 
function, it can have been set by the callee. It may be extra confusing 
that the callee takes (void *); or it may be being ultra-sophisticated 
in its analysis and noted that in one error path data is *not* set (and 
we then discard the error and use data anyway). As an experiment, you 
could try:

static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd)
{
         u8 data = 0;

         mipi_dsi_dcs_read(dsi_device, cmd, &data, sizeof(data));

         return data;
}

static u32 dcs_get_backlight(struct intel_connector *connector)
{
         struct intel_encoder *encoder = connector->encoder;
         struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
         struct mipi_dsi_device *dsi_device;
         enum port port;
         u8 data;

         /* FIXME: Need to take care of 16 bit brightness level */
         for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
                 dsi_device = intel_dsi->dsi_hosts[port]->device;
                 data = mipi_dsi_dcs_read1(dsi_device, 
MIPI_DCS_GET_DISPLAY_BRIGHTNESS);
                 break;
         }

         return data;
}

If it complains about that then it's a shortcoming in the loop analysis. 
If not you could try:

static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd)
{
         u8 data;
	ssize_t nbytes = sizeof(data);

	nbytes = mipi_dsi_dcs_read(dsi_device, cmd, &data, nbytes);
	return nbytes == sizeof(data) ? data : 0;
}

and if complains about that then it doesn't understand that passing 
&data allows it to be set. If it doesn't complain about this version, 
then the original error was actually correct, in the sense that data can 
indeed be used uninitialised if certain error paths can be taken.

Here's an R-b for your fix anyway ...

Reviewed-by: Dave Gordon <david.s.gordon@...el.com>

.Dave.

>  drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c b/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
> index ac7c6020c443..eec45856f910 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
> @@ -46,7 +46,7 @@ static u32 dcs_get_backlight(struct intel_connector *connector)
>  	struct intel_encoder *encoder = connector->encoder;
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	struct mipi_dsi_device *dsi_device;
> -	u8 data;
> +	u8 data = 0;
>  	enum port port;
>
>  	/* FIXME: Need to take care of 16 bit brightness level */
>