pseries/papr-hvpipe: Fix and simplify papr-hvpipe
Ritesh Harjani (IBM)
reviewercover 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
Ritesh Harjani (IBM)
reviewerRFC 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.
--- arch/powerpc/platforms/pseries/papr-hvpipe.c | 50 +++++++++----------- 1 file changed, 22 insertions(+), 28 deletions(-)
arch/powerpc/platforms/pseries/papr-hvpipe.c +22 -28
@@ -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
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 addsrc_infoto 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/0x2ecThe 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 thesrc_infoin 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_ADDfails, we can simply remove thesrc_infofrom the list.Cc Christian Brauner <[email protected]>Fixes 6d3789d347a7 ("papr-hvpipe: convert papr_hvpipe_dev_create_handle() to FD_PREPARE()")Reported-by Haren Myneni <[email protected]>Signed-off-by Ritesh Harjani (IBM) <[email protected]>---
Thanks for fixing this!
Ritesh Harjani (IBM)
reviewerRFC 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
@@ -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
Ritesh Harjani (IBM)
reviewerRFC 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
@@ -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
Ritesh Harjani (IBM)
reviewerRFC 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
@@ -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
@@ -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
Ritesh Harjani (IBM)
reviewerRFC 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
@@ -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
Ritesh Harjani (IBM)
reviewerRFC 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
@@ -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
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 ofcopy_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 inpapr_hvpipe_dev_create_handle
pseries/papr-hvpipe: Fix the usage ofcopy_to_user()pseries/papr-hvpipe: Simplifyspin_unlock()usage inpapr_hvpipe_handle_release
pseries/papr-hvpipe: Killtask_structpointer from structhvpipe_source_info
pseries/papr-hvpipe: Refactor and simplifyhvpipe_rtas_recv_msg()pseries/papr-hvpipe: Simplify error handling inpapr_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