linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks
@ 2023-06-22 16:12 Nathan Chancellor
  2023-06-22 16:43 ` Nick Desaulniers
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Nathan Chancellor @ 2023-06-22 16:12 UTC (permalink / raw)
  To: lee
  Cc: sean.wang, pavel, matthias.bgg, angelogioacchino.delregno,
	ndesaulniers, trix, linux-leds, linux-arm-kernel, linux-mediatek,
	llvm, patches, Nathan Chancellor

Clang's kernel Control Flow Integrity (kCFI) is a compiler-based
security mitigation that ensures the target of an indirect function call
matches the expected type of the call and trapping if they do not match
exactly. The warning -Wincompatible-function-pointer-types-strict aims
to catch these issues at compile time, which reveals:

 drivers/leds/leds-mt6323.c:598:49: error: incompatible function pointer types assigning to 'int (*)(struct led_classdev *, enum led_brightness)' from 'int (struct led_classdev *, unsigned int)' [-Werror,-Wincompatible-function-pointer-types-strict]
   598 |                         leds->led[reg]->cdev.brightness_set_blocking =
       |                                                                      ^
   599 |                                                 mt6323_wled_set_brightness;
       |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 drivers/leds/leds-mt6323.c:600:40: error: incompatible function pointer types assigning to 'enum led_brightness (*)(struct led_classdev *)' from 'unsigned int (struct led_classdev *)' [-Werror,-Wincompatible-function-pointer-types-strict]
   600 |                         leds->led[reg]->cdev.brightness_get =
       |                                                             ^
   601 |                                                 mt6323_get_wled_brightness;
       |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 2 errors generated.

While 'unsigned int' is ABI compatible with 'enum led_brightness' (hence
no warning from -Wincompatible-function-pointer-types) and the callers
of these callbacks use/pass the values as 'unsigned int', the mismatch
between the prototype and the called function will trip kCFI at runtime.

Change the types in the implementations to match the prototypes, clearing
up the warning and avoiding kCFI failures.

Fixes: 9bb0a9e0626c ("leds: leds-mt6323: Add support for WLEDs and MT6332")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/leds/leds-mt6323.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-mt6323.c b/drivers/leds/leds-mt6323.c
index e8fecfc2e90a..24f35bdb55fb 100644
--- a/drivers/leds/leds-mt6323.c
+++ b/drivers/leds/leds-mt6323.c
@@ -76,7 +76,7 @@ struct mt6323_led {
 	int			id;
 	struct mt6323_leds	*parent;
 	struct led_classdev	cdev;
-	unsigned int		current_brightness;
+	enum led_brightness	current_brightness;
 };
 
 /**
@@ -451,7 +451,7 @@ static int mtk_wled_hw_off(struct led_classdev *cdev)
 	return 0;
 }
 
-static unsigned int mt6323_get_wled_brightness(struct led_classdev *cdev)
+static enum led_brightness mt6323_get_wled_brightness(struct led_classdev *cdev)
 {
 	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
 	struct mt6323_leds *leds = led->parent;
@@ -471,7 +471,7 @@ static unsigned int mt6323_get_wled_brightness(struct led_classdev *cdev)
 }
 
 static int mt6323_wled_set_brightness(struct led_classdev *cdev,
-				      unsigned int brightness)
+				      enum led_brightness brightness)
 {
 	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
 	struct mt6323_leds *leds = led->parent;

---
base-commit: 7bd932d9adbcc5c5370d968bdb0b00385606bf3a
change-id: 20230621-mt6323-wled-wincompatible-function-pointer-types-strict-334f06d92ffb

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks
  2023-06-22 16:12 [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks Nathan Chancellor
@ 2023-06-22 16:43 ` Nick Desaulniers
  2023-06-22 16:54   ` Lee Jones
  2023-06-23  8:04 ` AngeloGioacchino Del Regno
  2023-06-26  9:10 ` Lee Jones
  2 siblings, 1 reply; 5+ messages in thread
From: Nick Desaulniers @ 2023-06-22 16:43 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: lee, sean.wang, pavel, matthias.bgg, angelogioacchino.delregno,
	trix, linux-leds, linux-arm-kernel, linux-mediatek, llvm, patches

On Thu, Jun 22, 2023 at 9:12 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> Clang's kernel Control Flow Integrity (kCFI) is a compiler-based
> security mitigation that ensures the target of an indirect function call
> matches the expected type of the call and trapping if they do not match
> exactly. The warning -Wincompatible-function-pointer-types-strict aims
> to catch these issues at compile time, which reveals:
>
>  drivers/leds/leds-mt6323.c:598:49: error: incompatible function pointer types assigning to 'int (*)(struct led_classdev *, enum led_brightness)' from 'int (struct led_classdev *, unsigned int)' [-Werror,-Wincompatible-function-pointer-types-strict]
>    598 |                         leds->led[reg]->cdev.brightness_set_blocking =
>        |                                                                      ^
>    599 |                                                 mt6323_wled_set_brightness;
>        |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
>  drivers/leds/leds-mt6323.c:600:40: error: incompatible function pointer types assigning to 'enum led_brightness (*)(struct led_classdev *)' from 'unsigned int (struct led_classdev *)' [-Werror,-Wincompatible-function-pointer-types-strict]
>    600 |                         leds->led[reg]->cdev.brightness_get =
>        |                                                             ^
>    601 |                                                 mt6323_get_wled_brightness;
>        |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
>  2 errors generated.
>
> While 'unsigned int' is ABI compatible with 'enum led_brightness' (hence
> no warning from -Wincompatible-function-pointer-types) and the callers
> of these callbacks use/pass the values as 'unsigned int', the mismatch
> between the prototype and the called function will trip kCFI at runtime.
>
> Change the types in the implementations to match the prototypes, clearing
> up the warning and avoiding kCFI failures.
>
> Fixes: 9bb0a9e0626c ("leds: leds-mt6323: Add support for WLEDs and MT6332")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

Thanks for the patch! Consider additionally having
mt6323_get_wled_brightness return LED_OFF rather than 0 in its
ternary.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>


> ---
>  drivers/leds/leds-mt6323.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-mt6323.c b/drivers/leds/leds-mt6323.c
> index e8fecfc2e90a..24f35bdb55fb 100644
> --- a/drivers/leds/leds-mt6323.c
> +++ b/drivers/leds/leds-mt6323.c
> @@ -76,7 +76,7 @@ struct mt6323_led {
>         int                     id;
>         struct mt6323_leds      *parent;
>         struct led_classdev     cdev;
> -       unsigned int            current_brightness;
> +       enum led_brightness     current_brightness;
>  };
>
>  /**
> @@ -451,7 +451,7 @@ static int mtk_wled_hw_off(struct led_classdev *cdev)
>         return 0;
>  }
>
> -static unsigned int mt6323_get_wled_brightness(struct led_classdev *cdev)
> +static enum led_brightness mt6323_get_wled_brightness(struct led_classdev *cdev)
>  {
>         struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
>         struct mt6323_leds *leds = led->parent;
> @@ -471,7 +471,7 @@ static unsigned int mt6323_get_wled_brightness(struct led_classdev *cdev)
>  }
>
>  static int mt6323_wled_set_brightness(struct led_classdev *cdev,
> -                                     unsigned int brightness)
> +                                     enum led_brightness brightness)
>  {
>         struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
>         struct mt6323_leds *leds = led->parent;
>
> ---
> base-commit: 7bd932d9adbcc5c5370d968bdb0b00385606bf3a
> change-id: 20230621-mt6323-wled-wincompatible-function-pointer-types-strict-334f06d92ffb
>
> Best regards,
> --
> Nathan Chancellor <nathan@kernel.org>
>
>


-- 
Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks
  2023-06-22 16:43 ` Nick Desaulniers
@ 2023-06-22 16:54   ` Lee Jones
  0 siblings, 0 replies; 5+ messages in thread
From: Lee Jones @ 2023-06-22 16:54 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Nathan Chancellor, sean.wang, pavel, matthias.bgg,
	angelogioacchino.delregno, trix, linux-leds, linux-arm-kernel,
	linux-mediatek, llvm, patches

On Thu, 22 Jun 2023, Nick Desaulniers wrote:

> On Thu, Jun 22, 2023 at 9:12 AM Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > Clang's kernel Control Flow Integrity (kCFI) is a compiler-based
> > security mitigation that ensures the target of an indirect function call
> > matches the expected type of the call and trapping if they do not match
> > exactly. The warning -Wincompatible-function-pointer-types-strict aims
> > to catch these issues at compile time, which reveals:
> >
> >  drivers/leds/leds-mt6323.c:598:49: error: incompatible function pointer types assigning to 'int (*)(struct led_classdev *, enum led_brightness)' from 'int (struct led_classdev *, unsigned int)' [-Werror,-Wincompatible-function-pointer-types-strict]
> >    598 |                         leds->led[reg]->cdev.brightness_set_blocking =
> >        |                                                                      ^
> >    599 |                                                 mt6323_wled_set_brightness;
> >        |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
> >  drivers/leds/leds-mt6323.c:600:40: error: incompatible function pointer types assigning to 'enum led_brightness (*)(struct led_classdev *)' from 'unsigned int (struct led_classdev *)' [-Werror,-Wincompatible-function-pointer-types-strict]
> >    600 |                         leds->led[reg]->cdev.brightness_get =
> >        |                                                             ^
> >    601 |                                                 mt6323_get_wled_brightness;
> >        |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
> >  2 errors generated.
> >
> > While 'unsigned int' is ABI compatible with 'enum led_brightness' (hence
> > no warning from -Wincompatible-function-pointer-types) and the callers
> > of these callbacks use/pass the values as 'unsigned int', the mismatch
> > between the prototype and the called function will trip kCFI at runtime.
> >
> > Change the types in the implementations to match the prototypes, clearing
> > up the warning and avoiding kCFI failures.
> >
> > Fixes: 9bb0a9e0626c ("leds: leds-mt6323: Add support for WLEDs and MT6332")
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> 
> Thanks for the patch! Consider additionally having
> mt6323_get_wled_brightness return LED_OFF rather than 0 in its
> ternary.

https://elixir.bootlin.com/linux/latest/source/include/linux/leds.h#L32

Perhaps this is not relevant for this older driver though?

I'd really like some more information from Pavel on the history.

> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> 
> > ---
> >  drivers/leds/leds-mt6323.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/leds/leds-mt6323.c b/drivers/leds/leds-mt6323.c
> > index e8fecfc2e90a..24f35bdb55fb 100644
> > --- a/drivers/leds/leds-mt6323.c
> > +++ b/drivers/leds/leds-mt6323.c
> > @@ -76,7 +76,7 @@ struct mt6323_led {
> >         int                     id;
> >         struct mt6323_leds      *parent;
> >         struct led_classdev     cdev;
> > -       unsigned int            current_brightness;
> > +       enum led_brightness     current_brightness;
> >  };
> >
> >  /**
> > @@ -451,7 +451,7 @@ static int mtk_wled_hw_off(struct led_classdev *cdev)
> >         return 0;
> >  }
> >
> > -static unsigned int mt6323_get_wled_brightness(struct led_classdev *cdev)
> > +static enum led_brightness mt6323_get_wled_brightness(struct led_classdev *cdev)
> >  {
> >         struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
> >         struct mt6323_leds *leds = led->parent;
> > @@ -471,7 +471,7 @@ static unsigned int mt6323_get_wled_brightness(struct led_classdev *cdev)
> >  }
> >
> >  static int mt6323_wled_set_brightness(struct led_classdev *cdev,
> > -                                     unsigned int brightness)
> > +                                     enum led_brightness brightness)
> >  {
> >         struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
> >         struct mt6323_leds *leds = led->parent;
> >
> > ---
> > base-commit: 7bd932d9adbcc5c5370d968bdb0b00385606bf3a
> > change-id: 20230621-mt6323-wled-wincompatible-function-pointer-types-strict-334f06d92ffb
> >
> > Best regards,
> > --
> > Nathan Chancellor <nathan@kernel.org>
> >
> >
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers

-- 
Lee Jones [李琼斯]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks
  2023-06-22 16:12 [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks Nathan Chancellor
  2023-06-22 16:43 ` Nick Desaulniers
@ 2023-06-23  8:04 ` AngeloGioacchino Del Regno
  2023-06-26  9:10 ` Lee Jones
  2 siblings, 0 replies; 5+ messages in thread
From: AngeloGioacchino Del Regno @ 2023-06-23  8:04 UTC (permalink / raw)
  To: Nathan Chancellor, lee
  Cc: sean.wang, pavel, matthias.bgg, ndesaulniers, trix, linux-leds,
	linux-arm-kernel, linux-mediatek, llvm, patches

Il 22/06/23 18:12, Nathan Chancellor ha scritto:
> Clang's kernel Control Flow Integrity (kCFI) is a compiler-based
> security mitigation that ensures the target of an indirect function call
> matches the expected type of the call and trapping if they do not match
> exactly. The warning -Wincompatible-function-pointer-types-strict aims
> to catch these issues at compile time, which reveals:
> 
>   drivers/leds/leds-mt6323.c:598:49: error: incompatible function pointer types assigning to 'int (*)(struct led_classdev *, enum led_brightness)' from 'int (struct led_classdev *, unsigned int)' [-Werror,-Wincompatible-function-pointer-types-strict]
>     598 |                         leds->led[reg]->cdev.brightness_set_blocking =
>         |                                                                      ^
>     599 |                                                 mt6323_wled_set_brightness;
>         |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
>   drivers/leds/leds-mt6323.c:600:40: error: incompatible function pointer types assigning to 'enum led_brightness (*)(struct led_classdev *)' from 'unsigned int (struct led_classdev *)' [-Werror,-Wincompatible-function-pointer-types-strict]
>     600 |                         leds->led[reg]->cdev.brightness_get =
>         |                                                             ^
>     601 |                                                 mt6323_get_wled_brightness;
>         |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
>   2 errors generated.
> 
> While 'unsigned int' is ABI compatible with 'enum led_brightness' (hence
> no warning from -Wincompatible-function-pointer-types) and the callers
> of these callbacks use/pass the values as 'unsigned int', the mismatch
> between the prototype and the called function will trip kCFI at runtime.
> 
> Change the types in the implementations to match the prototypes, clearing
> up the warning and avoiding kCFI failures.
> 
> Fixes: 9bb0a9e0626c ("leds: leds-mt6323: Add support for WLEDs and MT6332")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

leds need to deprecate said enumeration for real, since it's marked for
deprecation but, in the meanwhile, we can't have kCFI failures on this driver.

For this reason,

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Cheers,
Angelo

> ---
>   drivers/leds/leds-mt6323.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/leds-mt6323.c b/drivers/leds/leds-mt6323.c
> index e8fecfc2e90a..24f35bdb55fb 100644
> --- a/drivers/leds/leds-mt6323.c
> +++ b/drivers/leds/leds-mt6323.c
> @@ -76,7 +76,7 @@ struct mt6323_led {
>   	int			id;
>   	struct mt6323_leds	*parent;
>   	struct led_classdev	cdev;
> -	unsigned int		current_brightness;
> +	enum led_brightness	current_brightness;
>   };
>   
>   /**
> @@ -451,7 +451,7 @@ static int mtk_wled_hw_off(struct led_classdev *cdev)
>   	return 0;
>   }
>   
> -static unsigned int mt6323_get_wled_brightness(struct led_classdev *cdev)
> +static enum led_brightness mt6323_get_wled_brightness(struct led_classdev *cdev)
>   {
>   	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
>   	struct mt6323_leds *leds = led->parent;
> @@ -471,7 +471,7 @@ static unsigned int mt6323_get_wled_brightness(struct led_classdev *cdev)
>   }
>   
>   static int mt6323_wled_set_brightness(struct led_classdev *cdev,
> -				      unsigned int brightness)
> +				      enum led_brightness brightness)
>   {
>   	struct mt6323_led *led = container_of(cdev, struct mt6323_led, cdev);
>   	struct mt6323_leds *leds = led->parent;
> 
> ---
> base-commit: 7bd932d9adbcc5c5370d968bdb0b00385606bf3a
> change-id: 20230621-mt6323-wled-wincompatible-function-pointer-types-strict-334f06d92ffb
> 
> Best regards,



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks
  2023-06-22 16:12 [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks Nathan Chancellor
  2023-06-22 16:43 ` Nick Desaulniers
  2023-06-23  8:04 ` AngeloGioacchino Del Regno
@ 2023-06-26  9:10 ` Lee Jones
  2 siblings, 0 replies; 5+ messages in thread
From: Lee Jones @ 2023-06-26  9:10 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: sean.wang, pavel, matthias.bgg, angelogioacchino.delregno,
	ndesaulniers, trix, linux-leds, linux-arm-kernel, linux-mediatek,
	llvm, patches

On Thu, 22 Jun 2023, Nathan Chancellor wrote:

> Clang's kernel Control Flow Integrity (kCFI) is a compiler-based
> security mitigation that ensures the target of an indirect function call
> matches the expected type of the call and trapping if they do not match
> exactly. The warning -Wincompatible-function-pointer-types-strict aims
> to catch these issues at compile time, which reveals:
> 
>  drivers/leds/leds-mt6323.c:598:49: error: incompatible function pointer types assigning to 'int (*)(struct led_classdev *, enum led_brightness)' from 'int (struct led_classdev *, unsigned int)' [-Werror,-Wincompatible-function-pointer-types-strict]
>    598 |                         leds->led[reg]->cdev.brightness_set_blocking =
>        |                                                                      ^
>    599 |                                                 mt6323_wled_set_brightness;
>        |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
>  drivers/leds/leds-mt6323.c:600:40: error: incompatible function pointer types assigning to 'enum led_brightness (*)(struct led_classdev *)' from 'unsigned int (struct led_classdev *)' [-Werror,-Wincompatible-function-pointer-types-strict]
>    600 |                         leds->led[reg]->cdev.brightness_get =
>        |                                                             ^
>    601 |                                                 mt6323_get_wled_brightness;
>        |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
>  2 errors generated.
> 
> While 'unsigned int' is ABI compatible with 'enum led_brightness' (hence
> no warning from -Wincompatible-function-pointer-types) and the callers
> of these callbacks use/pass the values as 'unsigned int', the mismatch
> between the prototype and the called function will trip kCFI at runtime.
> 
> Change the types in the implementations to match the prototypes, clearing
> up the warning and avoiding kCFI failures.
> 
> Fixes: 9bb0a9e0626c ("leds: leds-mt6323: Add support for WLEDs and MT6332")
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/leds/leds-mt6323.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Applied, thanks

-- 
Lee Jones [李琼斯]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-06-26  9:11 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-22 16:12 [PATCH] leds: leds-mt6323: Adjust return/parameter types in wled get/set callbacks Nathan Chancellor
2023-06-22 16:43 ` Nick Desaulniers
2023-06-22 16:54   ` Lee Jones
2023-06-23  8:04 ` AngeloGioacchino Del Regno
2023-06-26  9:10 ` Lee Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).