On Mon, Jun 24, 2013 at 03:26:54PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris at chris-wilson.co.uk> writes:
>
> > It seems the request to vsnprintf will try to write to the byte past the
> > end of the maximum buffer, so trim the length by one byte.
> >
> > [76973.700434] BUG: unable to handle kernel paging request at 0000000000001000
> > [76973.700468] IP: [<ffffffff811fb84d>] strnlen+0xd/0x40
> > [76973.700494] PGD 10387b067 PUD 15411a067 PMD 0
> > [76973.700515] Oops: 0000 [#1] SMP
> > [76973.700529] Modules linked in: cpufreq_userspace cpufreq_powersave cpufreq_stats cpufreq_conservative binfmt_misc nfs lockd fscache sunrpc coretemp crc32c_intel ghash_clmulni_intel aesni_intel aes_x86_64 ablk_helper cryptd lrw gf128mul glue_helper iTCO_wdt iTCO_vendor_support microcode i2c_i801 lpc_ich mfd_core acpi_cpufreq mperf evdev processor ext4 crc16 jbd2 mbcache sd_mod crc_t10dif ahci libahci libata scsi_mod thermal fan
> > [76973.700718] CPU: 2 PID: 13279 Comm: intel_error_dec Not tainted 3.10.0-rc6+ #99
> > [76973.700746] Hardware name: Intel Corporation Shark Bay Client platform/Flathead Creek Crb, BIOS HSWLPTU1.86C.0109.R03.1301282055 01/28/2013
> > [76973.700793] task: ffff880153fa2100 ti: ffff880152310000 task.ti: ffff880152310000
> > [76973.700820] RIP: 0010:[<ffffffff811fb84d>] [<ffffffff811fb84d>] strnlen+0xd/0x40
> > [76973.700849] RSP: 0018:ffff880152311cb0 EFLAGS: 00010286
> > [76973.700869] RAX: ffffffff81774000 RBX: ffff880154000072 RCX: fffffffffffffffe
> > [76973.700895] RDX: 0000000000001000 RSI: ffffffffffffffff RDI: 0000000000001000
> > [76973.700920] RBP: ffff880152311cb0 R08: 000000000000ffff R09: 000000000000ffff
> > [76973.700948] R10: 0000000000000000 R11: 000000000000000f R12: 0000000000001000
> > [76973.700974] R13: ffff880154001000 R14: 00000000ffffffff R15: 0000000000000000
> > [76973.701000] FS: 00007f00fc942880(0000) GS:ffff88015e300000(0000) knlGS:0000000000000000
> > [76973.701029] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [76973.701050] CR2: 0000000000001000 CR3: 000000015939a000 CR4: 00000000001407e0
> > [76973.701075] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [76973.701103] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > [76973.701128] Stack:
> > [76973.701136] ffff880152311ce8 ffffffff811fdafb ffff880154000072 ffff880154001000
> > [76973.701167] ffff880152311d58 ffffffff817a703e ffffffff817a703e ffff880152311d48
> > [76973.701198] ffffffff811fe8a9 0000000a0000ffff 0000000000000001 ffff880154000000
> > [76973.701229] Call Trace:
> > [76973.701242] [<ffffffff811fdafb>] string.isra.5+0x3b/0xf0
> > [76973.701264] [<ffffffff811fe8a9>] vsnprintf+0x1d9/0x670
> > [76973.701286] [<ffffffff812d13c9>] i915_error_printf+0xb9/0x160
> > [76973.701311] [<ffffffff812d15fc>] print_error_buffers+0x18c/0x1e0
> > [76973.701335] [<ffffffff812d1a5e>] i915_error_state_read+0x40e/0x8e0
> > [76973.701361] [<ffffffff81114527>] vfs_read+0x97/0x160
> > [76973.701381] [<ffffffff81114f74>] SyS_read+0x44/0x90
> > [76973.701403] [<ffffffff8141a552>] system_call_fastpath+0x16/0x1b
> > [76973.701424] Code: c0 01 80 38 00 75 f7 48 29 f8 5d c3 31 c0 5d c3 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 48 8d 4e ff 48 85 f6 48 89 e5 74 2a <80> 3f 00 74 25 48 89 f8 31 d2 eb 10 0f 1f 80 00 00 00 00 48 83
> > [76973.701535] RIP [<ffffffff811fb84d>] strnlen+0xd/0x40
> > [76973.701550] RSP <ffff880152311cb0>
> > [76973.701559] CR2: 0000000000001000
> >
> > Regression from commit edc3d8848dc9fe2a470316363dab8ef211d77e01
> > Author: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > Date: Thu May 23 13:55:35 2013 +0300
> >
> > drm/i915: avoid big kmallocs on reading error state
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index ec64f95..c436af9 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -653,7 +653,7 @@ static const char *purgeable_flag(int purgeable)
> > static void i915_error_vprintf(struct drm_i915_error_state_buf *e,
> > const char *f, va_list args)
> > {
> > - unsigned len;
> > + int len, rem;
> >
> > if (!e->err && WARN(e->bytes > (e->size - 1), "overflow")) {
> > e->err = -ENOSPC;
> > @@ -666,6 +666,11 @@ static void i915_error_vprintf(struct drm_i915_error_state_buf *e,
> > /* Seek the first printf which is hits start position */
> > if (e->pos < e->start) {
> > len = vsnprintf(NULL, 0, f, args);
> > + if (len == -1) {
> > + e->err = -EIO;
> > + return;
> > + }
> > +
>
> -1 from vsnprintf would be broken vsnprintf. <0 would be better
> for paranoia.
Yeah, C99 says < 0 is failure, but the kernel's vsnprintf seems to never
return that.
>
> > if (e->pos + len <= e->start) {
> > e->pos += len;
> > return;
> > @@ -678,9 +683,14 @@ static void i915_error_vprintf(struct drm_i915_error_state_buf *e,
> > }
> > }
> >
> > - len = vsnprintf(e->buf + e->bytes, e->size - e->bytes, f, args);
> > - if (len >= e->size - e->bytes)
> > - len = e->size - e->bytes - 1;
>
> The returned len excludes the trailing null. Len is adjusted to omit
> the forced trailing null we got if we were truncated. So i fail to see
> how the existing code is broken in here.
>
> > + rem = e->size - e->bytes - 1;
>
> e-size - e->bytes passed to vsnprintf includes the trailing null so no
> need to adjust for it.
I'm also a bit confused about how this fixes things, since vsnprintf seems
to only ever call strnlen for the va_arg char * pointer for %s. I guess we
pass in NULL somewhere and have NULL->something at offset 0x1000. Although
exactly 1 page is a funny offset ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
