[PATCH] mISDN: Update parameter type of dsp_cmx_send()

[PATCH] mISDN: Update parameter type of dsp_cmx_send()

[Nathan Chancellor]

When booting a kernel with CONFIG_MISDN_DSP=y and CONFIG_CFI_CLANG=y,
there is a failure when dsp_cmx_send() is called indirectly from
call_timer_fn():

  [    0.371412] CFI failure at call_timer_fn+0x2f/0x150 (target: dsp_cmx_send+0x0/0x530; expected type: 0x92ada1e9)

The function pointer prototype that call_timer_fn() expects is

  void (*fn)(struct timer_list *)

whereas dsp_cmx_send() has a parameter type of 'void *', which causes
the control flow integrity checks to fail because the parameter types do
not match.

Change dsp_cmx_send()'s parameter type to be 'struct timer_list' to
match the expected prototype. The argument is unused anyways, so this
has no functional change, aside from avoiding the CFI failure.

Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202308020936.58787e6c-oliver.sang@intel.com
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
I am not sure if there is an appropriate fixes tag for this, I see this
area was modified by commit e313ac12eb13 ("mISDN: Convert timers to use
timer_setup()") but I don't think it was the original source of the
issue. It could also be commit cf68fffb66d6 ("add support for Clang
CFI") but I think that just exposes the problem/makes it fatal.

Also not sure who should take this or how soon it should go in, I'll let
that to maintainers to figure out :)
---
 drivers/isdn/mISDN/dsp.h      | 2 +-
 drivers/isdn/mISDN/dsp_cmx.c  | 2 +-
 drivers/isdn/mISDN/dsp_core.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/isdn/mISDN/dsp.h b/drivers/isdn/mISDN/dsp.h
index fa09d511a8ed..baf31258f5c9 100644
--- a/drivers/isdn/mISDN/dsp.h
+++ b/drivers/isdn/mISDN/dsp.h
@@ -247,7 +247,7 @@ extern void dsp_cmx_hardware(struct dsp_conf *conf, struct dsp *dsp);
 extern int dsp_cmx_conf(struct dsp *dsp, u32 conf_id);
 extern void dsp_cmx_receive(struct dsp *dsp, struct sk_buff *skb);
 extern void dsp_cmx_hdlc(struct dsp *dsp, struct sk_buff *skb);
-extern void dsp_cmx_send(void *arg);
+extern void dsp_cmx_send(struct timer_list *arg);
 extern void dsp_cmx_transmit(struct dsp *dsp, struct sk_buff *skb);
 extern int dsp_cmx_del_conf_member(struct dsp *dsp);
 extern int dsp_cmx_del_conf(struct dsp_conf *conf);
diff --git a/drivers/isdn/mISDN/dsp_cmx.c b/drivers/isdn/mISDN/dsp_cmx.c
index 357b87592eb4..61cb45c5d0d8 100644
--- a/drivers/isdn/mISDN/dsp_cmx.c
+++ b/drivers/isdn/mISDN/dsp_cmx.c
@@ -1614,7 +1614,7 @@ static u16	dsp_count; /* last sample count */
 static int	dsp_count_valid; /* if we have last sample count */
 
 void
-dsp_cmx_send(void *arg)
+dsp_cmx_send(struct timer_list *arg)
 {
 	struct dsp_conf *conf;
 	struct dsp_conf_member *member;
diff --git a/drivers/isdn/mISDN/dsp_core.c b/drivers/isdn/mISDN/dsp_core.c
index 386084530c2f..fae95f166688 100644
--- a/drivers/isdn/mISDN/dsp_core.c
+++ b/drivers/isdn/mISDN/dsp_core.c
@@ -1195,7 +1195,7 @@ static int __init dsp_init(void)
 	}
 
 	/* set sample timer */
-	timer_setup(&dsp_spl_tl, (void *)dsp_cmx_send, 0);
+	timer_setup(&dsp_spl_tl, dsp_cmx_send, 0);
 	dsp_spl_tl.expires = jiffies + dsp_tics;
 	dsp_spl_jiffies = dsp_spl_tl.expires;
 	add_timer(&dsp_spl_tl);

---
base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4
change-id: 20230802-fix-dsp_cmx_send-cfi-failure-2d4028b1ca63

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

Re: [PATCH] mISDN: Update parameter type of dsp_cmx_send()

[Sami Tolvanen]

Hi Nathan,

On Wed, Aug 2, 2023 at 10:40 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> When booting a kernel with CONFIG_MISDN_DSP=y and CONFIG_CFI_CLANG=y,
> there is a failure when dsp_cmx_send() is called indirectly from
> call_timer_fn():
>
>   [    0.371412] CFI failure at call_timer_fn+0x2f/0x150 (target: dsp_cmx_send+0x0/0x530; expected type: 0x92ada1e9)
>
> The function pointer prototype that call_timer_fn() expects is
>
>   void (*fn)(struct timer_list *)
>
> whereas dsp_cmx_send() has a parameter type of 'void *', which causes
> the control flow integrity checks to fail because the parameter types do
> not match.
>
> Change dsp_cmx_send()'s parameter type to be 'struct timer_list' to
> match the expected prototype. The argument is unused anyways, so this
> has no functional change, aside from avoiding the CFI failure.

Looks correct to me, thanks for fixing this!

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

Re: [PATCH] mISDN: Update parameter type of dsp_cmx_send()

[Kees Cook]

On Wed, Aug 02, 2023 at 10:40:29AM -0700, Nathan Chancellor wrote:
> When booting a kernel with CONFIG_MISDN_DSP=y and CONFIG_CFI_CLANG=y,
> there is a failure when dsp_cmx_send() is called indirectly from
> call_timer_fn():
> 
>   [    0.371412] CFI failure at call_timer_fn+0x2f/0x150 (target: dsp_cmx_send+0x0/0x530; expected type: 0x92ada1e9)
> 
> The function pointer prototype that call_timer_fn() expects is
> 
>   void (*fn)(struct timer_list *)
> 
> whereas dsp_cmx_send() has a parameter type of 'void *', which causes
> the control flow integrity checks to fail because the parameter types do
> not match.
> 
> Change dsp_cmx_send()'s parameter type to be 'struct timer_list' to
> match the expected prototype. The argument is unused anyways, so this
> has no functional change, aside from avoiding the CFI failure.
> 
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202308020936.58787e6c-oliver.sang@intel.com
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
> I am not sure if there is an appropriate fixes tag for this, I see this
> area was modified by commit e313ac12eb13 ("mISDN: Convert timers to use
> timer_setup()") but I don't think it was the original source of the
> issue. It could also be commit cf68fffb66d6 ("add support for Clang
> CFI") but I think that just exposes the problem/makes it fatal.

Oh man. I missed one! How did I miss that one? I think "Fixes:
e313ac12eb13" is the most correct. That was the patch that went through
trying to fix all the prototypes, and _did_ fix all the _other_ prototypes
in there.

Thanks for the patch!

Reviewed-by: Kees Cook <keescook@chromium.org>

> 
> Also not sure who should take this or how soon it should go in, I'll let
> that to maintainers to figure out :)

If no one speaks up, I'll snag it, but since this got aimed at netdev, I
suspect someone may pick it up. :)

-Kees

-- 
Kees Cook

Re: [PATCH] mISDN: Update parameter type of dsp_cmx_send()

[Nathan Chancellor]

On Wed, Aug 02, 2023 at 12:59:12PM -0700, Kees Cook wrote:
> On Wed, Aug 02, 2023 at 10:40:29AM -0700, Nathan Chancellor wrote:
> > When booting a kernel with CONFIG_MISDN_DSP=y and CONFIG_CFI_CLANG=y,
> > there is a failure when dsp_cmx_send() is called indirectly from
> > call_timer_fn():
> > 
> >   [    0.371412] CFI failure at call_timer_fn+0x2f/0x150 (target: dsp_cmx_send+0x0/0x530; expected type: 0x92ada1e9)
> > 
> > The function pointer prototype that call_timer_fn() expects is
> > 
> >   void (*fn)(struct timer_list *)
> > 
> > whereas dsp_cmx_send() has a parameter type of 'void *', which causes
> > the control flow integrity checks to fail because the parameter types do
> > not match.
> > 
> > Change dsp_cmx_send()'s parameter type to be 'struct timer_list' to
> > match the expected prototype. The argument is unused anyways, so this
> > has no functional change, aside from avoiding the CFI failure.
> > 
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Closes: https://lore.kernel.org/oe-lkp/202308020936.58787e6c-oliver.sang@intel.com
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > ---
> > I am not sure if there is an appropriate fixes tag for this, I see this
> > area was modified by commit e313ac12eb13 ("mISDN: Convert timers to use
> > timer_setup()") but I don't think it was the original source of the
> > issue. It could also be commit cf68fffb66d6 ("add support for Clang
> > CFI") but I think that just exposes the problem/makes it fatal.
> 
> Oh man. I missed one! How did I miss that one? I think "Fixes:
> e313ac12eb13" is the most correct. That was the patch that went through
> trying to fix all the prototypes, and _did_ fix all the _other_ prototypes
> in there.

Sounds reasonable to me. netdev folks, if you intend to take this, do
you want a v2 or can you pick it up with

Fixes: e313ac12eb13 ("mISDN: Convert timers to use timer_setup()")

added on top?

> Thanks for the patch!
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> > 
> > Also not sure who should take this or how soon it should go in, I'll let
> > that to maintainers to figure out :)
> 
> If no one speaks up, I'll snag it, but since this got aimed at netdev, I
> suspect someone may pick it up. :)

Sounds good, I do see it in the netdev patchwork, so we can watch it at
least.

Cheers,
Nathan

Re: [PATCH] mISDN: Update parameter type of dsp_cmx_send()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 02 Aug 2023 10:40:29 -0700 you wrote:
> When booting a kernel with CONFIG_MISDN_DSP=y and CONFIG_CFI_CLANG=y,
> there is a failure when dsp_cmx_send() is called indirectly from
> call_timer_fn():
> 
>   [    0.371412] CFI failure at call_timer_fn+0x2f/0x150 (target: dsp_cmx_send+0x0/0x530; expected type: 0x92ada1e9)
> 
> The function pointer prototype that call_timer_fn() expects is
> 
> [...]

Here is the summary with links:
  - mISDN: Update parameter type of dsp_cmx_send()
    https://git.kernel.org/netdev/net/c/1696ec865401

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html