linux-kernel - Re: [PATCH v2 2/2] i2c: core-smbus: fix a potential missing-check bug

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: <8f728c2d-b463-507e-2c36-fad22fd99dff@axentia.se>
Date:   Thu, 10 May 2018 15:36:21 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Wolfram Sang <wsa@...-dreams.de>, Wenwen Wang <wang6495@....edu>
Cc:     Kangjie Lu <kjlu@....edu>,
        "open list:I2C SUBSYSTEM" <linux-i2c@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] i2c: core-smbus: fix a potential missing-check bug

On 2018-05-10 13:16, Wolfram Sang wrote:
> On Sat, May 05, 2018 at 08:02:21AM -0500, Wenwen Wang wrote:
>> In i2c_smbus_xfer_emulated(), the function i2c_transfer() is invoked to
>> transfer i2c messages. The number of actual transferred messages is
>> returned and saved to 'status'. If 'status' is negative, that means an
>> error occurred during the transfer process. In that case, the value of
>> 'status' is an error code to indicate the reason of the transfer failure.
>> In most cases, i2c_transfer() can transfer 'num' messages with no error.
>> And so 'status' == 'num'. However, due to unexpected errors, it is probable
>> that only partial messages are transferred by i2c_transfer(). As a result,
>> 'status' != 'num'. This special case is not checked after the invocation of
>> i2c_transfer() and can potentially lead to unexpected issues in the
>> following execution since it is expected that 'status' == 'num'.
>>
>> This patch checks the return value of i2c_transfer() and returns an error
>> code -EIO if the number of actual transferred messages 'status' is not
>> equal to 'num'.
>>
>> Signed-off-by: Wenwen Wang <wang6495@....edu>
> 
> Applied to for-current, thanks!
> 

I meant to comment here but got side-tracked and never got around to it.
But see e.g. [1] and [2] for drivers that will not be happy with this
change. Maybe there are more of those? I did a scan of the drivers in
algos/ and busses/, but there are drivers all over the tree that
implements .master_xfer that I have not audited. Who knows what further
problems this patch will reveal? So, maybe we should be a bit
conservative and only WARN as a first step?

Yes, I know that I suggested this, sorry for not following up with this
in a more timely fashion...

Cheers,
Peter

[1] https://lkml.org/lkml/2018/5/9/871
[2] https://lkml.org/lkml/2018/5/9/877

PS. Also busses/i2c-rcar.c seems like it might return a short "success"
for some sequence of events. But I'm not sure about that one...