* [patch] tlb_uv: handle large snprintf() returns @ 2010-08-16 10:55 Dan Carpenter 2010-09-27 23:12 ` Andrew Morton 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2010-08-16 10:55 UTC (permalink / raw) To: Thomas Gleixner Cc: Ingo Molnar, H. Peter Anvin, x86, Cliff Wickman, Jack Steiner, Robin Holt, linux-kernel, kernel-janitors 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 <error27@gmail.com> diff --git a/arch/x86/kernel/tlb_uv.c b/arch/x86/kernel/tlb_uv.c index 312ef02..5e88b3a 100644 --- a/arch/x86/kernel/tlb_uv.c +++ b/arch/x86/kernel/tlb_uv.c @@ -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); } ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [patch] tlb_uv: handle large snprintf() returns 2010-08-16 10:55 [patch] tlb_uv: handle large snprintf() returns Dan Carpenter @ 2010-09-27 23:12 ` Andrew Morton 2010-09-29 8:41 ` [patch v2] tlb_uv: use allocated buffer in tunables_read() Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: Andrew Morton @ 2010-09-27 23:12 UTC (permalink / raw) To: Dan Carpenter Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Cliff Wickman, Jack Steiner, Robin Holt, linux-kernel, kernel-janitors On Mon, 16 Aug 2010 12:55:02 +0200 Dan Carpenter <error27@gmail.com> 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 <error27@gmail.com> > > diff --git a/arch/x86/kernel/tlb_uv.c b/arch/x86/kernel/tlb_uv.c > index 312ef02..5e88b3a 100644 > --- a/arch/x86/kernel/tlb_uv.c > +++ b/arch/x86/kernel/tlb_uv.c > @@ -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(). ^ permalink raw reply [flat|nested] 4+ messages in thread
* [patch v2] tlb_uv: use allocated buffer in tunables_read() 2010-09-27 23:12 ` Andrew Morton @ 2010-09-29 8:41 ` Dan Carpenter 2010-10-04 20:24 ` [tip:x86/uv] x86, UV: Use allocated buffer in tlb_uv.c:tunables_read() tip-bot for Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2010-09-29 8:41 UTC (permalink / raw) To: Andrew Morton Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, Cliff Wickman, Jack Steiner, Robin Holt, linux-kernel, kernel-janitors 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. Signed-off-by: Dan Carpenter <error27@gmail.com> --- 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. diff --git a/arch/x86/kernel/tlb_uv.c b/arch/x86/kernel/tlb_uv.c index 312ef02..33e77e4 100644 --- a/arch/x86/kernel/tlb_uv.c +++ b/arch/x86/kernel/tlb_uv.c @@ -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; } /* ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [tip:x86/uv] x86, UV: Use allocated buffer in tlb_uv.c:tunables_read() 2010-09-29 8:41 ` [patch v2] tlb_uv: use allocated buffer in tunables_read() Dan Carpenter @ 2010-10-04 20:24 ` tip-bot for Dan Carpenter 0 siblings, 0 replies; 4+ messages in thread From: tip-bot for Dan Carpenter @ 2010-10-04 20:24 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, hpa, mingo, holt, cpw, error27, akpm, steiner, tglx, mingo Commit-ID: b365a85c68161ea5db5476eb8845a91ceb1777ea Gitweb: http://git.kernel.org/tip/b365a85c68161ea5db5476eb8845a91ceb1777ea Author: Dan Carpenter <error27@gmail.com> AuthorDate: Wed, 29 Sep 2010 10:41:05 +0200 Committer: Ingo Molnar <mingo@elte.hu> 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. Suggested-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Dan Carpenter <error27@gmail.com> Cc: Cliff Wickman <cpw@sgi.com> Cc: Jack Steiner <steiner@sgi.com> Cc: Robin Holt <holt@sgi.com> LKML-Reference: <20100929083118.GA6376@bicker> Signed-off-by: Ingo Molnar <mingo@elte.hu> --- arch/x86/kernel/tlb_uv.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-) diff --git a/arch/x86/kernel/tlb_uv.c b/arch/x86/kernel/tlb_uv.c index 312ef02..33e77e4 100644 --- a/arch/x86/kernel/tlb_uv.c +++ b/arch/x86/kernel/tlb_uv.c @@ -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; } /* ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-10-04 20:25 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-08-16 10:55 [patch] tlb_uv: handle large snprintf() returns Dan Carpenter 2010-09-27 23:12 ` Andrew Morton 2010-09-29 8:41 ` [patch v2] tlb_uv: use allocated buffer in tunables_read() Dan Carpenter 2010-10-04 20:24 ` [tip:x86/uv] x86, UV: Use allocated buffer in tlb_uv.c:tunables_read() tip-bot for Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox