| 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:
<FR3P281MB175744F24BDD93870BB32ED0CE45A@FR3P281MB1757.DEUP281.PROD.OUTLOOK.COM>
Date: Fri, 27 Jun 2025 15:27:03 +0000
From: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
To: Jonathan Cameron <jic23@...nel.org>
CC: David Lechner <dlechner@...libre.com>,
Lars-Peter Clausen
<lars@...afoo.de>,
Nuno Sá <nuno.sa@...log.com>,
Andy
Shevchenko <andy@...nel.org>,
"linux-iio@...r.kernel.org"
<linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 1/3] iio: imu: inv_icm42600: move structure DMA buffers
at the end
>
>________________________________________
>From: Jonathan Cameron <jic23@...nel.org>
>Sent: Thursday, June 26, 2025 20:48
>To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
>Cc: David Lechner <dlechner@...libre.com>; Lars-Peter Clausen <lars@...afoo.de>; Nuno Sá <nuno.sa@...log.com>; Andy Shevchenko <andy@...nel.org>; linux-iio@...r.kernel.org <linux-iio@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
>Subject: Re: [PATCH v5 1/3] iio: imu: inv_icm42600: move structure DMA buffers at the end
>
>This Message Is From an External Sender
>This message came from outside your organization.
>
>On Tue, 24 Jun 2025 08:43:37 +0000
>Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com> wrote:
>
>> >
>> >________________________________________
>> >From: David Lechner <dlechner@...libre.com>
>> >Sent: Tuesday, June 24, 2025 00:13
>> >To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
>> >Cc: Jonathan Cameron <jic23@...nel.org>; Lars-Peter Clausen <lars@...afoo.de>; Nuno Sá <nuno.sa@...log.com>; Andy Shevchenko <andy@...nel.org>; linux-iio@...r.kernel.org <linux-iio@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
>> >Subject: Re: [PATCH v5 1/3] iio: imu: inv_icm42600: move structure DMA buffers at the end
>> >
>> >This Message Is From an External Sender
>> >This message came from outside your organization.
>> >
>> >On Mon, Jun 23, 2025 at 6:56 AM Jean-Baptiste Maneyrol via B4 Relay
>> ><devnull+jean-baptiste.maneyrol.tdk.com@...nel.org> wrote:
>> >>
>> >> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
>> >>
>> >> Move DMA buffers at the end of the structure to avoid overflow
>> >> bugs with unexpected effect.
>> >
>> >If there is an overflow bug, we should fix that rather than hiding it.
>> >
>> >If I misunderstood the problem and timestamp and fifo should not be in
>> >the DMA aligned area and there is a problem with DMA cache writing
>> >over them, then I think we should reword the commit message.
>> >
>> Hello David,
>>
>> this change was asked by Jonathan so that potential DMA overflow would be more
>> easily caught by writing outside of the structure rather than writing inside
>> and do unexpected behavior.
>>
>> >>
>> >> struct inv_icm42600_fifo has a DMA buffer at the end.
>> >>
>> >> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
>> >> ---
>> >> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 8 ++++----
>> >> 1 file changed, 4 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> >> index 55ed1ddaa8cb5dd410d17db3866fa0f22f18e9d2..9b2cce172670c5513f18d5979a5ff563e9af4cb3 100644
>> >> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> >> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
>> >> @@ -148,9 +148,9 @@ struct inv_icm42600_suspended {
>> >> * @suspended: suspended sensors configuration.
>> >> * @indio_gyro: gyroscope IIO device.
>> >> * @indio_accel: accelerometer IIO device.
>> >> - * @buffer: data transfer buffer aligned for DMA.
>> >> - * @fifo: FIFO management structure.
>> >> * @timestamp: interrupt timestamps.
>> >> + * @fifo: FIFO management structure.
>> >> + * @buffer: data transfer buffer aligned for DMA.
>> >> */
>> >> struct inv_icm42600_state {
>> >> struct mutex lock;
>> >> @@ -164,12 +164,12 @@ struct inv_icm42600_state {
>> >> struct inv_icm42600_suspended suspended;
>> >> struct iio_dev *indio_gyro;
>> >> struct iio_dev *indio_accel;
>> >> - u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
>> >> - struct inv_icm42600_fifo fifo;
>> >> struct {
>> >> s64 gyro;
>> >> s64 accel;
>> >> } timestamp;
>> >> + struct inv_icm42600_fifo fifo;
>> >
>> >I didn't look at how the drivers use timestamp and fifo, but if they
>> >are passed as a buffer to SPI, then they need to stay in the DMA
>> >aligned area of the struct.
>>
>> struct inv_icm42600_fifo has a buffer at its end that is passed to SPI.
>> Same things for buffer below. That's why both buffers are DMA
>> aligned.
>
>It's a tiny bit esoteric that this is relying on structure alignment rules
>that says (iirc) the structure element will be aligned to maximum of it's
>elements and there is tail padding to that size as well. Thus the whole
>struct inv_icm42600 is __aligned(IIO_DMA_MINALIGN) and the buffer in there
>is itself after padding to ensure that it is __aligned(IIO_DMA_MINALIGN)
>
>
>Anyhow, all I think this actually does is avoid one lot of padding
>(as well as making it slightly easier to reason about!)
>
>outer struct {
>stuff
>padding to align #1
>fifo {
> stuff
> padding to align
> buffer
> padding to fill up align
>}
>struct timestamp;
>///this bit will go away as it can fit in the padding #1 (probably)
>Padding to align
>////
>u8 buffer[2] __ailgned(IIO_DMA_MINALIGN)
>
>
Hello David and Jonathan,
what should I changed for this patch? Rewrite the commit message?
Thanks for your feedback,
JB
>
>>
>> >
>> >> + u8 buffer[2] __aligned(IIO_DMA_MINALIGN);
>> >> };
>> >>
>> >>
>> >>
>> >> --
>> >> 2.49.0
>> >>
>> >>
>> >
>>
>> Thanks,
>> JB
>
>
Powered by blists - more mailing lists