Set raw EDID data for Wayland OutputDevices
AbandonedPublic

Authored by dvratil on Jan 15 2018, 11:30 PM.

Details

Reviewers
kwin
Test Plan

kscreen-console correctly shows detailed output names decoded from EDID

Diff Detail

Repository
R108 KWin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
dvratil created this revision.Jan 15 2018, 11:30 PM
Restricted Application added a project: KWin. · View Herald TranscriptJan 15 2018, 11:30 PM
Restricted Application added a subscriber: KWin. · View Herald Transcript
dvratil requested review of this revision.Jan 15 2018, 11:30 PM

kscreen-console correctly shows detailed output names decoded from EDID

What's the use case for this? Do we really want to expose something to the protocol just for a debug tool? Exposing the edid to all applications is something I would rather not want to do.

@dvratil It will help other users as well. Some existing tools can adapt to Wayland, with the EDID information.

@graesslin What is the advantage not to have EDID information available? Traditional it is in X11.
Is there a migration- or access plan in place for EDID? (Link or text is appreciated)

We also use information from EDID in KScreen to display more user-friendly monitor names. I know those fields (eisaid, vendor name, serial number, etc.) are provided by KWin separately, but that makes life harder for tools like KScreen that also support X11 where this information is traditionally available from EDID.

Plus, why is there EDID field in the Wayland protocol then if there's no interest in providing the data for it? In that case, it should be documented that it will always be empty, or removed...

sebas added a subscriber: sebas.Jan 17 2018, 11:13 AM

The idea when we designed the OutputManagement protocol was to pass the raw edid to libkscreen, I wonder when and why that changed? If it did, we'd need to parse the edid info somewhere in kwin and pass through individual values, can be done, but I'm still not sure why the idea had changed, and we'd also create a wart in the protocol since we changed our mind way too late.

I'm just trying to understand the need for it. I fear exposing the edid creates a security risk. Due to that I want to make sure it is really required and not just added because we think in X11. And no, neither too complicated for kscreen nor was like that on X11 is a valid reason. And no app except kscreen should use that anyway. So if apps need it, that protocol here is pointless.

What is your security concern with exposing EDID? I can only think of abusing the EDID to uniquely identify user's computer. However, KWin is already exposing physical size, EISA ID and monitor serial number. The additional information in EDID are color balance, gamma correction, vendor and device name. I don't think any of that additional information present any more security risk that what is exposed right now.

Alternatively, I see that the OutputDevice interface also contains "manufacturer" and "model" fields. Right now the code in KWin sets the manufacturer to EISA ID, which is usually empty, thus useless to most (and sort of contradicts the Wayland documentation "Textual description of the manufacturer." IMO). We could change that to send the value of "vendor" EDID field (which is actual name like "Goldstar Company Ltd") and also set the "model" property (known as "name" in EDID) which, in addition to what is already exposed, would be enough for KScreen and the KScreen Wayland backend could just reconstruct the EDID object from those data (with gamma and other values simply missing, but we don't use those anywhere at this point). Would that be more acceptable, @graesslin ?

Would that be more acceptable,

yes! The values currently set btw are inspired by what weston did back at the time when I implemented the code.

My security concerns are about exposing the edid making the compositor attackable. I have seen displays with very broken edid information, so if one gets this and can have assumptions about how the compositor works it might be possible to trigger buffer overflows or similar nasty stuff through the edid. Yes, I'm slightly paranoid here. That's certainly an area which could use a fair amount of fuzzing.

with gamma and other values simply missing, but we don't use those anywhere at this point

KolorManager is parsing EDID information to build a on the fly color profile for almost any connected monitor by default. Thats is exactly what FireFox does back in times, when no color management system was always to be expected. Omitting EDID would be a serious regression compared to X11 times.
One other feature is KolorManager tries to find a ICC profile for a given monitor in the OpenICC Taxi DB.
Other Color Management software like DisplayCAL, ArgyllCMS need that information too create according ICC profiles, to interact with OpenICC Taxi DB or handle even more details of EDID.

with gamma and other values simply missing, but we don't use those anywhere at this point

KolorManager is parsing EDID information to build a on the fly color profile for almost any connected monitor by default. Thats is exactly what FireFox does back in times, when no color management system was always to be expected. Omitting EDID would be a serious regression compared to X11 times.

This protocol is for KScreen! No other application will be allowed to use it. We currently don't have the security in place, but once we do using this protocol will result in an application quit. If applications need the information we will make them available, but not through this protocol.

color correction is still an unsolved problem on Wayland and our phabricator instance is not the place to discuss anything related to color management on Wayland. And I'm not going to let half finished solutions go in again into KWin. I'm still angry with you for pushing the GSoC branch without consulting with the maintainer.

dvratil abandoned this revision.Jan 23 2018, 12:11 PM

Abandoning in favor of D10040 and D10041