| 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
| ||
|
Message-ID: <20181026144647.11926142@bbrezillon>
Date: Fri, 26 Oct 2018 14:46:47 +0200
From: Boris Brezillon <boris.brezillon@...tlin.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: Wolfram Sang <wsa@...-dreams.de>,
Linux I2C <linux-i2c@...r.kernel.org>,
Jonathan Corbet <corbet@....net>,
"open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
gregkh <gregkh@...uxfoundation.org>,
Przemyslaw Sroka <psroka@...ence.com>,
Arkadiusz Golec <agolec@...ence.com>,
Alan Douglas <adouglas@...ence.com>,
Bartosz Folta <bfolta@...ence.com>,
Damian Kos <dkos@...ence.com>,
Alicja Jurasik-Urbaniak <alicja@...ence.com>,
Cyprian Wronka <cwronka@...ence.com>,
Suresh Punnoose <sureshp@...ence.com>,
Rafal Ciepiela <rafalc@...ence.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
Nishanth Menon <nm@...com>, Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
DTML <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Vitor Soares <Vitor.Soares@...opsys.com>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Linus Walleij <linus.walleij@...aro.org>,
Xiang Lin <Xiang.Lin@...aptics.com>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
Sekhar Nori <nsekhar@...com>,
Przemyslaw Gaj <pgaj@...ence.com>,
Peter Rosin <peda@...ntia.se>,
Mike Shettel <mshettel@...eaurora.org>,
Stephen Boyd <swboyd@...omium.org>,
Joe Perches <joe@...ches.com>
Subject: Re: [PATCH v9 6/9] i3c: master: Add driver for Cadence IP
On Fri, 26 Oct 2018 12:01:52 +0200
Arnd Bergmann <arnd@...db.de> wrote:
> On Fri, Oct 26, 2018 at 9:57 AM Boris Brezillon
> <boris.brezillon@...tlin.com> wrote:
> > On Fri, 26 Oct 2018 09:43:25 +0200
> > Arnd Bergmann <arnd@...db.de> wrote:
> >
> > > On Thu, Oct 25, 2018 at 6:30 PM Boris Brezillon
> > > <boris.brezillon@...tlin.com> wrote:
> > > > On Thu, 25 Oct 2018 18:13:51 +0200 Arnd Bergmann <arnd@...db.de> wrote:
> > > > On Thu, Oct 25, 2018 at 6:07 PM Boris Brezillon <boris.brezillon@...tlin.com> wrote:
> > > > > > On Thu, 25 Oct 2018 17:30:26 +0200
> > > Ok. Is i3c_master_send_ccc_cmd_locked() what implements the public
> > > interfaces then, or is this something else?
> >
> > i3c_master_send_ccc_cmd_locked() calls master->ops->send_ccc_cmd(), so
> > it's part of the master controller interface.
> >
> > >
> > > If you place a buffer on the stack, it is not DMA capable, but
> > > it is guaranteed to be at least 32-bit word aligned, and should
> > > not cause an exception in readsl(), unless it starts with a couple of
> > > (not multiple of four) extra bytes that are not sent to the devices.
> > > Is that what happens here?
> >
> > Here is the report I received from Vitor:
> >
> > "
> > Hi Boris,
> >
> >
> > I'm trying this new patch-set version but I get some issues when use
> > readsl() function.
> >
> > Basically the system complain about memory alignment.
> >
>
> > > +static int i3c_master_getpid_locked(struct i3c_master_controller *master,
> > > + struct i3c_device_info *info)
> > > +{
> > > + struct i3c_ccc_getpid getpid;
> >
> > at this point the getpid struct it is already unaligned with
> >
> > i3c_master_getpid_locked:1129 getpid_add=0x9a249c7a
> >
> > > + struct i3c_ccc_cmd_dest dest = {
> > > + .addr = info->dyn_addr,
> > > + .payload.len = sizeof(struct i3c_ccc_getpid),
> > > + .payload.data = &getpid,
> > > + };
>
> > > +}
> > > +
> >
> > and them when
> >
> > static void dw_i3c_master_read_rx_fifo(struct dw_i3c_master *master,
> > u8 *bytes, int nbytes)
> > {
> > readsl(master->regs + RX_TX_DATA_PORT, bytes, nbytes / 4);
> > ...
> > }
>
> Ok, I spent an hour chasing the ARM implementation and finding
> no way this could go wrong here. I see that 'struct i3c_ccc_getpid'
> may be misaligned on the stack (it normally won't be), and that
> the ARM readsl() has a lot of extra code to handle unaligned
> output.
I didn't have this problem on xtensa either.
> However, the dump that Vitor reports
>
> > [ECR ]: 0x00230400 => Misaligned r/w from 0x9a249c7a
> > [EFA ]: 0x9a249c7a
> > [BLINK ]: dw_i3c_master_irq_handler+0x200/0x2fc [dw_i3c_master]
>
> Is from an arch/arc kernel that uses asm-generic/io.h, and
> that stores the output using a u32 pointer:
>
> static inline void readsl(const volatile void __iomem *addr, void *buffer,
> unsigned int count)
> {
> if (count) {
> u32 *buf = buffer;
>
> do {
> u32 x = __raw_readl(addr);
> *buf++ = x;
> } while (--count);
> }
> }
>
> This is apparently not allowed on ARC when 'buffer' is
> unaligned. I think what we need here is to use
> put_unaligned() instead of the pointer dereference.
> For architectures that can do unaligned accesses,
> the result is the same, but for ARC it will fix the problem.
Okay, so writesl()/readsl() should deal with unaligned pointers, and
default implementations should be fixed. I guess you'll send a patch to
use put/get_unaligned().
>
> > > One way to address this might be to always bounce any
> > > messages that are less than a cache line through a
> > > (pre-)kmallocated buffer, and require any longer messages
> > > to be cache capable. This could also solve the issue with
> > > readsl(), but it would be a rather confusing user interface.
> > >
> > > Another option might be to have separate interfaces for
> > > "short" and "long" messages at the API level and have
> > > distinct rules for those: short would always be bounced
> > > by the i3c code, and long puts restrictions on the buffer
> > > location.
> >
> > Hm, let's keep the API simple. I'll just mandate that all payload bufs
> > passed to i3c_master_send_ccc_cmd_locked() be dynamically allocated.
>
> Ok. What about i2c commands sent to the same i3c controller
> then?
Still not taken care of.
> Do we need to copy those to satisfy the requirements
> of the i3c layer?
I guess we should. The question is, should we do that unconditionally
or should we try to optimize thins with something like:
if (!virt_addr_valid(xfer->buf) ||
object_is_on_stack(xfer->buf))
/* Alloc bounce buf. */
else
/* Use provided buf. */
Powered by blists - more mailing lists