| Message ID | 20230802-fix-dsp_cmx_send-cfi-failure-v1-1-2f2e79b0178d@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1696ec8654016dad3b1baf6c024303e584400453 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mISDN: Update parameter type of dsp_cmx_send() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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
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
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
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!
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);
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(-) --- base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4 change-id: 20230802-fix-dsp_cmx_send-cfi-failure-2d4028b1ca63 Best regards,