From 98482377dc7295d0c70e251925b7cc14aff4c5ac Mon Sep 17 00:00:00 2001 From: Evan Green Date: Mon, 1 Jul 2019 10:30:30 -0700 Subject: [PATCH 1/3] ALSA: hda: Fix widget_mutex incomplete protection The widget_mutex was introduced to serialize callers to hda_widget_sysfs_{re}init. However, its protection of the sysfs widget array is incomplete. For example, it is acquired around the call to hda_widget_sysfs_reinit(), which actually creates the new array, but isn't still acquired when codec->num_nodes and codec->start_nid is updated. So the lock ensures one thread sets up the new array at a time, but doesn't ensure which thread's value will end up in codec->num_nodes. If a larger num_nodes wins but a smaller array was set up, the next call to refresh_widgets() will touch free memory as it iterates over codec->num_nodes that aren't there. The widget_lock really protects both the tree as well as codec->num_nodes, start_nid, and end_nid, so make sure it's held across that update. It should also be held during snd_hdac_get_sub_nodes(), so that a very old read from that function doesn't end up clobbering a later update. Fixes: ed180abba7f1 ("ALSA: hda: Fix race between creating and refreshing sysfs entries") Signed-off-by: Evan Green Signed-off-by: Takashi Iwai --- sound/hda/hdac_device.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c index 4769f4c03e14..11050bfd8068 100644 --- a/sound/hda/hdac_device.c +++ b/sound/hda/hdac_device.c @@ -399,27 +399,33 @@ static void setup_fg_nodes(struct hdac_device *codec) int snd_hdac_refresh_widgets(struct hdac_device *codec, bool sysfs) { hda_nid_t start_nid; - int nums, err; + int nums, err = 0; + /* + * Serialize against multiple threads trying to update the sysfs + * widgets array. + */ + mutex_lock(&codec->widget_lock); nums = snd_hdac_get_sub_nodes(codec, codec->afg, &start_nid); if (!start_nid || nums <= 0 || nums >= 0xff) { dev_err(&codec->dev, "cannot read sub nodes for FG 0x%02x\n", codec->afg); - return -EINVAL; + err = -EINVAL; + goto unlock; } if (sysfs) { - mutex_lock(&codec->widget_lock); err = hda_widget_sysfs_reinit(codec, start_nid, nums); - mutex_unlock(&codec->widget_lock); if (err < 0) - return err; + goto unlock; } codec->num_nodes = nums; codec->start_nid = start_nid; codec->end_nid = start_nid + nums; - return 0; +unlock: + mutex_unlock(&codec->widget_lock); + return err; } EXPORT_SYMBOL_GPL(snd_hdac_refresh_widgets); From 3450121997ce872eb7f1248417225827ea249710 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Tue, 2 Jul 2019 20:07:21 +0200 Subject: [PATCH 2/3] ALSA: line6: Fix write on zero-sized buffer LINE6 drivers allocate the buffers based on the value returned from usb_maxpacket() calls. The manipulated device may return zero for this, and this results in the kmalloc() with zero size (and it may succeed) while the other part of the driver code writes the packet data with the fixed size -- which eventually overwrites. This patch adds a simple sanity check for the invalid buffer size for avoiding that problem. Reported-by: syzbot+219f00fb49874dcaea17@syzkaller.appspotmail.com Cc: Signed-off-by: Takashi Iwai --- sound/usb/line6/pcm.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c index 72c6f8e82a7e..78c2d6cab3b5 100644 --- a/sound/usb/line6/pcm.c +++ b/sound/usb/line6/pcm.c @@ -560,6 +560,11 @@ int line6_init_pcm(struct usb_line6 *line6, line6pcm->max_packet_size_out = usb_maxpacket(line6->usbdev, usb_sndisocpipe(line6->usbdev, ep_write), 1); + if (!line6pcm->max_packet_size_in || !line6pcm->max_packet_size_out) { + dev_err(line6pcm->line6->ifcdev, + "cannot get proper max packet size\n"); + return -EINVAL; + } spin_lock_init(&line6pcm->out.lock); spin_lock_init(&line6pcm->in.lock); From 774a075ab5140bb4504e6026bf327021926c3e65 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Wed, 3 Jul 2019 14:35:12 +0200 Subject: [PATCH 3/3] ALSA: hda: Simplify snd_hdac_refresh_widgets() Along with the recent fix for the races of snd_hdac_refresh_widgets() it turned out that the instantiation of widgets sysfs at snd_hdac_sysfs_reinit() could cause a race. The race itself was already covered later by extending the mutex protection range, the commit 98482377dc72 ("ALSA: hda: Fix widget_mutex incomplete protection"), but this also indicated that the call of *_reinit() is basically superfluous, as the widgets shall be created sooner or later from snd_hdac_device_register(). This patch removes the redundant call of snd_hdac_sysfs_reinit() at first. By this removal, the sysfs argument itself in snd_hdac_refresh_widgets() becomes superfluous, too, because the only case sysfs=false is always with codec->widgets=NULL. So, we drop this redundant argument as well. Signed-off-by: Takashi Iwai --- include/sound/hdaudio.h | 2 +- sound/hda/hdac_device.c | 13 +++++-------- sound/hda/hdac_sysfs.c | 2 +- sound/pci/hda/hda_codec.c | 2 +- sound/soc/codecs/hdac_hdmi.c | 2 +- 5 files changed, 9 insertions(+), 12 deletions(-) diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index e8346784cf3f..f475293d0668 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -120,7 +120,7 @@ void snd_hdac_device_unregister(struct hdac_device *codec); int snd_hdac_device_set_chip_name(struct hdac_device *codec, const char *name); int snd_hdac_codec_modalias(struct hdac_device *hdac, char *buf, size_t size); -int snd_hdac_refresh_widgets(struct hdac_device *codec, bool sysfs); +int snd_hdac_refresh_widgets(struct hdac_device *codec); unsigned int snd_hdac_make_cmd(struct hdac_device *codec, hda_nid_t nid, unsigned int verb, unsigned int parm); diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c index 11050bfd8068..a265c1d68876 100644 --- a/sound/hda/hdac_device.c +++ b/sound/hda/hdac_device.c @@ -89,7 +89,7 @@ int snd_hdac_device_init(struct hdac_device *codec, struct hdac_bus *bus, fg = codec->afg ? codec->afg : codec->mfg; - err = snd_hdac_refresh_widgets(codec, false); + err = snd_hdac_refresh_widgets(codec); if (err < 0) goto error; @@ -394,9 +394,8 @@ static void setup_fg_nodes(struct hdac_device *codec) /** * snd_hdac_refresh_widgets - Reset the widget start/end nodes * @codec: the codec object - * @sysfs: re-initialize sysfs tree, too */ -int snd_hdac_refresh_widgets(struct hdac_device *codec, bool sysfs) +int snd_hdac_refresh_widgets(struct hdac_device *codec) { hda_nid_t start_nid; int nums, err = 0; @@ -414,11 +413,9 @@ int snd_hdac_refresh_widgets(struct hdac_device *codec, bool sysfs) goto unlock; } - if (sysfs) { - err = hda_widget_sysfs_reinit(codec, start_nid, nums); - if (err < 0) - goto unlock; - } + err = hda_widget_sysfs_reinit(codec, start_nid, nums); + if (err < 0) + goto unlock; codec->num_nodes = nums; codec->start_nid = start_nid; diff --git a/sound/hda/hdac_sysfs.c b/sound/hda/hdac_sysfs.c index 909d5ef1179c..e56e83325903 100644 --- a/sound/hda/hdac_sysfs.c +++ b/sound/hda/hdac_sysfs.c @@ -428,7 +428,7 @@ int hda_widget_sysfs_reinit(struct hdac_device *codec, int i; if (!codec->widgets) - return hda_widget_sysfs_init(codec); + return 0; tree = kmemdup(codec->widgets, sizeof(*tree), GFP_KERNEL); if (!tree) diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index fcdf2cd3783b..d1a0e0de80ac 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -1016,7 +1016,7 @@ int snd_hda_codec_update_widgets(struct hda_codec *codec) hda_nid_t fg; int err; - err = snd_hdac_refresh_widgets(&codec->core, true); + err = snd_hdac_refresh_widgets(&codec->core); if (err < 0) return err; diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 660e0587f399..6302ad5b7128 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -2043,7 +2043,7 @@ static int hdac_hdmi_dev_probe(struct hdac_device *hdev) "Failed in parse and map nid with err: %d\n", ret); return ret; } - snd_hdac_refresh_widgets(hdev, true); + snd_hdac_refresh_widgets(hdev); /* ASoC specific initialization */ ret = devm_snd_soc_register_component(&hdev->dev, &hdmi_hda_codec,