Skip to contentSkip to conversation

ALSA: bebob: potential info leak in hwdep_read()

PATCH In review Dan Carpenter 3 participants 4 messages Oct 7, 2020

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ALSA: bebob: potential info leak in hwdep_read()

The "count" variable needs to be capped on every path so that we don't copy too much information to the user.

Fixes 618eabeae711 ("ALSA: bebob: Add hwdep interface")
---
 sound/firewire/bebob/bebob_hwdep.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
sound/firewire/bebob/bebob_hwdep.c +1 -2
index 45b740f44c45..c362eb38ab90 100644
@@ -36,12 +36,11 @@ hwdep_read(struct snd_hwdep *hwdep, char __user *buf,  long count,
 	}
 
 	memset(&event, 0, sizeof(event));
+	count = min_t(long, count, sizeof(event.lock_status));
 	if (bebob->dev_lock_changed) {
 		event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS;
 		event.lock_status.status = (bebob->dev_lock_count > 0);
 		bebob->dev_lock_changed = false;
-
-		count = min_t(long, count, sizeof(event.lock_status));
 	}
 
 	spin_unlock_irq(&bebob->lock);
Signature · 1 line
2.28.0
maintainer
#1 lore raw

Hi,

Thanks for the patch.

Quoted 29 lines · Dan Carpenter

The "count" variable needs to be capped on every path so that we don't copy too much information to the user.

Fixes 618eabeae711 ("ALSA: bebob: Add hwdep interface")
---
 sound/firewire/bebob/bebob_hwdep.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
sound/firewire/bebob/bebob_hwdep.c +1 -2
index 45b740f44c45..c362eb38ab90 100644
@@ -36,12 +36,11 @@ hwdep_read(struct snd_hwdep *hwdep, char __user *buf,  long count,
 	}
 
 	memset(&event, 0, sizeof(event));
+	count = min_t(long, count, sizeof(event.lock_status));
 	if (bebob->dev_lock_changed) {
 		event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS;
 		event.lock_status.status = (bebob->dev_lock_count > 0);
 		bebob->dev_lock_changed = false;
-
-		count = min_t(long, count, sizeof(event.lock_status));
 	}
 
 	spin_unlock_irq(&bebob->lock);
Signature · 1 line
2.28.0

Indeed, the bug can leak the contents of kernel memory into user space unintentionally for the size indicated by ALSA HwDep application...

I will check the other drivers in ALSA firewire stack later for safe.

Thanks

Takashi Sakamoto

maintainer Acked-by
#2 lore raw

Oops, I forgot to add my tag to the former message.

Quoted 44 lines · Takashi Sakamoto

Hi,

Thanks for the patch.

Quoted 29 lines · Dan Carpenter

The "count" variable needs to be capped on every path so that we don't copy too much information to the user.

Fixes 618eabeae711 ("ALSA: bebob: Add hwdep interface")
---
 sound/firewire/bebob/bebob_hwdep.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
sound/firewire/bebob/bebob_hwdep.c +1 -2
index 45b740f44c45..c362eb38ab90 100644
@@ -36,12 +36,11 @@ hwdep_read(struct snd_hwdep *hwdep, char __user *buf,  long count,
 	}
 
 	memset(&event, 0, sizeof(event));
+	count = min_t(long, count, sizeof(event.lock_status));
 	if (bebob->dev_lock_changed) {
 		event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS;
 		event.lock_status.status = (bebob->dev_lock_count > 0);
 		bebob->dev_lock_changed = false;
-
-		count = min_t(long, count, sizeof(event.lock_status));
 	}
 
 	spin_unlock_irq(&bebob->lock);
Signature · 1 line
2.28.0

Indeed, the bug can leak the contents of kernel memory into user space unintentionally for the size indicated by ALSA HwDep application...

I will check the other drivers in ALSA firewire stack later for safe.

Thanks

Takashi Sakamoto