linux-kernel - Re: [PATCH 1/1] video: Handle HAS

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]

Message-ID: <26c23fb8557d806c12a246caa575e4f4fc4ea27a.camel@linux.ibm.com>
Date: Mon, 22 Apr 2024 10:34:14 +0200
From: Niklas Schnelle <schnelle@...ux.ibm.com>
To: Helge Deller <deller@...nel.org>
Cc: Helge Deller <deller@....de>, linux-fbdev@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, Arnd Bergmann <arnd@...nel.org>,
        Heiko
 Carstens <hca@...ux.ibm.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] video: Handle HAS_IOPORT dependencies

On Thu, 2024-04-11 at 16:00 +0200, Helge Deller wrote:
> * Niklas Schnelle <schnelle@...ux.ibm.com>:
> > In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
> > compile time. We thus need to #ifdef functions and their callsites which
> > unconditionally use these I/O accessors. In the include/video/vga.h
> > these are conveniently all those functions with the vga_io_* prefix.
> 
> Why don't you code it like in the patch below?
> inb_p(), outb_p() and outw() would then need to be defined externally
> without an implementation so that they would generate link time errors
> (instead of compile time errors).
> 
> diff --git a/include/video/vga.h b/include/video/vga.h
> index 947c0abd04ef..32c915e109fa 100644
> --- a/include/video/vga.h
> +++ b/include/video/vga.h
> @@ -203,18 +203,20 @@ extern int restore_vga(struct vgastate *state);
>  
>  static inline unsigned char vga_io_r (unsigned short port)
>  {
> -	return inb_p(port);
> +	return IS_ENABLED(CONFIG_HAS_IOPORT) ? inb_p(port) : 0;
>  }
>  
>  static inline void vga_io_w (unsigned short port, unsigned char val)
>  {
> -	outb_p(val, port);
> +	if (IS_ENABLED(CONFIG_HAS_IOPORT))
> +		outb_p(val, port);
>  }
>  
>  static inline void vga_io_w_fast (unsigned short port, unsigned char reg,
>  				  unsigned char val)
>  {
> -	outw(VGA_OUT16VAL (val, reg), port);
> +	if (IS_ENABLED(CONFIG_HAS_IOPORT))
> +		outw(VGA_OUT16VAL (val, reg), port);
>  }
>  
>  static inline unsigned char vga_mm_r (void __iomem *regbase, unsigned short port)
> 

This may be personal preference but I feel like link time errors would
be very late to catch a configuration that can't work. Also this would
bypass the __compiletime_error("inb()) requires CONFIG_HAS_IOPORT");
added instead of the in*()/out*() helpers to make it easy to spot the
problem.


I'm not a fan of #ifdeffery either but I think in this case it is
simple, well enough contained and overall there aren't that many spots
where we need to exclude just some sections of code vs entire drivers
with vga.h probably being the worst of them all.

Thanks,
Niklas