tlb_uv: handle large snprintf() returns
Subsystems (2)
PATCH 1/1 tlb_uv: handle large snprintf() returns
snprintf() returns the number of bytes that *would* have been copied if the buffer was large enough, so it can be larger than the size of the buffer. In this case it's ok, but let's put a cap on it anyway so it's easier to audit.
arch/x86/kernel/tlb_uv.c +3 -0
@@ -1012,6 +1012,9 @@ static ssize_t tunables_read(struct file *file, char __user *userbuf, timeoutsb4reset, ipi_reset_limit, complete_threshold, congested_response_us, congested_reps, congested_period); +if (ret >300 ) + ret =300 ; +return simple_read_from_buffer (userbuf, count, ppos, buf, ret); }
Quoted 12 lines · On Mon, 16 Aug 2010 12:55:02 +0200 Dan Carpenter <[email protected]> wrote:
snprintf()returns the number of bytes that *would* have been copied if the buffer was large enough, so it can be larger than the size of the buffer. In this case it's ok, but let's put a cap on it anyway so it's easier to audit.Signed-off-by Dan Carpenter <[email protected]>arch/x86/kernel/tlb_uv.c +0
-0
arch/x86/kernel/tlb_uv.c static ssize_t tunables_read(struct file *file, char __user *userbuf, · line 1019
Quoted 8 lines
arch/x86/kernel/tlb_uv.c +3
-0@@ -1012,6 +1012,9 @@ static ssize_t tunables_read(struct file *file, char __user *userbuf, timeoutsb4reset, ipi_reset_limit, complete_threshold, congested_response_us, congested_reps, congested_period); +if (ret >300 ) + ret =300 ; +return simple_read_from_buffer (userbuf, count, ppos, buf, ret); }
That 300-byte local array is yuk.
Duplicating the "300" later in the function (instead of using sizeof) is also yuk.
The code can be deyukked by using sprintf_to_user(), only we don't have one. But we do have kasprintf().
PATCH v2 1/1 tlb_uv: use allocated buffer in tunables_read()
The original code didn't check that the value returned from snprintf() was less than the size of the buffer. Although it didn't cause a runtime bug in this case, it makes the static checkers complain.
Andrew Morton suggested a dynamically sized buffer would be cleaner.
---
I don't have an x86_64 system so I haven't been able to compile this code. Sorry for that.
V2: The first version was yuk.
arch/x86/kernel/tlb_uv.c +8 -3
@@ -1001,10 +1001,10 @@ static int uv_ptc_seq_show(struct seq_file *file, void *data)static ssize_t tunables_read (struct file *file,char __user *userbuf,size_t count,loff_t *ppos) { -char buf[300 ]; +char *buf;int ret; - ret = snprintf(buf, 300, "%s %s %s\n%d %d %d %d %d %d %d %d %d\n", + buf = kasprintf(GFP_KERNEL, "%s %s %s\n%d %d %d %d %d %d %d %d %d\n", "max_bau_concurrent plugged_delay plugsb4reset", "timeoutsb4reset ipi_reset_limit complete_threshold", "congested_response_us congested_reps congested_period",
@@ -1012,7 +1012,12 @@ static ssize_t tunables_read(struct file *file, char __user *userbuf, timeoutsb4reset, ipi_reset_limit, complete_threshold, congested_response_us, congested_reps, congested_period); -return simple_read_from_buffer (userbuf, count, ppos, buf, ret); +if (!buf) +return -ENOMEM ; + + ret =simple_read_from_buffer (userbuf, count, ppos, buf,strlen (buf)); +kfree (buf); +return ret; }/*
[tip:x86/uv] x86, UV: Use allocated buffer in tlb_uv.c:tunables_read()
Commit-ID: b365a85c68161ea5db5476eb8845a91ceb1777ea Gitweb: http://git.kernel.org/tip/b365a85c68161ea5db5476eb8845a91ceb1777ea Author: Dan Carpenter <[email protected]> AuthorDate: Wed, 29 Sep 2010 10:41:05 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 30 Sep 2010 09:11:27 +0200
x86, UV: Use allocated buffer in tlb_uv.c:tunables_read()
The original code didn't check that the value returned from snprintf() was less than the size of the buffer. Although it didn't cause a runtime bug in this case, it makes the static checkers complain.
Andrew Morton suggested a dynamically sized buffer would be cleaner.
LKML-Reference: <20100929083118.GA6376@bicker>
--- arch/x86/kernel/tlb_uv.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
arch/x86/kernel/tlb_uv.c +8 -3
@@ -1001,10 +1001,10 @@ static int uv_ptc_seq_show(struct seq_file *file, void *data)static ssize_t tunables_read (struct file *file,char __user *userbuf,size_t count,loff_t *ppos) { -char buf[300 ]; +char *buf;int ret; - ret = snprintf(buf, 300, "%s %s %s\n%d %d %d %d %d %d %d %d %d\n", + buf = kasprintf(GFP_KERNEL, "%s %s %s\n%d %d %d %d %d %d %d %d %d\n", "max_bau_concurrent plugged_delay plugsb4reset", "timeoutsb4reset ipi_reset_limit complete_threshold", "congested_response_us congested_reps congested_period",
@@ -1012,7 +1012,12 @@ static ssize_t tunables_read(struct file *file, char __user *userbuf, timeoutsb4reset, ipi_reset_limit, complete_threshold, congested_response_us, congested_reps, congested_period); -return simple_read_from_buffer (userbuf, count, ppos, buf, ret); +if (!buf) +return -ENOMEM ; + + ret =simple_read_from_buffer (userbuf, count, ppos, buf,strlen (buf)); +kfree (buf); +return ret; }/*