linux-kernel - Re: [PATCH v2 2/2] iio: pressure: bmp280: Use char instead of s32 for data buffer

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]

Message-ID: <20240930095456.599f2304@jic23-huawei>
Date: Mon, 30 Sep 2024 09:54:56 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: dan.carpenter@...aro.org, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: pressure: bmp280: Use char instead of s32
 for data buffer


> > > -	s32 sensor_data[6] __aligned(8);
> > > +	struct {
> > > +		u8 buf[12];
> > > +		aligned_s64 ts;  
> > 
> > I'd missed that this depends on the number of channels.  It makes no functional
> > difference because the core code will happily write over the end of buf, but
> > from a representation point of view this might be
> > 
> > 		u8 buf[8];
> > 		aligned_s64 ts;
> > or
> > 		u8 buf[12];
> > 		aligned_s64 ts;
> > 
> > So given we can't actually fix on one or the other normal convention is
> > to just use something that forces a large enough aligned u8 buffer like
> > 		u8 buf[ALIGN(sizeof(s32) * BMP280_MAX_CHANNELS, 8) + sizeof(s64)]
> > 			__aligned(sizeof(s64));
> > 
> > Sorry for leading you astray on this!
> > 
> > Jonathan
> > 
> >   
> 
> I see your point. I can fix it in next version!
> 
> This is a neat issue here that requires indeed extra attention since
> this type of buffers is used basically by the majority of the drivers.
> Some type of runtime check in those registers would have been very
> very helpful, but I can't see of an easy way of doing it in the core
> code..
Adding size description has been on my todo list for a while so as to allow
the kernel to check the buffer is big enough and get rid of subtle overrun
bugs due to that oddity of the buffer needing to include the timestamp
space even though it's not obvious it will be written to.
That would also allow us to check alignment.  What we can't do is finer
grained checking of these structures but arguably we don't want to as this
is an elegance not correctness issue!

> 
> Thanks for the review :)
> 
> Cheers,
> Vasilis
> 
> > > +	} buffer;
> > >  
> > >  	/*
> > >  	 * DMA (thus cache coherency maintenance) may require the  
> >