[PATCH] [media] davinci/dm644x: work around ccdc_update_raw_params trainwreck

[PATCH] [media] davinci/dm644x: work around ccdc_update_raw_params trainwreck

[Arnd Bergmann]

Now that the davinci drivers can be enabled in compile tests on other
architectures, I ran into this warning on a 64-bit build:

drivers/media/platform/davinci/dm644x_ccdc.c: In function 'ccdc_update_raw_params':
drivers/media/platform/davinci/dm644x_ccdc.c:279:7: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]

While that looks fairly harmless (it would be fine on 32-bit), it was
just the tip of the iceberg:

- The function constantly mixes up pointers and phys_addr_t numbers
- This is part of a 'VPFE_CMD_S_CCDC_RAW_PARAMS' ioctl command that is
  described as an 'experimental ioctl that will change in future kernels',
  but if we have users that probably won't happen.
- The code to allocate the table never gets called after we copy_from_user
  the user input over the kernel settings, and then compare them
  for inequality.
- We then go on to use an address provided by user space as both the
  __user pointer for input and pass it through phys_to_virt to come up
  with a kernel pointer to copy the data to. This looks like a trivially
  exploitable root hole.

This patch disables all the obviously broken code, by zeroing out the
sensitive data provided by user space. I also fix the type confusion
here. If we think the ioctl has no stable users, we could consider
just removing it instead.

Fixes: 5f15fbb68fd7 ("V4L/DVB (12251): v4l: dm644x ccdc module for vpfe capture driver")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/platform/davinci/dm644x_ccdc.c | 40 +++++++++++++++++-----------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/davinci/dm644x_ccdc.c b/drivers/media/platform/davinci/dm644x_ccdc.c
index 740fbc7a8c14..1b42f50cad38 100644
--- a/drivers/media/platform/davinci/dm644x_ccdc.c
+++ b/drivers/media/platform/davinci/dm644x_ccdc.c
@@ -236,10 +236,22 @@ static int ccdc_update_raw_params(struct ccdc_config_params_raw *raw_params)
 {
 	struct ccdc_config_params_raw *config_params =
 				&ccdc_cfg.bayer.config_params;
-	unsigned int *fpc_virtaddr = NULL;
-	unsigned int *fpc_physaddr = NULL;
+	unsigned int *fpc_virtaddr;
+	phys_addr_t fpc_physaddr;
 
 	memcpy(config_params, raw_params, sizeof(*raw_params));
+
+	/*
+	 * FIXME: the code to copy the fault_pxl settings was present
+	 *	  in the original version but clearly could never
+	 *	  work and will interpret user-provided data in
+	 * 	  dangerous ways. Let's disable it completely to be
+	 *        on the safe side.
+	 */
+	config_params->fault_pxl.enable = 0;
+	config_params->fault_pxl.fp_num = 0;
+	config_params->fault_pxl.fpc_table_addr = 0;
+
 	/*
 	 * allocate memory for fault pixel table and copy the user
 	 * values to the table
@@ -247,16 +259,15 @@ static int ccdc_update_raw_params(struct ccdc_config_params_raw *raw_params)
 	if (!config_params->fault_pxl.enable)
 		return 0;
 
-	fpc_physaddr = (unsigned int *)config_params->fault_pxl.fpc_table_addr;
-	fpc_virtaddr = (unsigned int *)phys_to_virt(
-				(unsigned long)fpc_physaddr);
+	fpc_physaddr = config_params->fault_pxl.fpc_table_addr;
+	fpc_virtaddr = (unsigned int *)phys_to_virt(fpc_physaddr);
 	/*
 	 * Allocate memory for FPC table if current
 	 * FPC table buffer is not big enough to
 	 * accommodate FPC Number requested
 	 */
 	if (raw_params->fault_pxl.fp_num != config_params->fault_pxl.fp_num) {
-		if (fpc_physaddr != NULL) {
+		if (fpc_physaddr) {
 			free_pages((unsigned long)fpc_virtaddr,
 				   get_order
 				   (config_params->fault_pxl.fp_num *
@@ -270,13 +281,12 @@ static int ccdc_update_raw_params(struct ccdc_config_params_raw *raw_params)
 							 fault_pxl.fp_num *
 							 FP_NUM_BYTES));
 
-		if (fpc_virtaddr == NULL) {
+		if (fpc_virtaddr) {
 			dev_dbg(ccdc_cfg.dev,
 				"\nUnable to allocate memory for FPC");
 			return -EFAULT;
 		}
-		fpc_physaddr =
-		    (unsigned int *)virt_to_phys((void *)fpc_virtaddr);
+		fpc_physaddr = virt_to_phys(fpc_virtaddr);
 	}
 
 	/* Copy number of fault pixels and FPC table */
@@ -287,7 +297,7 @@ static int ccdc_update_raw_params(struct ccdc_config_params_raw *raw_params)
 		dev_dbg(ccdc_cfg.dev, "\n copy_from_user failed");
 		return -EFAULT;
 	}
-	config_params->fault_pxl.fpc_table_addr = (unsigned long)fpc_physaddr;
+	config_params->fault_pxl.fpc_table_addr = fpc_physaddr;
 	return 0;
 }
 
@@ -295,13 +305,13 @@ static int ccdc_close(struct device *dev)
 {
 	struct ccdc_config_params_raw *config_params =
 				&ccdc_cfg.bayer.config_params;
-	unsigned int *fpc_physaddr = NULL, *fpc_virtaddr = NULL;
+	phys_addr_t fpc_physaddr;
+	unsigned int *fpc_virtaddr;
 
-	fpc_physaddr = (unsigned int *)config_params->fault_pxl.fpc_table_addr;
+	fpc_physaddr = config_params->fault_pxl.fpc_table_addr;
 
-	if (fpc_physaddr != NULL) {
-		fpc_virtaddr = (unsigned int *)
-		    phys_to_virt((unsigned long)fpc_physaddr);
+	if (fpc_physaddr) {
+		fpc_virtaddr = phys_to_virt(fpc_physaddr);
 		free_pages((unsigned long)fpc_virtaddr,
 			   get_order(config_params->fault_pxl.fp_num *
 			   FP_NUM_BYTES));
-- 
2.9.0

Re: [PATCH] [media] davinci/dm644x: work around ccdc_update_raw_params trainwreck

[Lad, Prabhakar]

Hi Arnd,

Thanks for the patch.

On Fri, Jun 9, 2017 at 10:36 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> Now that the davinci drivers can be enabled in compile tests on other
> architectures, I ran into this warning on a 64-bit build:
>
> drivers/media/platform/davinci/dm644x_ccdc.c: In function 'ccdc_update_raw_params':
> drivers/media/platform/davinci/dm644x_ccdc.c:279:7: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>
> While that looks fairly harmless (it would be fine on 32-bit), it was
> just the tip of the iceberg:
>
> - The function constantly mixes up pointers and phys_addr_t numbers
> - This is part of a 'VPFE_CMD_S_CCDC_RAW_PARAMS' ioctl command that is
>   described as an 'experimental ioctl that will change in future kernels',
>   but if we have users that probably won't happen.
> - The code to allocate the table never gets called after we copy_from_user
>   the user input over the kernel settings, and then compare them
>   for inequality.
> - We then go on to use an address provided by user space as both the
>   __user pointer for input and pass it through phys_to_virt to come up
>   with a kernel pointer to copy the data to. This looks like a trivially
>   exploitable root hole.
>
> This patch disables all the obviously broken code, by zeroing out the
> sensitive data provided by user space. I also fix the type confusion
> here. If we think the ioctl has no stable users, we could consider
> just removing it instead.
>
I suspect there shouldn’t  be possible users of this IOCTL, better of  removing
the IOCTL itself.

Sekhar your call, as the latest PSP releases for 644x use the media
controller framework.

Cheers,
--Prabhakar Lad

Re: [PATCH] [media] davinci/dm644x: work around ccdc_update_raw_params trainwreck

[Sekhar Nori]

On Tuesday 20 June 2017 06:36 PM, Lad, Prabhakar wrote:
> Hi Arnd,
> 
> Thanks for the patch.
> 
> On Fri, Jun 9, 2017 at 10:36 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> Now that the davinci drivers can be enabled in compile tests on other
>> architectures, I ran into this warning on a 64-bit build:
>>
>> drivers/media/platform/davinci/dm644x_ccdc.c: In function 'ccdc_update_raw_params':
>> drivers/media/platform/davinci/dm644x_ccdc.c:279:7: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>
>> While that looks fairly harmless (it would be fine on 32-bit), it was
>> just the tip of the iceberg:
>>
>> - The function constantly mixes up pointers and phys_addr_t numbers
>> - This is part of a 'VPFE_CMD_S_CCDC_RAW_PARAMS' ioctl command that is
>>   described as an 'experimental ioctl that will change in future kernels',
>>   but if we have users that probably won't happen.
>> - The code to allocate the table never gets called after we copy_from_user
>>   the user input over the kernel settings, and then compare them
>>   for inequality.
>> - We then go on to use an address provided by user space as both the
>>   __user pointer for input and pass it through phys_to_virt to come up
>>   with a kernel pointer to copy the data to. This looks like a trivially
>>   exploitable root hole.
>>
>> This patch disables all the obviously broken code, by zeroing out the
>> sensitive data provided by user space. I also fix the type confusion
>> here. If we think the ioctl has no stable users, we could consider
>> just removing it instead.
>>
> I suspect there shouldn’t  be possible users of this IOCTL, better of  removing
> the IOCTL itself.
> 
> Sekhar your call, as the latest PSP releases for 644x use the media
> controller framework.

I do not have any personal experience with anyone using this support
with latest kernels. I too am okay with removing the broken support.

Since the header file that defines the ioctl is not in include/uapi/*, I
guess it cannot be considered stable userspace ABI? Also, there are
enough warnings about instability thrown in the comments surrounding the
ioctl in include/media/davinci/vpfe_capture.h.

Thanks,
Sekhar

Re: [PATCH] [media] davinci/dm644x: work around ccdc_update_raw_params trainwreck

[Arnd Bergmann]

On Tue, Jun 27, 2017 at 12:13 PM, Sekhar Nori <nsekhar@ti.com> wrote:
> On Tuesday 20 June 2017 06:36 PM, Lad, Prabhakar wrote:
>> Hi Arnd,
>>
>> Thanks for the patch.
>>
>> On Fri, Jun 9, 2017 at 10:36 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> Now that the davinci drivers can be enabled in compile tests on other
>>> architectures, I ran into this warning on a 64-bit build:
>>>
>>> drivers/media/platform/davinci/dm644x_ccdc.c: In function 'ccdc_update_raw_params':
>>> drivers/media/platform/davinci/dm644x_ccdc.c:279:7: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>
>>> While that looks fairly harmless (it would be fine on 32-bit), it was
>>> just the tip of the iceberg:
>>>
>>> - The function constantly mixes up pointers and phys_addr_t numbers
>>> - This is part of a 'VPFE_CMD_S_CCDC_RAW_PARAMS' ioctl command that is
>>>   described as an 'experimental ioctl that will change in future kernels',
>>>   but if we have users that probably won't happen.
>>> - The code to allocate the table never gets called after we copy_from_user
>>>   the user input over the kernel settings, and then compare them
>>>   for inequality.
>>> - We then go on to use an address provided by user space as both the
>>>   __user pointer for input and pass it through phys_to_virt to come up
>>>   with a kernel pointer to copy the data to. This looks like a trivially
>>>   exploitable root hole.
>>>
>>> This patch disables all the obviously broken code, by zeroing out the
>>> sensitive data provided by user space. I also fix the type confusion
>>> here. If we think the ioctl has no stable users, we could consider
>>> just removing it instead.
>>>
>> I suspect there shouldn’t  be possible users of this IOCTL, better of  removing
>> the IOCTL itself.
>>
>> Sekhar your call, as the latest PSP releases for 644x use the media
>> controller framework.
>
> I do not have any personal experience with anyone using this support
> with latest kernels. I too am okay with removing the broken support.

Ok, I think that would be good. Can one of you create that patch?
Note that we have two implementations of the ioctl, with different
data structures, depending on the specific hardware.

> Since the header file that defines the ioctl is not in include/uapi/*, I
> guess it cannot be considered stable userspace ABI? Also, there are
> enough warnings about instability thrown in the comments surrounding the
> ioctl in include/media/davinci/vpfe_capture.h.

This is not relevant really. The only thing that counts is whether there
is existing user space that has active users who complain if it breaks.

If you think nobody is using it, that is more important than code
comments or the location of the header file, but if someone complains
later anyway, we may end up reverting the removal and fix it differently.

       Arnd

Re: [PATCH] [media] davinci/dm644x: work around ccdc_update_raw_params trainwreck

[Lad, Prabhakar]

Hi Arnd,

On Tue, Jun 27, 2017 at 12:08 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Jun 27, 2017 at 12:13 PM, Sekhar Nori <nsekhar@ti.com> wrote:
>> On Tuesday 20 June 2017 06:36 PM, Lad, Prabhakar wrote:
>>> Hi Arnd,
>>>
>>> Thanks for the patch.
>>>
>>> On Fri, Jun 9, 2017 at 10:36 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> Now that the davinci drivers can be enabled in compile tests on other
>>>> architectures, I ran into this warning on a 64-bit build:
>>>>
>>>> drivers/media/platform/davinci/dm644x_ccdc.c: In function 'ccdc_update_raw_params':
>>>> drivers/media/platform/davinci/dm644x_ccdc.c:279:7: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>>>>
>>>> While that looks fairly harmless (it would be fine on 32-bit), it was
>>>> just the tip of the iceberg:
>>>>
>>>> - The function constantly mixes up pointers and phys_addr_t numbers
>>>> - This is part of a 'VPFE_CMD_S_CCDC_RAW_PARAMS' ioctl command that is
>>>>   described as an 'experimental ioctl that will change in future kernels',
>>>>   but if we have users that probably won't happen.
>>>> - The code to allocate the table never gets called after we copy_from_user
>>>>   the user input over the kernel settings, and then compare them
>>>>   for inequality.
>>>> - We then go on to use an address provided by user space as both the
>>>>   __user pointer for input and pass it through phys_to_virt to come up
>>>>   with a kernel pointer to copy the data to. This looks like a trivially
>>>>   exploitable root hole.
>>>>
>>>> This patch disables all the obviously broken code, by zeroing out the
>>>> sensitive data provided by user space. I also fix the type confusion
>>>> here. If we think the ioctl has no stable users, we could consider
>>>> just removing it instead.
>>>>
>>> I suspect there shouldn’t  be possible users of this IOCTL, better of  removing
>>> the IOCTL itself.
>>>
>>> Sekhar your call, as the latest PSP releases for 644x use the media
>>> controller framework.
>>
>> I do not have any personal experience with anyone using this support
>> with latest kernels. I too am okay with removing the broken support.
>
> Ok, I think that would be good. Can one of you create that patch?
> Note that we have two implementations of the ioctl, with different
> data structures, depending on the specific hardware.
>
I have posted a patch on top of yours.

Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

>> Since the header file that defines the ioctl is not in include/uapi/*, I
>> guess it cannot be considered stable userspace ABI? Also, there are
>> enough warnings about instability thrown in the comments surrounding the
>> ioctl in include/media/davinci/vpfe_capture.h.
>
> This is not relevant really. The only thing that counts is whether there
> is existing user space that has active users who complain if it breaks.
>
> If you think nobody is using it, that is more important than code
> comments or the location of the header file, but if someone complains
> later anyway, we may end up reverting the removal and fix it differently.
>
Agreed.

Cheers,
--Prabhakar Lad