linux-kernel - Re: [PATCH] tty: max310x: work around regmap->regcache data corruption

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <66946666-eb33-431d-9870-7046c39ffb4e@sirena.org.uk>
Date:   Mon, 4 Dec 2023 19:16:57 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Hugo Villeneuve <hugo@...ovil.com>
Cc:     Jan Kundrát <jan.kundrat@...net.cz>,
        Cosmin Tanislav <cosmin.tanislav@...log.com>,
        linux-serial@...r.kernel.org,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tty: max310x: work around regmap->regcache data
 corruption

On Mon, Dec 04, 2023 at 01:59:22PM -0500, Hugo Villeneuve wrote:

> Based on tas2770.c, I am assuming I could redefine the code to look
> like this:

> /* SC16IS7XX register definitions */
> #define SC16IS7XX_REG(page, reg)        ((page * 128) + reg)
> 
> #define SC16IS7XX_RHR_REG	SC16IS7XX_REG(0, 0x00) /* RX FIFO */
> #define SC16IS7XX_THR_REG	SC16IS7XX_REG(0, 0x00) /* TX FIFO */
> #define SC16IS7XX_IER_REG	SC16IS7XX_REG(0, 0x01) /* Interrupt enable */
> #define SC16IS7XX_IIR_REG	SC16IS7XX_REG(0, 0x02) /* Interrupt Identification (read) */
> #define SC16IS7XX_FCR_REG	SC16IS7XX_REG(0, 0x02) /* FIFO control (write) */
> #define SC16IS7XX_MCR_REG	SC16IS7XX_REG(0, 0x04) /* Modem Control */
> #define SC16IS7XX_LSR_REG	SC16IS7XX_REG(0, 0x05) /* Line Status */
> #define SC16IS7XX_MSR_REG	SC16IS7XX_REG(0, 0x06) /* Modem Status */
> #define SC16IS7XX_SPR_REG	SC16IS7XX_REG(0, 0x07) /* Scratch Pad */
> ...
> #define SC16IS7XX_EFR_REG	SC16IS7XX_REG(1, 0x02) /* Enhanced Features */
> #define SC16IS7XX_XON1_REG	SC16IS7XX_REG(1, 0x04) /* Xon1 word */
> #define SC16IS7XX_XON2_REG	SC16IS7XX_REG(1, 0x05) /* Xon2 word */
> #define SC16IS7XX_XOFF1_REG	SC16IS7XX_REG(1, 0x06) /* Xoff1 word */
> #define SC16IS7XX_XOFF2_REG	SC16IS7XX_REG(1, 0x07) /* Xoff2 word */

> static const struct regmap_range_cfg sc16is7xx_regmap_ranges[] = {
> 	{
> 		.range_min = 0,
> 		.range_max = 1 * 128,
> 		.selector_reg = SC16IS7XX_LCR_REG,
> 		.selector_mask = 0xff,
> 		.selector_shift = 0,
> 		.window_start = 0,
> 		.window_len = 128,
> 	},
> };

That looks plausible - I'd tend to make the range just completely
non-physical (eg, start at some unrepresentable value) so there's no
possible ambiguity with a physical address.  I'm also not clear why
you've made the window be 128 registers wide if it's just this range
from 2-7 that gets switched around, I'd do something more like

#define SC16IS7XX_RANGE_BASE 0x1000
#define SC16IS7XX_WINDOW_LEN 6
#define SC16IS7XX_PAGED_REG(page, reg) \
	(SC16IS7XX_RANGE_BASE + (SC16IS7XX_WINDOW_LEN * page) + reg)

	.range_min = SC16IS7XX_RANGE_BASE,
	.range_max = SC16IS7XX_RANGE_BASE + (2 * SC16IS7XX_WINDOW_LEN),
	.window_start = 2,
	.window_len = SC16IS7XX_WINDOW_LEN,

You could apply a - 2 offset to reg as well to make the defines for the
registers clearer.  The above means that any untranslated register you
see in I/O traces should be immediately obvious (and more likely to trip
up error handling and tell you about it if it goes wrong) and hopefully
also makes it easier to reason about what's going on.

> But here, selecting the proper "page" is not obvious,
> because to select page 1, we need to write a fixed value of 0xBF to
> the LCR register.

> And when selecting page 0, we must write the previous value that was
> in LCR _before_ we made the switch to page 1...

> How do we do that?

That's one reason why having the range cover all the registers gets
confusing.  That said the code does have special casing for a selector
register in the middle of the range since there were some devices that
just put the paging register in the middle of the range it controls for
some innovative region, the core will notice that this is a selector
register and not do any paging for that register.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)