linux-kernel - Re: [PATCH] tuners: tda8290: reduce stack usage with kasan

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] [day] [month] [year] [list]

Date:   Tue, 12 Dec 2017 08:02:47 -0800
From:   Joe Perches <joe@...ches.com>
To:     Arnd Bergmann <arnd@...db.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>
Cc:     Michael Ira Krufky <mkrufky@...uxtv.org>,
        linux-media <linux-media@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tuners: tda8290: reduce stack usage with kasan

On Tue, 2017-12-12 at 15:21 +0100, Arnd Bergmann wrote:
> On Tue, Dec 12, 2017 at 1:45 PM, Mauro Carvalho Chehab
> <mchehab@...nel.org> wrote:
> > Em Tue, 12 Dec 2017 03:42:32 -0800
> > Joe Perches <joe@...ches.com> escreveu:
> > 
> > > > I actually thought about marking them 'const' here before sending
> > > > (without noticing the changelog text) and then ran into what must
> > > > have led me to drop the 'const' originally: tuner_i2c_xfer_send()
> > > > takes a non-const pointer. This can be fixed but it requires
> > > > an ugly cast:
> > > 
> > > Casting away const is always a horrible hack.
> > > 
> > > Until it could be changed, my preference would
> > > be to update the changelog and perhaps add to
> > > the changelog the reason why it can not be const
> > > as detailed below.
> > > 
> > > ie: xfer_send and xfer_xend_recv both take a
> > >     non-const unsigned char *
> 
> Ok.
> 
> > Perhaps, on a separate changeset, we could change I2C routines to
> > accept const unsigned char pointers. This is unrelated to tda8290
> > KASAN fixes. So, it should go via I2C tree, and, once accepted
> > there, we can change V4L2 drivers (and other drivers) accordingly.
> 
> I don't see how that would work unfortunately. i2c_msg contains
> a pointer to the data, and that is used for both input and output,
> including arrays like
> 
>         struct i2c_msg msgs[] = {
>                 {
>                         .addr = dvo->slave_addr,
>                         .flags = 0,
>                         .len = 1,
>                         .buf = &addr,
>                 },
>                 {
>                         .addr = dvo->slave_addr,
>                         .flags = I2C_M_RD,
>                         .len = 1,
>                         .buf = val,
>                 }
>         };
> 
> that have one constant output pointer and one non-constant
> input pointer. We could add an anonymous union for 'buf'
> to make that two separate pointers, but that's barely any
> better than the cast, and it would break the named initializers
> in the example above, at least on older compilers. Adding
> a second pointer to i2c_msg would add a bit of bloat and
> also require tree-wide changes or ugly hacks.

Perhaps add something like

struct i2c_msg_set {
	__u16 addr;		/* slave address			*/
	__u16 flags;
	__u16 len;		/* msg length				*/
	const __u8 *buf;	/* pointer to read-only msg data	*/
};

struct i2c_msg_get {
	__u16 addr;		/* slave address			*/
	__u16 flags;
	__u16 len;		/* msg length				*/
	__u8 *buf;		/* pointer to writeable msg data	*/
};

to the uapi include and use that where appropriate
but where a write then read is done via a single
i2c_msg array, it's not really feasible either.

Probably better to avoid any churn and just mark
all these as static rather than static const.