linux-kernel - Re: [PATCH v3] serial: make uart_console_write->putchar()'s character an unsigned char

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]

Date:   Thu, 3 Mar 2022 09:55:17 +0000 (GMT)
From:   "Maciej W. Rozycki" <macro@...am.me.uk>
To:     Jiri Slaby <jslaby@...e.cz>
cc:     David Laight <David.Laight@...LAB.COM>,
        'Uwe Kleine-König' 
        <u.kleine-koenig@...gutronix.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Mateusz Holenko <mholenko@...micro.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Liviu Dudau <liviu.dudau@....com>,
        Baruch Siach <baruch@...s.co.il>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Paul Cercueil <paul@...pouillou.net>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Michal Simek <michal.simek@...inx.com>,
        Karol Gugala <kgugala@...micro.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Peter Korsgaard <peter@...sgaard.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Alexander Shiyan <shc_work@...l.ru>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Fabio Estevam <festevam@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Ludovic Desroches <ludovic.desroches@...rochip.com>,
        Andy Gross <agross@...nel.org>,
        "bcm-kernel-feedback-list@...adcom.com" 
        <bcm-kernel-feedback-list@...adcom.com>,
        NXP Linux Team <linux-imx@....com>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        Vineet Gupta <vgupta@...nel.org>,
        Orson Zhai <orsonzhai@...il.com>,
        Tobias Klauser <tklauser@...tanz.ch>,
        Patrice Chotard <patrice.chotard@...s.st.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Takao Orito <orito.takao@...ionext.com>,
        Vladimir Zapolskiy <vz@...ia.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Richard Genoud <richard.genoud@...il.com>,
        Chunyan Zhang <zhang.lyra@...il.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Taichi Sugaya <sugaya.taichi@...ionext.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Kevin Hilman <khilman@...libre.com>,
        Baolin Wang <baolin.wang7@...il.com>,
        Shawn Guo <shawnguo@...nel.org>,
        Andreas Färber <afaerber@...e.de>
Subject: Re: [PATCH v3] serial: make uart_console_write->putchar()'s character
 an unsigned char

On Thu, 3 Mar 2022, Jiri Slaby wrote:

> > The real problem is that using char (or short) for a function parameter
> > or result is very likely to require the compile add code to mask
> > the value to 8 (or 16) bits.
> > 
> > Remember that almost every time you do anything with a signed or unsigned
> > char/short variable the compiler has to use the integer promotion rules
> > to convert the value to int.
> > 
> > You'll almost certainly get better code if the value is left in an
> > int (or unsigned int) variable until the low 8 bits get written to
> > a buffer (or hardware register).
> 
> So should we use int/uint instead of more appropriate shorter types everywhere
> now? The answer is: definitely not. The assembly on x86 looks good (it uses
> movz, no ands), RISC architectures have to do what they chose to.

 We do have an issue, because we still have this:

void uart_console_write(struct uart_port *port, const char *s,
			unsigned int count,
			void (*putchar)(struct uart_port *, int))

and then:

		putchar(port, *s);

there.  Consequently on targets where plain `char' type is signed the 
value retrieved from `*s' has to be truncated in the call to `putchar'.  
And indeed it happens with the MIPS target:

803ae47c:	82050000 	lb	a1,0(s0)
803ae480:	26100001 	addiu	s0,s0,1
803ae484:	02402025 	move	a0,s2
803ae488:	0220f809 	jalr	s1
803ae48c:	30a500ff 	andi	a1,a1,0xff

vs current code:

803ae47c:	82050000 	lb	a1,0(s0)
803ae480:	26100001 	addiu	s0,s0,1
803ae484:	0220f809 	jalr	s1
803ae488:	02402025 	move	a0,s2

(NB the last instruction shown after the call instruction, JALR, is in the 
delay slot that is executed before the PC gets updated).  Now arguably the 
compiler might notice that and use an unsigned LBU load instruction rather 
than the signed LB load instruction, which would make the ANDI instruction 
redundant, but still I think we ought to avoid gratuitous type signedness 
changes.

 So I'd recommend changing `s' here to `const unsigned char *' or, as I 
previously suggested, maybe to `const u8 *' even.

  Maciej