linux-kernel - Re: [patch 06/15] clocksource: Split out user string input

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 29 Apr 2013 16:29:49 -0700
From:	John Stultz <john.stultz@...aro.org>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...e.hu>,
	Magnus Damm <magnus.damm@...il.com>
Subject: Re: [patch 06/15] clocksource: Split out user string input

On Thu, Apr 25, 2013 at 1:31 PM, Thomas Gleixner <tglx@...utronix.de> wrote:

> Split out the user string input for clocksource override. Preparatory
> patch for unbind.

This patch has a bug that causes the shell to hang when \n terminated 
clocksource strings are sent to the sysfs interface.

echo hpet > /sys/devices/system/clocksource/clocksource0/current_clocksource
<shell hang>


> +static size_t clocksource_get_uname(const char *buf, char *dst, size_t
> cnt)
> +{
> +       /* strings from sysfs write are not 0 terminated! */
> +       if (!cnt || cnt >= CS_NAME_LEN)
> +               return -EINVAL;
> +
> +       /* strip of \n: */
> +       if (buf[cnt-1] == '\n')
> +               cnt--;
> +       if (cnt > 0)
> +               memcpy(dst, buf, cnt);
> +       dst[cnt] = 0;
> +       return cnt;
> +}
> +
>  /**
>   * sysfs_override_clocksource - interface for manually overriding
> clocksource
>   * @dev:       unused
> @@ -847,22 +863,13 @@ static ssize_t sysfs_override_clocksourc
>                                           struct device_attribute *attr,
>                                           const char *buf, size_t count)
>  {
> -       size_t ret = count;
> -
> -       /* strings from sysfs write are not 0 terminated! */
> -       if (count >= sizeof(override_name))
> -               return -EINVAL;
> -
> -       /* strip of \n: */
> -       if (buf[count-1] == '\n')
> -               count--;
> +       size_t ret;
>
>         mutex_lock(&clocksource_mutex);
>
> -       if (count > 0)
> -               memcpy(override_name, buf, count);
> -       override_name[count] = 0;
> -       clocksource_select(false);
> +       ret = clocksource_get_uname(buf, override_name, count);
> +       if (ret >= 0)
> +               clocksource_select(false);
>
>         mutex_unlock(&clocksource_mutex);


The change above *looks* ok, but is subtlly different then what we had 
before. Note, in the old code, we saved count to ret, and never modified 
ret before returning it.

With the new code, we modify cnt, as we process the copied clocksource 
name, and return cnt, thus we don't return to the sysfs interface the 
same count value we were given, causing the write call to loop.

So you probably want something like the following patch to fix this.

thanks
-john



diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index e9c4f04..5f6c324 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -836,6 +836,8 @@ sysfs_show_current_clocksources(struct device *dev,

  static size_t clocksource_get_uname(const char *buf, char *dst, size_t 
cnt)
  {
+	size_t ret = cnt;
+
  	/* strings from sysfs write are not 0 terminated! */
  	if (!cnt || cnt >= CS_NAME_LEN)
  		return -EINVAL;
@@ -846,7 +848,7 @@ static size_t clocksource_get_uname(const char *buf, 
char *dst, size_t cnt)
  	if (cnt > 0)
  		memcpy(dst, buf, cnt);
  	dst[cnt] = 0;
-	return cnt;
+	return ret;
  }

  /**
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/