Re: [PATCH] ALSA: fix function cast warnings

[Takashi Iwai]

On Tue, 13 Feb 2024 11:09:56 +0100,
Arnd Bergmann wrote:
> 
> From: Arnd Bergmann <arnd@xxxxxxxx>
> 
> clang-16 points out a control flow integrity (kcfi) issue when event
> callbacks get converted to incompatible types:
> 
> sound/core/seq/seq_midi.c:135:30: error: cast from 'int (*)(struct snd_rawmidi_substream *, const char *, int)' to 'snd_seq_dump_func_t' (aka 'int (*)(void *, void *, int)') converts to incompatible function type [-Werror,-Wcast-function-type-strict]
>   135 |                 snd_seq_dump_var_event(ev, (snd_seq_dump_func_t)dump_midi, substream);
>       |                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> sound/core/seq/seq_virmidi.c:83:31: error: cast from 'int (*)(struct snd_rawmidi_substream *, const unsigned char *, int)' to 'snd_seq_dump_func_t' (aka 'int (*)(void *, void *, int)') converts to incompatible function type [-Werror,-Wcast-function-type-strict]
>    83 |                         snd_seq_dump_var_event(ev, (snd_seq_dump_func_t)snd_rawmidi_receive, vmidi->substream);
>       |                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Change these both to take a 'const void *' buffer and a 'void *' context,
> converting to the respective types in the callee. The change to 'const'
> buffers propagates to a couple of other functions.
> 
> The code was originally added with the initial ALSA merge in linux-2.5.4.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
>  include/sound/rawmidi.h            | 3 +--
>  include/sound/seq_kernel.h         | 2 +-
>  sound/core/rawmidi.c               | 6 +++---
>  sound/core/seq/oss/seq_oss_readq.c | 4 ++--
>  sound/core/seq/oss/seq_oss_readq.h | 2 +-
>  sound/core/seq/seq_memory.c        | 4 ++--
>  sound/core/seq/seq_midi.c          | 5 +++--
>  sound/core/seq/seq_virmidi.c       | 2 +-
>  8 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
> index f31cabf0158c..91947fb16e07 100644
> --- a/include/sound/rawmidi.h
> +++ b/include/sound/rawmidi.h
> @@ -161,8 +161,7 @@ int snd_rawmidi_free(struct snd_rawmidi *rmidi);
>  
>  /* callbacks */
>  
> -int snd_rawmidi_receive(struct snd_rawmidi_substream *substream,
> -			const unsigned char *buffer, int count);
> +int snd_rawmidi_receive(void *ptr, const void *buffer, int count);

If it were only about the type of the buffer argument being a void
pointer, it's fine.  But the substream argument should be explicitly
typed, otherwise it's confusing for other normal call patterns.

I guess the suitable fix for now would be to provide wrapper functions
that are used for callbacks and bridge to the actual function with
pointer cast, something like below.  Eventually we can put more const,
but it's basically irrelevant with the warning itself.


thanks,

Takashi

-- 8< --
--- a/sound/core/seq/seq_midi.c
+++ b/sound/core/seq/seq_midi.c
@@ -113,6 +113,12 @@ static int dump_midi(struct snd_rawmidi_substream *substream, const char *buf, i
 	return 0;
 }
 
+/* callback for snd_seq_dump_var_event(), bridging to dump_midi() */
+static int __dump_midi(void *ptr, void *buf, int count)
+{
+	return dump_midi(ptr, buf, count);
+}
+
 static int event_process_midi(struct snd_seq_event *ev, int direct,
 			      void *private_data, int atomic, int hop)
 {
@@ -132,7 +138,7 @@ static int event_process_midi(struct snd_seq_event *ev, int direct,
 			pr_debug("ALSA: seq_midi: invalid sysex event flags = 0x%x\n", ev->flags);
 			return 0;
 		}
-		snd_seq_dump_var_event(ev, (snd_seq_dump_func_t)dump_midi, substream);
+		snd_seq_dump_var_event(ev, __dump_midi, substream);
 		snd_midi_event_reset_decode(msynth->parser);
 	} else {
 		if (msynth->parser == NULL)
--- a/sound/core/seq/seq_virmidi.c
+++ b/sound/core/seq/seq_virmidi.c
@@ -62,6 +62,13 @@ static void snd_virmidi_init_event(struct snd_virmidi *vmidi,
 /*
  * decode input event and put to read buffer of each opened file
  */
+
+/* callback for snd_seq_dump_var_event(), bridging to snd_rawmidi_receive() */
+static int dump_to_rawmidi(void *ptr, void *buf, int count)
+{
+	return snd_rawmidi_receive(ptr, buf, count);
+}
+
 static int snd_virmidi_dev_receive_event(struct snd_virmidi_dev *rdev,
 					 struct snd_seq_event *ev,
 					 bool atomic)
@@ -80,7 +87,7 @@ static int snd_virmidi_dev_receive_event(struct snd_virmidi_dev *rdev,
 		if (ev->type == SNDRV_SEQ_EVENT_SYSEX) {
 			if ((ev->flags & SNDRV_SEQ_EVENT_LENGTH_MASK) != SNDRV_SEQ_EVENT_LENGTH_VARIABLE)
 				continue;
-			snd_seq_dump_var_event(ev, (snd_seq_dump_func_t)snd_rawmidi_receive, vmidi->substream);
+			snd_seq_dump_var_event(ev, dump_to_rawmidi, vmidi->substream);
 			snd_midi_event_reset_decode(vmidi->parser);
 		} else {
 			len = snd_midi_event_decode(vmidi->parser, msg, sizeof(msg), ev);