[PATCH next] i2c: Fix end of loop test in i2c_atr_find_mapping_by_addr()

[PATCH next] i2c: Fix end of loop test in i2c_atr_find_mapping_by_addr()

[Dan Carpenter]

When the list_for_each_entry_reverse() exits without hitting a break
then the list cursor points to invalid memory.  So this check for
if (c2a->fixed) is checking bogus memory.  Fix it by using a "found"
variable to track if we found what we were looking for or not.

Fixes: c3f55241882b ("i2c: Support dynamic address translation")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/i2c/i2c-atr.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index d5aa6738370c..1aeaecacc26c 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -240,6 +240,7 @@ i2c_atr_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
 	struct i2c_atr *atr = chan->atr;
 	struct i2c_atr_alias_pair *c2a;
 	struct list_head *alias_pairs;
+	bool found = false;
 	u16 alias;
 	int ret;
 
@@ -258,11 +259,14 @@ i2c_atr_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
 		if (unlikely(list_empty(alias_pairs)))
 			return NULL;
 
-		list_for_each_entry_reverse(c2a, alias_pairs, node)
-			if (!c2a->fixed)
+		list_for_each_entry_reverse(c2a, alias_pairs, node) {
+			if (!c2a->fixed) {
+				found = true;
 				break;
+			}
+		}
 
-		if (c2a->fixed)
+		if (!found)
 			return NULL;
 
 		atr->ops->detach_addr(atr, chan->chan_id, c2a->addr);
-- 
2.47.2

Re: [PATCH next] i2c: Fix end of loop test in i2c_atr_find_mapping_by_addr()

[Romain Gantois]

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

Hello Dan,

On Wednesday, 23 April 2025 10:21:18 CEST Dan Carpenter wrote:
> When the list_for_each_entry_reverse() exits without hitting a break
> then the list cursor points to invalid memory.  So this check for
> if (c2a->fixed) is checking bogus memory.  Fix it by using a "found"
> variable to track if we found what we were looking for or not.

IIUC the for loop ending condition in list_for_each_entry_reverse() is
"!list_entry_is_head(pos, head, member);", so even if the loop runs to 
completion, the pointer should still be valid right?

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH next] i2c: Fix end of loop test in i2c_atr_find_mapping_by_addr()

[Dan Carpenter]

On Wed, Apr 23, 2025 at 05:25:44PM +0200, Romain Gantois wrote:
> Hello Dan,
> 
> On Wednesday, 23 April 2025 10:21:18 CEST Dan Carpenter wrote:
> > When the list_for_each_entry_reverse() exits without hitting a break
> > then the list cursor points to invalid memory.  So this check for
> > if (c2a->fixed) is checking bogus memory.  Fix it by using a "found"
> > variable to track if we found what we were looking for or not.
> 
> IIUC the for loop ending condition in list_for_each_entry_reverse() is
> "!list_entry_is_head(pos, head, member);", so even if the loop runs to 
> completion, the pointer should still be valid right?
> 

head is &chan->alias_pairs.  pos is an offset off the head.  In this
case, the offset is zero.  So it's &chan->alias_pairs minus zero.

So we exit the list with c2a = (void *)&chan->alias_pairs.

If you look how struct i2c_atr_chan is declareted the next struct member
after alias_pairs is:

	struct i2c_atr_alias_pool *alias_pool;

So if (c2a->fixed) is poking around in the alias_pool pointer.  It's not
out of bounds but it's not valid either.

regards,
dan carpenter

Re: [PATCH next] i2c: Fix end of loop test in i2c_atr_find_mapping_by_addr()

[Tomi Valkeinen]

Hi,

On 23/04/2025 20:29, Dan Carpenter wrote:
> On Wed, Apr 23, 2025 at 05:25:44PM +0200, Romain Gantois wrote:
>> Hello Dan,
>>
>> On Wednesday, 23 April 2025 10:21:18 CEST Dan Carpenter wrote:
>>> When the list_for_each_entry_reverse() exits without hitting a break
>>> then the list cursor points to invalid memory.  So this check for
>>> if (c2a->fixed) is checking bogus memory.  Fix it by using a "found"
>>> variable to track if we found what we were looking for or not.
>>
>> IIUC the for loop ending condition in list_for_each_entry_reverse() is
>> "!list_entry_is_head(pos, head, member);", so even if the loop runs to
>> completion, the pointer should still be valid right?
>>
> 
> head is &chan->alias_pairs.  pos is an offset off the head.  In this
> case, the offset is zero.  So it's &chan->alias_pairs minus zero.
> 
> So we exit the list with c2a = (void *)&chan->alias_pairs.
> 
> If you look how struct i2c_atr_chan is declareted the next struct member
> after alias_pairs is:
> 
> 	struct i2c_atr_alias_pool *alias_pool;
> 
> So if (c2a->fixed) is poking around in the alias_pool pointer.  It's not
> out of bounds but it's not valid either.

Maybe it's just me, but I had hard time following that explanation. So 
here's mine:

The list head (i2c_atr_chan.alias_pairs) is not a full entry, it's just 
a struct list_head. When the for loop runs to completion, c2a doesn't 
point to a struct i2c_atr_alias_pair, so you can't access c2a->fixed.

For the patch:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH next] i2c: Fix end of loop test in i2c_atr_find_mapping_by_addr()

[Romain Gantois]

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

On Thursday, 24 April 2025 08:32:22 CEST Tomi Valkeinen wrote:
> Hi,
> 
> On 23/04/2025 20:29, Dan Carpenter wrote:
> > On Wed, Apr 23, 2025 at 05:25:44PM +0200, Romain Gantois wrote:
> >> Hello Dan,
> >> 
> >> On Wednesday, 23 April 2025 10:21:18 CEST Dan Carpenter wrote:
> >>> When the list_for_each_entry_reverse() exits without hitting a break
> >>> then the list cursor points to invalid memory.  So this check for
> >>> if (c2a->fixed) is checking bogus memory.  Fix it by using a "found"
> >>> variable to track if we found what we were looking for or not.
> >> 
> >> IIUC the for loop ending condition in list_for_each_entry_reverse() is
> >> "!list_entry_is_head(pos, head, member);", so even if the loop runs to
> >> completion, the pointer should still be valid right?
> > 
> > head is &chan->alias_pairs.  pos is an offset off the head.  In this
> > case, the offset is zero.  So it's &chan->alias_pairs minus zero.
> > 
> > So we exit the list with c2a = (void *)&chan->alias_pairs.
> > 
> > If you look how struct i2c_atr_chan is declareted the next struct member
> > 
> > after alias_pairs is:
> > 	struct i2c_atr_alias_pool *alias_pool;
> > 
> > So if (c2a->fixed) is poking around in the alias_pool pointer.  It's not
> > out of bounds but it's not valid either.
> 
> Maybe it's just me, but I had hard time following that explanation. So
> here's mine:
> 
> The list head (i2c_atr_chan.alias_pairs) is not a full entry, it's just
> a struct list_head. When the for loop runs to completion, c2a doesn't
> point to a struct i2c_atr_alias_pair, so you can't access c2a->fixed.

Ah I see, in that case thanks for the fix Dan!

Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH next] i2c: Fix end of loop test in i2c_atr_find_mapping_by_addr()

[Luca Ceresoli]

On Thu, 24 Apr 2025 09:10:43 +0200
Romain Gantois <romain.gantois@bootlin.com> wrote:

> On Thursday, 24 April 2025 08:32:22 CEST Tomi Valkeinen wrote:
> > Hi,
> > 
> > On 23/04/2025 20:29, Dan Carpenter wrote:  
> > > On Wed, Apr 23, 2025 at 05:25:44PM +0200, Romain Gantois wrote:  
> > >> Hello Dan,
> > >> 
> > >> On Wednesday, 23 April 2025 10:21:18 CEST Dan Carpenter wrote:  
> > >>> When the list_for_each_entry_reverse() exits without hitting a break
> > >>> then the list cursor points to invalid memory.  So this check for
> > >>> if (c2a->fixed) is checking bogus memory.  Fix it by using a "found"
> > >>> variable to track if we found what we were looking for or not.  
> > >> 
> > >> IIUC the for loop ending condition in list_for_each_entry_reverse() is
> > >> "!list_entry_is_head(pos, head, member);", so even if the loop runs to
> > >> completion, the pointer should still be valid right?  
> > > 
> > > head is &chan->alias_pairs.  pos is an offset off the head.  In this
> > > case, the offset is zero.  So it's &chan->alias_pairs minus zero.
> > > 
> > > So we exit the list with c2a = (void *)&chan->alias_pairs.
> > > 
> > > If you look how struct i2c_atr_chan is declareted the next struct member
> > > 
> > > after alias_pairs is:
> > > 	struct i2c_atr_alias_pool *alias_pool;
> > > 
> > > So if (c2a->fixed) is poking around in the alias_pool pointer.  It's not
> > > out of bounds but it's not valid either.  
> > 
> > Maybe it's just me, but I had hard time following that explanation. So
> > here's mine:
> > 
> > The list head (i2c_atr_chan.alias_pairs) is not a full entry, it's just
> > a struct list_head. When the for loop runs to completion, c2a doesn't
> > point to a struct i2c_atr_alias_pair, so you can't access c2a->fixed.  
> 
> Ah I see, in that case thanks for the fix Dan!
> 
> Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH next] i2c: Fix end of loop test in i2c_atr_find_mapping_by_addr()

[Wolfram Sang]

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

On Wed, Apr 23, 2025 at 11:21:18AM +0300, Dan Carpenter wrote:
> When the list_for_each_entry_reverse() exits without hitting a break
> then the list cursor points to invalid memory.  So this check for
> if (c2a->fixed) is checking bogus memory.  Fix it by using a "found"
> variable to track if we found what we were looking for or not.
> 
> Fixes: c3f55241882b ("i2c: Support dynamic address translation")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Applied to for-next with Tomi's description added, thanks!


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