[PATCH v2 0/3] stm class/intel_th: Fixes for v4.20

[PATCH v2 0/3] stm class/intel_th: Fixes for v4.20

[Alexander Shishkin]

Hi Greg,

This is a second attepmpt of [1]. This time commit references are fixed.
A new tag is also pushed, just in case.

I have 3 fixes for STM Class and Intel TH. Two things that I introduced in
v4.20-rc1 and one long-standing bug since v4.4. As usual, there is a signed
tag ready for pulling and individual patches follow. Please consider pulling
or applying. Thanks!

The following changes since commit 651022382c7f8da46cb4872a545ee1da6d097d2a:

  Linux 4.20-rc1 (2018-11-04 15:37:52 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/ash/stm.git tags/intel_th-fixes-for-greg-20181219

for you to fetch changes up to 28b39d95ea6f6b4ad8f6516cab135e85c577ebea:

  intel_th: msu: Fix an off-by-one in attribute store (2018-12-19 13:43:53 +0200)

----------------------------------------------------------------
stm class/intel_th: Fixes for v4.20

These are:
 * module refcount leak in stm class, introduced in v4.20-rc1
 * documentation fix
 * string parsing off-by-one in intel_th, also for -stable

----------------------------------------------------------------
Alexander Shishkin (3):
      stm class: Fix a module refcount leak in policy creation error path
      stm class: Add a reference to the SyS-T document
      intel_th: msu: Fix an off-by-one in attribute store

 Documentation/trace/index.rst    |  1 +
 drivers/hwtracing/intel_th/msu.c |  3 ++-
 drivers/hwtracing/stm/policy.c   | 12 +++++++-----
 3 files changed, 10 insertions(+), 6 deletions(-)

[PATCH v2 1/3] stm class: Fix a module refcount leak in policy creation error path

[Alexander Shishkin]

Commit c7fd62bc69d0 ("stm class: Introduce framing protocol drivers")
adds a bug into the error path of policy creation, that would do a
module_put() on a wrong module, if one tried to create a policy for
an stm device which already has a policy, using a different protocol.
IOW,

| mkdir /config/stp-policy/dummy_stm.0:p_basic.test
| mkdir /config/stp-policy/dummy_stm.0:p_sys-t.test # puts "p_basic"
| mkdir /config/stp-policy/dummy_stm.0:p_sys-t.test # "p_basic" -> -1

throws:

| general protection fault: 0000 [#1] SMP PTI
| CPU: 3 PID: 2887 Comm: mkdir
| RIP: 0010:module_put.part.31+0xe/0x90
| Call Trace:
|  module_put+0x13/0x20
|  stm_put_protocol+0x11/0x20 [stm_core]
|  stp_policy_make+0xf1/0x210 [stm_core]
|  ? __kmalloc+0x183/0x220
|  ? configfs_mkdir+0x10d/0x4c0
|  configfs_mkdir+0x169/0x4c0
|  vfs_mkdir+0x108/0x1c0
|  do_mkdirat+0xe8/0x110
|  __x64_sys_mkdir+0x1b/0x20
|  do_syscall_64+0x5a/0x140
|  entry_SYSCALL_64_after_hwframe+0x44/0xa9

Correct this sad mistake by calling calling 'put' on the correct
reference, which happens to match another error path in the same
function, so we consolidate the two at the same time.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Fixes: c7fd62bc69d0 ("stm class: Introduce framing protocol drivers")
Reported-by: Ammy Yi <ammy.yi@intel.com>
---
 drivers/hwtracing/stm/policy.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 0910ec807187..4b9e44b227d8 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -440,10 +440,8 @@ stp_policy_make(struct config_group *group, const char *name)
 
 	stm->policy = kzalloc(sizeof(*stm->policy), GFP_KERNEL);
 	if (!stm->policy) {
-		mutex_unlock(&stm->policy_mutex);
-		stm_put_protocol(pdrv);
-		stm_put_device(stm);
-		return ERR_PTR(-ENOMEM);
+		ret = ERR_PTR(-ENOMEM);
+		goto unlock_policy;
 	}
 
 	config_group_init_type_name(&stm->policy->group, name,
@@ -458,7 +456,11 @@ stp_policy_make(struct config_group *group, const char *name)
 	mutex_unlock(&stm->policy_mutex);
 
 	if (IS_ERR(ret)) {
-		stm_put_protocol(stm->pdrv);
+		/*
+		 * pdrv and stm->pdrv at this point can be quite different,
+		 * and only one of them needs to be 'put'
+		 */
+		stm_put_protocol(pdrv);
 		stm_put_device(stm);
 	}
 
-- 
2.19.2

[PATCH v2 2/3] stm class: Add a reference to the SyS-T document

[Alexander Shishkin]

Commit 4cb3653df0cd ("stm class: Document the MIPI SyS-T protocol usage")
added a document describing the SyS-T protocol usage, but forgot to add
it to the directory index. Fix that.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Fixes: 4cb3653df0cd ("stm class: Document the MIPI SyS-T protocol usage")
---
 Documentation/trace/index.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/trace/index.rst b/Documentation/trace/index.rst
index 306997941ba1..6b4107cf4b98 100644
--- a/Documentation/trace/index.rst
+++ b/Documentation/trace/index.rst
@@ -22,3 +22,4 @@ Linux Tracing Technologies
    hwlat_detector
    intel_th
    stm
+   sys-t
-- 
2.19.2

[PATCH v2 3/3] intel_th: msu: Fix an off-by-one in attribute store

[Alexander Shishkin]

The 'nr_pages' attribute of the 'msc' subdevices parses a comma-separated
list of window sizes, passed from userspace. However, there is a bug in
the string parsing logic wherein it doesn't exclude the comma character
from the range of characters as it consumes them. This leads to an
out-of-bounds access given a sufficiently long list. For example:

> # echo 8,8,8,8 > /sys/bus/intel_th/devices/0-msc0/nr_pages
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in memchr+0x1e/0x40
> Read of size 1 at addr ffff8803ffcebcd1 by task sh/825
>
> CPU: 3 PID: 825 Comm: npktest.sh Tainted: G        W         4.20.0-rc1+
> Call Trace:
>  dump_stack+0x7c/0xc0
>  print_address_description+0x6c/0x23c
>  ? memchr+0x1e/0x40
>  kasan_report.cold.5+0x241/0x308
>  memchr+0x1e/0x40
>  nr_pages_store+0x203/0xd00 [intel_th_msu]

Fix this by accounting for the comma character.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Fixes: ba82664c134ef ("intel_th: Add Memory Storage Unit driver")
Cc: stable@vger.kernel.org # v4.4+
---
 drivers/hwtracing/intel_th/msu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index d293e55553bd..ba7aaf421f36 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -1423,7 +1423,8 @@ nr_pages_store(struct device *dev, struct device_attribute *attr,
 		if (!end)
 			break;
 
-		len -= end - p;
+		/* consume the number and the following comma, hence +1 */
+		len -= end - p + 1;
 		p = end + 1;
 	} while (len);
 
-- 
2.19.2

Re: [PATCH v2 3/3] intel_th: msu: Fix an off-by-one in attribute store

[Greg Kroah-Hartman]

On Wed, Dec 19, 2018 at 05:19:22PM +0200, Alexander Shishkin wrote:
> The 'nr_pages' attribute of the 'msc' subdevices parses a comma-separated
> list of window sizes, passed from userspace. However, there is a bug in
> the string parsing logic wherein it doesn't exclude the comma character
> from the range of characters as it consumes them. This leads to an
> out-of-bounds access given a sufficiently long list. For example:
> 
> > # echo 8,8,8,8 > /sys/bus/intel_th/devices/0-msc0/nr_pages
> > ==================================================================
> > BUG: KASAN: slab-out-of-bounds in memchr+0x1e/0x40
> > Read of size 1 at addr ffff8803ffcebcd1 by task sh/825
> >
> > CPU: 3 PID: 825 Comm: npktest.sh Tainted: G        W         4.20.0-rc1+
> > Call Trace:
> >  dump_stack+0x7c/0xc0
> >  print_address_description+0x6c/0x23c
> >  ? memchr+0x1e/0x40
> >  kasan_report.cold.5+0x241/0x308
> >  memchr+0x1e/0x40
> >  nr_pages_store+0x203/0xd00 [intel_th_msu]
> 
> Fix this by accounting for the comma character.

Ugh, this is one major reason sysfs files are "one value per file".

You should never have to "parse" a sysfs file for something like this.

It's a bit too late now, but please, never create such a sysfs file
again :(

greg k-h