Skip to contentSkip to conversation

pseries/papr-hvpipe: Fix and simplify papr-hvpipe

RFC In review Ritesh Harjani (IBM) 3 participants 9 messages Apr 7 – Apr 9 PowerPC

lore
Share & tools
lore

Ritesh Harjani (IBM)

author reviewer
#0 lore raw

cover pseries/papr-hvpipe: Fix and simplify papr-hvpipe

Haren reported a UAF / null ptr deref issue here [1]. While reviewing that and going over papr-hvpipe code, I found couple of more issues around the usage of copy_to_user() and few refactoring which simplifies the code.

This patch series is an attempt to that. Note that this is only compile tested on pseries for now.

Haren, I will kindly need your help in verifying this please. Let me know if we have a selftests or any other test framework for this, which I can utilize too.

[1]: https://lore.kernel.org/linuxppc-dev/[email protected]/

Ritesh Harjani (IBM) (6):
pseries/papr-hvpipe: Fix null ptr deref in papr_hvpipe_dev_create_handle pseries/papr-hvpipe: Fix the usage of copy_to_user() pseries/papr-hvpipe: Simplify spin_unlock() usage in papr_hvpipe_handle_release pseries/papr-hvpipe: Kill task_struct pointer from struct hvpipe_source_info pseries/papr-hvpipe: Refactor and simplify hvpipe_rtas_recv_msg() pseries/papr-hvpipe: Simplify error handling in papr_hvpipe_init()

 arch/powerpc/platforms/pseries/papr-hvpipe.c | 135 +++++++++----------
 arch/powerpc/platforms/pseries/papr-hvpipe.h |   1 -
 2 files changed, 66 insertions(+), 70 deletions(-)

--
2.39.5

author reviewer
#1 lore raw

RFC 1/6 pseries/papr-hvpipe: Fix null ptr deref in papr_hvpipe_dev_create_handle

Getting the following kernel panic in papr_hvpipe_dev_create_handle() when trying to add src_info to the list.
Kernel attempted to write user page (0) - exploit attempt? (uid: 0)

 BUG: Kernel NULL pointer dereference on write at 0x00000000
 Faulting instruction address: 0xc0000000001b44a0
 Oops: Kernel access of bad area, sig: 11 [#1]
 ...
 Call Trace:
 papr_hvpipe_dev_ioctl+0x1f4/0x48c (unreliable)
 sys_ioctl+0x528/0x1064
 system_call_exception+0x128/0x360
 system_call_vectored_common+0x15c/0x2ec

The error handling with FD_PREPARE's file cleanup and __free(kfree) auto cleanup is getting too convoluted. This is mainly because we need to ensure only 1 user get the srcID handle. To simplify this, we allocate prepare the src_info in the beginning and add it to the global list under a spinlock after checking that no duplicates exist.

This simplify the error handling where if the FD_ADD fails, we can simply remove the src_info from the list.

Cc Christian Brauner <[email protected]>
Fixes 6d3789d347a7 ("papr-hvpipe: convert papr_hvpipe_dev_create_handle() to FD_PREPARE()")
---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 50 +++++++++-----------
 1 file changed, 22 insertions(+), 28 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +22 -28
index 14ae480d060a..ef10f5a5a4fa 100644
@@ -479,21 +479,8 @@ static const struct file_operations papr_hvpipe_handle_ops = {
 
 static int papr_hvpipe_dev_create_handle(u32 srcID)
 {
-	struct hvpipe_source_info *src_info __free(kfree) = NULL;
-
-	spin_lock(&hvpipe_src_list_lock);
-	/*
-	 * Do not allow more than one process communicates with
-	 * each source.
-	 */
-	src_info = hvpipe_find_source(srcID);
-	if (src_info) {
-		spin_unlock(&hvpipe_src_list_lock);
-		pr_err("pid(%d) is already using the source(%d)\n",
-				src_info->tsk->pid, srcID);
-		return -EALREADY;
-	}
-	spin_unlock(&hvpipe_src_list_lock);
+	struct hvpipe_source_info *src_info;
+	int fd;
 
 	src_info = kzalloc_obj(*src_info, GFP_KERNEL_ACCOUNT);
 	if (!src_info)
@@ -503,26 +490,33 @@ static int papr_hvpipe_dev_create_handle(u32 srcID)
 	src_info->tsk = current;
 	init_waitqueue_head(&src_info->recv_wqh);
 
-	FD_PREPARE(fdf, O_RDONLY | O_CLOEXEC,
-		   anon_inode_getfile("[papr-hvpipe]", &papr_hvpipe_handle_ops,
-				      (void *)src_info, O_RDWR));
-	if (fdf.err)
-		return fdf.err;
-
-	retain_and_null_ptr(src_info);
-	spin_lock(&hvpipe_src_list_lock);
 	/*
-	 * If two processes are executing ioctl() for the same
-	 * source ID concurrently, prevent the second process to
-	 * acquire FD.
+	 * Do not allow more than one process communicates with
+	 * each source.
 	 */
-	if (hvpipe_find_source(srcID)) {
+	spin_lock(&hvpipe_src_list_lock);
+	if(hvpipe_find_source(srcID)) {
 		spin_unlock(&hvpipe_src_list_lock);
+		pr_err("pid(%d) could not get the source(%d)\n",
+				src_info->tsk->pid, srcID);
+		kfree(src_info);
 		return -EALREADY;
 	}
 	list_add(&src_info->list, &hvpipe_src_list);
 	spin_unlock(&hvpipe_src_list_lock);
-	return fd_publish(fdf);
+
+	fd = FD_ADD(O_RDONLY | O_CLOEXEC,
+		   anon_inode_getfile("[papr-hvpipe]", &papr_hvpipe_handle_ops,
+				      (void *)src_info, O_RDWR));
+	if (fd < 0) {
+		spin_lock(&hvpipe_src_list_lock);
+		list_del(&src_info->list);
+		spin_unlock(&hvpipe_src_list_lock);
+		kfree(src_info);
+		return fd;
+	}
+
+	return fd;
 }
 
 /*

--
2.39.5

Reviewed-by on v1 1/6
#2 lore raw

Re: [RFC v1 1/6] pseries/papr-hvpipe: Fix null ptr deref in papr_hvpipe_dev_create_handle

Quoted 19 lines · Ritesh Harjani (IBM)

Getting the following kernel panic in papr_hvpipe_dev_create_handle() when trying to add src_info to the list.
Kernel attempted to write user page (0) - exploit attempt? (uid: 0)

 BUG: Kernel NULL pointer dereference on write at 0x00000000
 Faulting instruction address: 0xc0000000001b44a0
 Oops: Kernel access of bad area, sig: 11 [#1]
 ...
 Call Trace:
 papr_hvpipe_dev_ioctl+0x1f4/0x48c (unreliable)
 sys_ioctl+0x528/0x1064
 system_call_exception+0x128/0x360
 system_call_vectored_common+0x15c/0x2ec

The error handling with FD_PREPARE's file cleanup and __free(kfree) auto cleanup is getting too convoluted. This is mainly because we need to ensure only 1 user get the srcID handle. To simplify this, we allocate prepare the src_info in the beginning and add it to the global list under a spinlock after checking that no duplicates exist.

Quoted 8 lines

This simplify the error handling where if the FD_ADD fails, we can simply remove the src_info from the list.

Cc Christian Brauner <[email protected]>
Fixes 6d3789d347a7 ("papr-hvpipe: convert papr_hvpipe_dev_create_handle() to FD_PREPARE()")

---

Thanks for fixing this!

author reviewer
#3 lore raw

RFC 2/6 pseries/papr-hvpipe: Fix the usage of copy_to_user()

In reply to Ritesh Harjani (IBM)

copy_to_user() return bytes_not_copied to the user buffer. If there was an error writing bytes into the user buffer, i.e. if copy_to_user returns a non-zero value, then we simply return -EFAULT from the ->read() call.

---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +5 -4
index ef10f5a5a4fa..fffebbae113c 100644
@@ -206,10 +206,11 @@ static int hvpipe_rtas_recv_msg(char __user *buf, int size)
 					bytes_written, size);
 				bytes_written = size;
 			}
-			ret = copy_to_user(buf,
+			if (copy_to_user(buf,
 					rtas_work_area_raw_buf(work_area),
-					bytes_written);
-			if (!ret)
+					bytes_written))
+				ret = -EFAULT;
+			else
 				ret = bytes_written;
 		}
 	} else {
@@ -376,7 +377,7 @@ static ssize_t papr_hvpipe_handle_read(struct file *file,
 
 	ret = copy_to_user(buf, &hdr, HVPIPE_HDR_LEN);
 	if (ret)
-		return ret;
+		return -EFAULT;
 
 	/*
 	 * Message event has payload, so get the payload with
Signature · 1 line
2.39.5
author reviewer
#4 lore raw

RFC 3/6 pseries/papr-hvpipe: Simplify spin_unlock() usage in papr_hvpipe_handle_release

In reply to Ritesh Harjani (IBM)

Once the src_info is removed from the global list, no one can access it. This simplies the usage of spin_unlock() in papr_hvpipe_handle_release()

---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +2 -3
index fffebbae113c..57ec50e3e883 100644
@@ -455,6 +455,7 @@ static int papr_hvpipe_handle_release(struct inode *inode,
 	src_info = file->private_data;
 	list_del(&src_info->list);
 	file->private_data = NULL;
+	spin_unlock(&hvpipe_src_list_lock);
 	/*
 	 * If the pipe for this specific source has any pending
 	 * payload, issue recv HVPIPE RTAS so that pipe will not
@@ -462,10 +463,8 @@ static int papr_hvpipe_handle_release(struct inode *inode,
 	 */
 	if (src_info->hvpipe_status & HVPIPE_MSG_AVAILABLE) {
 		src_info->hvpipe_status = 0;
-		spin_unlock(&hvpipe_src_list_lock);
 		hvpipe_rtas_recv_msg(NULL, 0);
-	} else
-		spin_unlock(&hvpipe_src_list_lock);
+	}
 
 	kfree(src_info);
 	return 0;
Signature · 1 line
2.39.5
author reviewer
#5 lore raw

RFC 4/6 pseries/papr-hvpipe: Kill task_struct pointer from struct hvpipe_source_info

In reply to Ritesh Harjani (IBM)

We don't really use task_struct pointer for anything meaningful. So just kill it for now, and we can bring back later if we need this for any future debug purposes.

---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 5 ++---
 arch/powerpc/platforms/pseries/papr-hvpipe.h | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +2 -3
index 57ec50e3e883..7242a14eb5ec 100644
@@ -487,7 +487,6 @@ static int papr_hvpipe_dev_create_handle(u32 srcID)
 		return -ENOMEM;
 
 	src_info->srcID = srcID;
-	src_info->tsk = current;
 	init_waitqueue_head(&src_info->recv_wqh);
 
 	/*
@@ -497,8 +496,8 @@ static int papr_hvpipe_dev_create_handle(u32 srcID)
 	spin_lock(&hvpipe_src_list_lock);
 	if(hvpipe_find_source(srcID)) {
 		spin_unlock(&hvpipe_src_list_lock);
-		pr_err("pid(%d) could not get the source(%d)\n",
-				src_info->tsk->pid, srcID);
+		pr_err("pid(%s:%d) could not get the source(%d)\n",
+				current->comm, task_pid_nr(current), srcID);
 		kfree(src_info);
 		return -EALREADY;
 	}
arch/powerpc/platforms/pseries/papr-hvpipe.h +0 -1
index c343f4230865..4bdf7bb2fc4d 100644
@@ -21,7 +21,6 @@ struct hvpipe_source_info {
 	u32 srcID;
 	u32 hvpipe_status;
 	wait_queue_head_t recv_wqh;	 /* wake up poll() waitq */
-	struct task_struct *tsk;
 };
 
 /*
Signature · 1 line
2.39.5
author reviewer
#6 lore raw

RFC 5/6 pseries/papr-hvpipe: Refactor and simplify hvpipe_rtas_recv_msg()

In reply to Ritesh Harjani (IBM)

Simplify hvpipe_rtas_recv_msg() by removing three levels of nesting... if (!ret)
if (buf)
if (size < bytes_written)
... this refactoring of the funcion ensures bail out routines are handled early, to simplify the function logic.

---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 52 ++++++++++----------
 1 file changed, 26 insertions(+), 26 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +26 -26
index 7242a14eb5ec..f41f5c0a8418 100644
@@ -190,34 +190,34 @@ static int hvpipe_rtas_recv_msg(char __user *buf, int size)
 		return -ENOMEM;
 	}
 
-	ret = rtas_ibm_receive_hvpipe_msg(work_area, &srcID,
-					&bytes_written);
-	if (!ret) {
-		/*
-		 * Recv HVPIPE RTAS is successful.
-		 * When releasing FD or no one is waiting on the
-		 * specific source, issue recv HVPIPE RTAS call
-		 * so that pipe is not blocked - this func is called
-		 * with NULL buf.
-		 */
-		if (buf) {
-			if (size < bytes_written) {
-				pr_err("Received the payload size = %d, but the buffer size = %d\n",
-					bytes_written, size);
-				bytes_written = size;
-			}
-			if (copy_to_user(buf,
-					rtas_work_area_raw_buf(work_area),
-					bytes_written))
-				ret = -EFAULT;
-			else
-				ret = bytes_written;
-		}
-	} else {
-		pr_err("ibm,receive-hvpipe-msg failed with %d\n",
-				ret);
+	/*
+	 * Recv HVPIPE RTAS is successful.
+	 * When releasing FD or no one is waiting on the
+	 * specific source, issue recv HVPIPE RTAS call
+	 * so that pipe is not blocked - this func is called
+	 * with NULL buf.
+	 */
+	ret = rtas_ibm_receive_hvpipe_msg(work_area, &srcID, &bytes_written);
+	if (ret) {
+		pr_err("ibm,receive-hvpipe-msg failed with %d\n", ret);
+		goto out;
 	}
 
+	if (!buf)
+		goto out;
+
+	if (size < bytes_written) {
+		pr_err("Received the payload size = %d, but the buffer size = %d\n",
+				bytes_written, size);
+		bytes_written = size;
+	}
+
+	if (copy_to_user(buf, rtas_work_area_raw_buf(work_area), bytes_written))
+		ret = -EFAULT;
+	else
+		ret = bytes_written;
+
+out:
 	rtas_work_area_free(work_area);
 	return ret;
 }
Signature · 1 line
2.39.5
author reviewer
#7 lore raw

RFC 6/6 pseries/papr-hvpipe: Simplify error handling in papr_hvpipe_init()

In reply to Ritesh Harjani (IBM)

Remove such 3 levels of nesting patterns to check success return values from function calls.

ret = enable_hvpipe_IRQ()
if (!ret)
ret = set_hvpipe_sys_param(1)
if (!ret)
ret = misc_register()

Instead just bail out to "out*:" labels, in case of any error. This simplifies the init flow.

---
 arch/powerpc/platforms/pseries/papr-hvpipe.c | 26 +++++++++++---------
 1 file changed, 15 insertions(+), 11 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +15 -11
index f41f5c0a8418..03ec004a6701 100644
@@ -768,23 +768,27 @@ static int __init papr_hvpipe_init(void)
 	}
 
 	ret = enable_hvpipe_IRQ();
-	if (!ret) {
-		ret = set_hvpipe_sys_param(1);
-		if (!ret)
-			ret = misc_register(&papr_hvpipe_dev);
-	}
+	if (ret)
+		goto out_wq;
 
-	if (!ret) {
-		pr_info("hvpipe feature is enabled\n");
-		hvpipe_feature = true;
-		return 0;
-	}
+	ret = set_hvpipe_sys_param(1);
+	if (ret)
+		goto out_wq;
 
-	pr_err("hvpipe feature is not enabled %d\n", ret);
+	ret = misc_register(&papr_hvpipe_dev);
+	if (ret)
+		goto out_wq;
+
+	pr_info("hvpipe feature is enabled\n");
+	hvpipe_feature = true;
+	return 0;
+
+out_wq:
 	destroy_workqueue(papr_hvpipe_wq);
 out:
 	kfree(papr_hvpipe_work);
 	papr_hvpipe_work = NULL;
+	pr_err("hvpipe feature is not enabled %d\n", ret);
 	return ret;
 }
 machine_device_initcall(pseries, papr_hvpipe_init);

--
2.39.5

#8 lore raw
In reply to Ritesh Harjani (IBM)
Quoted 6 lines · Ritesh Harjani (IBM)

Haren reported a UAF / null ptr deref issue here [1]. While reviewing that and
going over papr-hvpipe code, I found couple of more issues around the usage of
copy_to_user() and few refactoring which simplifies the code.

Quoted 8 lines

This patch series is an attempt to that. Note that this is only compile tested
on pseries for now.

Haren, I will kindly need your help in verifying this please. Let me know if we
have a selftests or any other test framework for this, which I can utilize too.

Thanks for fixes and cleanup the code. The testing should be part of HMC/RCST framework. So we do not have selftests. I will work with HMC to verify these patches.

Quoted 23 lines

[1]:
https://lore.kernel.org/linuxppc-dev/[email protected]/

Ritesh Harjani (IBM) (6):
  pseries/papr-hvpipe: Fix null ptr deref in papr_hvpipe_dev_create_handle
  pseries/papr-hvpipe: Fix the usage of copy_to_user() pseries/papr-hvpipe: Simplify spin_unlock() usage in papr_hvpipe_handle_release
  pseries/papr-hvpipe: Kill task_struct pointer from struct hvpipe_source_info
  pseries/papr-hvpipe: Refactor and simplify hvpipe_rtas_recv_msg() pseries/papr-hvpipe: Simplify error handling in papr_hvpipe_init()

 arch/powerpc/platforms/pseries/papr-hvpipe.c | 135 +++++++++-------- --
 arch/powerpc/platforms/pseries/papr-hvpipe.h |   1 -

 2 files changed, 66 insertions(+), 70 deletions(-)

--
2.39.5