mfd: omap-usb-tll: annotate struct usbtll_omap with __counted_by

PATCH Applied Javier Carrasco 4 participants 9 messages Jun 20, 2024 – Jun 26, 2024

Subsystems (2)

Multifunction devices (MFD) OMAP2+ support

lore

Share & tools

lore

Javier Carrasco

author

2024-06-20 21:22 UTC #0 lore raw

cover mfd: omap-usb-tll: annotate struct usbtll_omap with __counted_by

The first patch carries out a straightforward annotation, given that the number of channels is initialized before any reference to the flexible array.

I might need some feedback on the second patch. I added a comment regarding the issue to provide clarification.

Signed-off-by Javier Carrasco <[email protected]>

---
Javier Carrasco (2):
mfd: omap-usb-tll: annotate struct usbtll_omap with __counted_by
mfd: omap-usb-tll: use struct_size to allocate tll

 drivers/mfd/omap-usb-tll.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

---
base-commit: b992b79ca8bc336fa8e2c80990b5af80ed8f36fd
change-id: 20240620-omap-usb-tll-counted_by-aac57c173018

Best regards,

Signature · 1 line

Javier Carrasco <[email protected]>

Javier Carrasco

author

2024-06-20 21:22 UTC #1 lore raw

PATCH 1/2 mfd: omap-usb-tll: annotate struct usbtll_omap with __counted_by

Use the __counted_by compiler attribute for the "struct clk *ch_clk[]" flexible array member to improve the results of array bound sanitizers.

The comments for the variables are no longer needed as it is now clear what is what.

Signed-off-by Javier Carrasco <[email protected]>

---
 drivers/mfd/omap-usb-tll.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/mfd/omap-usb-tll.c +2 -2

index b6303ddb013b..a091e5b0f21d 100644
@@ -98,8 +98,8 @@
 
 struct usbtll_omap {
 	void __iomem	*base;
-	int		nch;		/* num. of channels */
-	struct clk	*ch_clk[];	/* must be the last member */
+	int		nch;
+	struct clk	*ch_clk[] __counted_by(nch);
 };
 
 /*-------------------------------------------------------------------------*/

Signature · 1 line

2.40.1

Javier Carrasco

author

2024-06-20 21:23 UTC #3 lore raw

PATCH 2/2 mfd: omap-usb-tll: use struct_size to allocate tll

In reply to Javier Carrasco

Use the struct_size macro to calculate the size of the tll, which includes a trailing flexible array.

Signed-off-by Javier Carrasco <[email protected]>

---
The memory allocation used to be carried out in two steps:

tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL); tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch,

                           GFP_KERNEL);

Until commit 16c2004d9e4d ("mfd: omap-usb-tll: Allocate driver data at once") turned that into the current allocation:

tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),

                   GFP_KERNEL);

That has surprised me at first glance because I would have expected sizeof(tll->ch_clk[nch]) to return the size of a single pointer, not being equivalent to 'sizeof(struct clk *) * nch'.

I might be missing/misunderstanding something here because the commit is not new, and the error should be noticeable. Moreover, I don't have real hardware to test it. Hence why I didn't mark this patch as a fix.

I would be pleased to get feedback about this (why it is right as it is, or if that is actually a bug).

---
 drivers/mfd/omap-usb-tll.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/mfd/omap-usb-tll.c +1 -2

index a091e5b0f21d..5f25ac514ff2 100644
@@ -230,8 +230,7 @@ static int usbtll_omap_probe(struct platform_device *pdev)
 		break;
 	}
 
-	tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
-			   GFP_KERNEL);
+	tll = devm_kzalloc(dev, struct_size(tll, ch_clk, nch), GFP_KERNEL);
 	if (!tll) {
 		pm_runtime_put_sync(dev);
 		pm_runtime_disable(dev);

Signature · 1 line

2.40.1

Lee Jones

maintainer

2024-06-26 15:26 UTC #4 lore raw

Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll

Quoted 22 lines · Javier Carrasco

Use the struct_size macro to calculate the size of the tll, which includes a trailing flexible array.
Signed-off-by Javier Carrasco <[email protected]>
---
The memory allocation used to be carried out in two steps:
tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL); tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch,
                           GFP_KERNEL);
Until commit 16c2004d9e4d ("mfd: omap-usb-tll: Allocate driver data at once") turned that into the current allocation:
tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
                   GFP_KERNEL);
That has surprised me at first glance because I would have expected sizeof(tll->ch_clk[nch]) to return the size of a single pointer, not being equivalent to 'sizeof(struct clk *) * nch'.

Quoted 6 lines

I might be missing/misunderstanding something here because the commit is not new, and the error should be noticeable. Moreover, I don't have real hardware to test it. Hence why I didn't mark this patch as a fix.
I would be pleased to get feedback about this (why it is right as it is, or if that is actually a bug).

You don't need this H/W to test this our for yourself.

Mock-up the structs in a user-space C-program and print out the sizes.

Please report them all to justify the patch.

Quoted 22 lines

---
 drivers/mfd/omap-usb-tll.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/mfd/omap-usb-tll.c +1 -2

index a091e5b0f21d..5f25ac514ff2 100644
@@ -230,8 +230,7 @@ static int usbtll_omap_probe(struct platform_device *pdev)
 		break;
 	}
 
-	tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
-			   GFP_KERNEL);
+	tll = devm_kzalloc(dev, struct_size(tll, ch_clk, nch), GFP_KERNEL);
 	if (!tll) {
 		pm_runtime_put_sync(dev);
 		pm_runtime_disable(dev);

Signature · 2 lines

-- 
2.40.1

Signature · 1 line

Lee Jones [李琼斯]

Javier Carrasco

author

2024-06-26 17:26 UTC #5 lore raw

Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll

Quoted 28 lines · Lee Jones

Quoted 26 lines · Javier Carrasco
Use the struct_size macro to calculate the size of the tll, which includes a trailing flexible array.
Signed-off-by Javier Carrasco <[email protected]>
---
The memory allocation used to be carried out in two steps:
tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL); tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch,
                           GFP_KERNEL);
Until commit 16c2004d9e4d ("mfd: omap-usb-tll: Allocate driver data at once") turned that into the current allocation:
tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
                   GFP_KERNEL);
That has surprised me at first glance because I would have expected sizeof(tll->ch_clk[nch]) to return the size of a single pointer, not being equivalent to 'sizeof(struct clk *) * nch'.
I might be missing/misunderstanding something here because the commit is not new, and the error should be noticeable. Moreover, I don't have real hardware to test it. Hence why I didn't mark this patch as a fix.

Quoted 9 lines

Quoted 2 lines
I would be pleased to get feedback about this (why it is right as it is, or if that is actually a bug).
You don't need this H/W to test this our for yourself.
Mock-up the structs in a user-space C-program and print out the sizes.
Please report them all to justify the patch.

Values obviously depend on the architecture, but in general:

1.- Before commit 16c2004d9e4d:
1.1. tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL);

-> sizeof(struct usbtll_omap) = N

1.2 tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch,

                           GFP_KERNEL);

-> sizeof(struct clk *) * tll->nch = M * nch

Total = N + M * nch,
where M is the size of a single pointer.

2.- After commit 16c2004d9e4d:
tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),

                    GFP_KERNEL);

-> sizeof(*tll) = N
-> sizeof(tll->ch_clk[nch]) = sizeof(struct clk *) = M

Total = N + M
Therefore, it only allocates memory for a single pointer.

3.- struct_size (this patch):
sizeof(*tll) + nch * sizeof(struct clk *) = N + nch * M

What I meant with not having real hardware is that I could not replicate the whole code with all their structures to get exact sizes, which don't leave room for discussion or misunderstandings.

Best regards,
Javier Carrasco

Quoted 23 lines

Quoted 22 lines

---
 drivers/mfd/omap-usb-tll.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/mfd/omap-usb-tll.c +1 -2

index a091e5b0f21d..5f25ac514ff2 100644
@@ -230,8 +230,7 @@ static int usbtll_omap_probe(struct platform_device *pdev)
 		break;
 	}
 
-	tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
-			   GFP_KERNEL);
+	tll = devm_kzalloc(dev, struct_size(tll, ch_clk, nch), GFP_KERNEL);
 	if (!tll) {
 		pm_runtime_put_sync(dev);
 		pm_runtime_disable(dev);

Signature · 2 lines

-- 
2.40.1

Jann Horn

2024-06-26 17:57 UTC #6 lore raw

Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll

In reply to Javier Carrasco

Quoted 23 lines · Javier Carrasco

Use the struct_size macro to calculate the size of the tll, which includes a trailing flexible array.
Signed-off-by Javier Carrasco <[email protected]>
---
The memory allocation used to be carried out in two steps:
tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL); tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch,
                           GFP_KERNEL);
Until commit 16c2004d9e4d ("mfd: omap-usb-tll: Allocate driver data at once") turned that into the current allocation:
tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
                   GFP_KERNEL);
That has surprised me at first glance because I would have expected sizeof(tll->ch_clk[nch]) to return the size of a single pointer, not being equivalent to 'sizeof(struct clk *) * nch'.

Quoted 6 lines

I might be missing/misunderstanding something here because the commit is not new, and the error should be noticeable. Moreover, I don't have real hardware to test it. Hence why I didn't mark this patch as a fix.
I would be pleased to get feedback about this (why it is right as it is, or if that is actually a bug).

I might be a good idea to ask the people who wrote / accepted the patch that seems to have introduced the bug? I've CC'ed them.

The kernel slab allocator enforces a minimum alignment of KMALLOC_MIN_SIZE, which on MIPS seems to be based on the size of L1 cache lines or something like that? This allocator alignment might be enough to prevent the bug from actually causing an observable effect in many configurations - that could explain why it went undetected, assuming that nobody tested this in a KASAN build.

Quoted 23 lines

---
 drivers/mfd/omap-usb-tll.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/mfd/omap-usb-tll.c +1 -3

index a091e5b0f21d..5f25ac514ff2 100644
@@ -230,8 +230,7 @@ static int usbtll_omap_probe(struct platform_device *pdev)
                break;
        }
 
-       tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
-                          GFP_KERNEL);
+       tll = devm_kzalloc(dev, struct_size(tll, ch_clk, nch), GFP_KERNEL);
        if (!tll) {
                pm_runtime_put_sync(dev);
                pm_runtime_disable(dev);
 
--

2.40.1

Kees Cook

Reviewed-by on v1 2/2

2024-06-26 18:43 UTC #7 lore raw

Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll

In reply to Javier Carrasco

Quoted 6 lines · Javier Carrasco

Use the struct_size macro to calculate the size of the tll, which includes a trailing flexible array.
Signed-off-by Javier Carrasco <[email protected]>
---

I would actually include this entire bit below in the main commit log. It's the core of the "why" for this patch.

Quoted 16 lines

The memory allocation used to be carried out in two steps:
tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL); tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch,
                           GFP_KERNEL);
Until commit 16c2004d9e4d ("mfd: omap-usb-tll: Allocate driver data at once") turned that into the current allocation:
tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
                   GFP_KERNEL);
That has surprised me at first glance because I would have expected sizeof(tll->ch_clk[nch]) to return the size of a single pointer, not being equivalent to 'sizeof(struct clk *) * nch'.

Quoted 6 lines

I might be missing/misunderstanding something here because the commit is not new, and the error should be noticeable. Moreover, I don't have real hardware to test it. Hence why I didn't mark this patch as a fix.
I would be pleased to get feedback about this (why it is right as it is, or if that is actually a bug).

Yeah, I would include:

Fixes commit 16c2004d9e4d ("mfd: omap-usb-tll: Allocate driver data at once")

Because that was a clear mistake. I suspect they were intending to do this, which I've seen as a code pattern from time to time:

	devm_kzalloc(dev, offsetof(typeof(*tll), ch_clk[nch]));

But as Jann points out, "nch" is so small:

drivers/mfd/omap-usb-tll.c:81:#define OMAP_REV2_TLL_CHANNEL_COUNT 2
drivers/mfd/omap-usb-tll.c:82:#define OMAP_TLL_CHANNEL_COUNT 3
drivers/mfd/omap-usb-tll.c:220: nch = OMAP_TLL_CHANNEL_COUNT;
drivers/mfd/omap-usb-tll.c:224: nch = OMAP_REV2_TLL_CHANNEL_COUNT;

struct usbtll_omap {
        void __iomem    *base;
        int             nch;            /* num. of channels */
        struct clk      *ch_clk[];      /* must be the last member */
};

That this allocation was asking for 4 + 4 + 4 * 1 (12) instead of:

	4 + 4 + 4 * OMAP_TLL_CHANNEL_COUNT (20)
or
	4 + 4 + 4 * OMAP_REV2_TLL_CHANNEL_COUNT (16)

the latter would have ended up in the same kmalloc bucket (12 would be rounded up to 16), but with the ARM alignment issue, the minimum bucket size would effectively be tied to CONFIG_ARM_L1_CACHE_SHIFT, which could be as low as 5: 32 bytes minimum, so no bug to be had in the real world.

Reviewed-by Kees Cook <[email protected]>

-Kees

Quoted 22 lines

---
 drivers/mfd/omap-usb-tll.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/mfd/omap-usb-tll.c +1 -2

index a091e5b0f21d..5f25ac514ff2 100644
@@ -230,8 +230,7 @@ static int usbtll_omap_probe(struct platform_device *pdev)
 		break;
 	}
 
-	tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
-			   GFP_KERNEL);
+	tll = devm_kzalloc(dev, struct_size(tll, ch_clk, nch), GFP_KERNEL);
 	if (!tll) {
 		pm_runtime_put_sync(dev);
 		pm_runtime_disable(dev);

Signature · 2 lines

-- 
2.40.1

Signature · 1 line

Kees Cook

Javier Carrasco

author

2024-06-26 19:03 UTC #8 lore raw

Re: [PATCH 2/2] mfd: omap-usb-tll: use struct_size to allocate tll

Quoted 67 lines · Kees Cook

Quoted 6 lines · Javier Carrasco
Use the struct_size macro to calculate the size of the tll, which includes a trailing flexible array.
Signed-off-by Javier Carrasco <[email protected]>
---
I would actually include this entire bit below in the main commit log. It's the core of the "why" for this patch.
Quoted 16 lines
The memory allocation used to be carried out in two steps:
tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL); tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch,
                           GFP_KERNEL);
Until commit 16c2004d9e4d ("mfd: omap-usb-tll: Allocate driver data at once") turned that into the current allocation:
tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
                   GFP_KERNEL);
That has surprised me at first glance because I would have expected sizeof(tll->ch_clk[nch]) to return the size of a single pointer, not being equivalent to 'sizeof(struct clk *) * nch'.
Quoted 6 lines
I might be missing/misunderstanding something here because the commit is not new, and the error should be noticeable. Moreover, I don't have real hardware to test it. Hence why I didn't mark this patch as a fix.
I would be pleased to get feedback about this (why it is right as it is, or if that is actually a bug).
Yeah, I would include:
Fixes commit 16c2004d9e4d ("mfd: omap-usb-tll: Allocate driver data at once")
Because that was a clear mistake. I suspect they were intending to do this, which I've seen as a code pattern from time to time:
	devm_kzalloc(dev, offsetof(typeof(*tll), ch_clk[nch]));
But as Jann points out, "nch" is so small:
drivers/mfd/omap-usb-tll.c:81:#define OMAP_REV2_TLL_CHANNEL_COUNT 2
drivers/mfd/omap-usb-tll.c:82:#define OMAP_TLL_CHANNEL_COUNT 3
drivers/mfd/omap-usb-tll.c:220: nch = OMAP_TLL_CHANNEL_COUNT;
drivers/mfd/omap-usb-tll.c:224: nch = OMAP_REV2_TLL_CHANNEL_COUNT;
struct usbtll_omap {
        void __iomem    *base;
        int             nch;            /* num. of channels */
        struct clk      *ch_clk[];      /* must be the last member */
};
That this allocation was asking for 4 + 4 + 4 * 1 (12) instead of:
	4 + 4 + 4 * OMAP_TLL_CHANNEL_COUNT (20)
or
	4 + 4 + 4 * OMAP_REV2_TLL_CHANNEL_COUNT (16)
the latter would have ended up in the same kmalloc bucket (12 would be rounded up to 16), but with the ARM alignment issue, the minimum bucket size would effectively be tied to CONFIG_ARM_L1_CACHE_SHIFT, which could be as low as 5: 32 bytes minimum, so no bug to be had in the real world.

Quoted 4 lines

Reviewed-by Kees Cook <[email protected]>
-Kees

Thanks for the accurate clarification. That explains indeed why the bug went unnoticed.
A few more channels or members in the usbtll_omap structure would have triggered some alarms.

I will address your comments for v2.

Quoted 23 lines

Quoted 22 lines

---
 drivers/mfd/omap-usb-tll.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/mfd/omap-usb-tll.c +1 -2

index a091e5b0f21d..5f25ac514ff2 100644
@@ -230,8 +230,7 @@ static int usbtll_omap_probe(struct platform_device *pdev)
 		break;
 	}
 
-	tll = devm_kzalloc(dev, sizeof(*tll) + sizeof(tll->ch_clk[nch]),
-			   GFP_KERNEL);
+	tll = devm_kzalloc(dev, struct_size(tll, ch_clk, nch), GFP_KERNEL);
 	if (!tll) {
 		pm_runtime_put_sync(dev);
 		pm_runtime_disable(dev);

Signature · 2 lines

-- 
2.40.1