video: fbdev: arkfb: Cast ics5342_init() allocation type

PATCH In review Kees Cook 3 participants 6 messages Apr 26, 2025 – May 2, 2025 Framebuffer layer

Share & tools

Kees Cook

author

2025-04-26 06:23 UTC #0 lore raw

PATCH 1/1 video: fbdev: arkfb: Cast ics5342_init() allocation type

In preparation for making the kmalloc family of allocators type aware, we need to make sure that the returned type from the allocation matches the type of the variable being assigned. (Before, the allocator would always return "void *", which can be implicitly cast to any pointer type.)

The assigned type is "struct dac_info *" but the returned type will be "struct ics5342_info *", which has a larger allocation size. This is by design, as struct ics5342_info contains struct dac_info as its first member. Cast the allocation type to match the assignment.

Signed-off-by Kees Cook <[email protected]>

---
Cc: Helge Deller <[email protected]>
Cc: Javier Martinez Canillas <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: Zheyu Ma <[email protected]>
Cc: Samuel Thibault <[email protected]>
Cc: Jiapeng Chong <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>

---
 drivers/video/fbdev/arkfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/video/fbdev/arkfb.c +1 -1

index 082501feceb9..7d131e3d159a 100644
@@ -431,7 +431,7 @@ static struct dac_ops ics5342_ops = {
 
 static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
 {
-	struct dac_info *info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
+	struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
 
 	if (! info)
 		return NULL;

Signature · 1 line

2.34.1

Helge Deller

maintainer

2025-04-26 11:33 UTC #1 lore raw

Quoted 11 lines · Kees Cook

In preparation for making the kmalloc family of allocators type aware, we need to make sure that the returned type from the allocation matches the type of the variable being assigned. (Before, the allocator would always return "void *", which can be implicitly cast to any pointer type.)
The assigned type is "struct dac_info *" but the returned type will be "struct ics5342_info *", which has a larger allocation size. This is by design, as struct ics5342_info contains struct dac_info as its first member. Cast the allocation type to match the assignment.
Signed-off-by Kees Cook <[email protected]>

Thanks Kees!
I applied your patch, but wouldn't this untested patch be cleaner and fulfill the same purpose to match a kzalloc return type?

drivers/video/fbdev/arkfb.c +2 -1

index 7d131e3d159a..a57c8a992e11 100644
@@ -431,7 +431,8 @@ static struct dac_ops ics5342_ops = {
 
 static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
 {
-       struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
+       struct ics5342_info *ics_info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
+       struct dac_info *info = &ics_info->dac;
 
        if (! info)

Helge

---

Quoted 25 lines

Cc Helge Deller <[email protected]>
Cc Javier Martinez Canillas <[email protected]>
Cc Thomas Zimmermann <[email protected]>
Cc Zheyu Ma <[email protected]>
Cc Samuel Thibault <[email protected]>
Cc Jiapeng Chong <[email protected]>
Cc <[email protected]>
Cc <[email protected]>
---
  drivers/video/fbdev/arkfb.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/video/fbdev/arkfb.c +1 -1
index 082501feceb9..7d131e3d159a 100644
@@ -431,7 +431,7 @@ static struct dac_ops ics5342_ops = {
 
  static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
  {
-	struct dac_info *info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
+	struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
 
  	if (! info)
  		return NULL;

Geert Uytterhoeven

2025-04-28 06:36 UTC #2 lore raw

Hi Kees,

Quoted 6 lines · Helge Deller

Quoted 5 lines · Kees Cook
In preparation for making the kmalloc family of allocators type aware, we need to make sure that the returned type from the allocation matches the type of the variable being assigned. (Before, the allocator would always return "void *", which can be implicitly cast to any pointer type.)

Quoted 6 lines

Quoted 6 lines
The assigned type is "struct dac_info *" but the returned type will be "struct ics5342_info *", which has a larger allocation size. This is by design, as struct ics5342_info contains struct dac_info as its first member. Cast the allocation type to match the assignment.
Signed-off-by Kees Cook <[email protected]>

Thanks for your patch, which is now commit 8d2f0f5bbac87b9d ("fbdev:
arkfb: Cast ics5342_init() allocation type") in fbdev/for-next.

drivers/video/fbdev/arkfb.c static struct dac_ops ics5342_ops = { · line 434

Quoted 13 lines

I applied your patch, but wouldn't this untested patch be cleaner and fulfill the same purpose to match a kzalloc return type?

drivers/video/fbdev/arkfb.c +1 -1

index 7d131e3d159a..a57c8a992e11 100644
@@ -431,7 +431,8 @@ static struct dac_ops ics5342_ops = {
 
  static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
  {
-       struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
+       struct ics5342_info *ics_info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);

sizeof(*ics_info)?

drivers/video/fbdev/arkfb.c static struct dac_ops ics5342_ops = { · line 435

Quoted 1 line

drivers/video/fbdev/arkfb.c +1 -0
+       struct dac_info *info = &ics_info->dac;

Exactly my thought when I noticed this commit. Adding casts makes it harder to notice any future discrepancies.

Quoted 11 lines

drivers/video/fbdev/arkfb.c static struct dac_ops ics5342_ops = { · line 437

Quoted 11 lines

drivers/video/fbdev/arkfb.c +1 -1

@@ -431,7 +431,7 @@ static struct dac_ops ics5342_ops = {
 
  static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
  {
-     struct dac_info *info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
+     struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
 
      if (! info)
              return NULL;

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Helge Deller

maintainer

2025-04-29 20:17 UTC #3 lore raw

Quoted 35 lines · Geert Uytterhoeven

Hi Kees,
Quoted 6 lines · Helge Deller
Quoted 5 lines · Kees Cook
In preparation for making the kmalloc family of allocators type aware, we need to make sure that the returned type from the allocation matches the type of the variable being assigned. (Before, the allocator would always return "void *", which can be implicitly cast to any pointer type.)
Quoted 6 lines
Quoted 6 lines
The assigned type is "struct dac_info *" but the returned type will be "struct ics5342_info *", which has a larger allocation size. This is by design, as struct ics5342_info contains struct dac_info as its first member. Cast the allocation type to match the assignment.
Signed-off-by Kees Cook <[email protected]>
Thanks for your patch, which is now commit 8d2f0f5bbac87b9d ("fbdev:
arkfb: Cast ics5342_init() allocation type") in fbdev/for-next.
drivers/video/fbdev/arkfb.c static struct dac_ops ics5342_ops = { · line 434
Quoted 13 lines
I applied your patch, but wouldn't this untested patch be cleaner and fulfill the same purpose to match a kzalloc return type?
drivers/video/fbdev/arkfb.c +1 -1
index 7d131e3d159a..a57c8a992e11 100644
@@ -431,7 +431,8 @@ static struct dac_ops ics5342_ops = {
 
   static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
   {
-       struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
+       struct ics5342_info *ics_info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
sizeof(*ics_info)?

Quoted 4 lines

drivers/video/fbdev/arkfb.c static struct dac_ops ics5342_ops = { · line 435
Quoted 1 line
drivers/video/fbdev/arkfb.c +1 -0
+       struct dac_info *info = &ics_info->dac;
Exactly my thought when I noticed this commit. Adding casts makes it harder to notice any future discrepancies.

I've changed it accordingly.

Helge

Kees Cook

author

2025-04-29 20:26 UTC #4 lore raw

Quoted 36 lines · Helge Deller

Quoted 35 lines · Geert Uytterhoeven
Hi Kees,
Quoted 6 lines · Helge Deller
Quoted 5 lines · Kees Cook
In preparation for making the kmalloc family of allocators type aware, we need to make sure that the returned type from the allocation matches the type of the variable being assigned. (Before, the allocator would always return "void *", which can be implicitly cast to any pointer type.)
Quoted 6 lines
Quoted 6 lines
The assigned type is "struct dac_info *" but the returned type will be "struct ics5342_info *", which has a larger allocation size. This is by design, as struct ics5342_info contains struct dac_info as its first member. Cast the allocation type to match the assignment.
Signed-off-by Kees Cook <[email protected]>
Thanks for your patch, which is now commit 8d2f0f5bbac87b9d ("fbdev:
arkfb: Cast ics5342_init() allocation type") in fbdev/for-next.
drivers/video/fbdev/arkfb.c static struct dac_ops ics5342_ops = { · line 434
Quoted 13 lines
I applied your patch, but wouldn't this untested patch be cleaner and fulfill the same purpose to match a kzalloc return type?
drivers/video/fbdev/arkfb.c +1 -1
index 7d131e3d159a..a57c8a992e11 100644
@@ -431,7 +431,8 @@ static struct dac_ops ics5342_ops = {
 
   static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
   {
-       struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
+       struct ics5342_info *ics_info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
sizeof(*ics_info)?

Quoted 6 lines

Quoted 4 lines
drivers/video/fbdev/arkfb.c static struct dac_ops ics5342_ops = { · line 435
Quoted 1 line
drivers/video/fbdev/arkfb.c +1 -0
+       struct dac_info *info = &ics_info->dac;
Exactly my thought when I noticed this commit. Adding casts makes it harder to notice any future discrepancies.
I've changed it accordingly.

Thanks! Yeah, that's a much nicer solution.

Signature · 1 line

Kees Cook

Geert Uytterhoeven

2025-05-02 07:37 UTC #5 lore raw

In reply to Helge Deller

Hi Helge,

Quoted 34 lines · Helge Deller

Quoted 33 lines · Geert Uytterhoeven
Quoted 6 lines · Helge Deller
Quoted 5 lines · Kees Cook
In preparation for making the kmalloc family of allocators type aware, we need to make sure that the returned type from the allocation matches the type of the variable being assigned. (Before, the allocator would always return "void *", which can be implicitly cast to any pointer type.)
Quoted 6 lines
Quoted 6 lines
The assigned type is "struct dac_info *" but the returned type will be "struct ics5342_info *", which has a larger allocation size. This is by design, as struct ics5342_info contains struct dac_info as its first member. Cast the allocation type to match the assignment.
Signed-off-by Kees Cook <[email protected]>
Thanks for your patch, which is now commit 8d2f0f5bbac87b9d ("fbdev:
arkfb: Cast ics5342_init() allocation type") in fbdev/for-next.
drivers/video/fbdev/arkfb.c static struct dac_ops ics5342_ops = { · line 434
Quoted 13 lines
I applied your patch, but wouldn't this untested patch be cleaner and fulfill the same purpose to match a kzalloc return type?
drivers/video/fbdev/arkfb.c +1 -1
index 7d131e3d159a..a57c8a992e11 100644
@@ -431,7 +431,8 @@ static struct dac_ops ics5342_ops = {
 
   static struct dac_info * ics5342_init(dac_read_regs_t drr, dac_write_regs_t dwr, void *data)
   {
-       struct dac_info *info = (struct dac_info *)kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
+       struct ics5342_info *ics_info = kzalloc(sizeof(struct ics5342_info), GFP_KERNEL);
sizeof(*ics_info)?

Quoted 6 lines

Quoted 4 lines
drivers/video/fbdev/arkfb.c static struct dac_ops ics5342_ops = { · line 435
Quoted 1 line
drivers/video/fbdev/arkfb.c +1 -0
+       struct dac_info *info = &ics_info->dac;
Exactly my thought when I noticed this commit. Adding casts makes it harder to notice any future discrepancies.
I've changed it accordingly.

Thanks, but the one-line summary no longer matches what the commit is doing...

Commit f1a78a7d7827357c ("fbdev: arkfb: Cast ics5342_init() allocation type") in fbdev/for-next.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds