Re: [PATCH 30/30] fbdev: Make support for userspace interfaces configurable

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4429 bytes --]

Hi

Am 07.06.23 um 10:48 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> Thanks for your patch!
> 
> On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Add Kconfig option CONFIG_FB_DEVICE and make the virtual fbdev
>> device optional. If the new option has not been selected, fbdev
>> does not create a files in devfs or sysfs.
>>
>> Most modern Linux systems run a DRM-based graphics stack that uses
>> the kernel's framebuffer console, but has otherwise deprecated fbdev
>> support. Yet fbdev userspace interfaces are still present.
>>
>> The option makes it possible to use the fbdev subsystem as console
>> implementation without support for userspace. This closes potential
>> entry points to manipulate kernel or I/O memory via framebuffers. It
> 
> I'd leave out the part about manipulating kernel memory, as that's a
> driver bug, if possible.

The driver/core distinction is somewhat fuzzy: the recent bug with OOB 
access was introduced accidentally in shared helper code within DRM.

And whenever I want to modify the fbdev code, I have to start bugfixing 
first. Just look at this patchset. A good number of the patches are 
bugfixes. Driver or not, I no longer trust any of the fbdev code to get 
anything right.


> 
>> also prevents the execution of driver code via ioctl or sysfs, both
>> of which might allow malicious software to exploit bugs in the fbdev
>> code.
> 
> Of course disabling ioctls reduces the attack surface, and this is not
> limited to fbdev... ;-)

Other subsystems should do the same where possible. The specific problem 
with DRM-plus-fbdev is that the fbdev device doesn't provide any 
additional value. It's too limited in functionality (even by fbdev 
standards), a possible source for bugs, and today's userspace wants DRM 
functionality.


> 
> I'm wondering if it would be worthwhile to optionally provide a more
> limited userspace API for real fbdev drivers:
>    1. No access to MMIO, only to the mapped frame buffer,
>    2. No driver-specific ioctls, only the standard ones.

That issue is only relevant to fbdev drivers and would be a separate 
patchset. My concern lies with the current distributions, which don't 
need the fbdev device and shouldn't provide it for the given reasons.


> 
>> A small number of fbdev drivers require struct fbinfo.dev to be
>> initialized, usually for the support of sysfs interface. Make these
>> drivers depend on FB_DEVICE. They can later be fixed if necessary.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
>> --- a/drivers/video/fbdev/Kconfig
>> +++ b/drivers/video/fbdev/Kconfig
>> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
>>            combination with certain motherboards and monitors are known to
>>            suffer from this problem.
>>
>> +config FB_DEVICE
>> +        bool "Provide legacy /dev/fb* device"
> 
> Perhaps "default y if !DRM", although that does not help for a
> mixed drm/fbdev kernel build?

We could simply set it to "default y".  But OTOH is it worth making it a 
default? Distributions will set it to the value they need/want. The very 
few people that build their own kernels to get certain fbdev drivers 
will certainly be able to enable the option by hand as well.


> 
> Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE,
> to be selected by both FB and DRM_FBDEV_EMULATION?
> Then FB_DEVICE can depend on FB_CORE, and default to y if FB.

That wouldn't work. In Tumbleweed, we still have efifb and vesafb 
enabled under certain conditions; merely for the kernel console. We'd 
have to enable CONFIG_FB, which would bring back the device.

Best regards
Thomas

> 
>> +        depends on FB
>> +        help
>> +         Say Y here if you want the legacy /dev/fb* device file. It's
>> +         only required if you have userspace programs that depend on
>> +         fbdev for graphics output. This does not effect the framebuffer
> 
> affect
> 
>> +         console.
>> +
>>   config FB_DDC
>>          tristate
>>          depends on FB
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]