linux-kernel - Re: [PATCH 4.19 034/133] iio:humidity:hts221 Fix alignment and data leak issues

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: <20200723093118.00005572@Huawei.com>
Date:   Thu, 23 Jul 2020 09:31:18 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Pavel Machek <pavel@...x.de>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>,
        "Lars-Peter Clausen" <lars@...afoo.de>,
        Lorenzo Bianconi <lorenzo@...nel.org>
Subject: Re: [PATCH 4.19 034/133] iio:humidity:hts221 Fix alignment and data
 leak issues

On Wed, 22 Jul 2020 13:28:35 +0200
Pavel Machek <pavel@...x.de> wrote:

> Hi!
> 
> > From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > 
> > commit 5c49056ad9f3c786f7716da2dd47e4488fc6bd25 upstream.
> > 
> > One of a class of bugs pointed out by Lars in a recent review.
> > iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
> > to the size of the timestamp (8 bytes).  This is not guaranteed in
> > this driver which uses an array of smaller elements on the stack.  
> 
> I don't see documentation explaining alignment issues with
> iio_push_to_buffers_with_timestamp(). Perhaps comment near that
> function should explain that?

Hi Pavel,

Agreed.  It's a subtle corner case (hence we missed it for years)
so absolutely needs documenting.  The nasty part is that we don't
control the expectations of the consumers who get the data from
that interface.  They may make the reasonable assumption
that they aren't getting unaligned data,  particularly given the
effort we go to in ensuring natural alignment of elements within
the buffer.  It's a moderately fast path so any tricks with realigning
the data aren't sensible either.

> 
> And as it seems to be common problem, perhaps
> iio_push_to_buffers_with_timestamp should check alignment of its
> arguments?

It should indeed check this.  But... The reality is that lots
of platforms are fine with the alignment not being enforced.
So far we have precisely one confirmed report of the issue.

Until we have fixed all the users I'm not keen to add a check
that will be seen to 'break' existing working systems.
It's taking a while to get all these reviewed so I'm picking them
up as they get sufficient eyes on them.  A few drivers are more
fiddly to do so we don't yet have patches on the list.

I was thinking to do the documentation update and a check enforcing
it in one go, but perhaps given the slow nature of getting all the
users fixed we should look to document now and enforce later?

Jonathan

> 
> Thanks,
> 								Pavel