Skip to content

Commit 77af915

Browse files
Taogle2018Taogle2018
committed
Replace copy_from_user and memdup_user with safe memory copies.
This implementation will validate the size of copying against the maximum size of input buffer supplied by Windows. Change-Id: I1df0766b4e37415c12f42da57342c86f338c55b8
1 parent 53f2a06 commit 77af915

7 files changed

Lines changed: 66 additions & 60 deletions

File tree

‎aehd_main.c‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,33 @@ int aehdUpdateReturnBuffer(PIRP pIrp, u32 start, void *src, u32 size)
6464
return 0;
6565
}
6666

67+
int aehdCopyInputBuffer(PIRP pIrp, u32 start, void* dst, u32 size)
68+
{
69+
PIO_STACK_LOCATION pIoStack = IoGetCurrentIrpStackLocation(pIrp);
70+
unsigned char* pBuff = pIrp->AssociatedIrp.SystemBuffer;
71+
u32 buffSize = pIoStack->Parameters.DeviceIoControl.InputBufferLength;
72+
73+
if (((u64)start + (u64)size) > buffSize)
74+
return -E2BIG;
75+
76+
RtlCopyBytes(dst, pBuff + start, size);
77+
return 0;
78+
}
79+
80+
void* aehdMemdupUser(PIRP pIrp, u32 start, u32 size)
81+
{
82+
void* buf = kzalloc(size, GFP_KERNEL);
83+
if (!buf)
84+
return ERR_PTR(-ENOMEM);
85+
86+
if (aehdCopyInputBuffer(pIrp, start, buf, size)) {
87+
kfree(buf);
88+
return ERR_PTR(-EFAULT);
89+
90+
}
91+
return buf;
92+
}
93+
6794
VOID NTAPI aehdWaitSuspend(
6895
_In_ PKAPC Apc,
6996
_Inout_ PKNORMAL_ROUTINE* NormalRoutine,

‎aehd_main.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ struct aehd_device_extension {
2929
extern PVOID pZeroPage;
3030

3131
extern int aehdUpdateReturnBuffer(PIRP pIrp, u32 start, void *src, u32 size);
32+
extern int aehdCopyInputBuffer(PIRP pIrp, u32 start, void* dst, u32 size);
33+
extern void* aehdMemdupUser(PIRP pIrp, u32 start, u32 size);
34+
3235
extern void aehdWaitSuspend(
3336
_In_ PKAPC Apc,
3437
_Inout_ PKNORMAL_ROUTINE* NormalRoutine,

‎arch/x86/kvm/cpuid.c‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu)
164164
return 36;
165165
}
166166

167-
int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
167+
int kvm_vcpu_ioctl_set_cpuid(PIRP pIrp, struct kvm_vcpu *vcpu,
168168
struct kvm_cpuid *cpuid,
169169
struct kvm_cpuid_entry __user *entries)
170170
{
@@ -174,7 +174,7 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
174174
if (cpuid->nent > AEHD_MAX_CPUID_ENTRIES)
175175
goto out;
176176
r = -EFAULT;
177-
if (copy_from_user(&vcpu->arch.cpuid_entries, entries,
177+
if (aehdCopyInputBuffer(pIrp, sizeof(cpuid), &vcpu->arch.cpuid_entries,
178178
cpuid->nent * sizeof(struct kvm_cpuid_entry)))
179179
goto out;
180180
vcpu->arch.cpuid_nent = cpuid->nent;
@@ -594,7 +594,7 @@ static bool is_centaur_cpu(const struct kvm_cpuid_param *param)
594594
return 0;
595595
}
596596

597-
static bool sanity_check_entries(struct kvm_cpuid_entry __user *entries,
597+
static bool sanity_check_entries(PIRP pIrp, struct kvm_cpuid_entry __user *entries,
598598
__u32 num_entries, unsigned int ioctl_type)
599599
{
600600
int i;
@@ -612,7 +612,9 @@ static bool sanity_check_entries(struct kvm_cpuid_entry __user *entries,
612612
* sheds a tear.
613613
*/
614614
for (i = 0; i < num_entries; i++) {
615-
if (copy_from_user(pad, entries[i].padding, sizeof(pad)))
615+
u32 start = sizeof(struct kvm_cpuid) + i * sizeof(struct kvm_cpuid_entry)
616+
+ offsetof(struct kvm_cpuid_entry, padding);
617+
if (aehdCopyInputBuffer(pIrp, start, pad, sizeof(pad)))
616618
return true;
617619

618620
if (pad[0] || pad[1] || pad[2])
@@ -639,7 +641,7 @@ int kvm_dev_ioctl_get_cpuid(PIRP pIrp, struct kvm_cpuid *cpuid,
639641
if (cpuid->nent > AEHD_MAX_CPUID_ENTRIES)
640642
cpuid->nent = AEHD_MAX_CPUID_ENTRIES;
641643

642-
if (sanity_check_entries(entries, cpuid->nent, type))
644+
if (sanity_check_entries(pIrp, entries, cpuid->nent, type))
643645
return -EINVAL;
644646

645647
r = -ENOMEM;

‎arch/x86/kvm/cpuid.h‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,9 @@ struct kvm_cpuid_entry *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
1717
int kvm_dev_ioctl_get_cpuid(PIRP pIrp, struct kvm_cpuid *cpuid,
1818
struct kvm_cpuid_entry __user *entries,
1919
unsigned int type);
20-
int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
20+
int kvm_vcpu_ioctl_set_cpuid(PIRP Irp, struct kvm_vcpu *vcpu,
2121
struct kvm_cpuid *cpuid,
2222
struct kvm_cpuid_entry __user *entries);
23-
int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
24-
struct kvm_cpuid *cpuid,
25-
struct kvm_cpuid_entry __user *entries);
2623
int kvm_vcpu_ioctl_get_cpuid(struct kvm_vcpu *vcpu,
2724
struct kvm_cpuid *cpuid,
2825
struct kvm_cpuid_entry __user *entries);

‎arch/x86/kvm/x86.c‎

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1236,15 +1236,15 @@ static int msr_io(PIRP pIrp, struct kvm_vcpu *vcpu,
12361236
unsigned size;
12371237

12381238
r = -EFAULT;
1239-
if (copy_from_user(&msrs, user_msrs, sizeof msrs))
1239+
if (aehdCopyInputBuffer(pIrp, 0, &msrs, sizeof msrs))
12401240
goto out;
12411241

12421242
r = -E2BIG;
12431243
if (msrs.nmsrs >= MAX_IO_MSRS)
12441244
goto out;
12451245

12461246
size = sizeof(struct kvm_msr_entry) * msrs.nmsrs;
1247-
entries = memdup_user(user_msrs->entries, size);
1247+
entries = aehdMemdupUser(pIrp, sizeof(msrs), size);
12481248
if (IS_ERR(entries)) {
12491249
r = PTR_ERR(entries);
12501250
goto out;
@@ -1377,7 +1377,7 @@ long kvm_arch_dev_ioctl(struct aehd_device_extension *devext,
13771377
struct kvm_cpuid cpuid;
13781378

13791379
r = -EFAULT;
1380-
if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid))
1380+
if (aehdCopyInputBuffer(pIrp, 0, &cpuid, sizeof cpuid))
13811381
goto out;
13821382

13831383
r = kvm_dev_ioctl_get_cpuid(pIrp, &cpuid, cpuid_arg->entries,
@@ -1876,7 +1876,7 @@ long kvm_arch_vcpu_ioctl(struct aehd_device_extension *devext,
18761876
r = -EINVAL;
18771877
if (!lapic_in_kernel(vcpu))
18781878
goto out;
1879-
u.lapic = memdup_user(argp, sizeof(*u.lapic));
1879+
u.lapic = aehdMemdupUser(pIrp, 0, sizeof(*u.lapic));
18801880
if (IS_ERR(u.lapic))
18811881
return PTR_ERR(u.lapic);
18821882

@@ -1887,7 +1887,7 @@ long kvm_arch_vcpu_ioctl(struct aehd_device_extension *devext,
18871887
struct kvm_interrupt irq;
18881888

18891889
r = -EFAULT;
1890-
if (copy_from_user(&irq, argp, sizeof irq))
1890+
if (aehdCopyInputBuffer(pIrp, 0, &irq, sizeof irq))
18911891
goto out;
18921892
r = kvm_vcpu_ioctl_interrupt(vcpu, &irq);
18931893
break;
@@ -1905,9 +1905,10 @@ long kvm_arch_vcpu_ioctl(struct aehd_device_extension *devext,
19051905
struct kvm_cpuid cpuid;
19061906

19071907
r = -EFAULT;
1908-
if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid))
1908+
if (aehdCopyInputBuffer(pIrp, 0, &cpuid, sizeof cpuid))
19091909
goto out;
1910-
r = kvm_vcpu_ioctl_set_cpuid(vcpu, &cpuid,
1910+
1911+
r = kvm_vcpu_ioctl_set_cpuid(pIrp, vcpu, &cpuid,
19111912
cpuid_arg->entries);
19121913
break;
19131914
}
@@ -1916,7 +1917,7 @@ long kvm_arch_vcpu_ioctl(struct aehd_device_extension *devext,
19161917
struct kvm_cpuid cpuid;
19171918

19181919
r = -EFAULT;
1919-
if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid))
1920+
if (aehdCopyInputBuffer(pIrp, 0, &cpuid, sizeof cpuid))
19201921
goto out;
19211922
r = kvm_vcpu_ioctl_get_cpuid(vcpu, &cpuid,
19221923
cpuid_arg->entries);
@@ -1939,7 +1940,7 @@ long kvm_arch_vcpu_ioctl(struct aehd_device_extension *devext,
19391940
struct kvm_tpr_access_ctl tac;
19401941

19411942
r = -EFAULT;
1942-
if (copy_from_user(&tac, argp, sizeof tac))
1943+
if (aehdCopyInputBuffer(pIrp, 0, &tac, sizeof tac))
19431944
goto out;
19441945
r = vcpu_ioctl_tpr_access_reporting(vcpu, &tac);
19451946
if (r)
@@ -1955,7 +1956,7 @@ long kvm_arch_vcpu_ioctl(struct aehd_device_extension *devext,
19551956
if (!lapic_in_kernel(vcpu))
19561957
goto out;
19571958
r = -EFAULT;
1958-
if (copy_from_user(&va, argp, sizeof va))
1959+
if (aehdCopyInputBuffer(pIrp, 0, &va, sizeof va))
19591960
goto out;
19601961
idx = srcu_read_lock(&vcpu->kvm->srcu);
19611962
r = kvm_lapic_set_vapic_addr(vcpu, va.vapic_addr);
@@ -1975,7 +1976,7 @@ long kvm_arch_vcpu_ioctl(struct aehd_device_extension *devext,
19751976
struct kvm_vcpu_events events;
19761977

19771978
r = -EFAULT;
1978-
if (copy_from_user(&events, argp, sizeof(struct kvm_vcpu_events)))
1979+
if (aehdCopyInputBuffer(pIrp, 0, &events, sizeof(struct kvm_vcpu_events)))
19791980
break;
19801981

19811982
r = kvm_vcpu_ioctl_x86_set_vcpu_events(vcpu, &events);
@@ -1994,7 +1995,7 @@ long kvm_arch_vcpu_ioctl(struct aehd_device_extension *devext,
19941995
struct kvm_debugregs dbgregs;
19951996

19961997
r = -EFAULT;
1997-
if (copy_from_user(&dbgregs, argp,
1998+
if (aehdCopyInputBuffer(pIrp, 0, &dbgregs,
19981999
sizeof(struct kvm_debugregs)))
19992000
break;
20002001

@@ -2014,7 +2015,7 @@ long kvm_arch_vcpu_ioctl(struct aehd_device_extension *devext,
20142015
break;
20152016
}
20162017
case AEHD_SET_XSAVE: {
2017-
u.xsave = memdup_user(argp, sizeof(*u.xsave));
2018+
u.xsave = aehdMemdupUser(pIrp, 0, sizeof(*u.xsave));
20182019
if (IS_ERR(u.xsave))
20192020
return PTR_ERR(u.xsave);
20202021

@@ -2034,7 +2035,7 @@ long kvm_arch_vcpu_ioctl(struct aehd_device_extension *devext,
20342035
break;
20352036
}
20362037
case AEHD_SET_XCRS: {
2037-
u.xcrs = memdup_user(argp, sizeof(*u.xcrs));
2038+
u.xcrs = aehdMemdupUser(pIrp, 0, sizeof(*u.xcrs));
20382039
if (IS_ERR(u.xcrs))
20392040
return PTR_ERR(u.xcrs);
20402041

@@ -2295,7 +2296,7 @@ long kvm_arch_vm_ioctl(struct aehd_device_extension *devext,
22952296
/* 0: PIC master, 1: PIC slave, 2: IOAPIC */
22962297
struct kvm_irqchip *chip;
22972298

2298-
chip = memdup_user(argp, sizeof(*chip));
2299+
chip = aehdMemdupUser(pIrp, 0, sizeof(*chip));
22992300
if (IS_ERR(chip)) {
23002301
r = PTR_ERR(chip);
23012302
goto out;
@@ -2316,7 +2317,7 @@ long kvm_arch_vm_ioctl(struct aehd_device_extension *devext,
23162317
/* 0: PIC master, 1: PIC slave, 2: IOAPIC */
23172318
struct kvm_irqchip *chip;
23182319

2319-
chip = memdup_user(argp, sizeof(*chip));
2320+
chip = aehdMemdupUser(pIrp, 0, sizeof(*chip));
23202321
if (IS_ERR(chip)) {
23212322
r = PTR_ERR(chip);
23222323
goto out;
@@ -2346,7 +2347,7 @@ long kvm_arch_vm_ioctl(struct aehd_device_extension *devext,
23462347
struct kvm_enable_cap cap;
23472348

23482349
r = -EFAULT;
2349-
if (copy_from_user(&cap, argp, sizeof(cap)))
2350+
if (aehdCopyInputBuffer(pIrp, 0, &cap, sizeof(cap)))
23502351
goto out;
23512352
r = kvm_vm_ioctl_enable_cap(kvm, &cap);
23522353
break;

‎ntkrutils.h‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -949,18 +949,6 @@ static __inline void kvm_release_page(PMDL mdl)
949949
IoFreeMdl(mdl);
950950
}
951951

952-
/* We actually did not copy from *user* here. This function in kvm is used to
953-
* ioctl parameters. On Windows, we always use buffered io for device control.
954-
* Thus the address supplied to copy_from_user is address in kernel space.
955-
* Simple keep the function name here.
956-
* __copy_from/to_user is really copying from user space.
957-
*/
958-
static __inline size_t copy_from_user(void *dst, const void *src, size_t size)
959-
{
960-
memcpy(dst, src, size);
961-
return 0;
962-
}
963-
964952
static __inline size_t __copy_user_safe(void *dst, const void *src, size_t size,
965953
int from)
966954
{
@@ -1064,17 +1052,6 @@ static __inline void kunmap_atomic(PMDL mdl)
10641052
kunmap(mdl);
10651053
}
10661054

1067-
static __inline void *memdup_user(const void *user, size_t size)
1068-
{
1069-
void *buf = kzalloc(size, GFP_KERNEL);
1070-
1071-
if (!buf)
1072-
return ERR_PTR(-ENOMEM);
1073-
if (copy_from_user(buf, user, size))
1074-
return ERR_PTR(-EFAULT);
1075-
return buf;
1076-
}
1077-
10781055
/*
10791056
TSC
10801057
*/

‎virt/kvm/kvm_main.c‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1891,7 +1891,6 @@ NTSTATUS kvm_vcpu_ioctl(PDEVICE_OBJECT pDevObj, PIRP pIrp,
18911891
{
18921892
struct aehd_device_extension *devext = pDevObj->DeviceExtension;
18931893
struct kvm_vcpu *vcpu = devext->PrivData;
1894-
void __user *argp = (void __user *)pIrp->AssociatedIrp.SystemBuffer;
18951894
int r;
18961895
struct kvm_fpu *fpu = NULL;
18971896
struct kvm_sregs *kvm_sregs = NULL;
@@ -1941,7 +1940,7 @@ NTSTATUS kvm_vcpu_ioctl(PDEVICE_OBJECT pDevObj, PIRP pIrp,
19411940
struct kvm_regs *kvm_regs;
19421941

19431942
r = -ENOMEM;
1944-
kvm_regs = memdup_user(argp, sizeof(*kvm_regs));
1943+
kvm_regs = aehdMemdupUser(pIrp, 0, sizeof(*kvm_regs));
19451944
if (IS_ERR(kvm_regs)) {
19461945
r = PTR_ERR(kvm_regs);
19471946
goto out;
@@ -1965,7 +1964,7 @@ NTSTATUS kvm_vcpu_ioctl(PDEVICE_OBJECT pDevObj, PIRP pIrp,
19651964
break;
19661965
}
19671966
case AEHD_SET_SREGS: {
1968-
kvm_sregs = memdup_user(argp, sizeof(*kvm_sregs));
1967+
kvm_sregs = aehdMemdupUser(pIrp, 0, sizeof(*kvm_sregs));
19691968
if (IS_ERR(kvm_sregs)) {
19701969
r = PTR_ERR(kvm_sregs);
19711970
kvm_sregs = NULL;
@@ -1987,7 +1986,7 @@ NTSTATUS kvm_vcpu_ioctl(PDEVICE_OBJECT pDevObj, PIRP pIrp,
19871986
struct kvm_mp_state mp_state;
19881987

19891988
r = -EFAULT;
1990-
if (copy_from_user(&mp_state, argp, sizeof(mp_state)))
1989+
if (aehdCopyInputBuffer(pIrp, 0, &mp_state, sizeof(mp_state)))
19911990
goto out;
19921991
r = kvm_arch_vcpu_ioctl_set_mpstate(vcpu, &mp_state);
19931992
break;
@@ -1996,7 +1995,7 @@ NTSTATUS kvm_vcpu_ioctl(PDEVICE_OBJECT pDevObj, PIRP pIrp,
19961995
struct kvm_translation tr;
19971996

19981997
r = -EFAULT;
1999-
if (copy_from_user(&tr, argp, sizeof(tr)))
1998+
if (aehdCopyInputBuffer(pIrp, 0, &tr, sizeof(tr)))
20001999
goto out;
20012000
r = kvm_arch_vcpu_ioctl_translate(vcpu, &tr);
20022001
if (r)
@@ -2008,7 +2007,7 @@ NTSTATUS kvm_vcpu_ioctl(PDEVICE_OBJECT pDevObj, PIRP pIrp,
20082007
struct kvm_guest_debug dbg;
20092008

20102009
r = -EFAULT;
2011-
if (copy_from_user(&dbg, argp, sizeof(dbg)))
2010+
if (aehdCopyInputBuffer(pIrp, 0, &dbg, sizeof(dbg)))
20122011
goto out;
20132012
r = kvm_arch_vcpu_ioctl_set_guest_debug(vcpu, &dbg);
20142013
break;
@@ -2025,7 +2024,7 @@ NTSTATUS kvm_vcpu_ioctl(PDEVICE_OBJECT pDevObj, PIRP pIrp,
20252024
break;
20262025
}
20272026
case AEHD_SET_FPU: {
2028-
fpu = memdup_user(argp, sizeof(*fpu));
2027+
fpu = aehdMemdupUser(pIrp, 0, sizeof(*fpu));
20292028
if (IS_ERR(fpu)) {
20302029
r = PTR_ERR(fpu);
20312030
fpu = NULL;
@@ -2089,7 +2088,7 @@ NTSTATUS kvm_vm_ioctl(PDEVICE_OBJECT pDevObj, PIRP pIrp,
20892088
struct kvm_dirty_log log;
20902089

20912090
r = -EFAULT;
2092-
if (copy_from_user(&log, argp, sizeof(log)))
2091+
if (aehdCopyInputBuffer(pIrp, 0, &log, sizeof(log)))
20932092
goto out;
20942093
r = kvm_vm_ioctl_get_dirty_log(kvm, &log);
20952094
break;
@@ -2112,7 +2111,7 @@ NTSTATUS kvm_vm_ioctl(PDEVICE_OBJECT pDevObj, PIRP pIrp,
21122111
struct kvm_irq_level irq_event;
21132112

21142113
r = -EFAULT;
2115-
if (copy_from_user(&irq_event, argp, sizeof(irq_event)))
2114+
if (aehdCopyInputBuffer(pIrp, 0, &irq_event, sizeof(irq_event)))
21162115
goto out;
21172116

21182117
r = kvm_vm_ioctl_irq_line(kvm, &irq_event, true);
@@ -2135,7 +2134,7 @@ NTSTATUS kvm_vm_ioctl(PDEVICE_OBJECT pDevObj, PIRP pIrp,
21352134
struct kvm_irq_routing_entry *entries = NULL;
21362135

21372136
r = -EFAULT;
2138-
if (copy_from_user(&routing, argp, sizeof(routing)))
2137+
if (aehdCopyInputBuffer(pIrp, 0, &routing, sizeof(routing)))
21392138
goto out;
21402139
r = -EINVAL;
21412140
if (routing.nr > AEHD_MAX_IRQ_ROUTES)
@@ -2149,7 +2148,7 @@ NTSTATUS kvm_vm_ioctl(PDEVICE_OBJECT pDevObj, PIRP pIrp,
21492148
goto out;
21502149
r = -EFAULT;
21512150
urouting = argp;
2152-
if (copy_from_user(entries, urouting->entries,
2151+
if (aehdCopyInputBuffer(pIrp, offsetof(struct kvm_irq_routing, entries), entries,
21532152
routing.nr * sizeof(*entries)))
21542153
goto out_free_irq_routing;
21552154
}

0 commit comments

Comments
 (0)