[PATCH 1/3] Fix xf86HandleColormaps() crash at color depth 30.

[Michel Dänzer]

On 2017-12-22 02:49 AM, Mario Kleiner wrote:
> On 12/19/2017 09:58 AM, Michel Dänzer wrote:
>> On 2017-12-18 11:36 PM, Mario Kleiner wrote:
>>> The size of the X-Server pScreenPriv->PreAllocIndices
>>> array allocated within xf86HandleColormaps() is given
>>> by the given maxColors argument, but the range of
>>> indices by which the PreAllocIndices array is indexed
>>> in routines like CMapReinstallMap() seems to be up to
>>> 1023 on a 10 bpc / depth 30 screen, leading to an
>>> out-of-bounds access and server crash.
>>>
>>> Raising maxColors to 1024 fixes the crash at server
>>> startup with X-Screen color depth 30.
>>>
>>> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
>>> ---
>>>   src/drmmode_display.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
>>> index 7ad3235..67db86e 100644
>>> --- a/src/drmmode_display.c
>>> +++ b/src/drmmode_display.c
>>> @@ -2730,8 +2730,9 @@ Bool drmmode_setup_colormap(ScreenPtr pScreen,
>>> ScrnInfoPtr pScrn)
>>>                  "Initializing kms color map\n");
>>>       if (!miCreateDefColormap(pScreen))
>>>           return FALSE;
>>> +
>>>       /* all radeons support 10 bit CLUTs */
>>> -    if (!xf86HandleColormaps(pScreen, 256, 10,
>>> +    if (!xf86HandleColormaps(pScreen, 1024, 10,
>>>                    NULL, NULL,
>>>                    CMAP_PALETTED_TRUECOLOR
>>>   #if 0 /* This option messes up text mode! (eich at suse.de) */
>>>
>>
>> The hardware CLUT isn't actually used at depth 30, so a better solution
>> would be to skip the xf86HandleColormaps call (and probably also set
>> xf86CrtcFuncsRec::gamma_set = NULL) for pScrn->depth == 30.
>>
> 
> I know. Skipping the xf86HandleColormaps() function was the solution i
> first used, but then i thought we might want to keep it as
> future-proofing in case we'd implement hw gamma tables in the kms
> drivers for depth > 24 by switching to the piecewise linear hw gamma
> tables.

We can cross that bridge when we get there.


> Haven't looked yet what DC uses in amdgpu-kms, and what therefore makes
> sense for the amdgpu-ddx?

The legacy/non-atomic KMS gamma ramp has no effect at depth 30 with DC
as well, so the plan for xf86-video-amdgpu is what I described above.
Let's not diverge on this without good reason.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer