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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180515085833.4z4cvsxoqqbny53q@ninjato>
Date:   Tue, 15 May 2018 10:58:33 +0200
From:   Wolfram Sang <wsa@...-dreams.de>
To:     Peter Rosin <peda@...ntia.se>
Cc:     Wenwen Wang <wang6495@....edu>, 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

Hi Peter,

> >> 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?

I came to the conclusion: yes and no.

I think this patch is correct, so it is good to have. But true, it will
expose if other drivers have implemented the return value wrongly. So, I
removed this patch from for-current and plan to include it in for-next
instead if we can agree on that being a good way forward. That will
allow for one full cycle of testing and fixing the issues found. And
hopefully I have time to write a small coccinelle rule to find if
constant values are returned in a function declared as master_xfer.

> 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...

What do you mean with "short success for some sequence" here?

Thanks,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)