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

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

[Wenwen Wang]

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@umn.edu>
---
 drivers/i2c/i2c-core-smbus.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 7d7700f..e7a2d2f 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -467,6 +467,8 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	status = i2c_transfer(adapter, msg, num);
 	if (status < 0)
 		return status;
+	if (status != num)
+		return -EIO;
 
 	/* Check PEC if last message is a read */
 	if (i && (msg[num-1].flags & I2C_M_RD)) {
-- 
2.7.4

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

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@umn.edu>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Peter Rosin]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2414 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Peter Rosin]

On 2018-05-15 10:58, Wolfram Sang wrote:
> 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@umn.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.

That would be a good thing. Maybe a long term goal is to simply return
zero on success for .master_xfer, because currently the only expected
success value is the number of messages sent, but the caller is in all
likelihood already aware of that count, so it all seems rather like
something that is just pointless and easy to get wrong...

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

By "short" I mean not all requested messages transferred. By "success"
I mean non-negative.

I.e. when I look at rcar_i2c_master_xfer(), it sets things up for all
messages to be transferred, starts things off and waits for completion
(or timeout). But the driver is too involved for it to be easy to say
that either all messages are transferred or a negative error is
returned. I never managed to see that anyway.

And the function has that "ret = num - priv->msgs_left;" statement
indicating that whomever wrote it thought it perfectly ok to return
such a "short success" (which noone is expecting).

So, I'm simply uncertain about that .master_xfer implementation, that's
all.

Cheers,
Peter

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2291 bytes --]


> > hopefully I have time to write a small coccinelle rule to find if
> > constant values are returned in a function declared as master_xfer.
> 
> That would be a good thing.

Did that now and only found drivers which have a (meanwhile) needless
parameter check for 'num'. Will set you on CC for those fixes. I didn't
find any drivers incorrectly returning 0 instead of num. Except the ones
you already fixed.


> Maybe a long term goal is to simply return
> zero on success for .master_xfer, because currently the only expected
> success value is the number of messages sent, but the caller is in all
> likelihood already aware of that count, so it all seems rather like
> something that is just pointless and easy to get wrong...

Yes, the comment in the core is still true:

        /* REVISIT the fault reporting model here is weak:
         * ...

Returning 0 or -ERRNO sounds best to me, too. But we would need to make
sure there is no in-kernel user relying on the current behaviour. As you
said, this is a long-term goal at best.

> > What do you mean with "short success for some sequence" here?
> 
> By "short" I mean not all requested messages transferred. By "success"
> I mean non-negative.

Ah, I understand now, thanks.

> I.e. when I look at rcar_i2c_master_xfer(), it sets things up for all
> messages to be transferred, starts things off and waits for completion
> (or timeout). But the driver is too involved for it to be easy to say
> that either all messages are transferred or a negative error is
> returned. I never managed to see that anyway.

Well, I am the maintainer of that driver, so I can say something about
that :) The HW design has a flaw for older SoCs which makes the driver
prone to a race condition. This is why we set up new messages in
interrupt context, too. Everything else turned out to be too expensive.

> And the function has that "ret = num - priv->msgs_left;" statement
> indicating that whomever wrote it thought it perfectly ok to return
> such a "short success" (which noone is expecting).

I copied over that old behaviour when refactoring the driver. But I see
what you mean, and couldn't also really see a path where a "short
success" could actually happen.

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

On Thu, May 10, 2018 at 01:16:59PM +0200, 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@umn.edu>
> 
> Applied to for-current, thanks!

Reconsidered and applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]