linux-kernel - Re: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary

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: <20160216134918.GD2769@codeblueprint.co.uk>
Date:	Tue, 16 Feb 2016 13:49:18 +0000
From:	Matt Fleming <matt@...eblueprint.co.uk>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Alexander Popov <alpopov@...ecurity.com>,
	Arnd Bergmann <arnd@...db.de>,
	Tomi Valkeinen <tomi.valkeinen@...com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org, Peter Jones <pjones@...hat.com>,
	linux-efi@...r.kernel.org
Subject: Re: [PATCH RFC 1/1] x86: fix bad memory access in
 fb_is_primary_device()

[ Including Peter, the efifb maintainer. Original email is here,

    http://marc.info/?l=linux-kernel&m=145552936131335&w=2

  I've snipped some of the quoted text ]

On Tue, 16 Feb, at 08:55:22AM, Ingo Molnar wrote:
> 
> (I've Cc:-ed the EFI-FB and FB gents. Mail quoted below.)
> 
> * Alexander Popov <alpopov@...ecurity.com> wrote:
> 
> > Currently the code in fb_is_primary_device() contains to_pci_dev() macro
> > which is applied to dev from struct fb_info. In some cases this causes
> > bad memory access when fb_is_primary_device() handles fb_info of efifb.
> > The reason is that fb dev of efifb is embedded into struct platform_device
> > but not into struct pci_dev.
> > 
> > We can fix this by checking fb dev bus name in fb_is_primary_device().
> > 
> > It seems that this bug reveals some bigger problem with to_pci_dev(),
> > to_platform_device() and others, which just do container_of() and
> > don't check whether struct device is a part of the appropriate structure.
> > Should we do something more about it?
> > 
> > KASan report:

[...]

> > 
> > Signed-off-by: Alexander Popov <alpopov@...ecurity.com>
> > ---
> >  arch/x86/video/fbdev.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
> > index d5644bb..4999f78 100644
> > --- a/arch/x86/video/fbdev.c
> > +++ b/arch/x86/video/fbdev.c
> > @@ -18,11 +18,12 @@ int fb_is_primary_device(struct fb_info *info)
> >  	struct pci_dev *default_device = vga_default_device();
> >  	struct resource *res = NULL;
> >  
> > -	if (device)
> > -		pci_dev = to_pci_dev(device);
> > -
> > -	if (!pci_dev)
> > +	if (!device || !device->bus ||
> > +		    !device->bus->name || strcmp(device->bus->name, "pci")) {
> >  		return 0;
> > +	}
> > +
> > +	pci_dev = to_pci_dev(device);
> >  
> >  	if (default_device) {
> >  		if (pci_dev == default_device)
> > -- 
> > 1.9.1
> > 

I wonder if this issue could explain some of the efifb issues we've
seen reported on bugzilla.kernel.org in the past where switching from
efifb to some other framebuffer device caused hangs during boot. I'm
struggling to find the relevant bugzilla entries now, though.