Re: [PATCH] KVM: SVM: no need to call access_ok() in LAUNCH_MEASURE command
On 23/02/2018 19:36, Brijesh Singh wrote: > Using the access_ok() to validate the input before issuing the SEV > command does not buy us anything in this case. If userland is > giving us a garbage pointer then copy_to_user() will catch it when we try > to return the measurement. > > Suggested-by: Al Viro> Fixes: 0d0736f76347 (KVM: SVM: Add support for KVM_SEV_LAUNCH_MEASURE ...) > Cc: Paolo Bonzini > Cc: "Radim Krčmář" > Cc: Borislav Petkov > Cc: Tom Lendacky > Cc: linux-kernel@vger.kernel.org > Cc: Joerg Roedel > Signed-off-by: Brijesh Singh > --- > > We no longer need patch [1]. This patch implements Al Viro's recommendation > [2] > > [1] https://marc.info/?l=linux-kernel=151905677729098=2. > [2] https://marc.info/?l=linux-kernel=151923536116467=2 > > arch/x86/kvm/svm.c | 16 +++- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index b3e488a74828..ca69d53d7e6d 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -6236,16 +6236,18 @@ static int sev_launch_update_data(struct kvm *kvm, > struct kvm_sev_cmd *argp) > > static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) > { > + void __user *measure = (void __user *)(uintptr_t)argp->data; > struct kvm_sev_info *sev = >arch.sev_info; > struct sev_data_launch_measure *data; > struct kvm_sev_launch_measure params; > + void __user *p = NULL; > void *blob = NULL; > int ret; > > if (!sev_guest(kvm)) > return -ENOTTY; > > - if (copy_from_user(, (void __user *)(uintptr_t)argp->data, > sizeof(params))) > + if (copy_from_user(, measure, sizeof(params))) > return -EFAULT; > > data = kzalloc(sizeof(*data), GFP_KERNEL); > @@ -6256,17 +6258,13 @@ static int sev_launch_measure(struct kvm *kvm, struct > kvm_sev_cmd *argp) > if (!params.len) > goto cmd; > > - if (params.uaddr) { > + p = (void __user *)(uintptr_t)params.uaddr; > + if (p) { > if (params.len > SEV_FW_BLOB_MAX_SIZE) { > ret = -EINVAL; > goto e_free; > } > > - if (!access_ok(VERIFY_WRITE, params.uaddr, params.len)) { > - ret = -EFAULT; > - goto e_free; > - } > - > ret = -ENOMEM; > blob = kmalloc(params.len, GFP_KERNEL); > if (!blob) > @@ -6290,13 +6288,13 @@ static int sev_launch_measure(struct kvm *kvm, struct > kvm_sev_cmd *argp) > goto e_free_blob; > > if (blob) { > - if (copy_to_user((void __user *)(uintptr_t)params.uaddr, blob, > params.len)) > + if (copy_to_user(p, blob, params.len)) > ret = -EFAULT; > } > > done: > params.len = data->len; > - if (copy_to_user((void __user *)(uintptr_t)argp->data, , > sizeof(params))) > + if (copy_to_user(measure, , sizeof(params))) > ret = -EFAULT; > e_free_blob: > kfree(blob); > Queued, thanks Brijesh and Al. Paolo
Re: [PATCH] KVM: SVM: no need to call access_ok() in LAUNCH_MEASURE command
On 23/02/2018 19:36, Brijesh Singh wrote: > Using the access_ok() to validate the input before issuing the SEV > command does not buy us anything in this case. If userland is > giving us a garbage pointer then copy_to_user() will catch it when we try > to return the measurement. > > Suggested-by: Al Viro > Fixes: 0d0736f76347 (KVM: SVM: Add support for KVM_SEV_LAUNCH_MEASURE ...) > Cc: Paolo Bonzini > Cc: "Radim Krčmář" > Cc: Borislav Petkov > Cc: Tom Lendacky > Cc: linux-kernel@vger.kernel.org > Cc: Joerg Roedel > Signed-off-by: Brijesh Singh > --- > > We no longer need patch [1]. This patch implements Al Viro's recommendation > [2] > > [1] https://marc.info/?l=linux-kernel=151905677729098=2. > [2] https://marc.info/?l=linux-kernel=151923536116467=2 > > arch/x86/kvm/svm.c | 16 +++- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index b3e488a74828..ca69d53d7e6d 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -6236,16 +6236,18 @@ static int sev_launch_update_data(struct kvm *kvm, > struct kvm_sev_cmd *argp) > > static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) > { > + void __user *measure = (void __user *)(uintptr_t)argp->data; > struct kvm_sev_info *sev = >arch.sev_info; > struct sev_data_launch_measure *data; > struct kvm_sev_launch_measure params; > + void __user *p = NULL; > void *blob = NULL; > int ret; > > if (!sev_guest(kvm)) > return -ENOTTY; > > - if (copy_from_user(, (void __user *)(uintptr_t)argp->data, > sizeof(params))) > + if (copy_from_user(, measure, sizeof(params))) > return -EFAULT; > > data = kzalloc(sizeof(*data), GFP_KERNEL); > @@ -6256,17 +6258,13 @@ static int sev_launch_measure(struct kvm *kvm, struct > kvm_sev_cmd *argp) > if (!params.len) > goto cmd; > > - if (params.uaddr) { > + p = (void __user *)(uintptr_t)params.uaddr; > + if (p) { > if (params.len > SEV_FW_BLOB_MAX_SIZE) { > ret = -EINVAL; > goto e_free; > } > > - if (!access_ok(VERIFY_WRITE, params.uaddr, params.len)) { > - ret = -EFAULT; > - goto e_free; > - } > - > ret = -ENOMEM; > blob = kmalloc(params.len, GFP_KERNEL); > if (!blob) > @@ -6290,13 +6288,13 @@ static int sev_launch_measure(struct kvm *kvm, struct > kvm_sev_cmd *argp) > goto e_free_blob; > > if (blob) { > - if (copy_to_user((void __user *)(uintptr_t)params.uaddr, blob, > params.len)) > + if (copy_to_user(p, blob, params.len)) > ret = -EFAULT; > } > > done: > params.len = data->len; > - if (copy_to_user((void __user *)(uintptr_t)argp->data, , > sizeof(params))) > + if (copy_to_user(measure, , sizeof(params))) > ret = -EFAULT; > e_free_blob: > kfree(blob); > Queued, thanks Brijesh and Al. Paolo
[PATCH] KVM: SVM: no need to call access_ok() in LAUNCH_MEASURE command
Using the access_ok() to validate the input before issuing the SEV command does not buy us anything in this case. If userland is giving us a garbage pointer then copy_to_user() will catch it when we try to return the measurement. Suggested-by: Al ViroFixes: 0d0736f76347 (KVM: SVM: Add support for KVM_SEV_LAUNCH_MEASURE ...) Cc: Paolo Bonzini Cc: "Radim Krčmář" Cc: Borislav Petkov Cc: Tom Lendacky Cc: linux-kernel@vger.kernel.org Cc: Joerg Roedel Signed-off-by: Brijesh Singh --- We no longer need patch [1]. This patch implements Al Viro's recommendation [2] [1] https://marc.info/?l=linux-kernel=151905677729098=2. [2] https://marc.info/?l=linux-kernel=151923536116467=2 arch/x86/kvm/svm.c | 16 +++- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index b3e488a74828..ca69d53d7e6d 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -6236,16 +6236,18 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp) static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) { + void __user *measure = (void __user *)(uintptr_t)argp->data; struct kvm_sev_info *sev = >arch.sev_info; struct sev_data_launch_measure *data; struct kvm_sev_launch_measure params; + void __user *p = NULL; void *blob = NULL; int ret; if (!sev_guest(kvm)) return -ENOTTY; - if (copy_from_user(, (void __user *)(uintptr_t)argp->data, sizeof(params))) + if (copy_from_user(, measure, sizeof(params))) return -EFAULT; data = kzalloc(sizeof(*data), GFP_KERNEL); @@ -6256,17 +6258,13 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) if (!params.len) goto cmd; - if (params.uaddr) { + p = (void __user *)(uintptr_t)params.uaddr; + if (p) { if (params.len > SEV_FW_BLOB_MAX_SIZE) { ret = -EINVAL; goto e_free; } - if (!access_ok(VERIFY_WRITE, params.uaddr, params.len)) { - ret = -EFAULT; - goto e_free; - } - ret = -ENOMEM; blob = kmalloc(params.len, GFP_KERNEL); if (!blob) @@ -6290,13 +6288,13 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) goto e_free_blob; if (blob) { - if (copy_to_user((void __user *)(uintptr_t)params.uaddr, blob, params.len)) + if (copy_to_user(p, blob, params.len)) ret = -EFAULT; } done: params.len = data->len; - if (copy_to_user((void __user *)(uintptr_t)argp->data, , sizeof(params))) + if (copy_to_user(measure, , sizeof(params))) ret = -EFAULT; e_free_blob: kfree(blob); -- 2.14.3
[PATCH] KVM: SVM: no need to call access_ok() in LAUNCH_MEASURE command
Using the access_ok() to validate the input before issuing the SEV command does not buy us anything in this case. If userland is giving us a garbage pointer then copy_to_user() will catch it when we try to return the measurement. Suggested-by: Al Viro Fixes: 0d0736f76347 (KVM: SVM: Add support for KVM_SEV_LAUNCH_MEASURE ...) Cc: Paolo Bonzini Cc: "Radim Krčmář" Cc: Borislav Petkov Cc: Tom Lendacky Cc: linux-kernel@vger.kernel.org Cc: Joerg Roedel Signed-off-by: Brijesh Singh --- We no longer need patch [1]. This patch implements Al Viro's recommendation [2] [1] https://marc.info/?l=linux-kernel=151905677729098=2. [2] https://marc.info/?l=linux-kernel=151923536116467=2 arch/x86/kvm/svm.c | 16 +++- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index b3e488a74828..ca69d53d7e6d 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -6236,16 +6236,18 @@ static int sev_launch_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp) static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) { + void __user *measure = (void __user *)(uintptr_t)argp->data; struct kvm_sev_info *sev = >arch.sev_info; struct sev_data_launch_measure *data; struct kvm_sev_launch_measure params; + void __user *p = NULL; void *blob = NULL; int ret; if (!sev_guest(kvm)) return -ENOTTY; - if (copy_from_user(, (void __user *)(uintptr_t)argp->data, sizeof(params))) + if (copy_from_user(, measure, sizeof(params))) return -EFAULT; data = kzalloc(sizeof(*data), GFP_KERNEL); @@ -6256,17 +6258,13 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) if (!params.len) goto cmd; - if (params.uaddr) { + p = (void __user *)(uintptr_t)params.uaddr; + if (p) { if (params.len > SEV_FW_BLOB_MAX_SIZE) { ret = -EINVAL; goto e_free; } - if (!access_ok(VERIFY_WRITE, params.uaddr, params.len)) { - ret = -EFAULT; - goto e_free; - } - ret = -ENOMEM; blob = kmalloc(params.len, GFP_KERNEL); if (!blob) @@ -6290,13 +6288,13 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp) goto e_free_blob; if (blob) { - if (copy_to_user((void __user *)(uintptr_t)params.uaddr, blob, params.len)) + if (copy_to_user(p, blob, params.len)) ret = -EFAULT; } done: params.len = data->len; - if (copy_to_user((void __user *)(uintptr_t)argp->data, , sizeof(params))) + if (copy_to_user(measure, , sizeof(params))) ret = -EFAULT; e_free_blob: kfree(blob); -- 2.14.3