| Message ID | 1525525341-10046-1-git-send-email-wang6495@umn.edu |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/2] i2c: core-smbus: fix a potential uninitialization bug | expand |
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!
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...
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
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
> > 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
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!
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)) {
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(+)