* high_memory address in /proc/dri/*/vma
@ 2011-12-20 0:09 Ben Hutchings
2011-12-20 0:14 ` Kees Cook
2011-12-20 16:32 ` Daniel Vetter
0 siblings, 2 replies; 9+ messages in thread
From: Ben Hutchings @ 2011-12-20 0:09 UTC (permalink / raw)
To: Kees Cook; +Cc: Dave Airlie, dri-devel
[Re-sent to the right address, I hope.]
Kees, in commit 01e2f533a234dc62d16c0d3d4fb9d71cf1ce50c3 ("drm: do not
leak kernel addresses via /proc/dri/*/vma") you changed the logging of
high_memory:
- seq_printf(m, "vma use count: %d, high_memory = %p, 0x%08llx\n",
+ seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n",
atomic_read(&dev->vma_count),
- high_memory, (u64)virt_to_phys(high_memory));
+ high_memory, (void *)virt_to_phys(high_memory));
This doesn't make sense because the physical address may be truncated
(in theory at least).
I think it would make more sense to make this entire file readable by
root only, but I don't know whether anything depends on being able to
read it. Its existence is conditional on DRM_DEBUG_CODE != 0 but that
is always true at the moment.
Ben.
--
Ben Hutchings
Humans are not rational beings; they are rationalising beings.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: high_memory address in /proc/dri/*/vma
2011-12-20 0:09 high_memory address in /proc/dri/*/vma Ben Hutchings
@ 2011-12-20 0:14 ` Kees Cook
2011-12-20 2:18 ` Ben Hutchings
2011-12-20 16:32 ` Daniel Vetter
1 sibling, 1 reply; 9+ messages in thread
From: Kees Cook @ 2011-12-20 0:14 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Dave Airlie, dri-devel
On Mon, Dec 19, 2011 at 4:09 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> Kees, in commit 01e2f533a234dc62d16c0d3d4fb9d71cf1ce50c3 ("drm: do not
> leak kernel addresses via /proc/dri/*/vma") you changed the logging of
> high_memory:
>
> - seq_printf(m, "vma use count: %d, high_memory = %p, 0x%08llx\n",
> + seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n",
> atomic_read(&dev->vma_count),
> - high_memory, (u64)virt_to_phys(high_memory));
> + high_memory, (void *)virt_to_phys(high_memory));
>
> This doesn't make sense because the physical address may be truncated
> (in theory at least).
Leaking even a truncated address is still a problem, IMO. Or do you
mean there is some side-effect causing a problem?
> I think it would make more sense to make this entire file readable by
> root only, but I don't know whether anything depends on being able to
> read it. Its existence is conditional on DRM_DEBUG_CODE != 0 but that
> is always true at the moment.
The kptr_restrict syscall (that controls %pK behavior) has 3 modes,
including one that hides these values even from the root user, so I
would prefer this stays as-is.
Sorry I'm being dense, but what problem is %pK causing here? I'd be
happy to help get it fixed.
-Kees
--
Kees Cook
ChromeOS Security
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: high_memory address in /proc/dri/*/vma
2011-12-20 0:14 ` Kees Cook
@ 2011-12-20 2:18 ` Ben Hutchings
2011-12-20 2:35 ` Kees Cook
0 siblings, 1 reply; 9+ messages in thread
From: Ben Hutchings @ 2011-12-20 2:18 UTC (permalink / raw)
To: Kees Cook; +Cc: Dave Airlie, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1816 bytes --]
On Mon, 2011-12-19 at 16:14 -0800, Kees Cook wrote:
> On Mon, Dec 19, 2011 at 4:09 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > Kees, in commit 01e2f533a234dc62d16c0d3d4fb9d71cf1ce50c3 ("drm: do not
> > leak kernel addresses via /proc/dri/*/vma") you changed the logging of
> > high_memory:
> >
> > - seq_printf(m, "vma use count: %d, high_memory = %p, 0x%08llx\n",
> > + seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n",
> > atomic_read(&dev->vma_count),
> > - high_memory, (u64)virt_to_phys(high_memory));
> > + high_memory, (void *)virt_to_phys(high_memory));
> >
> > This doesn't make sense because the physical address may be truncated
> > (in theory at least).
>
> Leaking even a truncated address is still a problem, IMO. Or do you
> mean there is some side-effect causing a problem?
I mean that this the conversion to void * can be a narrowing conversion,
so when printing of kernel pointers is enabled the full physical address
may not be displayed.
> > I think it would make more sense to make this entire file readable by
> > root only, but I don't know whether anything depends on being able to
> > read it. Its existence is conditional on DRM_DEBUG_CODE != 0 but that
> > is always true at the moment.
>
> The kptr_restrict syscall (that controls %pK behavior) has 3 modes,
> including one that hides these values even from the root user, so I
> would prefer this stays as-is.
>
> Sorry I'm being dense, but what problem is %pK causing here? I'd be
> happy to help get it fixed.
The problem is that it is not suitable for printing physical addresses,
because they are not pointers.
Ben.
--
Ben Hutchings
Humans are not rational beings; they are rationalising beings.
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: high_memory address in /proc/dri/*/vma
2011-12-20 2:18 ` Ben Hutchings
@ 2011-12-20 2:35 ` Kees Cook
2011-12-20 4:23 ` Ben Hutchings
0 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2011-12-20 2:35 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Dave Airlie, dri-devel
On Mon, Dec 19, 2011 at 6:18 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Mon, 2011-12-19 at 16:14 -0800, Kees Cook wrote:
>> On Mon, Dec 19, 2011 at 4:09 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
>> > Kees, in commit 01e2f533a234dc62d16c0d3d4fb9d71cf1ce50c3 ("drm: do not
>> > leak kernel addresses via /proc/dri/*/vma") you changed the logging of
>> > high_memory:
>> >
>> > - seq_printf(m, "vma use count: %d, high_memory = %p, 0x%08llx\n",
>> > + seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n",
>> > atomic_read(&dev->vma_count),
>> > - high_memory, (u64)virt_to_phys(high_memory));
>> > + high_memory, (void *)virt_to_phys(high_memory));
>> >
>> > This doesn't make sense because the physical address may be truncated
>> > (in theory at least).
>>
>> Leaking even a truncated address is still a problem, IMO. Or do you
>> mean there is some side-effect causing a problem?
>
> I mean that this the conversion to void * can be a narrowing conversion,
> so when printing of kernel pointers is enabled the full physical address
> may not be displayed.
Ah-ha, okay, I see what you mean now. This is, as you say, only a
problem "in theory". Is it worth fixing currently? If so, we probably
need to add the "K" option to %x in some fashion.
-Kees
--
Kees Cook
ChromeOS Security
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: high_memory address in /proc/dri/*/vma
2011-12-20 2:35 ` Kees Cook
@ 2011-12-20 4:23 ` Ben Hutchings
0 siblings, 0 replies; 9+ messages in thread
From: Ben Hutchings @ 2011-12-20 4:23 UTC (permalink / raw)
To: Kees Cook; +Cc: Dave Airlie, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1604 bytes --]
On Mon, 2011-12-19 at 18:35 -0800, Kees Cook wrote:
> On Mon, Dec 19, 2011 at 6:18 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > On Mon, 2011-12-19 at 16:14 -0800, Kees Cook wrote:
> >> On Mon, Dec 19, 2011 at 4:09 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> >> > Kees, in commit 01e2f533a234dc62d16c0d3d4fb9d71cf1ce50c3 ("drm: do not
> >> > leak kernel addresses via /proc/dri/*/vma") you changed the logging of
> >> > high_memory:
> >> >
> >> > - seq_printf(m, "vma use count: %d, high_memory = %p, 0x%08llx\n",
> >> > + seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n",
> >> > atomic_read(&dev->vma_count),
> >> > - high_memory, (u64)virt_to_phys(high_memory));
> >> > + high_memory, (void *)virt_to_phys(high_memory));
> >> >
> >> > This doesn't make sense because the physical address may be truncated
> >> > (in theory at least).
> >>
> >> Leaking even a truncated address is still a problem, IMO. Or do you
> >> mean there is some side-effect causing a problem?
> >
> > I mean that this the conversion to void * can be a narrowing conversion,
> > so when printing of kernel pointers is enabled the full physical address
> > may not be displayed.
>
> Ah-ha, okay, I see what you mean now. This is, as you say, only a
> problem "in theory". Is it worth fixing currently?
I don't know.
> If so, we probably need to add the "K" option to %x in some fashion.
Something like that, yes.
Ben.
--
Ben Hutchings
Humans are not rational beings; they are rationalising beings.
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: high_memory address in /proc/dri/*/vma
2011-12-20 0:09 high_memory address in /proc/dri/*/vma Ben Hutchings
2011-12-20 0:14 ` Kees Cook
@ 2011-12-20 16:32 ` Daniel Vetter
2011-12-20 17:59 ` Kees Cook
2011-12-20 19:03 ` Ben Hutchings
1 sibling, 2 replies; 9+ messages in thread
From: Daniel Vetter @ 2011-12-20 16:32 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Dave Airlie, Kees Cook, dri-devel
On Tue, Dec 20, 2011 at 12:09:39AM +0000, Ben Hutchings wrote:
> [Re-sent to the right address, I hope.]
>
> Kees, in commit 01e2f533a234dc62d16c0d3d4fb9d71cf1ce50c3 ("drm: do not
> leak kernel addresses via /proc/dri/*/vma") you changed the logging of
> high_memory:
>
> - seq_printf(m, "vma use count: %d, high_memory = %p, 0x%08llx\n",
> + seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n",
> atomic_read(&dev->vma_count),
> - high_memory, (u64)virt_to_phys(high_memory));
> + high_memory, (void *)virt_to_phys(high_memory));
>
> This doesn't make sense because the physical address may be truncated
> (in theory at least).
>
> I think it would make more sense to make this entire file readable by
> root only, but I don't know whether anything depends on being able to
> read it. Its existence is conditional on DRM_DEBUG_CODE != 0 but that
> is always true at the moment.
Afaik (and I've done quite some code history checking) the proc files are
not relied upon by userspace (up to about 10 years back). Patch to kill
them all is pending and should hit either 3.3 or 3.4.
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: high_memory address in /proc/dri/*/vma
2011-12-20 16:32 ` Daniel Vetter
@ 2011-12-20 17:59 ` Kees Cook
2011-12-20 19:03 ` Ben Hutchings
1 sibling, 0 replies; 9+ messages in thread
From: Kees Cook @ 2011-12-20 17:59 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Dave Airlie, Ben Hutchings, dri-devel
On Tue, Dec 20, 2011 at 8:32 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Dec 20, 2011 at 12:09:39AM +0000, Ben Hutchings wrote:
>> [Re-sent to the right address, I hope.]
>>
>> Kees, in commit 01e2f533a234dc62d16c0d3d4fb9d71cf1ce50c3 ("drm: do not
>> leak kernel addresses via /proc/dri/*/vma") you changed the logging of
>> high_memory:
>>
>> - seq_printf(m, "vma use count: %d, high_memory = %p, 0x%08llx\n",
>> + seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n",
>> atomic_read(&dev->vma_count),
>> - high_memory, (u64)virt_to_phys(high_memory));
>> + high_memory, (void *)virt_to_phys(high_memory));
>>
>> This doesn't make sense because the physical address may be truncated
>> (in theory at least).
>>
>> I think it would make more sense to make this entire file readable by
>> root only, but I don't know whether anything depends on being able to
>> read it. Its existence is conditional on DRM_DEBUG_CODE != 0 but that
>> is always true at the moment.
>
> Afaik (and I've done quite some code history checking) the proc files are
> not relied upon by userspace (up to about 10 years back). Patch to kill
> them all is pending and should hit either 3.3 or 3.4.
That works too. :)
--
Kees Cook
ChromeOS Security
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: high_memory address in /proc/dri/*/vma
2011-12-20 16:32 ` Daniel Vetter
2011-12-20 17:59 ` Kees Cook
@ 2011-12-20 19:03 ` Ben Hutchings
1 sibling, 0 replies; 9+ messages in thread
From: Ben Hutchings @ 2011-12-20 19:03 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Dave Airlie, Kees Cook, dri-devel
On Tue, Dec 20, 2011 at 05:32:13PM +0100, Daniel Vetter wrote:
> On Tue, Dec 20, 2011 at 12:09:39AM +0000, Ben Hutchings wrote:
> > [Re-sent to the right address, I hope.]
> >
> > Kees, in commit 01e2f533a234dc62d16c0d3d4fb9d71cf1ce50c3 ("drm: do not
> > leak kernel addresses via /proc/dri/*/vma") you changed the logging of
> > high_memory:
> >
> > - seq_printf(m, "vma use count: %d, high_memory = %p, 0x%08llx\n",
> > + seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n",
> > atomic_read(&dev->vma_count),
> > - high_memory, (u64)virt_to_phys(high_memory));
> > + high_memory, (void *)virt_to_phys(high_memory));
> >
> > This doesn't make sense because the physical address may be truncated
> > (in theory at least).
> >
> > I think it would make more sense to make this entire file readable by
> > root only, but I don't know whether anything depends on being able to
> > read it. Its existence is conditional on DRM_DEBUG_CODE != 0 but that
> > is always true at the moment.
>
> Afaik (and I've done quite some code history checking) the proc files are
> not relied upon by userspace (up to about 10 years back). Patch to kill
> them all is pending and should hit either 3.3 or 3.4.
Great, that'll be one more warning gone. :-)
Ben.
--
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert Camus
^ permalink raw reply [flat|nested] 9+ messages in thread
* high_memory address in /proc/dri/*/vma
@ 2011-12-19 1:23 Ben Hutchings
0 siblings, 0 replies; 9+ messages in thread
From: Ben Hutchings @ 2011-12-19 1:23 UTC (permalink / raw)
To: Kees Cook; +Cc: Dave Airlie, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 928 bytes --]
Kees, in commit 01e2f533a234dc62d16c0d3d4fb9d71cf1ce50c3 ("drm: do not
leak kernel addresses via /proc/dri/*/vma") you changed the logging of
high_memory:
- seq_printf(m, "vma use count: %d, high_memory = %p, 0x%08llx\n",
+ seq_printf(m, "vma use count: %d, high_memory = %pK, 0x%pK\n",
atomic_read(&dev->vma_count),
- high_memory, (u64)virt_to_phys(high_memory));
+ high_memory, (void *)virt_to_phys(high_memory));
This doesn't make sense because the physical address may be truncated
(in theory at least).
I think it would make more sense to make this entire file readable by
root only, but I don't know whether anything depends on being able to
read it. Its existence is conditional on DRM_DEBUG_CODE != 0 but that
is always true at the moment.
Ben.
--
Ben Hutchings
Humans are not rational beings; they are rationalising beings.
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-12-20 19:03 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-20 0:09 high_memory address in /proc/dri/*/vma Ben Hutchings
2011-12-20 0:14 ` Kees Cook
2011-12-20 2:18 ` Ben Hutchings
2011-12-20 2:35 ` Kees Cook
2011-12-20 4:23 ` Ben Hutchings
2011-12-20 16:32 ` Daniel Vetter
2011-12-20 17:59 ` Kees Cook
2011-12-20 19:03 ` Ben Hutchings
-- strict thread matches above, loose matches on Subject: below --
2011-12-19 1:23 Ben Hutchings
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox