linux-kernel - Re: [PATCH v4 1/2] x86: Add basic support for the Congatec CGEB BIOS interface

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: <87sev6m14n.ffs@tglx>
Date: Thu, 15 Aug 2024 14:17:12 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Mary Strodl <mstrodl@....rit.edu>, linux-kernel@...r.kernel.org
Cc: akpm@...ux-foundation.org, urezki@...il.com, hch@...radead.org,
 linux-mm@...ck.org, lee@...nel.org, andi.shyti@...nel.org,
 linux-i2c@...r.kernel.org, s.hauer@...gutronix.de,
 christian.gmeiner@...il.com, Mary Strodl <mstrodl@....rit.edu>
Subject: Re: [PATCH v4 1/2] x86: Add basic support for the Congatec CGEB
 BIOS interface

Mary!

On Wed, Aug 14 2024 at 14:47, Mary Strodl wrote:

So this caught the attention of my mail filter because the subsystem
prefix is 'x86:' while it obviously should be 'mfd:'

> The Congatec CGEB is a BIOS interface found on some Congatec x86
> modules. It provides access to on board peripherals like I2C busses
> and watchdogs. This driver contains the basic support for accessing
> the CGEB interface and registers the child devices.
>
> Signed-off-by: Sascha Hauer <s.hauer@...gutronix.de>
> Signed-off-by: Christian Gmeiner <christian.gmeiner@...il.com>
> Signed-off-by: Mary Strodl <mstrodl@....rit.edu>

This Signed-off-by chain is invalid. See Documentation/process/

Aisde of that. How is any of this supposed to work. What is the user
mode helper doing and why does this need this netlink indirection?

And looking at that reference implementation on github makes me just
shudder:

        board.mem_fd = open("/dev/mem", O_RDONLY);

and then it allocates anon memory:

	board->code = mmap(NULL, msg->code.length,
		           PROT_READ | PROT_WRITE,
			   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);

which it hands back to the kernel so the kernel can copy stuff into it
followed by some other command:

         mprotect(board->code, msg->code.length,
		  PROT_EXEC | PROT_READ | PROT_WRITE)

Seriously? RWX mappings?

So this copies a code blob out of the BIOS region into user space and
lets user space use this blob to interact with the BIOS extension,
right?

So an i2c transfer does a full round trip:

   i2c::xfer -> cgeb -> netlink -> user helper -> execute random code ->
   user helper -> netlink -> cgeb -> ...

right?

Has anyone tried to analyze what this BIOS provided code blob is
actually doing?

All it does is to poke at a range of IOPORTS with in*(), out*() and that
poking mechanism depends on the generation of that CGEB implementation.

Congatec even provides the GPL2 licenced source for this pokery as a
kernel driver.

Three generations of interfaces and for each the poking is about 200
lines of unreadable, malformated gunk, which can be probably condensed
to 100 lines of readable kernel code for each generation.

Add a bunch of helpers which set up the various transfers for the
subdevices and you can spare all this horrible nonsense with user mode
helper, executing random BIOS provided gunk, netlink and completely ill
defined data structures.

Thanks,

        tglx