linux-kernel - Re: [PATCH v3 8/9] i2c: Support dynamic address translation

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: <21399801.1it9Cbg30u@fw-rgant>
Date: Tue, 03 Dec 2024 09:59:03 +0100
From: Romain Gantois <romain.gantois@...tlin.com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Kory Maincent <kory.maincent@...tlin.com>, linux-i2c@...r.kernel.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 linux-media@...r.kernel.org, linux-gpio@...r.kernel.org,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Luca Ceresoli <luca.ceresoli@...tlin.com>,
 Andi Shyti <andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Derek Kiernan <derek.kiernan@....com>, Dragan Cvetic <dragan.cvetic@....com>,
 Arnd Bergmann <arnd@...db.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Linus Walleij <linus.walleij@...aro.org>,
 Bartosz Golaszewski <brgl@...ev.pl>, Cosmin Tanislav <demonsingur@...il.com>
Subject: Re: [PATCH v3 8/9] i2c: Support dynamic address translation

Hi,

On vendredi 29 novembre 2024 10:54:35 heure normale d’Europe centrale Tomi Valkeinen wrote:
> Hi Romain,
> 
...
> > ATR channel's translation table whenever an I2C transaction with unmapped
> > clients is requested.
> > 
> > Add a mutex to protect alias_list. This prevents
> > i2c_atr_dynamic_attach/detach_addr from racing with the bus notifier
> > handler to modify alias_list.
> > 
> > Signed-off-by: Romain Gantois <romain.gantois@...tlin.com>
> > ---
> > 
> >   drivers/i2c/i2c-atr.c         | 244
> >   ++++++++++++++++++++++++++++++++----------
> >   drivers/media/i2c/ds90ub960.c |   2 +-
> >   include/linux/i2c-atr.h       |  13 ++-
> >   3 files changed, 202 insertions(+), 57 deletions(-)
> 
> This fails with:
> 
> WARNING: CPU: 1 PID: 360 at lib/list_debug.c:35
> __list_add_valid_or_report+0xe4/0x100
> 
> as the i2c_atr_create_c2a() calls list_add(), but i2c_atr_attach_addr(),
> which is changed to use i2c_atr_create_c2a(), also calls list_add().
> 
> Also, if you add i2c_atr_create_c2a() which hides the allocation and
> list_add, I think it makes sense to add a i2c_atr_destroy_c2a() to
> revert that.
> 

Sure, I just thought that it was safer to have an explicit "kfree" in the
code, as it would be clear that the c2a pointer shouldn't be used after this.
But setting the pointer to NULL after calling i2c_atr_destroy_c2a() would
essentially achieve the same thing, so I'll be going with your suggestion.

> There's also a memory error "BUG: KASAN: slab-use-after-free in
> __lock_acquire+0xc4/0x375c" (see below) when unloading the ub960 or
> ub953 driver. I haven't looked at that yet.
> 

I don't have the hardware to actually reproduce this but I'll see if I can
find out what the problem is by reading the code.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com