Re: [PATCH] drm/fb-helper: Fix of-by-one in setcmap_pseudo_palette

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Sean Paul <seanpaul@chromium.org>
Cc: "DRI Development" <dri-devel@lists.freedesktop.org>,
	"Peter Rosin" <peda@axenita.se>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Gustavo Padovan" <gustavo@padovan.org>,
	"David Airlie" <airlied@linux.ie>,
	stable <stable@vger.kernel.org>,
	"Deposite Pirate" <dpirate@metalpunks.info>,
	"Bill Fraser" <bill.fraser@gmail.com>,
	"Michel Dänzer" <michel@daenzer.net>
Subject: Re: [PATCH] drm/fb-helper: Fix of-by-one in setcmap_pseudo_palette
Date: Tue, 16 Jan 2018 23:27:28 +0100	[thread overview]
Message-ID: <CAKMK7uF3KowC9g3tzHbcYAHH7gTTir0Q_YbA=B+4bjbCaBvxdw@mail.gmail.com> (raw)
In-Reply-To: <20180116220032.ceu2lay3jfsjncj4@art_vandelay>

On Tue, Jan 16, 2018 at 11:00 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Fri, Jan 12, 2018 at 10:08:49PM +0100, Daniel Vetter wrote:
>> On Fri, Jan 12, 2018 at 3:08 PM, Sean Paul <seanpaul@chromium.org> wrote:
>> > On Fri, Jan 12, 2018 at 4:48 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> >> [Fair warning: This is pure conjecture right now.]
>> >>
>> >> In
>> >>
>> >> commit b8e2b0199cc377617dc238f5106352c06dcd3fa2
>> >> Author: Peter Rosin <peda@axentia.se>
>> >> Date:   Tue Jul 4 12:36:57 2017 +0200
>> >>
>> >>     drm/fb-helper: factor out pseudo-palette
>> >>
>> >> Peter extracted the pseudo palette computation, but seems to have done
>> >> an off-by-one. I spotted that +1, but then noticed that we've passed
>> >> start++ to (now gone) setcolreg function, so it seemed to all match
>> >> up. Except post vs. pre-increment ftw.
>> >>
>> >> Result is that the palette is off-by-one, and the forground color
>> >> (slot 0) ends up black, rendering a fairly unreadable console.
>> >>
>> >> What baffles me is that on some systems it still seems to work
>> >> somehow, which lead us all down a wild goose chase trying to add
>> >> load_lut calls back in in various places (which was also intentionally
>> >> removed, but really doesn't seem to be the real root cause).
>> >>
>> >> Fixes: b8e2b0199cc3 ("drm/fb-helper: factor out pseudo-palette")
>> >> Cc: Peter Rosin <peda@axenita.se>
>> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> >> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> >> Cc: Gustavo Padovan <gustavo@padovan.org>
>> >> Cc: Sean Paul <seanpaul@chromium.org>
>> >> Cc: David Airlie <airlied@linux.ie>
>> >> Cc: dri-devel@lists.freedesktop.org
>> >> Cc: <stable@vger.kernel.org> # v4.14+
>> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198123
>> >> Reported-by: Deposite Pirate <dpirate@metalpunks.info>
>> >> Reported-by: Bill Fraser <bill.fraser@gmail.com>
>> >> Cc: Deposite Pirate <dpirate@metalpunks.info>
>> >> Cc: Bill Fraser <bill.fraser@gmail.com>
>> >> Cc: Michel Dänzer <michel@daenzer.net>
>> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/drm_fb_helper.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> >> index 035784ddd133..1c3a200c4a10 100644
>> >> --- a/drivers/gpu/drm/drm_fb_helper.c
>> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> >> @@ -1295,7 +1295,7 @@ static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info)
>> >>                         mask <<= info->var.transp.offset;
>> >>                         value |= mask;
>> >>                 }
>> >> -               palette[cmap->start + i] = value;
>> >> +               palette[cmap->start] = value;
>> >
>> > I don't think this is equivalent to what was there before. Before
>> > there we set palette[cmap->start]->palette[cmap->start + cmap->len -
>> > 1], now we're only setting palette[cmap->start].
>> >
>> > In the previous version (before Peter's change), we wrote
>> > palette[cmap->start] twice. So while there is an off-by-one bug, I
>> > don't think this fixes the issue.
>>
>> Well this patch is complete nonsense, no idea what I was thinking ...
>
> [Sending this out since it's in my tree and I'll forget soon]
>
> Equivalent behavior would be:

Are you sure? On 2nd look I think there isn't actually any off-by-one
going on in the referenced patch. The only thing I can see is that the
old code filled out the first entries of an oversized table (more than
16 entries), and only returned -EINVAL when getting to entry 17. The
new one doesn't do that. But more testing in the bug report hints at
this not being the real bug (and I didn't find any callers in the
fbcon code that would indicate there is such a bug).
-Daniel

>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 035784ddd133..df6709768723 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1295,7 +1295,8 @@ static int setcmap_pseudo_palette(struct fb_cmap *cmap, struct fb_info *info)
>                         mask <<= info->var.transp.offset;
>                         value |= mask;
>                 }
> -               palette[cmap->start + i] = value;
> +               if (i > 0)
> +                       palette[cmap->start + i - 1] = value;
>         }
>
>         return 0;
>
>
>> -Daniel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch