| Message ID | 20170602161523.14052-1-fbarrat@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Fred, Good catch. Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes: > Fix error path if we can't copy user structure on > CXL_IOCTL_START_WORK ioctl. > > Signed-off-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com> > Cc: stable@vger.kernel.org > --- > drivers/misc/cxl/file.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/misc/cxl/file.c b/drivers/misc/cxl/file.c > index 17b433f1ce23..caa44adfa60e 100644 > --- a/drivers/misc/cxl/file.c > +++ b/drivers/misc/cxl/file.c > @@ -160,10 +160,8 @@ static long afu_ioctl_start_work(struct cxl_context *ctx, > /* Do this outside the status_mutex to avoid a circular dependency with > * the locking in cxl_mmap_fault() */ > if (copy_from_user(&work, uwork, > - sizeof(struct cxl_ioctl_start_work))) { > - rc = -EFAULT; > - goto out; > - } > + sizeof(struct cxl_ioctl_start_work))) Bike-shedding a bit, but s/sizeof(struct cxl_ioctl_start_work)))/sizeof(work)/ would look much cleaner Reviewed-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
On 03/06/17 02:15, Frederic Barrat wrote: > Fix error path if we can't copy user structure on > CXL_IOCTL_START_WORK ioctl. > > Signed-off-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com> > Cc: stable@vger.kernel.org Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes: > Fix error path if we can't copy user structure on > CXL_IOCTL_START_WORK ioctl. To be clear the error is that returning via the out label will unlock cxl->status_mutex, which has not been locked. Please spell it out for me :) This should be: Fixes: 0712dc7e73e5 ("cxl: Fix issues when unmapping contexts") Am I right? cheers > diff --git a/drivers/misc/cxl/file.c b/drivers/misc/cxl/file.c > index 17b433f1ce23..caa44adfa60e 100644 > --- a/drivers/misc/cxl/file.c > +++ b/drivers/misc/cxl/file.c > @@ -160,10 +160,8 @@ static long afu_ioctl_start_work(struct cxl_context *ctx, > /* Do this outside the status_mutex to avoid a circular dependency with > * the locking in cxl_mmap_fault() */ > if (copy_from_user(&work, uwork, > - sizeof(struct cxl_ioctl_start_work))) { > - rc = -EFAULT; > - goto out; > - } > + sizeof(struct cxl_ioctl_start_work))) > + return -EFAULT; > > mutex_lock(&ctx->status_mutex); > if (ctx->status != OPENED) { > -- > 2.11.0
Le 06/06/2017 à 11:20, Michael Ellerman a écrit : > Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes: > >> Fix error path if we can't copy user structure on >> CXL_IOCTL_START_WORK ioctl. > > To be clear the error is that returning via the out label will unlock > cxl->status_mutex, which has not been locked. > > Please spell it out for me :) > > This should be: > > Fixes: 0712dc7e73e5 ("cxl: Fix issues when unmapping contexts") > > Am I right? That's correct. I'm about to send a v2 to address Vaibhav's comment and I'll fix the above as well. Thanks, Fred > cheers > >> diff --git a/drivers/misc/cxl/file.c b/drivers/misc/cxl/file.c >> index 17b433f1ce23..caa44adfa60e 100644 >> --- a/drivers/misc/cxl/file.c >> +++ b/drivers/misc/cxl/file.c >> @@ -160,10 +160,8 @@ static long afu_ioctl_start_work(struct cxl_context *ctx, >> /* Do this outside the status_mutex to avoid a circular dependency with >> * the locking in cxl_mmap_fault() */ >> if (copy_from_user(&work, uwork, >> - sizeof(struct cxl_ioctl_start_work))) { >> - rc = -EFAULT; >> - goto out; >> - } >> + sizeof(struct cxl_ioctl_start_work))) >> + return -EFAULT; >> >> mutex_lock(&ctx->status_mutex); >> if (ctx->status != OPENED) { >> -- >> 2.11.0 >
Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes: > Le 06/06/2017 à 11:20, Michael Ellerman a écrit : >> Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes: >> >>> Fix error path if we can't copy user structure on >>> CXL_IOCTL_START_WORK ioctl. >> >> To be clear the error is that returning via the out label will unlock >> cxl->status_mutex, which has not been locked. >> >> Please spell it out for me :) >> >> This should be: >> >> Fixes: 0712dc7e73e5 ("cxl: Fix issues when unmapping contexts") >> >> Am I right? > > That's correct. I'm about to send a v2 to address Vaibhav's comment and > I'll fix the above as well. Thanks. cheers
diff --git a/drivers/misc/cxl/file.c b/drivers/misc/cxl/file.c index 17b433f1ce23..caa44adfa60e 100644 --- a/drivers/misc/cxl/file.c +++ b/drivers/misc/cxl/file.c @@ -160,10 +160,8 @@ static long afu_ioctl_start_work(struct cxl_context *ctx, /* Do this outside the status_mutex to avoid a circular dependency with * the locking in cxl_mmap_fault() */ if (copy_from_user(&work, uwork, - sizeof(struct cxl_ioctl_start_work))) { - rc = -EFAULT; - goto out; - } + sizeof(struct cxl_ioctl_start_work))) + return -EFAULT; mutex_lock(&ctx->status_mutex); if (ctx->status != OPENED) {
Fix error path if we can't copy user structure on CXL_IOCTL_START_WORK ioctl. Signed-off-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com> Cc: stable@vger.kernel.org --- drivers/misc/cxl/file.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)