ACK: [PATCH 1/2] efi_runtime: Copied the structure from userland locally in kernel space

[Alex Hung]

On 14-10-21 07:50 PM, Matt Fleming wrote:
> From: Pradeep Gaddam <pradeep.r.gaddam at intel.com>
>
> When we have structures that contain pointers, we cannot just do double
> dereference on it as in struct->pointer->value. This will end up as a
> Page Fault. To fix this problem, I changed efi_runtime kernel module to
> first fetch the entire structure into a local copy on stack and use that
> to reference the other data members of the struct.
>
> Signed-off-by: Pradeep Gaddam <Pradeep.r.Gaddam at intel.com>
> ---
>   efi_runtime/efi_runtime.c | 50 ++++++++++++++++++++++++++++-------------------
>   1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c
> index 4f9d5545f2f5..59609bc956b2 100644
> --- a/efi_runtime/efi_runtime.c
> +++ b/efi_runtime/efi_runtime.c
> @@ -301,44 +301,51 @@ static long efi_runtime_set_variable(unsigned long arg)
>   static long efi_runtime_get_time(unsigned long arg)
>   {
>   	struct efi_gettime __user *pgettime;
> +	struct efi_gettime  pgettime_local;
> +	EFI_TIME_CAPABILITIES __user *cap_local;
>   	efi_status_t status;
>   	efi_time_cap_t cap;
>   	efi_time_t eft;
>   
>   	status = efi.get_time(&eft, &cap);
>   	pgettime = (struct efi_gettime __user *)arg;
> -	if (put_user(status, pgettime->status))
> +	if (copy_from_user(&pgettime_local, pgettime, sizeof(pgettime_local)))
> +		return -EFAULT;
> +
> +	cap_local = (EFI_TIME_CAPABILITIES *)pgettime_local.Capabilities;
> +	if (put_user(status, pgettime_local.status))
>   		return -EFAULT;
>   	if (status != EFI_SUCCESS) {
>   		printk(KERN_ERR "efitime: can't read time\n");
>   		return -EINVAL;
>   	}
>   	if (put_user(cap.resolution,
> -				&pgettime->Capabilities->Resolution) ||
> -				put_user(cap.accuracy,
> -				&pgettime->Capabilities->Accuracy) ||
> -				put_user(cap.sets_to_zero,
> -				&pgettime->Capabilities->SetsToZero))
> +				&(cap_local->Resolution)) ||
> +				put_user(cap.accuracy, &(cap_local->Accuracy)) ||
> +				put_user(cap.sets_to_zero,&(cap_local->SetsToZero)))
>   		return -EFAULT;
> -	return copy_to_user(pgettime->Time, &eft,
> +	return copy_to_user(pgettime_local.Time, &eft,
>   			sizeof(EFI_TIME)) ? -EFAULT : 0;
>   }
>   
>   static long efi_runtime_set_time(unsigned long arg)
>   {
>   	struct efi_settime __user *psettime;
> +	struct efi_settime psettime_local;
>   	efi_status_t status;
>   	EFI_TIME efi_time;
>   	efi_time_t eft;
>   
>   	psettime = (struct efi_settime __user *)arg;
> -	if (copy_from_user(&efi_time, psettime->Time,
> +	if (copy_from_user(&psettime_local, psettime, sizeof(psettime_local)))
> +		return -EFAULT;
> +	if (copy_from_user(&efi_time, psettime_local.Time,
>   					sizeof(EFI_TIME)))
>   		return -EFAULT;
>   	convert_to_efi_time(&eft, &efi_time);
>   	status = efi.set_time(&eft);
>   
> -	if (put_user(status, psettime->status))
> +	if (put_user(status, psettime_local.status))
>   		return -EFAULT;
>   
>   	return status == EFI_SUCCESS ? 0 : -EINVAL;
> @@ -347,6 +354,7 @@ static long efi_runtime_set_time(unsigned long arg)
>   static long efi_runtime_get_waketime(unsigned long arg)
>   {
>   	struct efi_getwakeuptime __user *pgetwakeuptime;
> +	struct efi_getwakeuptime pgetwakeuptime_local;
>   	unsigned char enabled, pending;
>   	efi_status_t status;
>   	EFI_TIME efi_time;
> @@ -357,24 +365,25 @@ static long efi_runtime_get_waketime(unsigned long arg)
>   
>   	pgetwakeuptime = (struct efi_getwakeuptime __user *)arg;
>   
> -	if (put_user(status, pgetwakeuptime->status))
> +	if (copy_from_user(&pgetwakeuptime_local, pgetwakeuptime, sizeof(pgetwakeuptime_local)))
> +		return -EFAULT;
> +	if (put_user(status, pgetwakeuptime_local.status))
>   		return -EFAULT;
>   	if (status != EFI_SUCCESS)
>   		return -EINVAL;
> -
> -	if (put_user(enabled, pgetwakeuptime->Enabled) ||
> -			put_user(pending, pgetwakeuptime->Pending))
> +	if (put_user(enabled, pgetwakeuptime_local.Enabled) ||
> +				put_user(pending, pgetwakeuptime_local.Pending))
>   		return -EFAULT;
> -
>   	convert_from_efi_time(&eft, &efi_time);
>   
> -	return copy_to_user(pgetwakeuptime->Time, &efi_time,
> +	return copy_to_user(pgetwakeuptime_local.Time, &efi_time,
>   			sizeof(EFI_TIME)) ? -EFAULT : 0;
>   }
>   
>   static long efi_runtime_set_waketime(unsigned long arg)
>   {
>   	struct efi_setwakeuptime __user *psetwakeuptime;
> +	struct efi_setwakeuptime psetwakeuptime_local;
>   	unsigned char enabled;
>   	efi_status_t status;
>   	EFI_TIME efi_time;
> @@ -382,17 +391,18 @@ static long efi_runtime_set_waketime(unsigned long arg)
>   
>   	psetwakeuptime = (struct efi_setwakeuptime __user *)arg;
>   
> -	if (get_user(enabled, &psetwakeuptime->Enabled) ||
> -				copy_from_user(&efi_time,
> -				psetwakeuptime->Time,
> -				sizeof(EFI_TIME)))
> +	if (copy_from_user(&psetwakeuptime_local, psetwakeuptime, sizeof(psetwakeuptime_local)))
> +		return -EFAULT;
> +
> +	if (get_user(enabled, &(psetwakeuptime_local.Enabled)) ||
> +				copy_from_user(&efi_time, psetwakeuptime_local.Time, sizeof(EFI_TIME)))
>   		return -EFAULT;
>   
>   	convert_to_efi_time(&eft, &efi_time);
>   
>   	status = efi.set_wakeup_time(enabled, &eft);
>   
> -	if (put_user(status, psetwakeuptime->status))
> +	if (put_user(status, psetwakeuptime_local.status))
>   		return -EFAULT;
>   
>   	return status == EFI_SUCCESS ? 0 : -EINVAL;

Acked-by: Alex Hung <alex.hung at canonical.com>