| Message ID | 20240620-omap-usb-tll-counted_by-v1-2-77797834bb9a@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mfd: omap-usb-tll: annotate struct usbtll_omap with __counted_by | expand |
On Thu, 20 Jun 2024, Javier Carrasco wrote: > Use the struct_size macro to calculate the size of the tll, which > includes a trailing flexible array. > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > --- > 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). 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. > --- > drivers/mfd/omap-usb-tll.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c > index a091e5b0f21d..5f25ac514ff2 100644 > --- a/drivers/mfd/omap-usb-tll.c > +++ b/drivers/mfd/omap-usb-tll.c > @@ -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 >
On 26/06/2024 17:26, Lee Jones wrote: > On Thu, 20 Jun 2024, Javier Carrasco wrote: > >> Use the struct_size macro to calculate the size of the tll, which >> includes a trailing flexible array. >> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >> >> --- >> 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). > > 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 >> --- >> drivers/mfd/omap-usb-tll.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c >> index a091e5b0f21d..5f25ac514ff2 100644 >> --- a/drivers/mfd/omap-usb-tll.c >> +++ b/drivers/mfd/omap-usb-tll.c >> @@ -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 >> >
On Thu, Jun 20, 2024 at 11:23 PM Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > > Use the struct_size macro to calculate the size of the tll, which > includes a trailing flexible array. > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > --- > 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). 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. > --- > drivers/mfd/omap-usb-tll.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c > index a091e5b0f21d..5f25ac514ff2 100644 > --- a/drivers/mfd/omap-usb-tll.c > +++ b/drivers/mfd/omap-usb-tll.c > @@ -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 > >
On Thu, Jun 20, 2024 at 11:22:34PM +0200, Javier Carrasco wrote: > Use the struct_size macro to calculate the size of the tll, which > includes a trailing flexible array. > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > > --- I would actually include this entire bit below in the main commit log. It's the core of the "why" for this patch. > 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). 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 <kees@kernel.org> -Kees > --- > drivers/mfd/omap-usb-tll.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c > index a091e5b0f21d..5f25ac514ff2 100644 > --- a/drivers/mfd/omap-usb-tll.c > +++ b/drivers/mfd/omap-usb-tll.c > @@ -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 >
On 26/06/2024 20:43, Kees Cook wrote: > On Thu, Jun 20, 2024 at 11:22:34PM +0200, Javier Carrasco wrote: >> Use the struct_size macro to calculate the size of the tll, which >> includes a trailing flexible array. >> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >> >> --- > > I would actually include this entire bit below in the main commit log. > It's the core of the "why" for this patch. > >> 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). > > 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 <kees@kernel.org> > > -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. >> --- >> drivers/mfd/omap-usb-tll.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c >> index a091e5b0f21d..5f25ac514ff2 100644 >> --- a/drivers/mfd/omap-usb-tll.c >> +++ b/drivers/mfd/omap-usb-tll.c >> @@ -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 >> >
diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c index a091e5b0f21d..5f25ac514ff2 100644 --- a/drivers/mfd/omap-usb-tll.c +++ b/drivers/mfd/omap-usb-tll.c @@ -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);
Use the struct_size macro to calculate the size of the tll, which includes a trailing flexible array. Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- 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(-)