| Message ID | 1442314179-9706-6-git-send-email-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 15, 2015 at 08:49:35PM +1000, Alexey Kardashevskiy wrote: > At the moment pages used for TCE tables (in addition to pages addressed > by TCEs) are not counted in locked_vm counter so a malicious userspace > tool can call ioctl(KVM_CREATE_SPAPR_TCE) as many times as RLIMIT_NOFILE and > lock a lot of memory. > > This adds counting for pages used for TCE tables. > > This counts the number of pages required for a table plus pages for > the kvmppc_spapr_tce_table struct (TCE table descriptor) itself. > > This does not change the amount of (de)allocated memory. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > arch/powerpc/kvm/book3s_64_vio.c | 51 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 50 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c > index 9526c34..b70787d 100644 > --- a/arch/powerpc/kvm/book3s_64_vio.c > +++ b/arch/powerpc/kvm/book3s_64_vio.c > @@ -45,13 +45,56 @@ static long kvmppc_stt_npages(unsigned long window_size) > * sizeof(u64), PAGE_SIZE) / PAGE_SIZE; > } > > +static long kvmppc_account_memlimit(long npages, bool inc) > +{ > + long ret = 0; > + const long bytes = sizeof(struct kvmppc_spapr_tce_table) + > + (abs(npages) * sizeof(struct page *)); Why abs(npages)? Can npages be negative? If so, what does that mean? Paul. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/30/2015 01:06 PM, Paul Mackerras wrote: > On Tue, Sep 15, 2015 at 08:49:35PM +1000, Alexey Kardashevskiy wrote: >> At the moment pages used for TCE tables (in addition to pages addressed >> by TCEs) are not counted in locked_vm counter so a malicious userspace >> tool can call ioctl(KVM_CREATE_SPAPR_TCE) as many times as RLIMIT_NOFILE and >> lock a lot of memory. >> >> This adds counting for pages used for TCE tables. >> >> This counts the number of pages required for a table plus pages for >> the kvmppc_spapr_tce_table struct (TCE table descriptor) itself. >> >> This does not change the amount of (de)allocated memory. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> arch/powerpc/kvm/book3s_64_vio.c | 51 +++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 50 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c >> index 9526c34..b70787d 100644 >> --- a/arch/powerpc/kvm/book3s_64_vio.c >> +++ b/arch/powerpc/kvm/book3s_64_vio.c >> @@ -45,13 +45,56 @@ static long kvmppc_stt_npages(unsigned long window_size) >> * sizeof(u64), PAGE_SIZE) / PAGE_SIZE; >> } >> >> +static long kvmppc_account_memlimit(long npages, bool inc) >> +{ >> + long ret = 0; >> + const long bytes = sizeof(struct kvmppc_spapr_tce_table) + >> + (abs(npages) * sizeof(struct page *)); > > Why abs(npages)? Can npages be negative? If so, what does that mean? Leftover from older versions when there was one shared account_memlimit(long npages). It does not make sense here, I need to remove it.
On Tue, Sep 15, 2015 at 08:49:35PM +1000, Alexey Kardashevskiy wrote: > At the moment pages used for TCE tables (in addition to pages addressed > by TCEs) are not counted in locked_vm counter so a malicious userspace > tool can call ioctl(KVM_CREATE_SPAPR_TCE) as many times as RLIMIT_NOFILE and > lock a lot of memory. > > This adds counting for pages used for TCE tables. > > This counts the number of pages required for a table plus pages for > the kvmppc_spapr_tce_table struct (TCE table descriptor) itself. Hmm. Does it make sense to account for the descriptor struct itself? I mean there are lots of little structures the kernel will allocate on a process's behalf, and I don't think most of them get accounted against locked vm. > This does not change the amount of (de)allocated memory. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > arch/powerpc/kvm/book3s_64_vio.c | 51 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 50 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c > index 9526c34..b70787d 100644 > --- a/arch/powerpc/kvm/book3s_64_vio.c > +++ b/arch/powerpc/kvm/book3s_64_vio.c > @@ -45,13 +45,56 @@ static long kvmppc_stt_npages(unsigned long window_size) > * sizeof(u64), PAGE_SIZE) / PAGE_SIZE; > } > > +static long kvmppc_account_memlimit(long npages, bool inc) > +{ > + long ret = 0; > + const long bytes = sizeof(struct kvmppc_spapr_tce_table) + > + (abs(npages) * sizeof(struct page *)); > + const long stt_pages = ALIGN(bytes, PAGE_SIZE) / PAGE_SIZE; Overflow checks might be useful here, I'm not sure. > + > + if (!current || !current->mm) > + return ret; /* process exited */ > + > + npages += stt_pages; > + > + down_write(¤t->mm->mmap_sem); > + > + if (inc) { > + long locked, lock_limit; > + > + locked = current->mm->locked_vm + npages; > + lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > + if (locked > lock_limit && !capable(CAP_IPC_LOCK)) > + ret = -ENOMEM; > + else > + current->mm->locked_vm += npages; > + } else { > + if (npages > current->mm->locked_vm) Should this be a WARN_ON? It means something has gone wrong previously in the accounting, doesn't it? > + npages = current->mm->locked_vm; > + > + current->mm->locked_vm -= npages; > + } > + > + pr_debug("[%d] RLIMIT_MEMLOCK KVM %c%ld %ld/%ld%s\n", current->pid, > + inc ? '+' : '-', > + npages << PAGE_SHIFT, > + current->mm->locked_vm << PAGE_SHIFT, > + rlimit(RLIMIT_MEMLOCK), > + ret ? " - exceeded" : ""); > + > + up_write(¤t->mm->mmap_sem); > + > + return ret; > +} > + > static void release_spapr_tce_table(struct rcu_head *head) > { > struct kvmppc_spapr_tce_table *stt = container_of(head, > struct kvmppc_spapr_tce_table, rcu); > int i; > + long npages = kvmppc_stt_npages(stt->window_size); > > - for (i = 0; i < kvmppc_stt_npages(stt->window_size); i++) > + for (i = 0; i < npages; i++) > __free_page(stt->pages[i]); > > kfree(stt); > @@ -89,6 +132,7 @@ static int kvm_spapr_tce_release(struct inode *inode, struct file *filp) > > kvm_put_kvm(stt->kvm); > > + kvmppc_account_memlimit(kvmppc_stt_npages(stt->window_size), false); > call_rcu(&stt->rcu, release_spapr_tce_table); > > return 0; > @@ -114,6 +158,11 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm, > } > > npages = kvmppc_stt_npages(args->window_size); > + ret = kvmppc_account_memlimit(npages, true); > + if (ret) { > + stt = NULL; > + goto fail; > + } > > stt = kzalloc(sizeof(*stt) + npages * sizeof(struct page *), > GFP_KERNEL);
diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c index 9526c34..b70787d 100644 --- a/arch/powerpc/kvm/book3s_64_vio.c +++ b/arch/powerpc/kvm/book3s_64_vio.c @@ -45,13 +45,56 @@ static long kvmppc_stt_npages(unsigned long window_size) * sizeof(u64), PAGE_SIZE) / PAGE_SIZE; } +static long kvmppc_account_memlimit(long npages, bool inc) +{ + long ret = 0; + const long bytes = sizeof(struct kvmppc_spapr_tce_table) + + (abs(npages) * sizeof(struct page *)); + const long stt_pages = ALIGN(bytes, PAGE_SIZE) / PAGE_SIZE; + + if (!current || !current->mm) + return ret; /* process exited */ + + npages += stt_pages; + + down_write(¤t->mm->mmap_sem); + + if (inc) { + long locked, lock_limit; + + locked = current->mm->locked_vm + npages; + lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; + if (locked > lock_limit && !capable(CAP_IPC_LOCK)) + ret = -ENOMEM; + else + current->mm->locked_vm += npages; + } else { + if (npages > current->mm->locked_vm) + npages = current->mm->locked_vm; + + current->mm->locked_vm -= npages; + } + + pr_debug("[%d] RLIMIT_MEMLOCK KVM %c%ld %ld/%ld%s\n", current->pid, + inc ? '+' : '-', + npages << PAGE_SHIFT, + current->mm->locked_vm << PAGE_SHIFT, + rlimit(RLIMIT_MEMLOCK), + ret ? " - exceeded" : ""); + + up_write(¤t->mm->mmap_sem); + + return ret; +} + static void release_spapr_tce_table(struct rcu_head *head) { struct kvmppc_spapr_tce_table *stt = container_of(head, struct kvmppc_spapr_tce_table, rcu); int i; + long npages = kvmppc_stt_npages(stt->window_size); - for (i = 0; i < kvmppc_stt_npages(stt->window_size); i++) + for (i = 0; i < npages; i++) __free_page(stt->pages[i]); kfree(stt); @@ -89,6 +132,7 @@ static int kvm_spapr_tce_release(struct inode *inode, struct file *filp) kvm_put_kvm(stt->kvm); + kvmppc_account_memlimit(kvmppc_stt_npages(stt->window_size), false); call_rcu(&stt->rcu, release_spapr_tce_table); return 0; @@ -114,6 +158,11 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm, } npages = kvmppc_stt_npages(args->window_size); + ret = kvmppc_account_memlimit(npages, true); + if (ret) { + stt = NULL; + goto fail; + } stt = kzalloc(sizeof(*stt) + npages * sizeof(struct page *), GFP_KERNEL);
At the moment pages used for TCE tables (in addition to pages addressed by TCEs) are not counted in locked_vm counter so a malicious userspace tool can call ioctl(KVM_CREATE_SPAPR_TCE) as many times as RLIMIT_NOFILE and lock a lot of memory. This adds counting for pages used for TCE tables. This counts the number of pages required for a table plus pages for the kvmppc_spapr_tce_table struct (TCE table descriptor) itself. This does not change the amount of (de)allocated memory. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- arch/powerpc/kvm/book3s_64_vio.c | 51 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-)