[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.
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
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.
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
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.
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
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. :)
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.
dri-devel@lists.freedesktop.org
-
Ben Hutchings -
Daniel Vetter -
Kees Cook