[PATCH] drm: do not leak kernel addresses via /proc/dri/*/vma
In the continuing effort to avoid kernel addresses leaking to unprivileged
users, this patch switches to %pK for /proc/dri/*/vma.
Signed-off-by: Kees Cook kees.c...@canonical.com
---
drivers/gpu/drm/drm_info.c |9 +
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
index 3cdbaf3..be9a9c0 100644
--- a/drivers/gpu/drm/drm_info.c
+++ b/drivers/gpu/drm/drm_info.c
@@ -283,17 +283,18 @@ int drm_vma_info(struct seq_file *m, void *data)
#endif
mutex_lock(dev-struct_mutex);
- 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));
list_for_each_entry(pt, dev-vmalist, head) {
vma = pt-vma;
if (!vma)
continue;
seq_printf(m,
- \n%5d 0x%08lx-0x%08lx %c%c%c%c%c%c 0x%08lx000,
- pt-pid, vma-vm_start, vma-vm_end,
+ \n%5d 0x%pK-0x%pK %c%c%c%c%c%c 0x%08lx000,
+ pt-pid,
+ (void *)vma-vm_start, (void *)vma-vm_end,
vma-vm_flags VM_READ ? 'r' : '-',
vma-vm_flags VM_WRITE ? 'w' : '-',
vma-vm_flags VM_EXEC ? 'x' : '-',
--
1.7.2.3
--
Kees Cook
Ubuntu Security Team
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: high_memory address in /proc/dri/*/vma
On Mon, Dec 19, 2011 at 4:09 PM, Ben Hutchings b...@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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: high_memory address in /proc/dri/*/vma
On Mon, Dec 19, 2011 at 6:18 PM, Ben Hutchings b...@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 b...@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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: high_memory address in /proc/dri/*/vma
On Tue, Dec 20, 2011 at 8:32 AM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Dec 20, 2011 at 12:09:39AM +, 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: use simple attribute in debugfs routines
This replaces the manual read/write routines in debugfs with the common
simple attribute helpers. Doing this gets rid of repeated copy/pasting
of copy_from_user and value formatting code.
Signed-off-by: Kees Cook keesc...@chromium.org
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_debugfs.c | 394 +--
1 file changed, 98 insertions(+), 296 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index aae3148..d86c304 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -849,76 +849,42 @@ static const struct file_operations i915_error_state_fops
= {
.release = i915_error_state_release,
};
-static ssize_t
-i915_next_seqno_read(struct file *filp,
-char __user *ubuf,
-size_t max,
-loff_t *ppos)
+static int
+i915_next_seqno_get(void *data, u64 *val)
{
- struct drm_device *dev = filp-private_data;
+ struct drm_device *dev = data;
drm_i915_private_t *dev_priv = dev-dev_private;
- char buf[80];
- int len;
int ret;
ret = mutex_lock_interruptible(dev-struct_mutex);
if (ret)
return ret;
- len = snprintf(buf, sizeof(buf),
- next_seqno : 0x%x\n,
- dev_priv-next_seqno);
-
+ *val = dev_priv-next_seqno;
mutex_unlock(dev-struct_mutex);
- if (len sizeof(buf))
- len = sizeof(buf);
-
- return simple_read_from_buffer(ubuf, max, ppos, buf, len);
+ return 0;
}
-static ssize_t
-i915_next_seqno_write(struct file *filp,
- const char __user *ubuf,
- size_t cnt,
- loff_t *ppos)
-{
- struct drm_device *dev = filp-private_data;
- char buf[20];
- u32 val = 1;
+static int
+i915_next_seqno_set(void *data, u64 val)
+{
+ struct drm_device *dev = data;
int ret;
- if (cnt 0) {
- if (cnt sizeof(buf) - 1)
- return -EINVAL;
-
- if (copy_from_user(buf, ubuf, cnt))
- return -EFAULT;
- buf[cnt] = 0;
-
- ret = kstrtouint(buf, 0, val);
- if (ret 0)
- return ret;
- }
-
ret = mutex_lock_interruptible(dev-struct_mutex);
if (ret)
return ret;
ret = i915_gem_set_seqno(dev, val);
-
mutex_unlock(dev-struct_mutex);
- return ret ?: cnt;
+ return ret;
}
-static const struct file_operations i915_next_seqno_fops = {
- .owner = THIS_MODULE,
- .open = simple_open,
- .read = i915_next_seqno_read,
- .write = i915_next_seqno_write,
- .llseek = default_llseek,
-};
+DEFINE_SIMPLE_ATTRIBUTE(i915_next_seqno_fops,
+ i915_next_seqno_get, i915_next_seqno_set,
+ next_seqno : 0x%llx\n);
static int i915_rstdby_delays(struct seq_file *m, void *unused)
{
@@ -1680,105 +1646,51 @@ static int i915_dpio_info(struct seq_file *m, void
*data)
return 0;
}
-static ssize_t
-i915_wedged_read(struct file *filp,
-char __user *ubuf,
-size_t max,
-loff_t *ppos)
+static int
+i915_wedged_get(void *data, u64 *val)
{
- struct drm_device *dev = filp-private_data;
+ struct drm_device *dev = data;
drm_i915_private_t *dev_priv = dev-dev_private;
- char buf[80];
- int len;
- len = snprintf(buf, sizeof(buf),
- wedged : %d\n,
- atomic_read(dev_priv-gpu_error.reset_counter));
+ *val = atomic_read(dev_priv-gpu_error.reset_counter);
- if (len sizeof(buf))
- len = sizeof(buf);
-
- return simple_read_from_buffer(ubuf, max, ppos, buf, len);
+ return 0;
}
-static ssize_t
-i915_wedged_write(struct file *filp,
- const char __user *ubuf,
- size_t cnt,
- loff_t *ppos)
+static int
+i915_wedged_set(void *data, u64 val)
{
- struct drm_device *dev = filp-private_data;
- char buf[20];
- int val = 1;
-
- if (cnt 0) {
- if (cnt sizeof(buf) - 1)
- return -EINVAL;
+ struct drm_device *dev = data;
- if (copy_from_user(buf, ubuf, cnt))
- return -EFAULT;
- buf[cnt] = 0;
-
- val = simple_strtoul(buf, NULL, 0);
- }
-
- DRM_INFO(Manually setting wedged to %d\n, val);
+ DRM_INFO(Manually setting wedged to %llu\n, val);
i915_handle_error(dev, val);
- return cnt;
+ return 0;
}
-static const struct file_operations i915_wedged_fops = {
- .owner = THIS_MODULE,
- .open = simple_open,
- .read = i915_wedged_read,
- .write = i915_wedged_write
[PATCH] drm/i915: restrict kernel address leak in debugfs
Masks kernel address info-leak in object dumps with the %pK suffix,
so they cannot be used to target kernel memory corruption attacks if
the kptr_restrict sysctl is set.
Signed-off-by: Kees Cook keesc...@chromium.org
Cc: sta...@vger.kernel.org
---
drivers/gpu/drm/i915/i915_debugfs.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index aae3148..7299ea4 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -103,7 +103,7 @@ static const char *cache_level_str(int type)
static void
describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
{
- seq_printf(m, %p: %s%s %8zdKiB %02x %02x %d %d %d%s%s%s,
+ seq_printf(m, %pK: %s%s %8zdKiB %02x %02x %d %d %d%s%s%s,
obj-base,
get_pin_flag(obj),
get_tiling_flag(obj),
--
1.7.9.5
--
Kees Cook
Chrome OS Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: clarify reasoning for the access_ok call
This clarifies the comment above the access_ok check so a missing VERIFY_READ doesn't alarm anyone. Signed-off-by: Kees Cook keesc...@chromium.org Cc: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_execbuffer.c |6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 2f2daeb..752e399 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -747,7 +747,11 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec, length = exec[i].relocation_count * sizeof(struct drm_i915_gem_relocation_entry); - /* we may also need to update the presumed offsets */ + /* +* Validate memory range. Since we may need to update the +* presumed offsets, use VERIFY_WRITE. (Writable implies +* readable.) +*/ if (!access_ok(VERIFY_WRITE, ptr, length)) return -EFAULT; -- 1.7.9.5 -- Kees Cook Chrome OS Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/radeon: check incoming cliprects pointer
The boxes parameter points into userspace memory. It should be verified
like any other operation against user memory.
Signed-off-by: Kees Cook keesc...@chromium.org
Cc: sta...@vger.kernel.org
---
drivers/gpu/drm/radeon/r300_cmdbuf.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/r300_cmdbuf.c
b/drivers/gpu/drm/radeon/r300_cmdbuf.c
index 865e2c9..60170ea 100644
--- a/drivers/gpu/drm/radeon/r300_cmdbuf.c
+++ b/drivers/gpu/drm/radeon/r300_cmdbuf.c
@@ -75,7 +75,7 @@ static int r300_emit_cliprects(drm_radeon_private_t *dev_priv,
OUT_RING(CP_PACKET0(R300_RE_CLIPRECT_TL_0, nr * 2 - 1));
for (i = 0; i nr; ++i) {
- if (DRM_COPY_FROM_USER_UNCHECKED
+ if (DRM_COPY_FROM_USER
(box, cmdbuf-boxes[n + i], sizeof(box))) {
DRM_ERROR(copy cliprect faulted\n);
return -EFAULT;
--
1.7.9.5
--
Kees Cook
Chrome OS Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: clarify reasoning for the access_ok call
On Mon, Mar 11, 2013 at 1:51 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Mon, Mar 11, 2013 at 12:26:30PM -0700, Kees Cook wrote: This clarifies the comment above the access_ok check so a missing VERIFY_READ doesn't alarm anyone. Do we really need to copy the interface documentation? /** * access_ok: - Checks if a user space pointer is valid * @type: Type of access: %VERIFY_READ or %VERIFY_WRITE. Note that *%VERIFY_WRITE is a superset of %VERIFY_READ - if it is safe *to write to a block, it is always safe to read from it. * @addr: User space pointer to start of block to check * @size: Size of block to check */ -Chris Probably not. It just seemed like the existing comment was insufficient after the removal of the redundant VERIFY_READ check that happened recently. -Kees -- Kees Cook Chrome OS Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: bounds check execbuffer relocations
On Mon, Mar 11, 2013 at 1:52 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Mon, Mar 11, 2013 at 12:27:16PM -0700, Kees Cook wrote: It is possible to wrap the counter used to allocate the buffer for relocation copies. This could lead to heap writing overflows. Seems a sensible check, just in the wrong location. You need to do the checking upfront in validate_exec_list() so that the error condition is always hit and that the limits are applied consistently to all execbuffers. I opted for it here because it kept it out of the fast path which didn't need this check (it uses a list rather than an array). I will move it to validate_exec_list(). Thanks! -Kees -- Kees Cook Chrome OS Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/i915: bounds check execbuffer relocation count
It is possible to wrap the counter used to allocate the buffer for
relocation copies. This could lead to heap writing overflows.
Signed-off-by: Kees Cook keesc...@chromium.org
Reported-by: Pinkie Pie
Cc: sta...@vger.kernel.org
---
v2:
- move check into validate_exec_list
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c |4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 752e399..72d7841 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -732,6 +732,7 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
int count)
{
int i;
+ int total = 0;
for (i = 0; i count; i++) {
char __user *ptr = (char __user *)(uintptr_t)exec[i].relocs_ptr;
@@ -744,6 +745,9 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
if (exec[i].relocation_count
INT_MAX / sizeof(struct drm_i915_gem_relocation_entry))
return -EINVAL;
+ if (exec[i].relocation_count INT_MAX - total)
+ return -ENOMEM;
+ total += exec[i].relocation_count;
length = exec[i].relocation_count *
sizeof(struct drm_i915_gem_relocation_entry);
--
1.7.9.5
--
Kees Cook
Chrome OS Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/i915: clarify reasoning for the access_ok call
This clarifies the comment above the access_ok check so a missing VERIFY_READ doesn't alarm anyone. Signed-off-by: Kees Cook keesc...@chromium.org Cc: Daniel Vetter daniel.vet...@ffwll.ch --- v2: - rewrote comment, thanks to Chris Wilson --- drivers/gpu/drm/i915/i915_gem_execbuffer.c |6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index bf7ceca..89c4039 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -751,7 +751,11 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec, length = exec[i].relocation_count * sizeof(struct drm_i915_gem_relocation_entry); - /* we may also need to update the presumed offsets */ + /* +* We must check that the entire relocation array is safe +* to read, but since we may need to update the presumed +* offsets during execution, check for full write access. +*/ if (!access_ok(VERIFY_WRITE, ptr, length)) return -EFAULT; -- 1.7.9.5 -- Kees Cook Chrome OS Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/i915: bounds check execbuffer relocation count
On Mon, Mar 11, 2013 at 3:00 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Mon, Mar 11, 2013 at 02:23:29PM -0700, Kees Cook wrote: It is possible to wrap the counter used to allocate the buffer for relocation copies. This could lead to heap writing overflows. I'd keep the return value as EINVAL so that we can continue to distinguish between the user passing garbage and hitting an oom. And total_relocs is preferrable to total, which also leads us to think more carefully about the error condition. I think the check should be against INT_MAX / sizeof(struct reloc_entry) for consistency with our other guard against overflows whilst allocating. I've ended up with this: int max_alloc = INT_MAX / sizeof(struct drm_i915_gem_relocation_entry); ... /* First check for malicious input causing overflow */ if (exec[i].relocation_count max_alloc) return -EINVAL; if (exec[i].relocation_count max_alloc - total_relocs) return -EINVAL; total_relocs += exec[i].relocation_count; And looking at that, I wonder if we should just eliminate the first if entirely? -Kees -- Kees Cook Chrome OS Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: use simple attribute in debugfs routines
On Mon, Mar 11, 2013 at 4:03 PM, Daniel Vetter dan...@ffwll.ch wrote: On Sun, Mar 10, 2013 at 02:10:06PM -0700, Kees Cook wrote: This replaces the manual read/write routines in debugfs with the common simple attribute helpers. Doing this gets rid of repeated copy/pasting of copy_from_user and value formatting code. Signed-off-by: Kees Cook keesc...@chromium.org Cc: Daniel Vetter daniel.vet...@ffwll.ch Queued for -next, thanks for the patch. When sending drm/i915 patches, please also cc intel-...@lists.freedesktop.org (it's open for non-subscribers non without nagging you're mail is in the moderation queue replies). Ah-ha, sure. Should MAINTAINERS be updated to reflect this? It got skipped as a destination due to the subscribers-only suffix. -Kees -- Kees Cook Chrome OS Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3] drm/i915: bounds check execbuffer relocation count
It is possible to wrap the counter used to allocate the buffer for
relocation copies. This could lead to heap writing overflows.
CVE-2013-0913
v3: collapse test, improve comment
v2: move check into validate_exec_list
Signed-off-by: Kees Cook keesc...@chromium.org
Reported-by: Pinkie Pie
Cc: sta...@vger.kernel.org
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index b3a40ee..094ba41 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -732,6 +732,8 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
int count)
{
int i;
+ int relocs_total = 0;
+ int relocs_max = INT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
for (i = 0; i count; i++) {
char __user *ptr = (char __user *)(uintptr_t)exec[i].relocs_ptr;
@@ -740,10 +742,13 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
if (exec[i].flags __EXEC_OBJECT_UNKNOWN_FLAGS)
return -EINVAL;
- /* First check for malicious input causing overflow */
- if (exec[i].relocation_count
- INT_MAX / sizeof(struct drm_i915_gem_relocation_entry))
+ /* First check for malicious input causing overflow in
+* the worst case where we need to allocate the entire
+* relocation tree as a single array.
+*/
+ if (exec[i].relocation_count relocs_max - relocs_total)
return -EINVAL;
+ relocs_total += exec[i].relocation_count;
length = exec[i].relocation_count *
sizeof(struct drm_i915_gem_relocation_entry);
--
1.7.9.5
--
Kees Cook
Chrome OS Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: use do_div() as needed in debugfs code
This replaces the open-coded divisions in the debugfs code by calls to do_div(). Signed-off-by: Kees Cook keesc...@chromium.org Cc: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_debugfs.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index d86c304..6f3cbf8 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1814,9 +1814,9 @@ i915_max_freq_set(void *data, u64 val) /* * Turbo will still be enabled, but won't go above the set value. */ - dev_priv-rps.max_delay = val / GT_FREQUENCY_MULTIPLIER; - - gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER); + do_div(val, GT_FREQUENCY_MULTIPLIER); + dev_priv-rps.max_delay = val; + gen6_set_rps(dev, val); mutex_unlock(dev_priv-rps.hw_lock); return 0; @@ -1865,9 +1865,9 @@ i915_min_freq_set(void *data, u64 val) /* * Turbo will still be enabled, but won't go below the set value. */ - dev_priv-rps.min_delay = val / GT_FREQUENCY_MULTIPLIER; - - gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER); + do_div(val, GT_FREQUENCY_MULTIPLIER); + dev_priv-rps.min_delay = val; + gen6_set_rps(dev, val); mutex_unlock(dev_priv-rps.hw_lock); return 0; -- 1.7.9.5 -- Kees Cook Chrome OS Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: linux-next: build failure after merge of the final tree (drm-intel tree related)
On Mon, Mar 11, 2013 at 9:22 PM, Stephen Rothwell s...@canb.auug.org.au wrote: Hi all, After merging the final tree, today's linux-next build (i386 defconfig) failed like this: drivers/built-in.o: In function `i915_min_freq_set': i915_debugfs.c:(.text+0xb1adc): undefined reference to `__udivdi3' drivers/built-in.o: In function `i915_max_freq_set': i915_debugfs.c:(.text+0xb1bac): undefined reference to `__udivdi3' Caused by commit 2389cc500686 (drm/i915: use simple attribute in debugfs routines) from the drm-intel tree. I have reverted that commit for today. Ah-ha, thanks. I've sent a follow-up patch to fix this. -Kees -- Kees Cook Chrome OS Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] drm/i915: bounds check execbuffer relocation count
On Thu, Mar 14, 2013 at 9:57 AM, Daniel Vetter daniel.vet...@ffwll.ch wrote: On Wed, Mar 13, 2013 at 9:28 PM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Mar 12, 2013 at 09:07:46AM +, Chris Wilson wrote: On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: It is possible to wrap the counter used to allocate the buffer for relocation copies. This could lead to heap writing overflows. CVE-2013-0913 v3: collapse test, improve comment v2: move check into validate_exec_list Signed-off-by: Kees Cook keesc...@chromium.org Reported-by: Pinkie Pie Cc: sta...@vger.kernel.org Looks good to me. The only bikeshed that remains is whether we should just collapse the two variables into one, but the current 'max - count' is more idiomatic and so preferrable. Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Picked up for -fixes, thanks for the patch. I've forgotten to dump my wishlist: Can I have an i-g-t for this? For this bug here specifically an execbuf with just one buffer with too many relocs plus another execbuf with two buffers with relocation so that the 2nd relocation list will overflow should be sufficient. Sure thing. Where do these live? (Or what docs should I read for this?) I'm assuming i-g-t means intel graphics test? :) -Kees -- Kees Cook Chrome OS Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: refactor call to request_module
This reduces the size of the stack frame when calling request_module().
Performing the sprintf before the call is not needed.
Signed-off-by: Kees Cook keesc...@chromium.org
---
drivers/gpu/drm/drm_encoder_slave.c |6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_encoder_slave.c
b/drivers/gpu/drm/drm_encoder_slave.c
index 48c52f7..0cfb60f 100644
--- a/drivers/gpu/drm/drm_encoder_slave.c
+++ b/drivers/gpu/drm/drm_encoder_slave.c
@@ -54,16 +54,12 @@ int drm_i2c_encoder_init(struct drm_device *dev,
struct i2c_adapter *adap,
const struct i2c_board_info *info)
{
- char modalias[sizeof(I2C_MODULE_PREFIX)
- + I2C_NAME_SIZE];
struct module *module = NULL;
struct i2c_client *client;
struct drm_i2c_encoder_driver *encoder_drv;
int err = 0;
- snprintf(modalias, sizeof(modalias),
-%s%s, I2C_MODULE_PREFIX, info-type);
- request_module(modalias);
+ request_module(%s%s, I2C_MODULE_PREFIX, info-type);
client = i2c_new_device(adap, info);
if (!client) {
--
1.7.9.5
--
Kees Cook
Chrome OS Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: refactor call to request_module
On Wed, May 8, 2013 at 12:22 AM, Paul Menzel paulepan...@users.sourceforge.net wrote: Am Dienstag, den 07.05.2013, 12:32 -0700 schrieb Kees Cook: This reduces the size of the stack frame when calling request_module(). Performing the sprintf before the call is not needed. Good fine. Do you have any hard numbers for the record? Did you find this just by reading the code or are there any problems with stack sizes on some systems? (This patch would be good alone for decreasing the number of code lines. ;-)) No hard numbers; I just saw it while reviewing callers to request_module(). :) Signed-off-by: Kees Cook keesc...@chromium.org --- drivers/gpu/drm/drm_encoder_slave.c |6 +- 1 file changed, 1 insertion(+), 5 deletions(-) Acked-by: Paul Menzel paulepan...@users.sourceforge.net Thanks! -Kees -- Kees Cook Chrome OS Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[RESEND][PATCH] drm/radeon: check incoming cliprects pointer
The boxes parameter points into userspace memory. It should be verified
like any other operation against user memory.
Signed-off-by: Kees Cook keesc...@chromium.org
Cc: sta...@vger.kernel.org
---
drivers/gpu/drm/radeon/r300_cmdbuf.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/r300_cmdbuf.c
b/drivers/gpu/drm/radeon/r300_cmdbuf.c
index 865e2c9..60170ea 100644
--- a/drivers/gpu/drm/radeon/r300_cmdbuf.c
+++ b/drivers/gpu/drm/radeon/r300_cmdbuf.c
@@ -75,7 +75,7 @@ static int r300_emit_cliprects(drm_radeon_private_t *dev_priv,
OUT_RING(CP_PACKET0(R300_RE_CLIPRECT_TL_0, nr * 2 - 1));
for (i = 0; i nr; ++i) {
- if (DRM_COPY_FROM_USER_UNCHECKED
+ if (DRM_COPY_FROM_USER
(box, cmdbuf-boxes[n + i], sizeof(box))) {
DRM_ERROR(copy cliprect faulted\n);
return -EFAULT;
--
1.7.9.5
--
Kees Cook
Chrome OS Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
i915: build warning in i915_hangcheck_elapsed
9107e9d227e3b0893829baee4ac59feb874d4c23 introduced score, and while it's actually not possible for it to be uninitialized (given the return values of ring_stuck()), the warning should be suppressed. It probably just needs a default: added to the switch statement. drivers/gpu/drm/i915/i915_irq.c:1943:27: warning: ‘score’ may be used uninitialized in this function [-Wuninitialized] -Kees -- Kees Cook Chrome OS Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] tree-wide: replace config_enabled() with IS_ENABLED()
On Mon, Jun 6, 2016 at 5:20 AM, Masahiro Yamada wrote: > The use of config_enabled() against config options is ambiguous. > In practical terms, config_enabled() is equivalent to IS_BUILTIN(), > but the author might have used it for the meaning of IS_ENABLED(). > Using IS_ENABLED(), IS_BUILTIN(), IS_MODULE() etc. makes the > intention clearer. > > This commit replaces config_enabled() with IS_ENABLED() where > possible. This commit is only touching bool config options. > > I noticed two cases where config_enabled() is used against a tristate > option: > > - config_enabled(CONFIG_HWMON) > [ drivers/net/wireless/ath/ath10k/thermal.c ] > > - config_enabled(CONFIG_BACKLIGHT_CLASS_DEVICE) > [ drivers/gpu/drm/gma500/opregion.c ] > > I did not touch them because they should be converted to IS_BUILTIN() > in order to keep the logic, but I was not sure it was the authors' > intention. > > Signed-off-by: Masahiro Yamada Sounds good to me! :) Acked-by: Kees Cook -- Kees Cook Chrome OS & Brillo Security
[PATCH] drm/amdgpu: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c| 84 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c| 10 +--
.../drm/amd/powerplay/hwmgr/cz_clockpowergating.c | 12 +++-
drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c | 58 +++
drivers/gpu/drm/amd/powerplay/hwmgr/smu7_thermal.c | 22 +++---
5 files changed, 96 insertions(+), 90 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
index 9ada56c16a58..fd1ee40c41a2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
@@ -1197,51 +1197,51 @@ static int amdgpu_cgs_call_acpi_method(struct
cgs_device *cgs_device,
}
static const struct cgs_ops amdgpu_cgs_ops = {
- amdgpu_cgs_gpu_mem_info,
- amdgpu_cgs_gmap_kmem,
- amdgpu_cgs_gunmap_kmem,
- amdgpu_cgs_alloc_gpu_mem,
- amdgpu_cgs_free_gpu_mem,
- amdgpu_cgs_gmap_gpu_mem,
- amdgpu_cgs_gunmap_gpu_mem,
- amdgpu_cgs_kmap_gpu_mem,
- amdgpu_cgs_kunmap_gpu_mem,
- amdgpu_cgs_read_register,
- amdgpu_cgs_write_register,
- amdgpu_cgs_read_ind_register,
- amdgpu_cgs_write_ind_register,
- amdgpu_cgs_read_pci_config_byte,
- amdgpu_cgs_read_pci_config_word,
- amdgpu_cgs_read_pci_config_dword,
- amdgpu_cgs_write_pci_config_byte,
- amdgpu_cgs_write_pci_config_word,
- amdgpu_cgs_write_pci_config_dword,
- amdgpu_cgs_get_pci_resource,
- amdgpu_cgs_atom_get_data_table,
- amdgpu_cgs_atom_get_cmd_table_revs,
- amdgpu_cgs_atom_exec_cmd_table,
- amdgpu_cgs_create_pm_request,
- amdgpu_cgs_destroy_pm_request,
- amdgpu_cgs_set_pm_request,
- amdgpu_cgs_pm_request_clock,
- amdgpu_cgs_pm_request_engine,
- amdgpu_cgs_pm_query_clock_limits,
- amdgpu_cgs_set_camera_voltages,
- amdgpu_cgs_get_firmware_info,
- amdgpu_cgs_rel_firmware,
- amdgpu_cgs_set_powergating_state,
- amdgpu_cgs_set_clockgating_state,
- amdgpu_cgs_get_active_displays_info,
- amdgpu_cgs_notify_dpm_enabled,
- amdgpu_cgs_call_acpi_method,
- amdgpu_cgs_query_system_info,
- amdgpu_cgs_is_virtualization_enabled
+ .gpu_mem_info = amdgpu_cgs_gpu_mem_info,
+ .gmap_kmem = amdgpu_cgs_gmap_kmem,
+ .gunmap_kmem = amdgpu_cgs_gunmap_kmem,
+ .alloc_gpu_mem = amdgpu_cgs_alloc_gpu_mem,
+ .free_gpu_mem = amdgpu_cgs_free_gpu_mem,
+ .gmap_gpu_mem = amdgpu_cgs_gmap_gpu_mem,
+ .gunmap_gpu_mem = amdgpu_cgs_gunmap_gpu_mem,
+ .kmap_gpu_mem = amdgpu_cgs_kmap_gpu_mem,
+ .kunmap_gpu_mem = amdgpu_cgs_kunmap_gpu_mem,
+ .read_register = amdgpu_cgs_read_register,
+ .write_register = amdgpu_cgs_write_register,
+ .read_ind_register = amdgpu_cgs_read_ind_register,
+ .write_ind_register = amdgpu_cgs_write_ind_register,
+ .read_pci_config_byte = amdgpu_cgs_read_pci_config_byte,
+ .read_pci_config_word = amdgpu_cgs_read_pci_config_word,
+ .read_pci_config_dword = amdgpu_cgs_read_pci_config_dword,
+ .write_pci_config_byte = amdgpu_cgs_write_pci_config_byte,
+ .write_pci_config_word = amdgpu_cgs_write_pci_config_word,
+ .write_pci_config_dword = amdgpu_cgs_write_pci_config_dword,
+ .get_pci_resource = amdgpu_cgs_get_pci_resource,
+ .atom_get_data_table = amdgpu_cgs_atom_get_data_table,
+ .atom_get_cmd_table_revs = amdgpu_cgs_atom_get_cmd_table_revs,
+ .atom_exec_cmd_table = amdgpu_cgs_atom_exec_cmd_table,
+ .create_pm_request = amdgpu_cgs_create_pm_request,
+ .destroy_pm_request = amdgpu_cgs_destroy_pm_request,
+ .set_pm_request = amdgpu_cgs_set_pm_request,
+ .pm_request_clock = amdgpu_cgs_pm_request_clock,
+ .pm_request_engine = amdgpu_cgs_pm_request_engine,
+ .pm_query_clock_limits = amdgpu_cgs_pm_query_clock_limits,
+ .set_camera_voltages = amdgpu_cgs_set_camera_voltages,
+ .get_firmware_info = amdgpu_cgs_get_firmware_info,
+ .rel_firmware = amdgpu_cgs_rel_firmware,
+ .set_powergating_state = amdgpu_cgs_set_powergating_state,
+ .set_clockgating_state = amdgpu_cgs_set_clockgating_state,
+ .get_active_displays_info = amdgpu_cgs_get_active_displays_info,
+ .notify_dpm_enabled = amdgpu_cgs_notify_dpm_enabled,
+ .call_acpi_method = amdgpu_cgs_call_acpi_method,
+ .query_system_info = amdgpu_cgs_query_system_info,
+ .is_virtualization_enabled = amdgpu_cgs_is_virtualization_enabled,
};
static const struct cgs_os_ops amdgpu_cgs_os_ops = {
- amdgpu_cgs_add_irq_source,
- amdgpu_cgs_irq_get,
- amdgpu_cgs_irq_put
[PATCH] drm/ttm: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/virtio/virtgpu_ttm.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c
b/drivers/gpu/drm/virtio/virtgpu_ttm.c
index 4a1de9f81193..63b3d5d35cf6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
@@ -198,11 +198,11 @@ static void ttm_bo_man_debug(struct ttm_mem_type_manager
*man,
}
static const struct ttm_mem_type_manager_func virtio_gpu_bo_manager_func = {
- ttm_bo_man_init,
- ttm_bo_man_takedown,
- ttm_bo_man_get_node,
- ttm_bo_man_put_node,
- ttm_bo_man_debug
+ .init = ttm_bo_man_init,
+ .takedown = ttm_bo_man_takedown,
+ .get_node = ttm_bo_man_get_node,
+ .put_node = ttm_bo_man_put_node,
+ .debug = ttm_bo_man_debug
};
static int virtio_gpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
--
2.7.4
--
Kees Cook
Nexus Security
[PATCH] drm/ttm: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/ttm/ttm_bo_manager.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c
b/drivers/gpu/drm/ttm/ttm_bo_manager.c
index aa0bd054d3e9..aea6a01500e1 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_manager.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_manager.c
@@ -148,10 +148,10 @@ static void ttm_bo_man_debug(struct ttm_mem_type_manager
*man,
}
const struct ttm_mem_type_manager_func ttm_bo_manager_func = {
- ttm_bo_man_init,
- ttm_bo_man_takedown,
- ttm_bo_man_get_node,
- ttm_bo_man_put_node,
- ttm_bo_man_debug
+ .init = ttm_bo_man_init,
+ .takedown = ttm_bo_man_takedown,
+ .get_node = ttm_bo_man_get_node,
+ .put_node = ttm_bo_man_put_node,
+ .debug = ttm_bo_man_debug
};
EXPORT_SYMBOL(ttm_bo_manager_func);
--
2.7.4
--
Kees Cook
Nexus Security
[PATCH] drm/nouveau: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/nouveau/nouveau_ttm.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index a6dbe8258040..ec4668a41e01 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -107,10 +107,10 @@ nouveau_vram_manager_new(struct ttm_mem_type_manager *man,
}
const struct ttm_mem_type_manager_func nouveau_vram_manager = {
- nouveau_vram_manager_init,
- nouveau_vram_manager_fini,
- nouveau_vram_manager_new,
- nouveau_vram_manager_del,
+ .init = nouveau_vram_manager_init,
+ .takedown = nouveau_vram_manager_fini,
+ .get_node = nouveau_vram_manager_new,
+ .put_node = nouveau_vram_manager_del,
};
static int
@@ -184,11 +184,11 @@ nouveau_gart_manager_debug(struct ttm_mem_type_manager
*man, const char *prefix)
}
const struct ttm_mem_type_manager_func nouveau_gart_manager = {
- nouveau_gart_manager_init,
- nouveau_gart_manager_fini,
- nouveau_gart_manager_new,
- nouveau_gart_manager_del,
- nouveau_gart_manager_debug
+ .init = nouveau_gart_manager_init,
+ .takedown = nouveau_gart_manager_fini,
+ .get_node = nouveau_gart_manager_new,
+ .put_node = nouveau_gart_manager_del,
+ .debug = nouveau_gart_manager_debug
};
/*XXX*/
@@ -257,11 +257,11 @@ nv04_gart_manager_debug(struct ttm_mem_type_manager *man,
const char *prefix)
}
const struct ttm_mem_type_manager_func nv04_gart_manager = {
- nv04_gart_manager_init,
- nv04_gart_manager_fini,
- nv04_gart_manager_new,
- nv04_gart_manager_del,
- nv04_gart_manager_debug
+ .init = nv04_gart_manager_init,
+ .takedown = nv04_gart_manager_fini,
+ .get_node = nv04_gart_manager_new,
+ .put_node = nv04_gart_manager_del,
+ .debug = nv04_gart_manager_debug
};
int
--
2.7.4
--
Kees Cook
Nexus Security
[PATCH] drm/vmwgfx: use designated initializers
Prepare to mark sensitive kernel structures for randomization by making
sure they're using designated initializers. These were identified during
allyesconfig builds of x86, arm, and arm64, with most initializer fixes
extracted from grsecurity.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
index 170b61be1e4e..fec7348cea2c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c
@@ -164,9 +164,9 @@ static void vmw_gmrid_man_debug(struct ttm_mem_type_manager
*man,
}
const struct ttm_mem_type_manager_func vmw_gmrid_manager_func = {
- vmw_gmrid_man_init,
- vmw_gmrid_man_takedown,
- vmw_gmrid_man_get_node,
- vmw_gmrid_man_put_node,
- vmw_gmrid_man_debug
+ .init = vmw_gmrid_man_init,
+ .takedown = vmw_gmrid_man_takedown,
+ .get_node = vmw_gmrid_man_get_node,
+ .put_node = vmw_gmrid_man_put_node,
+ .debug = vmw_gmrid_man_debug
};
--
2.7.4
--
Kees Cook
Nexus Security
[PATCH] drm/ttm: use designated initializers
On Sun, Dec 18, 2016 at 10:53 PM, Alexander Stein wrote: > Hello Kees, > > While understanding what your patches (I've seen the other ones as well) do > themself, I still don't get what your intention is, e.g. why you need this? > Apart from a better readability. > > On Friday 16 December 2016 16:59:29, Kees Cook wrote: >> Prepare to mark sensitive kernel structures for randomization by making >> sure they're using designated initializers. > > Can you please elaborate what you mean with that sentence? Hi! Sure: the coming (and optional) gcc plugin "randstruct" performs structure layout randomization, which means that static initializers cannot be "ordered" (they must be "designated"), since the place were layout randomization occurs happens separate from how static initializers are applied. This change from ordered to designated is just to help the compiler do the right thing. -Kees -- Kees Cook Nexus Security
[PATCH] drm/ttm: make sure format string cannot leak in
While zone->name is currently hard coded, the call to kobject_init_and_add()
should follow the more defensive argument list usage (as already done in
other places in ttm_memory.c) where "%s" is used instead of directly passing
in a variable as a format string.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/ttm/ttm_memory.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/ttm/ttm_memory.c b/drivers/gpu/drm/ttm/ttm_memory.c
index fa53df487875..1e688b603e46 100644
--- a/drivers/gpu/drm/ttm/ttm_memory.c
+++ b/drivers/gpu/drm/ttm/ttm_memory.c
@@ -300,7 +300,8 @@ static int ttm_mem_init_highmem_zone(struct ttm_mem_global
*glob,
zone->glob = glob;
glob->zone_highmem = zone;
ret = kobject_init_and_add(
- >kobj, _mem_zone_kobj_type, >kobj, zone->name);
+ >kobj, _mem_zone_kobj_type, >kobj, "%s",
+ zone->name);
if (unlikely(ret != 0)) {
kobject_put(>kobj);
return ret;
--
1.9.1
--
Kees Cook
Chrome OS Security
[PATCH] drm: refactor call to request_module
This reduces the size of the stack frame when calling request_module().
Performing the sprintf before the call is not needed.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/drm_encoder_slave.c |6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_encoder_slave.c
b/drivers/gpu/drm/drm_encoder_slave.c
index 48c52f7..0cfb60f 100644
--- a/drivers/gpu/drm/drm_encoder_slave.c
+++ b/drivers/gpu/drm/drm_encoder_slave.c
@@ -54,16 +54,12 @@ int drm_i2c_encoder_init(struct drm_device *dev,
struct i2c_adapter *adap,
const struct i2c_board_info *info)
{
- char modalias[sizeof(I2C_MODULE_PREFIX)
- + I2C_NAME_SIZE];
struct module *module = NULL;
struct i2c_client *client;
struct drm_i2c_encoder_driver *encoder_drv;
int err = 0;
- snprintf(modalias, sizeof(modalias),
-"%s%s", I2C_MODULE_PREFIX, info->type);
- request_module(modalias);
+ request_module("%s%s", I2C_MODULE_PREFIX, info->type);
client = i2c_new_device(adap, info);
if (!client) {
--
1.7.9.5
--
Kees Cook
Chrome OS Security
[PATCH] drm: refactor call to request_module
On Wed, May 8, 2013 at 12:22 AM, Paul Menzel wrote: > Am Dienstag, den 07.05.2013, 12:32 -0700 schrieb Kees Cook: >> This reduces the size of the stack frame when calling request_module(). >> Performing the sprintf before the call is not needed. > > Good fine. Do you have any hard numbers for the record? > > Did you find this just by reading the code or are there any problems > with stack sizes on some systems? > > (This patch would be good alone for decreasing the number of code > lines. ;-)) No hard numbers; I just saw it while reviewing callers to request_module(). :) >> Signed-off-by: Kees Cook >> --- >> drivers/gpu/drm/drm_encoder_slave.c |6 +- >> 1 file changed, 1 insertion(+), 5 deletions(-) > > Acked-by: Paul Menzel Thanks! -Kees -- Kees Cook Chrome OS Security
[RESEND][PATCH] drm/radeon: check incoming cliprects pointer
The "boxes" parameter points into userspace memory. It should be verified
like any other operation against user memory.
Signed-off-by: Kees Cook
Cc: stable at vger.kernel.org
---
drivers/gpu/drm/radeon/r300_cmdbuf.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/r300_cmdbuf.c
b/drivers/gpu/drm/radeon/r300_cmdbuf.c
index 865e2c9..60170ea 100644
--- a/drivers/gpu/drm/radeon/r300_cmdbuf.c
+++ b/drivers/gpu/drm/radeon/r300_cmdbuf.c
@@ -75,7 +75,7 @@ static int r300_emit_cliprects(drm_radeon_private_t *dev_priv,
OUT_RING(CP_PACKET0(R300_RE_CLIPRECT_TL_0, nr * 2 - 1));
for (i = 0; i < nr; ++i) {
- if (DRM_COPY_FROM_USER_UNCHECKED
+ if (DRM_COPY_FROM_USER
(, >boxes[n + i], sizeof(box))) {
DRM_ERROR("copy cliprect faulted\n");
return -EFAULT;
--
1.7.9.5
--
Kees Cook
Chrome OS Security
i915: build warning in i915_hangcheck_elapsed
9107e9d227e3b0893829baee4ac59feb874d4c23 introduced "score", and while it's actually not possible for it to be uninitialized (given the return values of ring_stuck()), the warning should be suppressed. It probably just needs a "default:" added to the switch statement. drivers/gpu/drm/i915/i915_irq.c:1943:27: warning: ?score? may be used uninitialized in this function [-Wuninitialized] -Kees -- Kees Cook Chrome OS Security
nouveau nvaa clock missing break?
Hi,
Just curious if this code is missing a "break" or not...
/drivers/gpu/drm/nouveau/core/subdev/clock/nvaa.c: 373 in nvaa_clock_prog()
switch (priv->vsrc) {
case nv_clk_src_cclk:
mast |= 0x0040;
default:
nv_wr32(clk, 0x4600, priv->vdiv);
}
Coverity noticed it as CID 1135671. If it's intentional, it might be
nice to add a "fall through" comment there.
Coverity also complained about read_pll (CID 1135670) where post_div
could (unlikely) be 0 and used for a divide-by-zero.
Thanks!
-Kees
--
Kees Cook
Chrome OS Security
[PATCH] drm/i915: use simple attribute in debugfs routines
This replaces the manual read/write routines in debugfs with the common
simple attribute helpers. Doing this gets rid of repeated copy/pasting
of copy_from_user and value formatting code.
Signed-off-by: Kees Cook
Cc: Daniel Vetter
---
drivers/gpu/drm/i915/i915_debugfs.c | 394 +--
1 file changed, 98 insertions(+), 296 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index aae3148..d86c304 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -849,76 +849,42 @@ static const struct file_operations i915_error_state_fops
= {
.release = i915_error_state_release,
};
-static ssize_t
-i915_next_seqno_read(struct file *filp,
-char __user *ubuf,
-size_t max,
-loff_t *ppos)
+static int
+i915_next_seqno_get(void *data, u64 *val)
{
- struct drm_device *dev = filp->private_data;
+ struct drm_device *dev = data;
drm_i915_private_t *dev_priv = dev->dev_private;
- char buf[80];
- int len;
int ret;
ret = mutex_lock_interruptible(>struct_mutex);
if (ret)
return ret;
- len = snprintf(buf, sizeof(buf),
- "next_seqno : 0x%x\n",
- dev_priv->next_seqno);
-
+ *val = dev_priv->next_seqno;
mutex_unlock(>struct_mutex);
- if (len > sizeof(buf))
- len = sizeof(buf);
-
- return simple_read_from_buffer(ubuf, max, ppos, buf, len);
+ return 0;
}
-static ssize_t
-i915_next_seqno_write(struct file *filp,
- const char __user *ubuf,
- size_t cnt,
- loff_t *ppos)
-{
- struct drm_device *dev = filp->private_data;
- char buf[20];
- u32 val = 1;
+static int
+i915_next_seqno_set(void *data, u64 val)
+{
+ struct drm_device *dev = data;
int ret;
- if (cnt > 0) {
- if (cnt > sizeof(buf) - 1)
- return -EINVAL;
-
- if (copy_from_user(buf, ubuf, cnt))
- return -EFAULT;
- buf[cnt] = 0;
-
- ret = kstrtouint(buf, 0, );
- if (ret < 0)
- return ret;
- }
-
ret = mutex_lock_interruptible(>struct_mutex);
if (ret)
return ret;
ret = i915_gem_set_seqno(dev, val);
-
mutex_unlock(>struct_mutex);
- return ret ?: cnt;
+ return ret;
}
-static const struct file_operations i915_next_seqno_fops = {
- .owner = THIS_MODULE,
- .open = simple_open,
- .read = i915_next_seqno_read,
- .write = i915_next_seqno_write,
- .llseek = default_llseek,
-};
+DEFINE_SIMPLE_ATTRIBUTE(i915_next_seqno_fops,
+ i915_next_seqno_get, i915_next_seqno_set,
+ "next_seqno : 0x%llx\n");
static int i915_rstdby_delays(struct seq_file *m, void *unused)
{
@@ -1680,105 +1646,51 @@ static int i915_dpio_info(struct seq_file *m, void
*data)
return 0;
}
-static ssize_t
-i915_wedged_read(struct file *filp,
-char __user *ubuf,
-size_t max,
-loff_t *ppos)
+static int
+i915_wedged_get(void *data, u64 *val)
{
- struct drm_device *dev = filp->private_data;
+ struct drm_device *dev = data;
drm_i915_private_t *dev_priv = dev->dev_private;
- char buf[80];
- int len;
- len = snprintf(buf, sizeof(buf),
- "wedged : %d\n",
- atomic_read(_priv->gpu_error.reset_counter));
+ *val = atomic_read(_priv->gpu_error.reset_counter);
- if (len > sizeof(buf))
- len = sizeof(buf);
-
- return simple_read_from_buffer(ubuf, max, ppos, buf, len);
+ return 0;
}
-static ssize_t
-i915_wedged_write(struct file *filp,
- const char __user *ubuf,
- size_t cnt,
- loff_t *ppos)
+static int
+i915_wedged_set(void *data, u64 val)
{
- struct drm_device *dev = filp->private_data;
- char buf[20];
- int val = 1;
-
- if (cnt > 0) {
- if (cnt > sizeof(buf) - 1)
- return -EINVAL;
+ struct drm_device *dev = data;
- if (copy_from_user(buf, ubuf, cnt))
- return -EFAULT;
- buf[cnt] = 0;
-
- val = simple_strtoul(buf, NULL, 0);
- }
-
- DRM_INFO("Manually setting wedged to %d\n", val);
+ DRM_INFO("Manually setting wedged to %llu\n", val);
i915_handle_error(dev, val);
- return cnt;
+ return 0;
}
-static const struct file_operations i915_wedged_fops = {
- .owner = THIS_MODULE,
- .open = si
[PATCH] drm/i915: restrict kernel address leak in debugfs
Masks kernel address info-leak in object dumps with the %pK suffix,
so they cannot be used to target kernel memory corruption attacks if
the kptr_restrict sysctl is set.
Signed-off-by: Kees Cook
Cc: stable at vger.kernel.org
---
drivers/gpu/drm/i915/i915_debugfs.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index aae3148..7299ea4 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -103,7 +103,7 @@ static const char *cache_level_str(int type)
static void
describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
{
- seq_printf(m, "%p: %s%s %8zdKiB %02x %02x %d %d %d%s%s%s",
+ seq_printf(m, "%pK: %s%s %8zdKiB %02x %02x %d %d %d%s%s%s",
>base,
get_pin_flag(obj),
get_tiling_flag(obj),
--
1.7.9.5
--
Kees Cook
Chrome OS Security
[PATCH] drm/i915: clarify reasoning for the access_ok call
This clarifies the comment above the access_ok check so a missing VERIFY_READ doesn't alarm anyone. Signed-off-by: Kees Cook Cc: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_execbuffer.c |6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 2f2daeb..752e399 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -747,7 +747,11 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec, length = exec[i].relocation_count * sizeof(struct drm_i915_gem_relocation_entry); - /* we may also need to update the presumed offsets */ + /* +* Validate memory range. Since we may need to update the +* presumed offsets, use VERIFY_WRITE. (Writable implies +* readable.) +*/ if (!access_ok(VERIFY_WRITE, ptr, length)) return -EFAULT; -- 1.7.9.5 -- Kees Cook Chrome OS Security
[PATCH] drm/radeon: check incoming cliprects pointer
The "boxes" parameter points into userspace memory. It should be verified
like any other operation against user memory.
Signed-off-by: Kees Cook
Cc: stable at vger.kernel.org
---
drivers/gpu/drm/radeon/r300_cmdbuf.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/r300_cmdbuf.c
b/drivers/gpu/drm/radeon/r300_cmdbuf.c
index 865e2c9..60170ea 100644
--- a/drivers/gpu/drm/radeon/r300_cmdbuf.c
+++ b/drivers/gpu/drm/radeon/r300_cmdbuf.c
@@ -75,7 +75,7 @@ static int r300_emit_cliprects(drm_radeon_private_t *dev_priv,
OUT_RING(CP_PACKET0(R300_RE_CLIPRECT_TL_0, nr * 2 - 1));
for (i = 0; i < nr; ++i) {
- if (DRM_COPY_FROM_USER_UNCHECKED
+ if (DRM_COPY_FROM_USER
(, >boxes[n + i], sizeof(box))) {
DRM_ERROR("copy cliprect faulted\n");
return -EFAULT;
--
1.7.9.5
--
Kees Cook
Chrome OS Security
[PATCH] drm/i915: bounds check execbuffer relocations
It is possible to wrap the counter used to allocate the buffer for
relocation copies. This could lead to heap writing overflows.
Signed-off-by: Kees Cook
Reported-by: Pinkie Pie
Cc: stable at vger.kernel.org
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 752e399..62eaa99 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -585,7 +585,8 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
struct drm_i915_gem_object *obj;
bool need_relocs;
int *reloc_offset;
- int i, total, ret;
+ int ret;
+ unsigned int i, total;
int count = args->buffer_count;
/* We may process another execbuffer during the unlock... */
@@ -600,8 +601,13 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
mutex_unlock(>struct_mutex);
total = 0;
- for (i = 0; i < count; i++)
+ for (i = 0; i < count; i++) {
+ if (exec[i].relocation_count > UINT_MAX - total) {
+ mutex_lock(>struct_mutex);
+ return -ENOMEM;
+ }
total += exec[i].relocation_count;
+ }
reloc_offset = drm_malloc_ab(count, sizeof(*reloc_offset));
reloc = drm_malloc_ab(total, sizeof(*reloc));
--
1.7.9.5
--
Kees Cook
Chrome OS Security
[PATCH] drm/i915: clarify reasoning for the access_ok call
On Mon, Mar 11, 2013 at 1:51 PM, Chris Wilson wrote: > On Mon, Mar 11, 2013 at 12:26:30PM -0700, Kees Cook wrote: >> This clarifies the comment above the access_ok check so a missing >> VERIFY_READ doesn't alarm anyone. > > Do we really need to copy the interface documentation? > > /** > * access_ok: - Checks if a user space pointer is valid > * @type: Type of access: %VERIFY_READ or %VERIFY_WRITE. Note that > *%VERIFY_WRITE is a superset of %VERIFY_READ - if it is safe > *to write to a block, it is always safe to read from it. > * @addr: User space pointer to start of block to check > * @size: Size of block to check > */ > -Chris Probably not. It just seemed like the existing comment was insufficient after the removal of the redundant VERIFY_READ check that happened recently. -Kees -- Kees Cook Chrome OS Security
[PATCH] drm/i915: bounds check execbuffer relocations
On Mon, Mar 11, 2013 at 1:52 PM, Chris Wilson wrote: > On Mon, Mar 11, 2013 at 12:27:16PM -0700, Kees Cook wrote: >> It is possible to wrap the counter used to allocate the buffer for >> relocation copies. This could lead to heap writing overflows. > > Seems a sensible check, just in the wrong location. You need to do the > checking upfront in validate_exec_list() so that the error condition is > always hit and that the limits are applied consistently to all > execbuffers. I opted for it here because it kept it out of the fast path which didn't need this check (it uses a list rather than an array). I will move it to validate_exec_list(). Thanks! -Kees -- Kees Cook Chrome OS Security
[PATCH v2] drm/i915: bounds check execbuffer relocation count
It is possible to wrap the counter used to allocate the buffer for
relocation copies. This could lead to heap writing overflows.
Signed-off-by: Kees Cook
Reported-by: Pinkie Pie
Cc: stable at vger.kernel.org
---
v2:
- move check into validate_exec_list
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c |4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 752e399..72d7841 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -732,6 +732,7 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
int count)
{
int i;
+ int total = 0;
for (i = 0; i < count; i++) {
char __user *ptr = (char __user *)(uintptr_t)exec[i].relocs_ptr;
@@ -744,6 +745,9 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
if (exec[i].relocation_count >
INT_MAX / sizeof(struct drm_i915_gem_relocation_entry))
return -EINVAL;
+ if (exec[i].relocation_count > INT_MAX - total)
+ return -ENOMEM;
+ total += exec[i].relocation_count;
length = exec[i].relocation_count *
sizeof(struct drm_i915_gem_relocation_entry);
--
1.7.9.5
--
Kees Cook
Chrome OS Security
[PATCH v2] drm/i915: clarify reasoning for the access_ok call
This clarifies the comment above the access_ok check so a missing VERIFY_READ doesn't alarm anyone. Signed-off-by: Kees Cook Cc: Daniel Vetter --- v2: - rewrote comment, thanks to Chris Wilson --- drivers/gpu/drm/i915/i915_gem_execbuffer.c |6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index bf7ceca..89c4039 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -751,7 +751,11 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec, length = exec[i].relocation_count * sizeof(struct drm_i915_gem_relocation_entry); - /* we may also need to update the presumed offsets */ + /* +* We must check that the entire relocation array is safe +* to read, but since we may need to update the presumed +* offsets during execution, check for full write access. +*/ if (!access_ok(VERIFY_WRITE, ptr, length)) return -EFAULT; -- 1.7.9.5 -- Kees Cook Chrome OS Security
[PATCH v2] drm/i915: bounds check execbuffer relocation count
On Mon, Mar 11, 2013 at 3:00 PM, Chris Wilson wrote: > On Mon, Mar 11, 2013 at 02:23:29PM -0700, Kees Cook wrote: >> It is possible to wrap the counter used to allocate the buffer for >> relocation copies. This could lead to heap writing overflows. > > I'd keep the return value as EINVAL so that we can continue to > distinguish between the user passing garbage and hitting an oom. And > total_relocs is preferrable to total, which also leads us to think more > carefully about the error condition. I think the check should be against > INT_MAX / sizeof(struct reloc_entry) for consistency with our other > guard against overflows whilst allocating. I've ended up with this: int max_alloc = INT_MAX / sizeof(struct drm_i915_gem_relocation_entry); ... /* First check for malicious input causing overflow */ if (exec[i].relocation_count > max_alloc) return -EINVAL; if (exec[i].relocation_count > max_alloc - total_relocs) return -EINVAL; total_relocs += exec[i].relocation_count; And looking at that, I wonder if we should just eliminate the first if entirely? -Kees -- Kees Cook Chrome OS Security
[PATCH] drm/i915: use do_div() as needed in debugfs code
This replaces the open-coded divisions in the debugfs code by calls to do_div(). Signed-off-by: Kees Cook Cc: Daniel Vetter --- drivers/gpu/drm/i915/i915_debugfs.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index d86c304..6f3cbf8 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1814,9 +1814,9 @@ i915_max_freq_set(void *data, u64 val) /* * Turbo will still be enabled, but won't go above the set value. */ - dev_priv->rps.max_delay = val / GT_FREQUENCY_MULTIPLIER; - - gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER); + do_div(val, GT_FREQUENCY_MULTIPLIER); + dev_priv->rps.max_delay = val; + gen6_set_rps(dev, val); mutex_unlock(_priv->rps.hw_lock); return 0; @@ -1865,9 +1865,9 @@ i915_min_freq_set(void *data, u64 val) /* * Turbo will still be enabled, but won't go below the set value. */ - dev_priv->rps.min_delay = val / GT_FREQUENCY_MULTIPLIER; - - gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER); + do_div(val, GT_FREQUENCY_MULTIPLIER); + dev_priv->rps.min_delay = val; + gen6_set_rps(dev, val); mutex_unlock(_priv->rps.hw_lock); return 0; -- 1.7.9.5 -- Kees Cook Chrome OS Security
[PATCH] drm/i915: use simple attribute in debugfs routines
On Mon, Mar 11, 2013 at 4:03 PM, Daniel Vetter wrote: > On Sun, Mar 10, 2013 at 02:10:06PM -0700, Kees Cook wrote: >> This replaces the manual read/write routines in debugfs with the common >> simple attribute helpers. Doing this gets rid of repeated copy/pasting >> of copy_from_user and value formatting code. >> >> Signed-off-by: Kees Cook >> Cc: Daniel Vetter > > Queued for -next, thanks for the patch. When sending drm/i915 patches, > please also cc intel-gfx at lists.freedesktop.org (it's open for > non-subscribers non without nagging "you're mail is in the moderation > queue" replies). Ah-ha, sure. Should MAINTAINERS be updated to reflect this? It got skipped as a destination due to the "subscribers-only" suffix. -Kees -- Kees Cook Chrome OS Security
[PATCH v3] drm/i915: bounds check execbuffer relocation count
It is possible to wrap the counter used to allocate the buffer for
relocation copies. This could lead to heap writing overflows.
CVE-2013-0913
v3: collapse test, improve comment
v2: move check into validate_exec_list
Signed-off-by: Kees Cook
Reported-by: Pinkie Pie
Cc: stable at vger.kernel.org
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index b3a40ee..094ba41 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -732,6 +732,8 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
int count)
{
int i;
+ int relocs_total = 0;
+ int relocs_max = INT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
for (i = 0; i < count; i++) {
char __user *ptr = (char __user *)(uintptr_t)exec[i].relocs_ptr;
@@ -740,10 +742,13 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
if (exec[i].flags & __EXEC_OBJECT_UNKNOWN_FLAGS)
return -EINVAL;
- /* First check for malicious input causing overflow */
- if (exec[i].relocation_count >
- INT_MAX / sizeof(struct drm_i915_gem_relocation_entry))
+ /* First check for malicious input causing overflow in
+* the worst case where we need to allocate the entire
+* relocation tree as a single array.
+*/
+ if (exec[i].relocation_count > relocs_max - relocs_total)
return -EINVAL;
+ relocs_total += exec[i].relocation_count;
length = exec[i].relocation_count *
sizeof(struct drm_i915_gem_relocation_entry);
--
1.7.9.5
--
Kees Cook
Chrome OS Security
linux-next: build failure after merge of the final tree (drm-intel tree related)
On Mon, Mar 11, 2013 at 9:22 PM, Stephen Rothwell
wrote:
> Hi all,
>
> After merging the final tree, today's linux-next build (i386 defconfig)
> failed like this:
>
> drivers/built-in.o: In function `i915_min_freq_set':
> i915_debugfs.c:(.text+0xb1adc): undefined reference to `__udivdi3'
> drivers/built-in.o: In function `i915_max_freq_set':
> i915_debugfs.c:(.text+0xb1bac): undefined reference to `__udivdi3'
>
> Caused by commit 2389cc500686 ("drm/i915: use simple attribute in debugfs
> routines") from the drm-intel tree.
>
> I have reverted that commit for today.
Ah-ha, thanks. I've sent a follow-up patch to fix this.
-Kees
--
Kees Cook
Chrome OS Security
[PATCH v3] drm/i915: bounds check execbuffer relocation count
On Thu, Mar 14, 2013 at 9:57 AM, Daniel Vetter wrote: > On Wed, Mar 13, 2013 at 9:28 PM, Daniel Vetter wrote: >> On Tue, Mar 12, 2013 at 09:07:46AM +, Chris Wilson wrote: >>> On Mon, Mar 11, 2013 at 05:31:45PM -0700, Kees Cook wrote: >>> > It is possible to wrap the counter used to allocate the buffer for >>> > relocation copies. This could lead to heap writing overflows. >>> > >>> > CVE-2013-0913 >>> > >>> > v3: collapse test, improve comment >>> > v2: move check into validate_exec_list >>> > >>> > Signed-off-by: Kees Cook >>> > Reported-by: Pinkie Pie >>> > Cc: stable at vger.kernel.org >>> >>> Looks good to me. The only bikeshed that remains is whether we should >>> just collapse the two variables into one, but the current 'max - count' >>> is more idiomatic and so preferrable. >>> Reviewed-by: Chris Wilson >> >> Picked up for -fixes, thanks for the patch. > > I've forgotten to dump my wishlist: Can I have an i-g-t for this? For > this bug here specifically an execbuf with just one buffer with too > many relocs plus another execbuf with two buffers with relocation so > that the 2nd relocation list will overflow should be sufficient. Sure thing. Where do these live? (Or what docs should I read for this?) I'm assuming i-g-t means "intel graphics test"? :) -Kees -- Kees Cook Chrome OS Security
[LKP] [PATCH] drm/radeon: Try to init amdkfd only if 64 bit kernel
On Sun, Jan 4, 2015 at 8:28 PM, Rusty Russell wrote:
> Oded Gabbay writes:
>> On 12/24/2014 01:01 AM, Rusty Russell wrote:
>>> Oded Gabbay writes:
>>>> I didn't say it doesn't always work.
>>>> The actual thing that doesn't work is the define symbol_get and only in a
>>>> specific case of 32bit kernel AND CONFIG_MODULES is unset AND
>>>> CONFIG_RANDOMIZE_BASE is set.
>>>> The define in that case is:
>>>> #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak)); &(x); })
>>>>
>>>> Why it doesn't work (doesn't return NULL when symbol doesn't exists) ?
>>>
>>> Hmm, I'd guess CONFIG_RANDOMIZE_BASE is relocating NULL symbols...
>>>
>>> No, I can't reproduce this. Please send your .config privately.
>>>
>>> Here's my test case:
>>>
>>> diff --git a/init/main.c b/init/main.c
>>> index 61b993767db5..a3ee1ec97ec3 100644
>>> --- a/init/main.c
>>> +++ b/init/main.c
>>> @@ -683,6 +683,12 @@ asmlinkage __visible void __init start_kernel(void)
>>>
>>> ftrace_init();
>>>
>>> +{
>>> +extern void nonexistent_fn(void);
>>> +printk("symbol_get(nonexistent_fn) = %p\n",
>>> + symbol_get(nonexistent_fn));
>>> +}
>>> +
>>> /* Do the rest non-__init'ed, we're now alive */
>>> rest_init();
>>> }
>>>
>>> Thanks,
>>> Rusty.
>>>
>> Hi Rusty,
>>
>> Attached is the bad config file. (config-bad)
>> I have narrowed the changes you need to do to the config file in order to
>> reproduce this bug.
>> The base assumption is a 32-bit kernel and without modules support. Rest of
>> the
>> config file is pretty standard, IMO.
>> Then, its not enough to enable CONFIG_RANDOMIZE_BASE like I wrote in my
>> original
>> post. You need also to unset CONFIG_HIBERNATION.
>
> Indeed, thanks; your config breaks as reported. With CONFIG_HIBERNATION
> the kernel offset is 0, so we don't see this.
>
> Kees, as far as I can tell you need another 0-terminated vmlinux.relocs
> section for weak symbols. These should not be relocated if already 0.
A few questions:
Why doesn't this break on 32-bit without kASLR? 32-bit does relocation
by default, even without CONFIG_RANDOMIZE_BASE.
Are there any symbols that are NULL that aren't weak? I'd expect all
strong symbols to have non-zero offsets, but I must be
misunderstanding something here.
-Kees
>
> Put this somewhere to test. It fails for x86_64, too:
>
> diff --git a/init/main.c b/init/main.c
> index 61b993767db5..c9e0195c792a 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -683,6 +683,12 @@ asmlinkage __visible void __init start_kernel(void)
>
> ftrace_init();
>
> + {
> + extern void __attribute__((weak)) nonexistent_fn(void);
> + printk("nonexistent_fn = %p\n", nonexistent_fn);
> + BUG_ON(nonexistent_fn != NULL);
> + }
> +
> /* Do the rest non-__init'ed, we're now alive */
> rest_init();
> }
>
> Cheers,
> Rusty.
--
Kees Cook
Chrome OS Security
[LKP] [PATCH] drm/radeon: Try to init amdkfd only if 64 bit kernel
On Tue, Jan 6, 2015 at 2:58 PM, Rusty Russell wrote: > Kees Cook writes: >> On Sun, Jan 4, 2015 at 8:28 PM, Rusty Russell >> wrote: >>> Kees, as far as I can tell you need another 0-terminated vmlinux.relocs >>> section for weak symbols. These should not be relocated if already 0. >> >> A few questions: >> >> Why doesn't this break on 32-bit without kASLR? 32-bit does relocation >> by default, even without CONFIG_RANDOMIZE_BASE. > > Well, the offset was 0 until I removed CONFIG_HIBERNATE. > >> Are there any symbols that are NULL that aren't weak? I'd expect all >> strong symbols to have non-zero offsets, but I must be >> misunderstanding something here. > > I don't think there would be. Anyway, you might be able to filter them > out in x86/tools/relocs itself. I've been travelling last week and this, so I haven't had time to take a close look yet. Hopefully I can work on this next week or later this week. -Kees -- Kees Cook Chrome OS Security
[LKP] [PATCH] drm/radeon: Try to init amdkfd only if 64 bit kernel
On Sun, Jan 4, 2015 at 8:28 PM, Rusty Russell wrote:
> Oded Gabbay writes:
>> On 12/24/2014 01:01 AM, Rusty Russell wrote:
>>> Oded Gabbay writes:
>>>> I didn't say it doesn't always work.
>>>> The actual thing that doesn't work is the define symbol_get and only in a
>>>> specific case of 32bit kernel AND CONFIG_MODULES is unset AND
>>>> CONFIG_RANDOMIZE_BASE is set.
>>>> The define in that case is:
>>>> #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak)); &(x); })
>>>>
>>>> Why it doesn't work (doesn't return NULL when symbol doesn't exists) ?
>>>
>>> Hmm, I'd guess CONFIG_RANDOMIZE_BASE is relocating NULL symbols...
>>>
>>> No, I can't reproduce this. Please send your .config privately.
>>>
>>> Here's my test case:
>>>
>>> diff --git a/init/main.c b/init/main.c
>>> index 61b993767db5..a3ee1ec97ec3 100644
>>> --- a/init/main.c
>>> +++ b/init/main.c
>>> @@ -683,6 +683,12 @@ asmlinkage __visible void __init start_kernel(void)
>>>
>>> ftrace_init();
>>>
>>> +{
>>> +extern void nonexistent_fn(void);
>>> +printk("symbol_get(nonexistent_fn) = %p\n",
>>> + symbol_get(nonexistent_fn));
>>> +}
>>> +
>>> /* Do the rest non-__init'ed, we're now alive */
>>> rest_init();
>>> }
>>>
>>> Thanks,
>>> Rusty.
>>>
>> Hi Rusty,
>>
>> Attached is the bad config file. (config-bad)
>> I have narrowed the changes you need to do to the config file in order to
>> reproduce this bug.
>> The base assumption is a 32-bit kernel and without modules support. Rest of
>> the
>> config file is pretty standard, IMO.
>> Then, its not enough to enable CONFIG_RANDOMIZE_BASE like I wrote in my
>> original
>> post. You need also to unset CONFIG_HIBERNATION.
>
> Indeed, thanks; your config breaks as reported. With CONFIG_HIBERNATION
> the kernel offset is 0, so we don't see this.
>
> Kees, as far as I can tell you need another 0-terminated vmlinux.relocs
> section for weak symbols. These should not be relocated if already 0.
>
> Put this somewhere to test. It fails for x86_64, too:
>
> diff --git a/init/main.c b/init/main.c
> index 61b993767db5..c9e0195c792a 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -683,6 +683,12 @@ asmlinkage __visible void __init start_kernel(void)
>
> ftrace_init();
>
> + {
> + extern void __attribute__((weak)) nonexistent_fn(void);
> + printk("nonexistent_fn = %p\n", nonexistent_fn);
> + BUG_ON(nonexistent_fn != NULL);
> + }
> +
> /* Do the rest non-__init'ed, we're now alive */
> rest_init();
> }
Hm, I can't reproduce this on v3.19-rc4. My nonexistent_fn comes back
NULL regardless of CONFIG and kaslr on/off states I've tried. Could
this be a (yet another) linker bug? What was the toolchain used? I
built with gcc 4.8.2 and binutils 2.24.
-Kees
--
Kees Cook
Chrome OS Security
high_memory address in /proc/dri/*/vma
On Mon, Dec 19, 2011 at 4:09 PM, Ben Hutchings 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(>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
high_memory address in /proc/dri/*/vma
On Mon, Dec 19, 2011 at 6:18 PM, Ben Hutchings wrote:
> On Mon, 2011-12-19 at 16:14 -0800, Kees Cook wrote:
>> On Mon, Dec 19, 2011 at 4:09 PM, Ben Hutchings
>> 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(>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
high_memory address in /proc/dri/*/vma
On Tue, Dec 20, 2011 at 8:32 AM, Daniel Vetter wrote:
> On Tue, Dec 20, 2011 at 12:09:39AM +, 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(>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
[PATCH] drm: do not leak kernel addresses via /proc/dri/*/vma
In the continuing effort to avoid kernel addresses leaking to unprivileged
users, this patch switches to %pK for /proc/dri/*/vma.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/drm_info.c |9 +
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c
index 3cdbaf3..be9a9c0 100644
--- a/drivers/gpu/drm/drm_info.c
+++ b/drivers/gpu/drm/drm_info.c
@@ -283,17 +283,18 @@ int drm_vma_info(struct seq_file *m, void *data)
#endif
mutex_lock(>struct_mutex);
- 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(>vma_count),
- high_memory, (u64)virt_to_phys(high_memory));
+ high_memory, (void *)virt_to_phys(high_memory));
list_for_each_entry(pt, >vmalist, head) {
vma = pt->vma;
if (!vma)
continue;
seq_printf(m,
- "\n%5d 0x%08lx-0x%08lx %c%c%c%c%c%c 0x%08lx000",
- pt->pid, vma->vm_start, vma->vm_end,
+ "\n%5d 0x%pK-0x%pK %c%c%c%c%c%c 0x%08lx000",
+ pt->pid,
+ (void *)vma->vm_start, (void *)vma->vm_end,
vma->vm_flags & VM_READ ? 'r' : '-',
vma->vm_flags & VM_WRITE ? 'w' : '-',
vma->vm_flags & VM_EXEC ? 'x' : '-',
--
1.7.2.3
--
Kees Cook
Ubuntu Security Team
[PATCH] drm: do not leak kernel addresses via /proc/dri/*/vma
Hi Corbin, On Sat, Feb 12, 2011 at 10:13:04AM -0800, Corbin Simpson wrote: > On Fri, Feb 11, 2011 at 7:29 PM, Kees Cook wrote: > > In the continuing effort to avoid kernel addresses leaking to unprivileged > > users, this patch switches to %pK for /proc/dri/*/vma. > > This is a highly reasonable patch. Does 0x%pK show up as 0x0x0 in the > log, or just 0x0? Other than that... > Reviewed-by: Corbin Simpson Thanks! The default for %p (and %pK) is without the 0x prefix, and 0-padded to sizeof(void*) character. So 0x%pK will show as 0x on 32bit to a regular user, etc. -Kees -- Kees Cook Ubuntu Security Team
[PATCH] drm, video: fix use-before-NULL-check
Fix potential crashes due to use-before-NULL situations.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/drm_fb_helper.c |3 ++-
drivers/media/video/em28xx/em28xx-video.c |3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index de82e20..8dd7e6f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -94,10 +94,11 @@ static bool
drm_fb_helper_connector_parse_command_line(struct drm_fb_helper_conn
int i;
enum drm_connector_force force = DRM_FORCE_UNSPECIFIED;
struct drm_fb_helper_cmdline_mode *cmdline_mode;
- struct drm_connector *connector = fb_helper_conn->connector;
+ struct drm_connector *connector;
if (!fb_helper_conn)
return false;
+ connector = fb_helper_conn->connector;
cmdline_mode = _helper_conn->cmdline_mode;
if (!mode_option)
diff --git a/drivers/media/video/em28xx/em28xx-video.c
b/drivers/media/video/em28xx/em28xx-video.c
index 7b9ec6e..95a4b60 100644
--- a/drivers/media/video/em28xx/em28xx-video.c
+++ b/drivers/media/video/em28xx/em28xx-video.c
@@ -277,12 +277,13 @@ static void em28xx_copy_vbi(struct em28xx *dev,
{
void *startwrite, *startread;
int offset;
- int bytesperline = dev->vbi_width;
+ int bytesperline;
if (dev == NULL) {
em28xx_isocdbg("dev is null\n");
return;
}
+ bytesperline = dev->vbi_width;
if (dma_q == NULL) {
em28xx_isocdbg("dma_q is null\n");
--
1.7.1
--
Kees Cook
Ubuntu Security Team
[PATCH] drm: i915: Avoid format string expansion from engine names
While highly unlikely, this makes sure that the string built from engine names won't be processed as a format string. Signed-off-by: Kees Cook <keesc...@chromium.org> --- drivers/gpu/drm/i915/intel_hangcheck.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_hangcheck.c b/drivers/gpu/drm/i915/intel_hangcheck.c index f05971f5586f..be3550cec8e4 100644 --- a/drivers/gpu/drm/i915/intel_hangcheck.c +++ b/drivers/gpu/drm/i915/intel_hangcheck.c @@ -407,7 +407,7 @@ static void hangcheck_declare_hang(struct drm_i915_private *i915, "%s, ", engine->name); msg[len-2] = '\0'; - return i915_handle_error(i915, hung, msg); + return i915_handle_error(i915, hung, "%s", msg); } /* -- 2.7.4 -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] format-security: move static strings to const
While examining output from trial builds with -Wformat-security enabled,
many strings were found that should be defined as "const", or as a char
array instead of char pointer. This makes some static analysis easier,
by producing fewer false positives.
As these are all trivial changes, it seemed best to put them all in
a single patch rather than chopping them up per maintainer.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
arch/arm/mach-omap2/board-n8x0.c | 2 +-
arch/mips/dec/prom/init.c | 6 +++---
arch/mips/kernel/traps.c | 4 ++--
drivers/char/dsp56k.c | 2 +-
drivers/cpufreq/powernow-k8.c | 3 ++-
drivers/gpu/drm/drm_fb_helper.c | 2 +-
drivers/net/ethernet/amd/atarilance.c | 4 ++--
drivers/net/ethernet/amd/declance.c | 2 +-
drivers/net/ethernet/amd/sun3lance.c | 3 ++-
drivers/net/ethernet/cirrus/mac89x0.c | 2 +-
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h | 2 +-
drivers/net/ethernet/natsemi/sonic.h | 2 +-
drivers/net/ethernet/toshiba/tc35815.c| 2 +-
drivers/net/fddi/defxx.c | 2 +-
drivers/net/hippi/rrunner.c | 3 ++-
drivers/staging/most/mostcore/core.c | 2 +-
drivers/tty/n_hdlc.c | 10 +-
drivers/tty/serial/st-asc.c | 2 +-
net/decnet/af_decnet.c| 3 ++-
19 files changed, 31 insertions(+), 27 deletions(-)
diff --git a/arch/arm/mach-omap2/board-n8x0.c b/arch/arm/mach-omap2/board-n8x0.c
index 6b6fda65fb3b..91272db09fa3 100644
--- a/arch/arm/mach-omap2/board-n8x0.c
+++ b/arch/arm/mach-omap2/board-n8x0.c
@@ -117,7 +117,7 @@ static struct musb_hdrc_platform_data tusb_data = {
static void __init n8x0_usb_init(void)
{
int ret = 0;
- static char announce[] __initdata = KERN_INFO "TUSB 6010\n";
+ static const char announce[] __initconst = KERN_INFO "TUSB 6010\n";
/* PM companion chip power control pin */
ret = gpio_request_one(TUSB6010_GPIO_ENABLE, GPIOF_OUT_INIT_LOW,
diff --git a/arch/mips/dec/prom/init.c b/arch/mips/dec/prom/init.c
index 4e1761e0a09a..d88eb7a6662b 100644
--- a/arch/mips/dec/prom/init.c
+++ b/arch/mips/dec/prom/init.c
@@ -88,7 +88,7 @@ void __init which_prom(s32 magic, s32 *prom_vec)
void __init prom_init(void)
{
extern void dec_machine_halt(void);
- static char cpu_msg[] __initdata =
+ static const char cpu_msg[] __initconst =
"Sorry, this kernel is compiled for a wrong CPU type!\n";
s32 argc = fw_arg0;
s32 *argv = (void *)fw_arg1;
@@ -111,7 +111,7 @@ void __init prom_init(void)
#if defined(CONFIG_CPU_R3000)
if ((current_cpu_type() == CPU_R4000SC) ||
(current_cpu_type() == CPU_R4400SC)) {
- static char r4k_msg[] __initdata =
+ static const char r4k_msg[] __initconst =
"Please recompile with \"CONFIG_CPU_R4x00 = y\".\n";
printk(cpu_msg);
printk(r4k_msg);
@@ -122,7 +122,7 @@ void __init prom_init(void)
#if defined(CONFIG_CPU_R4X00)
if ((current_cpu_type() == CPU_R3000) ||
(current_cpu_type() == CPU_R3000A)) {
- static char r3k_msg[] __initdata =
+ static const char r3k_msg[] __initconst =
"Please recompile with \"CONFIG_CPU_R3000 = y\".\n";
printk(cpu_msg);
printk(r3k_msg);
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index c7d17cfb32f6..2c717db50380 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -2256,8 +2256,8 @@ void set_handler(unsigned long offset, void *addr,
unsigned long size)
local_flush_icache_range(ebase + offset, ebase + offset + size);
}
-static char panic_null_cerr[] =
- "Trying to set NULL cache error exception handler";
+static const char panic_null_cerr[] =
+ "Trying to set NULL cache error exception handler\n";
/*
* Install uncached CPU exception handler.
diff --git a/drivers/char/dsp56k.c b/drivers/char/dsp56k.c
index 50aa9ba91f25..0d7b577e0ff0 100644
--- a/drivers/char/dsp56k.c
+++ b/drivers/char/dsp56k.c
@@ -489,7 +489,7 @@ static const struct file_operations dsp56k_fops = {
/** Init and module functions **/
-static char banner[] __initdata = KERN_INFO "DSP56k driver installed\n";
+static const char banner[] __initconst = KERN_INFO "DSP56k driver installed\n";
static int __init dsp56k_init_driver(void)
{
diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
index 0b5bf135b090..062d71434e47 100644
--- a/drivers/cpufreq/powernow-k8.c
Re: [PATCH] format-security: move static strings to const
On Thu, Apr 6, 2017 at 1:48 AM, Jani Nikula <jani.nik...@linux.intel.com> wrote:
> On Thu, 06 Apr 2017, Kees Cook <keesc...@chromium.org> wrote:
>> While examining output from trial builds with -Wformat-security enabled,
>> many strings were found that should be defined as "const", or as a char
>> array instead of char pointer. This makes some static analysis easier,
>> by producing fewer false positives.
>>
>> As these are all trivial changes, it seemed best to put them all in
>> a single patch rather than chopping them up per maintainer.
>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>> b/drivers/gpu/drm/drm_fb_helper.c
>> index f6d4d9700734..1ff9d5912b83 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -2331,7 +2331,7 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
>> int __init drm_fb_helper_modinit(void)
>> {
>> #if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
>> - const char *name = "fbcon";
>> + const char name[] = "fbcon";
>
> I'd always write the former out of habit. Why should I start using the
> latter? What makes it better?
For me, it's mainly two reasons: sizeof() and -Wformat-security behavior.
The compiler treats "sizeof" differently. E.g. "sizeof(var)" shows the
allocation size for the array, and pointer size for the char pointer.
When doing things like snprintf(buf, sizeof(buf), ...) will do the
right thing, etc. (This is a poor example for a _const_ string, but
the point is that some calculations still work better with the array
over the pointer.)
The other situation (which is why I noted this to change them) is that
gcc's handling of them is different when faced with -Wformat-security
since it doesn't like to believe that const char pointers are actually
const for the purposes of being a format string.
> What keeps the kernel from accumulating tons more of the former?
Right now, nothing. The good news is that they're relatively rare, and
I notice them when they're added (since I have a -Wformat-security
tree). We could add a warning to checkpatch for suggesting const char
var[] over const char *var, perhaps?
> Here's an interesting comparison of the generated code. I'm a bit
> surprised by what gcc does, I would have expected no difference, like
> clang. https://godbolt.org/g/OdqUvN
Here's your example with sizeof() added, if you're curious...
https://godbolt.org/g/U1zIZK
> The other changes adding const in this patch are, of course, good.
Thanks!
-Kees
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/powerplay: rv: Use designated initializers
On Thu, Jul 27, 2017 at 6:43 PM, Alex Deucher <alexdeuc...@gmail.com> wrote:
> On Tue, Jul 25, 2017 at 5:47 PM, Kees Cook <keesc...@chromium.org> wrote:
>> As done for vega10 in commit 3ddd396f6b57 ("drm/amd/powerplay: Use
>> designated initializers") mark other tableFunction entries with designated
>> initializers. The randstruct plugin requires designated initializers for
>> structures that are entirely function pointers.
>>
>> Cc: Rex Zhu <rex@amd.com>
>> Cc: Hawking Zhang <hawking.zh...@amd.com>
>> Cc: Alex Deucher <alexander.deuc...@amd.com>
>> Signed-off-by: Kees Cook <keesc...@chromium.org>
>> ---
>> If I can get an Ack for this, I'll carry it in the gcc-plugins tree, unless
>> you think this is worth landing for v4.13, in which case, please take it
>> now. :)
>>
>
> Acked-by: Alex Deucher <alexander.deuc...@amd.com>
>
> I'm happy to take this through my tree if that is ok with you.
Since the randstruct patch depends on this fix, it's likely best to go
through my tree unless you can get this into v4.13. (Since then when
the randstruct patch lands in v4.14, it'll already be there.) I'm fine
either way.
Thanks!
-Kees
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/powerplay: rv: Use designated initializers
On Fri, Jul 28, 2017 at 2:13 AM, Christian König
<christian.koe...@amd.com> wrote:
> Am 28.07.2017 um 03:43 schrieb Alex Deucher:
>>
>> On Tue, Jul 25, 2017 at 5:47 PM, Kees Cook <keesc...@chromium.org> wrote:
>>>
>>> As done for vega10 in commit 3ddd396f6b57 ("drm/amd/powerplay: Use
>>> designated initializers") mark other tableFunction entries with
>>> designated
>>> initializers. The randstruct plugin requires designated initializers for
>>> structures that are entirely function pointers.
>>>
>>> Cc: Rex Zhu <rex@amd.com>
>>> Cc: Hawking Zhang <hawking.zh...@amd.com>
>>> Cc: Alex Deucher <alexander.deuc...@amd.com>
>>> Signed-off-by: Kees Cook <keesc...@chromium.org>
>>> ---
>>> If I can get an Ack for this, I'll carry it in the gcc-plugins tree,
>>> unless
>>> you think this is worth landing for v4.13, in which case, please take it
>>> now. :)
>>>
>> Acked-by: Alex Deucher <alexander.deuc...@amd.com>
>>
>> I'm happy to take this through my tree if that is ok with you.
>
>
> I'm wondering a bit how the plugin detects that it can randomize a structure
> layout?
>
> We have a couple of structs where this would be fatal.
Automatic randomization only happen on struct that are entirely
function pointers.
See:
https://git.kernel.org/linus/313dd1b629219db50cad532dba6a3b3b22ffe622
And:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?id=914e2dfc61195a95868ae5c750690a7f1c87bc66
If you have any structures that are shared externally from the kernel,
I can mark them with __no_randomize_layout.
-Kees
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amd/powerplay: rv: Use designated initializers
As done for vega10 in commit 3ddd396f6b57 ("drm/amd/powerplay: Use
designated initializers") mark other tableFunction entries with designated
initializers. The randstruct plugin requires designated initializers for
structures that are entirely function pointers.
Cc: Rex Zhu <rex@amd.com>
Cc: Hawking Zhang <hawking.zh...@amd.com>
Cc: Alex Deucher <alexander.deuc...@amd.com>
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
If I can get an Ack for this, I'll carry it in the gcc-plugins tree, unless
you think this is worth landing for v4.13, in which case, please take it
now. :)
Thanks!
---
drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
index 4c7f430b36eb..8e6cfd89c7e0 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
@@ -308,8 +308,8 @@ static int rv_tf_set_num_active_display(struct pp_hwmgr
*hwmgr, void *input,
}
static const struct phm_master_table_item rv_set_power_state_list[] = {
- { NULL, rv_tf_set_clock_limit },
- { NULL, rv_tf_set_num_active_display },
+ { .tableFunction = rv_tf_set_clock_limit },
+ { .tableFunction = rv_tf_set_num_active_display },
{ }
};
@@ -382,7 +382,7 @@ static int rv_tf_disable_gfx_off(struct pp_hwmgr *hwmgr,
}
static const struct phm_master_table_item rv_disable_dpm_list[] = {
- {NULL, rv_tf_disable_gfx_off},
+ { .tableFunction = rv_tf_disable_gfx_off },
{ },
};
@@ -407,7 +407,7 @@ static int rv_tf_enable_gfx_off(struct pp_hwmgr *hwmgr,
}
static const struct phm_master_table_item rv_enable_dpm_list[] = {
- {NULL, rv_tf_enable_gfx_off},
+ { .tableFunction = rv_tf_enable_gfx_off },
{ },
};
--
2.7.4
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/13] Get rid of DocBook
On Sun, May 14, 2017 at 8:38 AM, Mauro Carvalho Chehab <mche...@s-opensource.com> wrote: > As just one book (lsm) was missing conversion, let's convert it > and store as if it were a plain text file under Documentation/lsm.txt, > adding a notice that it requires update. I could probably fold this change into my rst-ification of Documentation/security/ https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/rst Specifically, the new Documentation/security/LSM.rst was rather short. I think your lsm.txt and this one could be likely merged. -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/13] Get rid of DocBook
On Sun, May 14, 2017 at 3:00 PM, Mauro Carvalho Chehab <mche...@s-opensource.com> wrote: > Em Sun, 14 May 2017 14:05:09 -0700 > Kees Cook <keesc...@chromium.org> escreveu: > >> On Sun, May 14, 2017 at 8:38 AM, Mauro Carvalho Chehab >> <mche...@s-opensource.com> wrote: >> > As just one book (lsm) was missing conversion, let's convert it >> > and store as if it were a plain text file under Documentation/lsm.txt, >> > adding a notice that it requires update. >> >> I could probably fold this change into my rst-ification of >> Documentation/security/ >> >> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/rst >> >> Specifically, the new Documentation/security/LSM.rst was rather short. >> I think your lsm.txt and this one could be likely merged. > > Yeah, makes sense. I'm not sure what would be the best way to > proceed, as, currently, after removing lsm.tmpl, my patch > series remove DocBook, as everything else was already removed. > > I see a few ways for us to proceed: > > 1) You could submit the patches you have so far to docs -next, > to be merged before my patch series; I sent my series already (it got CCed to linux-doc); it's collected a few Acks already. I'm not sure what the timing for that means, though. > 2) I could send you a patch based on your tree. I'll need to > rebase this series to be applied on the top of your tree + docs-next, > with would require that your patch series would be merged before, > at docs -next; > > 3) we can handle both series independently. When both gets > merged at docs -next, a simple patch, either written by you or > me, could merge both files. I don't see a reason for us to make the trees depend on each other. Let's just proceed as-is and whenever we're in a position to merge the LSM docs, we can do that. > IMO, (3) is simpler, but if you prefer, we can do on some other > way. Agreed. -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: Convert timers to use timer_setup()
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
Cc: Daniel Vetter <daniel.vet...@intel.com>
Cc: Jani Nikula <jani.nik...@linux.intel.com>
Cc: David Airlie <airl...@linux.ie>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
Cc: Oscar Mateo <oscar.ma...@intel.com>
Cc: intel-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Cc: Thomas Gleixner <t...@linutronix.de>
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
This requires commit 686fef928bba ("timer: Prepare to change timer
callback argument type") in v4.14-rc3, but should be otherwise
stand-alone.
---
drivers/gpu/drm/i915/selftests/mock_engine.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c
b/drivers/gpu/drm/i915/selftests/mock_engine.c
index fc0fd7498689..331c2b09869e 100644
--- a/drivers/gpu/drm/i915/selftests/mock_engine.c
+++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
@@ -32,9 +32,9 @@ static struct mock_request *first_request(struct mock_engine
*engine)
link);
}
-static void hw_delay_complete(unsigned long data)
+static void hw_delay_complete(struct timer_list *t)
{
- struct mock_engine *engine = (typeof(engine))data;
+ struct mock_engine *engine = from_timer(engine, t, hw_delay);
struct mock_request *request;
spin_lock(>hw_lock);
@@ -161,9 +161,7 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private
*i915,
/* fake hw queue */
spin_lock_init(>hw_lock);
- setup_timer(>hw_delay,
- hw_delay_complete,
- (unsigned long)engine);
+ timer_setup(>hw_delay, hw_delay_complete, 0);
INIT_LIST_HEAD(>hw_queue);
return >base;
--
2.7.4
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: Convert timers to use timer_setup()
On Thu, Oct 5, 2017 at 6:45 AM, Joonas Lahtinen
<joonas.lahti...@linux.intel.com> wrote:
> On Wed, 2017-10-04 at 17:54 -0700, Kees Cook wrote:
>> In preparation for unconditionally passing the struct timer_list pointer to
>> all timer callbacks, switch to using the new timer_setup() and from_timer()
>> to pass the timer pointer explicitly.
>>
>> Cc: Daniel Vetter <daniel.vet...@intel.com>
>> Cc: Jani Nikula <jani.nik...@linux.intel.com>
>> Cc: David Airlie <airl...@linux.ie>
>> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
>> Cc: Oscar Mateo <oscar.ma...@intel.com>
>> Cc: intel-...@lists.freedesktop.org
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: Thomas Gleixner <t...@linutronix.de>
>> Signed-off-by: Kees Cook <keesc...@chromium.org>
>
>
>
>> @@ -32,9 +32,9 @@ static struct mock_request *first_request(struct
>> mock_engine *engine)
>> link);
>> }
>>
>> -static void hw_delay_complete(unsigned long data)
>> +static void hw_delay_complete(struct timer_list *t)
>> {
>> - struct mock_engine *engine = (typeof(engine))data;
>> + struct mock_engine *engine = from_timer(engine, t, hw_delay);
>
> The order is bit strange to me, it's not same as with container_of, but
> I guess GCC will complain for getting it wrong. It's also slightly
> different doing the typeof for you, so I guess it makes sense, so:
Yeah, this seemed to be the least bad of several options. Other things
ended up being either very long, named unlike anything else already in
the kernel, etc.
> Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
Thanks!
> Do you expect for us to merge or are you looking to merge all timer
> changes from single tree?
If you have -rc3 in your tree already, please take this into your
tree. If you prefer the timer tree to carry it, that can happen too.
tglx suggested to me that it was better for maintainers to carry the
changes.
-Kees
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 150/156] drm/etnaviv: Convert timers to use timer_setup()
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
Cc: Lucas Stach <l.st...@pengutronix.de>
Cc: Russell King <linux+etna...@armlinux.org.uk>
Cc: Christian Gmeiner <christian.gmei...@gmail.com>
Cc: David Airlie <airl...@linux.ie>
Cc: etna...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 8197e1d6ed11..e19cbe05da2a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -968,9 +968,9 @@ static void hangcheck_timer_reset(struct etnaviv_gpu *gpu)
round_jiffies_up(jiffies + DRM_ETNAVIV_HANGCHECK_JIFFIES));
}
-static void hangcheck_handler(unsigned long data)
+static void hangcheck_handler(struct timer_list *t)
{
- struct etnaviv_gpu *gpu = (struct etnaviv_gpu *)data;
+ struct etnaviv_gpu *gpu = from_timer(gpu, t, hangcheck_timer);
u32 fence = gpu->completed_fence;
bool progress = false;
@@ -1765,8 +1765,7 @@ static int etnaviv_gpu_bind(struct device *dev, struct
device *master,
INIT_WORK(>recover_work, recover_worker);
init_waitqueue_head(>fence_event);
- setup_deferrable_timer(>hangcheck_timer, hangcheck_handler,
- (unsigned long)gpu);
+ timer_setup(>hangcheck_timer, hangcheck_handler, TIMER_DEFERRABLE);
priv->gpu[priv->num_gpus++] = gpu;
--
2.7.4
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: Convert timers to use timer_setup()
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
Cc: Daniel Vetter <daniel.vet...@intel.com>
Cc: Jani Nikula <jani.nik...@linux.intel.com>
Cc: David Airlie <airl...@linux.ie>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
Cc: Oscar Mateo <oscar.ma...@intel.com>
Cc: intel-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
Reviewed-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com> # for mock_engine
---
This patch includes additional timers since the last time it was sent.
---
drivers/gpu/drm/i915/i915_sw_fence.c | 8 +++-
drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 --
drivers/gpu/drm/i915/selftests/mock_engine.c | 8 +++-
3 files changed, 14 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c
b/drivers/gpu/drm/i915/i915_sw_fence.c
index ca33cc08cb07..e8ca67a129d2 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -369,9 +369,9 @@ struct i915_sw_dma_fence_cb {
struct irq_work work;
};
-static void timer_i915_sw_fence_wake(unsigned long data)
+static void timer_i915_sw_fence_wake(struct timer_list *t)
{
- struct i915_sw_dma_fence_cb *cb = (struct i915_sw_dma_fence_cb *)data;
+ struct i915_sw_dma_fence_cb *cb = from_timer(cb, t, timer);
struct i915_sw_fence *fence;
fence = xchg(>fence, NULL);
@@ -434,9 +434,7 @@ int i915_sw_fence_await_dma_fence(struct i915_sw_fence
*fence,
i915_sw_fence_await(fence);
cb->dma = NULL;
- __setup_timer(>timer,
- timer_i915_sw_fence_wake, (unsigned long)cb,
- TIMER_IRQSAFE);
+ timer_setup(>timer, timer_i915_sw_fence_wake, TIMER_IRQSAFE);
init_irq_work(>work, irq_i915_sw_fence_work);
if (timeout) {
cb->dma = dma_fence_get(dma);
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c
b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 29c62d481cef..48e1ba01ccf8 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -74,9 +74,10 @@ static noinline void missed_breadcrumb(struct
intel_engine_cs *engine)
set_bit(engine->id, >i915->gpu_error.missed_irq_rings);
}
-static void intel_breadcrumbs_hangcheck(unsigned long data)
+static void intel_breadcrumbs_hangcheck(struct timer_list *t)
{
- struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
+ struct intel_engine_cs *engine = from_timer(engine, t,
+ breadcrumbs.hangcheck);
struct intel_breadcrumbs *b = >breadcrumbs;
if (!b->irq_armed)
@@ -108,9 +109,10 @@ static void intel_breadcrumbs_hangcheck(unsigned long data)
}
}
-static void intel_breadcrumbs_fake_irq(unsigned long data)
+static void intel_breadcrumbs_fake_irq(struct timer_list *t)
{
- struct intel_engine_cs *engine = (struct intel_engine_cs *)data;
+ struct intel_engine_cs *engine = from_timer(engine, t,
+ breadcrumbs.fake_irq);
struct intel_breadcrumbs *b = >breadcrumbs;
/* The timer persists in case we cannot enable interrupts,
@@ -787,12 +789,8 @@ int intel_engine_init_breadcrumbs(struct intel_engine_cs
*engine)
spin_lock_init(>rb_lock);
spin_lock_init(>irq_lock);
- setup_timer(>fake_irq,
- intel_breadcrumbs_fake_irq,
- (unsigned long)engine);
- setup_timer(>hangcheck,
- intel_breadcrumbs_hangcheck,
- (unsigned long)engine);
+ timer_setup(>fake_irq, intel_breadcrumbs_fake_irq, 0);
+ timer_setup(>hangcheck, intel_breadcrumbs_hangcheck, 0);
/* Spawn a thread to provide a common bottom-half for all signals.
* As this is an asynchronous interface we cannot steal the current
diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c
b/drivers/gpu/drm/i915/selftests/mock_engine.c
index fc0fd7498689..331c2b09869e 100644
--- a/drivers/gpu/drm/i915/selftests/mock_engine.c
+++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
@@ -32,9 +32,9 @@ static struct mock_request *first_request(struct mock_engine
*engine)
link);
}
-static void hw_delay_complete(unsigned long data)
+static void hw_delay_complete(struct timer_list *t)
{
- struct mock_engine *engine = (typeof(engine))data;
+ struct mock_engine *engine = from_timer(engine, t, hw_delay);
struct mock_request *request;
spin_lock(>hw_l
[PATCH] video: fbdev: pxa3xx_gcu: Convert timers to use timer_setup()
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
This also fixes the use of the "priv" variable in QERROR(), since it was
pointing to struct timer_list, not struct pxa3xx_gcu_priv.
Cc: Bartlomiej Zolnierkiewicz <b.zolnier...@samsung.com>
Cc: "Gustavo A. R. Silva" <gust...@embeddedor.com>
Cc: Himanshu Jha <himanshujha199...@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-fb...@vger.kernel.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
If you can take this for v4.15, please do. Otherwise, I can carry it in the
timers tree in late rc1. Thanks!
---
drivers/video/fbdev/pxa3xx-gcu.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/video/fbdev/pxa3xx-gcu.c b/drivers/video/fbdev/pxa3xx-gcu.c
index 164fb524744a..208cd5a5e15c 100644
--- a/drivers/video/fbdev/pxa3xx-gcu.c
+++ b/drivers/video/fbdev/pxa3xx-gcu.c
@@ -512,24 +512,26 @@ pxa3xx_gcu_mmap(struct file *file, struct vm_area_struct
*vma)
#ifdef PXA3XX_GCU_DEBUG_TIMER
static struct timer_list pxa3xx_gcu_debug_timer;
+static struct pxa3xx_gcu_priv *debug_timer_priv;
-static void pxa3xx_gcu_debug_timedout(unsigned long ptr)
+static void pxa3xx_gcu_debug_timedout(struct timer_list *unused)
{
- struct pxa3xx_gcu_priv *priv = (struct pxa3xx_gcu_priv *) ptr;
+ struct pxa3xx_gcu_priv *priv = debug_timer_priv;
QERROR("Timer DUMP");
- /* init the timer structure */
- setup_timer(_gcu_debug_timer, pxa3xx_gcu_debug_timedout, ptr);
mod_timer(_gcu_debug_timer, jiffies + 5 * HZ);
}
-static void pxa3xx_gcu_init_debug_timer(void)
+static void pxa3xx_gcu_init_debug_timer(struct pxa3xx_gcu_priv *priv)
{
- pxa3xx_gcu_debug_timedout((unsigned long) _gcu_debug_timer);
+ /* init the timer structure */
+ debug_timer_priv = priv;
+ timer_setup(_gcu_debug_timer, pxa3xx_gcu_debug_timedout, 0);
+ pxa3xx_gcu_debug_timedout(NULL);
}
#else
-static inline void pxa3xx_gcu_init_debug_timer(void) {}
+static inline void pxa3xx_gcu_init_debug_timer(struct pxa3xx_gcu_priv *) {}
#endif
static int
@@ -666,7 +668,7 @@ static int pxa3xx_gcu_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, priv);
priv->resource_mem = r;
pxa3xx_gcu_reset(priv);
- pxa3xx_gcu_init_debug_timer();
+ pxa3xx_gcu_init_debug_timer(priv);
dev_info(dev, "registered @0x%p, DMA 0x%p (%d bytes), IRQ %d\n",
(void *) r->start, (void *) priv->shared_phys,
--
2.7.4
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] video: fbdev: Convert timers to use timer_setup()
On Thu, Nov 9, 2017 at 9:08 AM, Bartlomiej Zolnierkiewicz
<b.zolnier...@samsung.com> wrote:
> On Tuesday, October 24, 2017 08:20:26 AM Kees Cook wrote:
>
>> diff --git a/drivers/video/fbdev/pxa3xx-gcu.c
>> b/drivers/video/fbdev/pxa3xx-gcu.c
>> index 933619da1a94..e88447eac32c 100644
>> --- a/drivers/video/fbdev/pxa3xx-gcu.c
>> +++ b/drivers/video/fbdev/pxa3xx-gcu.c
>> @@ -513,16 +513,10 @@ pxa3xx_gcu_mmap(struct file *file, struct
>> vm_area_struct *vma)
>> #ifdef PXA3XX_GCU_DEBUG_TIMER
>> static struct timer_list pxa3xx_gcu_debug_timer;
>>
>> -static void pxa3xx_gcu_debug_timedout(unsigned long ptr)
>> +static void pxa3xx_gcu_debug_timedout(struct timer_list *unused)
>> {
>> - struct pxa3xx_gcu_priv *priv = (struct pxa3xx_gcu_priv *) ptr;
>> -
>> QERROR("Timer DUMP");
>
> QERROR() macro is using priv so this code now fails to build.
Ah, yes, I see. However, in looking at this, I don't think it ever
worked. The existing code does:
static void pxa3xx_gcu_init_debug_timer(void)
{
pxa3xx_gcu_debug_timedout((unsigned long) _gcu_debug_timer);
}
...
static void pxa3xx_gcu_debug_timedout(unsigned long ptr)
{
struct pxa3xx_gcu_priv *priv = (struct pxa3xx_gcu_priv *) ptr;
QERROR("Timer DUMP");
...
But pxa3xx_gcu_debug_timer is not a struct pxa3xx_gcu_priv, so
QERROR() will be reporting garbage.
I will send a patch that fixes this and switches to timer_setup(),
though it might make more sense to just remove the debug timer
entirely.
> [ Please compile these timer changes with PXA3XX_GCU_DEBUG and
> PXA3XX_GCU_DEBUG_TIMER defined. ]
Hm, these appear to be manual knobs? What do you think about wiring
these up to a Kconfig so build tests for things like "make
allmodconfig" would enable it? This would allow for much greater build
coverage.
> Also please port your changes over fbdvev-for-next tree as
> currently this patch doesn't apply (fbdev tree contains
> "video: fbdev: pxa3xx_gcu: Use setup_timer and mod_timer"
> cleanup).
Sure thing, thanks!
-Kees
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [BUG] drm: vc4: refcount_t: increment on 0; use-after-free.
restore_fbdev_mode_atomic+0x1d8/0x1e8 [drm_kms_helper]
>> > >> [7.813113] restore_fbdev_mode+0x28/0x160 [drm_kms_helper]
>> > >> [7.813223]
>> > >> drm_fb_helper_restore_fbdev_mode_unlocked.part.24+0x28/0x90
>> > >> [drm_kms_helper]
>> > >> [7.813331] drm_fb_helper_set_par+0x54/0xa8 [drm_kms_helper]
>> > >> [7.813346] fbcon_init+0x4e8/0x538
>> > >> [7.813357] visual_init+0xb4/0x108
>> > >> [7.813366] do_bind_con_driver+0x1b8/0x3a0
>> > >> [7.813373] do_take_over_console+0x150/0x1d0
>> > >> [7.813380] do_fbcon_takeover+0x6c/0xf0
>> > >> [7.813387] fbcon_event_notify+0x8fc/0x928
>> > >> [7.813399] notifier_call_chain+0x50/0x90
>> > >> [7.813406] __blocking_notifier_call_chain+0x4c/0x90
>> > >> [7.813413] blocking_notifier_call_chain+0x14/0x20
>> > >> [7.813420] fb_notifier_call_chain+0x1c/0x28
>> > >> [7.813426] register_framebuffer+0x1d0/0x2d8
>> > >> [7.813533] __drm_fb_helper_initial_config_and_unlock+0x1e8/0x350
>> > >> [drm_kms_helper]
>> > >> [7.813639] drm_fb_helper_initial_config+0x40/0x58 [drm_kms_helper]
>> > >> [7.813747] drm_fbdev_cma_init_with_funcs+0x88/0x158
>> > >> [drm_kms_helper]
>> > >> [7.813855] drm_fbdev_cma_init+0x14/0x28 [drm_kms_helper]
>> > >> [7.813943] vc4_kms_load+0xa4/0xf0 [vc4]
>> > >> [7.814026] vc4_drm_bind+0x100/0x168 [vc4]
>> > >> [7.814038] try_to_bring_up_master+0x144/0x1a8
>> > >> [7.814044] component_master_add_with_match+0x9c/0xe0
>> > >> [7.814128] vc4_platform_drm_probe+0xb4/0xf0 [vc4]
>> > >> [7.814137] platform_drv_probe+0x58/0xc0
>> > >> [7.814146] driver_probe_device+0x224/0x308
>> > >> [7.814153] __driver_attach+0xac/0xb0
>> > >> [7.814161] bus_for_each_dev+0x64/0xa0
>> > >> [7.814169] driver_attach+0x20/0x28
>> > >> [7.814177] bus_add_driver+0x108/0x228
>> > >> [7.814184] driver_register+0x60/0xf8
>> > >> [7.814190] __platform_driver_register+0x40/0x48
>> > >> [7.814274] vc4_drm_register+0x38/0x1000 [vc4]
>> > >> [7.814283] do_one_initcall+0x38/0x120
>> > >> [7.814295] do_init_module+0x58/0x1b8
>> > >> [7.814304] load_module+0x1fa8/0x2280
>> > >> [7.814311] SyS_finit_module+0xc0/0xd0
>> > >> [7.814318] __sys_trace_return+0x0/0x4
>> > >> [7.814325] ---[ end trace 3348554eb91e19a1 ]---
>> > >
>> > > Looks like I didn't test this code with CONFIG_REFCOUNT_FULL enabled :-/.
>> > >
>> > > Anyway, can you try to apply the following diff and let me know if it
>> > > fixes the problem?
>> > >
>> > > Thanks,
>> > >
>> > > Boris
>> > >
>> > > --->8---
>> > > diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
>> > > index 4ae45d7dac42..2decc8e2c79f 100644
>> > > --- a/drivers/gpu/drm/vc4/vc4_bo.c
>> > > +++ b/drivers/gpu/drm/vc4/vc4_bo.c
>> > > @@ -637,7 +637,8 @@ int vc4_bo_inc_usecnt(struct vc4_bo *bo)
>> > > mutex_lock(>madv_lock);
>> > > switch (bo->madv) {
>> > > case VC4_MADV_WILLNEED:
>> > > - refcount_inc(>usecnt);
>> > > + if (!refcount_inc_not_zero(>usecnt))
>> > > + refcount_set(>usecnt, 1);
>> >
>> > This is racy.
>>
>> Well, in this case it's not, because this section is protected by a
>> mutex.
>>
>> > You need a refcount_inc_allow_zero (if that exists).
>>
>> I searched for something like that, but it seems that there's no such
>> helper.
>
> In that case I think the right thing is to switch to atomic_t. That means
> no overflow protection, but if we don't have a "this can be 0, I know what
> we're doing" style refcount, then I don't think it's suitable.
>
> Adding Kees to confirm or clarify or ...
Adding Elena for more details. IIRC there have been two cases:
1) refcount == 0 doesn't mean the object has been released. The
solution tends to be a +1 to the count everywhere.
2) refcount == 0 means the object was released, but the refcount gets
reused for the next allocation. The solution tends to be to use
refcount_set() in the new initialization instead of refcount_inc().
It's not clear to me if either apply here, though.
-Kees
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: video: fbdev: Convert timers to use timer_setup()
On Mon, Nov 13, 2017 at 5:45 PM, Guenter Roeck <li...@roeck-us.net> wrote: > On Tue, Oct 24, 2017 at 08:20:26AM -0700, Kees Cook wrote: >> In preparation for unconditionally passing the struct timer_list pointer to >> all timer callbacks, switch to using the new timer_setup() and from_timer() >> to pass the timer pointer explicitly. One tracking pointer was added, and >> one initialization was cleaned up. >> >> Cc: Bartlomiej Zolnierkiewicz <b.zolnier...@samsung.com> >> Cc: Benjamin Herrenschmidt <b...@kernel.crashing.org> >> Cc: Tomi Valkeinen <tomi.valkei...@ti.com> >> Cc: David Lechner <da...@lechnology.com> >> Cc: Daniel Vetter <daniel.vet...@ffwll.ch> >> Cc: Sean Paul <seanp...@chromium.org> >> Cc: Jean Delvare <jdelv...@suse.de> >> Cc: Hans de Goede <hdego...@redhat.com> >> Cc: "Gustavo A. R. Silva" <gust...@embeddedor.com> >> Cc: linux-fb...@vger.kernel.org >> Cc: dri-devel@lists.freedesktop.org >> Cc: linux-o...@vger.kernel.org >> Signed-off-by: Kees Cook <keesc...@chromium.org> > > Hi Kees, > > this patch causes a large number of qemu crashes. > > Unable to handle kernel NULL pointer dereference at virtual address 0194 > pgd = c0004000 > [0194] *pgd= > Internal error: Oops: 5 [#1] ARM > Modules linked in: > CPU: 0 PID: 0 Comm: swapper Not tainted 4.14.0-next-20171113 #1 > Hardware name: ARM-Versatile (Device Tree Support) > task: c04df238 task.stack: c04da000 > PC is at queue_work_on+0x1c/0x48 > ... > [] (queue_work_on) from [] > (cursor_timer_handler+0x20/0x44) > [] (cursor_timer_handler) from [] > (call_timer_fn+0x24/0xa0) > [] (call_timer_fn) from [] (expire_timers+0x7c/0x8c) > [] (expire_timers) from [] (run_timer_softirq+0x88/0x184) > [] (run_timer_softirq) from [] (__do_softirq+0xe0/0x238) > [] (__do_softirq) from [] (irq_exit+0xb4/0xd0) > [] (irq_exit) from [] (__handle_domain_irq+0x50/0xa8) > [] (__handle_domain_irq) from [] > (vic_handle_irq+0x54/0x94) > [] (vic_handle_irq) from [] (__irq_svc+0x68/0x84) > > See > http://kerneltests.org/builders/qemu-arm-next/builds/806/steps/qemubuildcommand/logs/stdio > for complete crash logs. > > Reverting the patch fixes the problem. > > Images for various other architectures crash as well in next-20171113, > but I didn't bisect those. It looks like there are additional (possibly irq > related) problems in the latest -next kernel; I don't know if those are > also related to timer changes. I think this is already fixed here: https://marc.info/?l=linux-fbdev=151056635200583=2 If not, please let me know! :) -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vc4: Convert timers to use timer_setup()
On Mon, Oct 30, 2017 at 4:49 PM, Eric Anholt <e...@anholt.net> wrote: > Kees Cook <keesc...@chromium.org> writes: > >> In preparation for unconditionally passing the struct timer_list pointer to >> all timer callbacks, switch to using the new timer_setup() and from_timer() >> to pass the timer pointer explicitly. > > Reviewed and applied to drm-misc-next. Thanks! Thanks! I happened to notice that this was in next-20171102, but missing in next-20171103. Did it get removed, or am I misunderstanding something? -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: gma500: Convert timers to use timer_setup()
On Tue, Oct 31, 2017 at 3:18 AM, Daniel Vetter <dan...@ffwll.ch> wrote: > On Mon, Oct 30, 2017 at 03:05:29PM -0700, Kees Cook wrote: >> On Mon, Oct 30, 2017 at 3:08 AM, Daniel Vetter <dan...@ffwll.ch> wrote: >> > On Tue, Oct 24, 2017 at 08:16:09AM -0700, Kees Cook wrote: >> >> In preparation for unconditionally passing the struct timer_list pointer >> >> to >> >> all timer callbacks, switch to using the new timer_setup() and >> >> from_timer() >> >> to pass the timer pointer explicitly. >> >> >> >> Cc: Patrik Jakobsson <patrik.r.jakobs...@gmail.com> >> >> Cc: David Airlie <airl...@linux.ie> >> >> Cc: dri-devel@lists.freedesktop.org >> >> Signed-off-by: Kees Cook <keesc...@chromium.org> >> > >> > Do you expect drm folks to apply this, or is this part of a larger >> > refactoring? >> >> If the drm tree includes -rc3, you can carry these. If you don't want >> to carry these and want the timer tree to carry them, we can do that >> too. > > Applied to drm-misc-next for 4.16 (we're way past freeze for 4.15 > already). Since this is one of the few remaining "non-trivial" users of the ancient init_timer() API, would you mind if the timers tree carried this for 4.15? I'm trying to entirely remove the init_timer() API (and if I can, remove the old setup_*timer() API too). Thanks! -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: Use copy_from_user() in fence copying
There's no good reason to separate the access_ok() from the copy,
especially since the access_ok() size is hard-coded instead of using
sizeof(). Instead, just use copy_from_user() directly.
Fixes: cf6e7bac6357 ("drm/i915: Add support for drm syncobjs")
Cc: Jason Ekstrand <ja...@jlekstrand.net>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 435ed95df144..1da703213b17 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -2087,8 +2087,6 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args,
return ERR_PTR(-EINVAL);
user = u64_to_user_ptr(args->cliprects_ptr);
- if (!access_ok(VERIFY_READ, user, nfences * 2 * sizeof(u32)))
- return ERR_PTR(-EFAULT);
fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
__GFP_NOWARN | GFP_KERNEL);
@@ -2099,7 +2097,7 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args,
struct drm_i915_gem_exec_fence fence;
struct drm_syncobj *syncobj;
- if (__copy_from_user(, user++, sizeof(fence))) {
+ if (copy_from_user(, user++, sizeof(fence))) {
err = -EFAULT;
goto err;
}
--
2.7.4
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: Use copy_from_user() in fence copying
On Fri, Dec 8, 2017 at 2:17 AM, David Laight <david.lai...@aculab.com> wrote:
> From: Kees Cook
>> Sent: 06 December 2017 20:29
>>
>> There's no good reason to separate the access_ok() from the copy,
>> especially since the access_ok() size is hard-coded instead of using
>> sizeof(). Instead, just use copy_from_user() directly.
>
> Looks like an optimisation to save doing the access_ok() check
> for every 'fence'.
If it really makes a difference, okay, but access_ok() checks are fast. :P
> OTOH 'user copy hardening' probably makes a much larger performance
> degradation on this kind of code.
> (Might be ok here because probably refers to something in the
> current stack frame.)
Well, the good news there is that it's using sizeof(fence), so no
hardening check is done (it's not a size that changes at runtime).
What I didn't like is that the access_ok() doesn't use sizeof(fence)
(it is currently correct: 2 u32s == sizeof(fence)) but that kind of
fragility keeps me up at night. ;)
So, fixing either would be fine, but if we're going to touch it, it
seems best to just do away with the __copy_*() usage instead.
-Kees
>
> David
>
>> Fixes: cf6e7bac6357 ("drm/i915: Add support for drm syncobjs")
>> Cc: Jason Ekstrand <ja...@jlekstrand.net>
>> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
>> Signed-off-by: Kees Cook <keesc...@chromium.org>
>> ---
>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index 435ed95df144..1da703213b17 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -2087,8 +2087,6 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args,
>> return ERR_PTR(-EINVAL);
>>
>> user = u64_to_user_ptr(args->cliprects_ptr);
>> - if (!access_ok(VERIFY_READ, user, nfences * 2 * sizeof(u32)))
>> - return ERR_PTR(-EFAULT);
>>
>> fences = kvmalloc_array(args->num_cliprects, sizeof(*fences),
>> __GFP_NOWARN | GFP_KERNEL);
>> @@ -2099,7 +2097,7 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args,
>> struct drm_i915_gem_exec_fence fence;
>> struct drm_syncobj *syncobj;
>>
>> - if (__copy_from_user(, user++, sizeof(fence))) {
>> + if (copy_from_user(, user++, sizeof(fence))) {
>> err = -EFAULT;
>> goto err;
>> }
>> --
>> 2.7.4
>>
>>
>> --
>> Kees Cook
>> Pixel Security
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915/selftests: Convert timers to use timer_setup()
On Wed, Oct 25, 2017 at 4:16 PM, Chris Wilson <ch...@chris-wilson.co.uk> wrote: > Quoting Kees Cook (2017-10-25 15:05:13) >> On Wed, Oct 25, 2017 at 3:11 PM, Chris Wilson <ch...@chris-wilson.co.uk> >> wrote: >> > Quoting Chris Wilson (2017-10-25 11:24:19) >> >> Quoting Chris Wilson (2017-10-24 17:17:09) >> >> > Quoting Kees Cook (2017-10-24 16:13:44) >> >> > > In preparation for unconditionally passing the struct timer_list >> >> > > pointer to >> >> > > all timer callbacks, switch to using the new timer_setup() and >> >> > > from_timer() >> >> > > to pass the timer pointer explicitly. >> >> > > >> >> > > Cc: Jani Nikula <jani.nik...@linux.intel.com> >> >> > > Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com> >> >> > > Cc: Rodrigo Vivi <rodrigo.v...@intel.com> >> >> > > Cc: David Airlie <airl...@linux.ie> >> >> > > Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com> >> >> > > Cc: Chris Wilson <ch...@chris-wilson.co.uk> >> >> > > Cc: intel-...@lists.freedesktop.org >> >> > > Cc: dri-devel@lists.freedesktop.org >> >> > > Signed-off-by: Kees Cook <keesc...@chromium.org> >> >> > >> >> > Thank you for saving me from having to do this myself, >> >> > Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk> >> >> >> >> I've a small batch of selftests patches queued, so added this one and >> >> will push to drm-intel-next-queued shortly. >> > >> > Oh dear, major faux pas. There is no timer_setup_on_stack yet. >> >> Argh. Right, sorry. That's only in -next. Since this is mainly a >> mechanical change, should I carry this in the timer tree, or wait >> until the merge window for it to go via i915? > > Jani has the final word, but my understanding is that there will be no > more from i915 towards the 4.15 merge. Hmm, the origin of this timer, > > commit 214707fc2ce08d09982bc4fe4b7a1c1f010e82be > Author: Chris Wilson <ch...@chris-wilson.co.uk> > Date: Thu Oct 12 13:57:25 2017 +0100 > > drm/i915/selftests: Wrap a timer into a i915_sw_fence > > did make it into 4.15, so it would have been better to put into a > separate tree for the 4.15 merge window anyway. In hindsight, yes this > probably wants to be carried in the timer tree to be applied after i915. > (I guess there will be a few other stragglers that need to be converted > at the end of the merge window anyway.) Yeah, it's going to be messy, but I'll manage. I'll be carrying a lot of other stuff as well. Avoiding conflicts will be the trick. Wheee. :) -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915/selftests: Convert timers to use timer_setup()
On Wed, Oct 25, 2017 at 3:11 PM, Chris Wilson <ch...@chris-wilson.co.uk> wrote: > Quoting Chris Wilson (2017-10-25 11:24:19) >> Quoting Chris Wilson (2017-10-24 17:17:09) >> > Quoting Kees Cook (2017-10-24 16:13:44) >> > > In preparation for unconditionally passing the struct timer_list pointer >> > > to >> > > all timer callbacks, switch to using the new timer_setup() and >> > > from_timer() >> > > to pass the timer pointer explicitly. >> > > >> > > Cc: Jani Nikula <jani.nik...@linux.intel.com> >> > > Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com> >> > > Cc: Rodrigo Vivi <rodrigo.v...@intel.com> >> > > Cc: David Airlie <airl...@linux.ie> >> > > Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com> >> > > Cc: Chris Wilson <ch...@chris-wilson.co.uk> >> > > Cc: intel-...@lists.freedesktop.org >> > > Cc: dri-devel@lists.freedesktop.org >> > > Signed-off-by: Kees Cook <keesc...@chromium.org> >> > >> > Thank you for saving me from having to do this myself, >> > Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk> >> >> I've a small batch of selftests patches queued, so added this one and >> will push to drm-intel-next-queued shortly. > > Oh dear, major faux pas. There is no timer_setup_on_stack yet. Argh. Right, sorry. That's only in -next. Since this is mainly a mechanical change, should I carry this in the timer tree, or wait until the merge window for it to go via i915? -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: gma500: Convert timers to use timer_setup()
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
Cc: Patrik Jakobsson <patrik.r.jakobs...@gmail.com>
Cc: David Airlie <airl...@linux.ie>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
drivers/gpu/drm/gma500/psb_lid.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/gma500/psb_lid.c b/drivers/gpu/drm/gma500/psb_lid.c
index 1d2ebb5e530f..be6dda58fcae 100644
--- a/drivers/gpu/drm/gma500/psb_lid.c
+++ b/drivers/gpu/drm/gma500/psb_lid.c
@@ -23,9 +23,9 @@
#include "psb_intel_reg.h"
#include
-static void psb_lid_timer_func(unsigned long data)
+static void psb_lid_timer_func(struct timer_list *t)
{
- struct drm_psb_private * dev_priv = (struct drm_psb_private *)data;
+ struct drm_psb_private *dev_priv = from_timer(dev_priv, t, lid_timer);
struct drm_device *dev = (struct drm_device *)dev_priv->dev;
struct timer_list *lid_timer = _priv->lid_timer;
unsigned long irq_flags;
@@ -77,10 +77,8 @@ void psb_lid_timer_init(struct drm_psb_private *dev_priv)
spin_lock_init(_priv->lid_lock);
spin_lock_irqsave(_priv->lid_lock, irq_flags);
- init_timer(lid_timer);
+ timer_setup(lid_timer, psb_lid_timer_func, 0);
- lid_timer->data = (unsigned long)dev_priv;
- lid_timer->function = psb_lid_timer_func;
lid_timer->expires = jiffies + PSB_LID_DELAY;
add_timer(lid_timer);
--
2.7.4
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] video: fbdev: Convert timers to use timer_setup()
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly. One tracking pointer was added, and
one initialization was cleaned up.
Cc: Bartlomiej Zolnierkiewicz <b.zolnier...@samsung.com>
Cc: Benjamin Herrenschmidt <b...@kernel.crashing.org>
Cc: Tomi Valkeinen <tomi.valkei...@ti.com>
Cc: David Lechner <da...@lechnology.com>
Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
Cc: Sean Paul <seanp...@chromium.org>
Cc: Jean Delvare <jdelv...@suse.de>
Cc: Hans de Goede <hdego...@redhat.com>
Cc: "Gustavo A. R. Silva" <gust...@embeddedor.com>
Cc: linux-fb...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-o...@vger.kernel.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
drivers/video/fbdev/aty/radeon_base.c | 8 +++-
drivers/video/fbdev/core/fbcon.c | 10 +-
drivers/video/fbdev/core/fbcon.h | 1 +
drivers/video/fbdev/omap/hwa742.c | 6 ++
drivers/video/fbdev/omap2/omapfb/dss/dsi.c | 6 ++
drivers/video/fbdev/pxa3xx-gcu.c | 12
6 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/drivers/video/fbdev/aty/radeon_base.c
b/drivers/video/fbdev/aty/radeon_base.c
index 8ad1643e7d1c..4d77daeecf99 100644
--- a/drivers/video/fbdev/aty/radeon_base.c
+++ b/drivers/video/fbdev/aty/radeon_base.c
@@ -1454,9 +1454,9 @@ static void radeon_write_pll_regs(struct radeonfb_info
*rinfo, struct radeon_reg
/*
* Timer function for delayed LVDS panel power up/down
*/
-static void radeon_lvds_timer_func(unsigned long data)
+static void radeon_lvds_timer_func(struct timer_list *t)
{
- struct radeonfb_info *rinfo = (struct radeonfb_info *)data;
+ struct radeonfb_info *rinfo = from_timer(rinfo, t, lvds_timer);
radeon_engine_idle();
@@ -2291,9 +2291,7 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
rinfo->pdev = pdev;
spin_lock_init(>reg_lock);
- init_timer(>lvds_timer);
- rinfo->lvds_timer.function = radeon_lvds_timer_func;
- rinfo->lvds_timer.data = (unsigned long)rinfo;
+ timer_setup(>lvds_timer, radeon_lvds_timer_func, 0);
c1 = ent->device >> 8;
c2 = ent->device & 0xff;
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 04612f938bab..3b4a96379128 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -395,10 +395,10 @@ static void fb_flashcursor(struct work_struct *work)
console_unlock();
}
-static void cursor_timer_handler(unsigned long dev_addr)
+static void cursor_timer_handler(struct timer_list *t)
{
- struct fb_info *info = (struct fb_info *) dev_addr;
- struct fbcon_ops *ops = info->fbcon_par;
+ struct fbcon_ops *ops = from_timer(ops, t, cursor_timer);
+ struct fb_info *info = ops->info;
queue_work(system_power_efficient_wq, >queue);
mod_timer(>cursor_timer, jiffies + ops->cur_blink_jiffies);
@@ -414,8 +414,7 @@ static void fbcon_add_cursor_timer(struct fb_info *info)
if (!info->queue.func)
INIT_WORK(>queue, fb_flashcursor);
- setup_timer(>cursor_timer, cursor_timer_handler,
- (unsigned long) info);
+ timer_setup(>cursor_timer, cursor_timer_handler, 0);
mod_timer(>cursor_timer, jiffies + ops->cur_blink_jiffies);
ops->flags |= FBCON_FLAGS_CURSOR_TIMER;
}
@@ -714,6 +713,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc,
struct fb_info *info,
if (!err) {
ops->cur_blink_jiffies = HZ / 5;
+ ops->info = info;
info->fbcon_par = ops;
if (vc)
diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h
index 18f3ac144237..9f7744fbc962 100644
--- a/drivers/video/fbdev/core/fbcon.h
+++ b/drivers/video/fbdev/core/fbcon.h
@@ -69,6 +69,7 @@ struct fbcon_ops {
struct timer_list cursor_timer; /* Cursor timer */
struct fb_cursor cursor_state;
struct display *p;
+ struct fb_info *info;
intcurrcon;/* Current VC. */
intcur_blink_jiffies;
intcursor_flash;
diff --git a/drivers/video/fbdev/omap/hwa742.c
b/drivers/video/fbdev/omap/hwa742.c
index a4ee65b8f918..6199d4806193 100644
--- a/drivers/video/fbdev/omap/hwa742.c
+++ b/drivers/video/fbdev/omap/hwa742.c
@@ -474,7 +474,7 @@ static void auto_update_complete(void *data)
jiffies + HWA742_AUTO_UPDATE_TIME);
}
-static void hwa742_update_window_auto(unsigned long arg)
+static void hwa742_update_window_auto(struct timer_list *unused)
{
[PATCH] drm/vc4: Convert timers to use timer_setup()
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
Cc: Eric Anholt <e...@anholt.net>
Cc: David Airlie <airl...@linux.ie>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
drivers/gpu/drm/vc4/vc4_bo.c | 9 +++--
drivers/gpu/drm/vc4/vc4_gem.c | 10 --
2 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
index 3afdbf4bc10b..ee07e072cd2a 100644
--- a/drivers/gpu/drm/vc4/vc4_bo.c
+++ b/drivers/gpu/drm/vc4/vc4_bo.c
@@ -461,10 +461,9 @@ static void vc4_bo_cache_time_work(struct work_struct
*work)
mutex_unlock(>bo_lock);
}
-static void vc4_bo_cache_time_timer(unsigned long data)
+static void vc4_bo_cache_time_timer(struct timer_list *t)
{
- struct drm_device *dev = (struct drm_device *)data;
- struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct vc4_dev *vc4 = from_timer(vc4, t, bo_cache.time_timer);
schedule_work(>bo_cache.time_work);
}
@@ -768,9 +767,7 @@ int vc4_bo_cache_init(struct drm_device *dev)
INIT_LIST_HEAD(>bo_cache.time_list);
INIT_WORK(>bo_cache.time_work, vc4_bo_cache_time_work);
- setup_timer(>bo_cache.time_timer,
- vc4_bo_cache_time_timer,
- (unsigned long)dev);
+ timer_setup(>bo_cache.time_timer, vc4_bo_cache_time_timer, 0);
return 0;
}
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index d0c6bfb68c4e..ce6bca06f937 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -281,10 +281,10 @@ vc4_reset_work(struct work_struct *work)
}
static void
-vc4_hangcheck_elapsed(unsigned long data)
+vc4_hangcheck_elapsed(struct timer_list *t)
{
- struct drm_device *dev = (struct drm_device *)data;
- struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct vc4_dev *vc4 = from_timer(vc4, t, hangcheck.timer);
+ struct drm_device *dev = vc4->dev;
uint32_t ct0ca, ct1ca;
unsigned long irqflags;
struct vc4_exec_info *bin_exec, *render_exec;
@@ -1091,9 +1091,7 @@ vc4_gem_init(struct drm_device *dev)
spin_lock_init(>job_lock);
INIT_WORK(>hangcheck.reset_work, vc4_reset_work);
- setup_timer(>hangcheck.timer,
- vc4_hangcheck_elapsed,
- (unsigned long)dev);
+ timer_setup(>hangcheck.timer, vc4_hangcheck_elapsed, 0);
INIT_WORK(>job_done_work, vc4_job_done_work);
--
2.7.4
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915/selftests: Convert timers to use timer_setup()
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
Cc: Jani Nikula <jani.nik...@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
Cc: David Airlie <airl...@linux.ie>
Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: intel-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
drivers/gpu/drm/i915/selftests/lib_sw_fence.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/selftests/lib_sw_fence.c
b/drivers/gpu/drm/i915/selftests/lib_sw_fence.c
index 3790fdf44a1a..b26f07b55d86 100644
--- a/drivers/gpu/drm/i915/selftests/lib_sw_fence.c
+++ b/drivers/gpu/drm/i915/selftests/lib_sw_fence.c
@@ -49,9 +49,9 @@ void onstack_fence_fini(struct i915_sw_fence *fence)
i915_sw_fence_fini(fence);
}
-static void timed_fence_wake(unsigned long data)
+static void timed_fence_wake(struct timer_list *t)
{
- struct timed_fence *tf = (struct timed_fence *)data;
+ struct timed_fence *tf = from_timer(tf, t, timer);
i915_sw_fence_commit(>fence);
}
@@ -60,7 +60,7 @@ void timed_fence_init(struct timed_fence *tf, unsigned long
expires)
{
onstack_fence_init(>fence);
- setup_timer_on_stack(>timer, timed_fence_wake, (unsigned long)tf);
+ timer_setup_on_stack(>timer, timed_fence_wake, 0);
if (time_after(expires, jiffies))
mod_timer(>timer, expires);
--
2.7.4
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: gma500: Convert timers to use timer_setup()
On Mon, Oct 30, 2017 at 3:08 AM, Daniel Vetter <dan...@ffwll.ch> wrote: > On Tue, Oct 24, 2017 at 08:16:09AM -0700, Kees Cook wrote: >> In preparation for unconditionally passing the struct timer_list pointer to >> all timer callbacks, switch to using the new timer_setup() and from_timer() >> to pass the timer pointer explicitly. >> >> Cc: Patrik Jakobsson <patrik.r.jakobs...@gmail.com> >> Cc: David Airlie <airl...@linux.ie> >> Cc: dri-devel@lists.freedesktop.org >> Signed-off-by: Kees Cook <keesc...@chromium.org> > > Do you expect drm folks to apply this, or is this part of a larger > refactoring? If the drm tree includes -rc3, you can carry these. If you don't want to carry these and want the timer tree to carry them, we can do that too. > A notch more context in the commit message would help ... Sorry about that, my added context for this go lost in later conversion patches. -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/nouveau/secboot: remove VLA usage
On Thu, Mar 15, 2018 at 7:05 PM, Ben Skeggs <skeg...@gmail.com> wrote: > On 14 March 2018 at 21:08, Thierry Reding <thierry.red...@gmail.com> wrote: >> On Tue, Mar 13, 2018 at 11:24:11AM -0500, Gustavo A. R. Silva wrote: >>> In preparation to enabling -Wvla, remove VLA. In this particular >>> case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local >>> variable cmdline_size. Also, remove cmdline_size as it is not >>> actually useful anymore. >>> >>> The use of stack Variable Length Arrays needs to be avoided, as they >>> can be a vector for stack exhaustion, which can be both a runtime bug >>> or a security flaw. Also, in general, as code evolves it is easy to >>> lose track of how big a VLA can get. Thus, we can end up having runtime >>> failures that are hard to debug. >>> >>> Also, fixed as part of the directive to remove all VLAs from >>> the kernel: https://lkml.org/lkml/2018/3/7/621 >>> >>> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com> >>> --- >>> Changes in v2: >>> - Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change >>>is based on the feedback provided by David Laight. Thanks David. >>> >>> drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 +++ >>> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> Reviewed-by: Thierry Reding <tred...@nvidia.com> > Thanks everyone. I've taken the patch in my tree. Hi! Just checking in on this -- I don't see this patch in linux-next. Is this queued somewhere else? Thanks! -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/nouveau/secboot: remove VLA usage
On Wed, May 23, 2018 at 5:36 PM, Ben Skeggs <bske...@redhat.com> wrote: > On Thu, May 24, 2018 at 8:48 AM, Kees Cook <keesc...@chromium.org> wrote: >> On Thu, Apr 26, 2018 at 4:25 PM, Kees Cook <keesc...@chromium.org> wrote: >>> On Thu, Mar 15, 2018 at 7:05 PM, Ben Skeggs <skeg...@gmail.com> wrote: >>>> On 14 March 2018 at 21:08, Thierry Reding <thierry.red...@gmail.com> wrote: >>>>> On Tue, Mar 13, 2018 at 11:24:11AM -0500, Gustavo A. R. Silva wrote: >>>>>> In preparation to enabling -Wvla, remove VLA. In this particular >>>>>> case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local >>>>>> variable cmdline_size. Also, remove cmdline_size as it is not >>>>>> actually useful anymore. >>>>>> >>>>>> The use of stack Variable Length Arrays needs to be avoided, as they >>>>>> can be a vector for stack exhaustion, which can be both a runtime bug >>>>>> or a security flaw. Also, in general, as code evolves it is easy to >>>>>> lose track of how big a VLA can get. Thus, we can end up having runtime >>>>>> failures that are hard to debug. >>>>>> >>>>>> Also, fixed as part of the directive to remove all VLAs from >>>>>> the kernel: https://lkml.org/lkml/2018/3/7/621 >>>>>> >>>>>> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com> >>>>>> --- >>>>>> Changes in v2: >>>>>> - Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change >>>>>>is based on the feedback provided by David Laight. Thanks David. >>>>>> >>>>>> drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 >>>>>> +++ >>>>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>>>> >>>>> Reviewed-by: Thierry Reding <tred...@nvidia.com> >>>> Thanks everyone. I've taken the patch in my tree. >>> >>> Hi! >>> >>> Just checking in on this -- I don't see this patch in linux-next. Is >>> this queued somewhere else? >> >> Hi, it's been another month; I still don't see this in linux-next. >> Daniel, can this go via drm-misc? > It's already queued in drm-next. Ah-ha, great, thanks! Looks like I just got unlucky with linux-next pausing on the 17th and this getting committed on the 18th. :) But, yes, I see it now: https://cgit.freedesktop.org/drm/drm/commit/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c?id=7bf5b70befd7817b9e42acbd2291b2042ea1bf81 -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/nouveau/secboot: remove VLA usage
On Thu, Apr 26, 2018 at 4:25 PM, Kees Cook <keesc...@chromium.org> wrote: > On Thu, Mar 15, 2018 at 7:05 PM, Ben Skeggs <skeg...@gmail.com> wrote: >> On 14 March 2018 at 21:08, Thierry Reding <thierry.red...@gmail.com> wrote: >>> On Tue, Mar 13, 2018 at 11:24:11AM -0500, Gustavo A. R. Silva wrote: >>>> In preparation to enabling -Wvla, remove VLA. In this particular >>>> case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local >>>> variable cmdline_size. Also, remove cmdline_size as it is not >>>> actually useful anymore. >>>> >>>> The use of stack Variable Length Arrays needs to be avoided, as they >>>> can be a vector for stack exhaustion, which can be both a runtime bug >>>> or a security flaw. Also, in general, as code evolves it is easy to >>>> lose track of how big a VLA can get. Thus, we can end up having runtime >>>> failures that are hard to debug. >>>> >>>> Also, fixed as part of the directive to remove all VLAs from >>>> the kernel: https://lkml.org/lkml/2018/3/7/621 >>>> >>>> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com> >>>> --- >>>> Changes in v2: >>>> - Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change >>>>is based on the feedback provided by David Laight. Thanks David. >>>> >>>> drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 >>>> +++ >>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> Reviewed-by: Thierry Reding <tred...@nvidia.com> >> Thanks everyone. I've taken the patch in my tree. > > Hi! > > Just checking in on this -- I don't see this patch in linux-next. Is > this queued somewhere else? Hi, it's been another month; I still don't see this in linux-next. Daniel, can this go via drm-misc? -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/nouveau/secboot/acr: Remove VLA usage
In the quest to remove all stack VLA usage from the kernel[1], this
allocates the working buffers before starting the writing so it won't
abort in the middle. This needs an initial walk of the lists to figure
out how large the buffer should be.
[1]
https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
.../nouveau/nvkm/subdev/secboot/acr_r352.c| 25 ---
.../nouveau/nvkm/subdev/secboot/acr_r367.c| 16 +++-
2 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
index a721354249ce..d02e183717dc 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
@@ -414,6 +414,20 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
{
struct ls_ucode_img *_img;
u32 pos = 0;
+ u32 max_desc_size = 0;
+ u8 *gdesc;
+
+ /* Figure out how large we need gdesc to be. */
+ list_for_each_entry(_img, imgs, node) {
+ const struct acr_r352_ls_func *ls_func =
+ acr->func->ls_func[_img->falcon_id];
+
+ max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
+ }
+
+ gdesc = kmalloc(max_desc_size, GFP_KERNEL);
+ if (!gdesc)
+ return -ENOMEM;
nvkm_kmap(wpr_blob);
@@ -421,7 +435,6 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
struct ls_ucode_img_r352 *img = ls_ucode_img_r352(_img);
const struct acr_r352_ls_func *ls_func =
acr->func->ls_func[_img->falcon_id];
- u8 gdesc[ls_func->bl_desc_size];
nvkm_gpuobj_memcpy_to(wpr_blob, pos, >wpr_header,
sizeof(img->wpr_header));
@@ -447,6 +460,8 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
nvkm_done(wpr_blob);
+ kfree(gdesc);
+
return 0;
}
@@ -771,7 +786,11 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
*falcon,
struct fw_bl_desc *hsbl_desc;
void *bl, *blob_data, *hsbl_code, *hsbl_data;
u32 code_size;
- u8 bl_desc[bl_desc_size];
+ u8 *bl_desc;
+
+ bl_desc = kzalloc(bl_desc_size, GFP_KERNEL);
+ if (!bl_desc)
+ return -ENOMEM;
/* Find the bootloader descriptor for our blob and copy it */
if (blob == acr->load_blob) {
@@ -802,7 +821,6 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
*falcon,
code_size, hsbl_desc->start_tag, 0, false);
/* Generate the BL header */
- memset(bl_desc, 0, bl_desc_size);
acr->func->generate_hs_bl_desc(load_hdr, bl_desc, offset);
/*
@@ -811,6 +829,7 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
*falcon,
nvkm_falcon_load_dmem(falcon, bl_desc, hsbl_desc->dmem_load_off,
bl_desc_size, 0);
+ kfree(bl_desc);
return hsbl_desc->start_tag << 8;
}
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
index 866877b88797..978ad0790367 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
@@ -265,6 +265,19 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
{
struct ls_ucode_img *_img;
u32 pos = 0;
+ u32 max_desc_size = 0;
+ u8 *gdesc;
+
+ list_for_each_entry(_img, imgs, node) {
+ const struct acr_r352_ls_func *ls_func =
+ acr->func->ls_func[_img->falcon_id];
+
+ max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
+ }
+
+ gdesc = kmalloc(max_desc_size, GFP_KERNEL);
+ if (!gdesc)
+ return -ENOMEM;
nvkm_kmap(wpr_blob);
@@ -272,7 +285,6 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
struct ls_ucode_img_r367 *img = ls_ucode_img_r367(_img);
const struct acr_r352_ls_func *ls_func =
acr->func->ls_func[_img->falcon_id];
- u8 gdesc[ls_func->bl_desc_size];
nvkm_gpuobj_memcpy_to(wpr_blob, pos, >wpr_header,
sizeof(img->wpr_header));
@@ -298,6 +310,8 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct
list_head *imgs,
nvkm_done(wpr_blob);
+ kfree(gdesc);
+
return 0;
}
--
2.17.0
--
Kees Cook
Pixel Security
__
Re: [PATCH] drm/gma500: Remove VLA
On Mon, Apr 9, 2018 at 2:06 PM, Laura Abbott <labb...@redhat.com> wrote:
>
> There's an ongoing effort to remove VLAs[1] from the kernel to eventually
> turn on -Wvla. Switch to a reasonable upper bound for the VLAs in
> the gma500 driver.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Laura Abbott <labb...@redhat.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Daniel, can this go via you, or what's the best path for this patch?
Thanks!
-Kees
> ---
> This was a little hard to figure out but I think 32 should be a
> comfortable upper bound based on all the structures I saw. Of course I
> can't test it.
> ---
> drivers/gpu/drm/gma500/psb_intel_sdvo.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> index 84507912be84..3d4fa9f6b94c 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> @@ -429,13 +429,20 @@ static const char *cmd_status_names[] = {
> "Scaling not supported"
> };
>
> +#define MAX_ARG_LEN 32
> +
> static bool psb_intel_sdvo_write_cmd(struct psb_intel_sdvo *psb_intel_sdvo,
> u8 cmd,
> const void *args, int args_len)
> {
> - u8 buf[args_len*2 + 2], status;
> - struct i2c_msg msgs[args_len + 3];
> + u8 buf[MAX_ARG_LEN*2 + 2], status;
> + struct i2c_msg msgs[MAX_ARG_LEN + 3];
> int i, ret;
>
> + if (args_len > MAX_ARG_LEN) {
> + DRM_ERROR("Need to increase arg length\n");
> + return false;
> + }
> +
> psb_intel_sdvo_debug_write(psb_intel_sdvo, cmd, args, args_len);
>
> for (i = 0; i < args_len; i++) {
> --
> 2.14.3
>
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv2] drm/i2c: tda998x: Remove VLA usage
On Tue, Apr 10, 2018 at 6:03 PM, Laura Abbott <labb...@redhat.com> wrote:
> There's an ongoing effort to remove VLAs[1] from the kernel to eventually
> turn on -Wvla. The vla in reg_write_range is based on the length of data
> passed. The one use of a non-constant size for this range is bounded by
> the size buffer passed to hdmi_infoframe_pack which is a fixed size.
> Switch to this upper bound.
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Laura Abbott <labb...@redhat.com>
Reviewed-by: Kees Cook <keesc...@chromium.org>
Same question for this patch: who's best to take this?
Thanks!
-Kees
> ---
> v2: Switch to make the buffer size more transparent and add a bounds
> check.
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 9e67a7b4e3a4..c8b6029b7839 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -466,13 +466,22 @@ reg_read_range(struct tda998x_priv *priv, u16 reg, char
> *buf, int cnt)
> return ret;
> }
>
> +#define MAX_WRITE_RANGE_BUF 32
> +
> static void
> reg_write_range(struct tda998x_priv *priv, u16 reg, u8 *p, int cnt)
> {
> struct i2c_client *client = priv->hdmi;
> - u8 buf[cnt+1];
> + /* This is the maximum size of the buffer passed in */
> + u8 buf[MAX_WRITE_RANGE_BUF + 1];
> int ret;
>
> + if (cnt > MAX_WRITE_RANGE_BUF) {
> + dev_err(>dev, "Fixed write buffer too small (%d)\n",
> + MAX_WRITE_RANGE_BUF);
> + return;
> + }
> +
> buf[0] = REG2ADDR(reg);
> memcpy([1], p, cnt);
>
> @@ -679,7 +688,7 @@ static void
> tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr,
> union hdmi_infoframe *frame)
> {
> - u8 buf[32];
> + u8 buf[MAX_WRITE_RANGE_BUF];
> ssize_t len;
>
> len = hdmi_infoframe_pack(frame, buf, sizeof(buf));
> --
> 2.14.3
>
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Nouveau] [PATCH] drm/nouveau/secboot/acr: Remove VLA usage
On Fri, Jun 22, 2018 at 2:40 PM, Karol Herbst wrote: > Your patch is fine as it is though, because it doesn't add a new > issue, it just showed us there is a potential one. We should keep that > in mind and see how we want to fix that up. I can imagine that this > might cause some issues in some places, maybe it is totally fine. Okay, thanks! Who can take the patch into their tree? -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Nouveau] [PATCH] drm/nouveau/secboot/acr: Remove VLA usage
On Fri, Jun 22, 2018 at 10:50 AM, Karol Herbst wrote:
> On Thu, May 24, 2018 at 7:24 PM, Kees Cook wrote:
>> In the quest to remove all stack VLA usage from the kernel[1], this
>> allocates the working buffers before starting the writing so it won't
>> abort in the middle. This needs an initial walk of the lists to figure
>> out how large the buffer should be.
>>
>> [1]
>> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>>
>> Signed-off-by: Kees Cook
>> ---
>> .../nouveau/nvkm/subdev/secboot/acr_r352.c| 25 ---
>> .../nouveau/nvkm/subdev/secboot/acr_r367.c| 16 +++-
>> 2 files changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>> b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>> index a721354249ce..d02e183717dc 100644
>> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
>> @@ -414,6 +414,20 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
>> list_head *imgs,
>> {
>> struct ls_ucode_img *_img;
>> u32 pos = 0;
>> + u32 max_desc_size = 0;
>> + u8 *gdesc;
>> +
>> + /* Figure out how large we need gdesc to be. */
>> + list_for_each_entry(_img, imgs, node) {
>> + const struct acr_r352_ls_func *ls_func =
>> +
>> acr->func->ls_func[_img->falcon_id];
>> +
>> + max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
>> + }
>> +
>> + gdesc = kmalloc(max_desc_size, GFP_KERNEL);
>> + if (!gdesc)
>> + return -ENOMEM;
>>
>> nvkm_kmap(wpr_blob);
>>
>> @@ -421,7 +435,6 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
>> list_head *imgs,
>> struct ls_ucode_img_r352 *img = ls_ucode_img_r352(_img);
>> const struct acr_r352_ls_func *ls_func =
>>
>> acr->func->ls_func[_img->falcon_id];
>> - u8 gdesc[ls_func->bl_desc_size];
>>
>
> if there are no guarantees that (ls_func->bl_desc_size & 0x4 == 0),
> then we need to memset a bit more, because 4 bytes at the time are
> actually copied inside nvkm_gpuobj_memcpy_to later in that code, but
> the last 4 bytes are only partly memset to 0.
I think this is unchanged from the original code, yes? The memset() is
always against bl_desc_size; I haven't changed that.
> If ls_func->bl_desc_size is always a multiple of 0x4, then it isn't as
> important, but still better to be fixed. Or maybe
> nvkm_gpuobj_memcpy_to should do that handling and check if the size is
> a multiple of 0x4 and otherwise handle that case?
>
> Same is valid for the changes in the r367 file.
Should I resend with both the allocation and the memset getting
rounded up to the next multiple of 4?
-Kees
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amdgpu/pm: Remove VLA usage
In the quest to remove all stack VLA usage from the kernel[1], this
uses the maximum sane buffer size and removes copy/paste code.
[1]
https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
Signed-off-by: Kees Cook
---
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 100 +++--
1 file changed, 42 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index b455da487782..5eb98cde22ed 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -593,40 +593,59 @@ static ssize_t amdgpu_get_pp_dpm_sclk(struct device *dev,
return snprintf(buf, PAGE_SIZE, "\n");
}
-static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
- struct device_attribute *attr,
- const char *buf,
- size_t count)
+/*
+ * Worst case: 32 bits individually specified, in octal at 12 characters
+ * per line (+1 for \n).
+ */
+#define AMDGPU_MASK_BUF_MAX(32 * 13)
+
+static ssize_t amdgpu_read_mask(const char *buf, size_t count, uint32_t *mask)
{
- struct drm_device *ddev = dev_get_drvdata(dev);
- struct amdgpu_device *adev = ddev->dev_private;
int ret;
long level;
- uint32_t mask = 0;
char *sub_str = NULL;
char *tmp;
- char buf_cpy[count];
+ char buf_cpy[AMDGPU_MASK_BUF_MAX + 1];
const char delimiter[3] = {' ', '\n', '\0'};
+ size_t bytes;
- memcpy(buf_cpy, buf, count+1);
+ *mask = 0;
+
+ bytes = min(count, sizeof(buf_cpy) - 1);
+ memcpy(buf_cpy, buf, bytes);
+ buf_cpy[bytes] = '\0';
tmp = buf_cpy;
while (tmp[0]) {
- sub_str = strsep(, delimiter);
+ sub_str = strsep(, delimiter);
if (strlen(sub_str)) {
ret = kstrtol(sub_str, 0, );
-
- if (ret) {
- count = -EINVAL;
- goto fail;
- }
- mask |= 1 << level;
+ if (ret)
+ return -EINVAL;
+ *mask |= 1 << level;
} else
break;
}
+
+ return 0;
+}
+
+static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t count)
+{
+ struct drm_device *ddev = dev_get_drvdata(dev);
+ struct amdgpu_device *adev = ddev->dev_private;
+ int ret;
+ uint32_t mask = 0;
+
+ ret = amdgpu_read_mask(buf, count, );
+ if (ret)
+ return ret;
+
if (adev->powerplay.pp_funcs->force_clock_level)
amdgpu_dpm_force_clock_level(adev, PP_SCLK, mask);
-fail:
return count;
}
@@ -651,32 +670,15 @@ static ssize_t amdgpu_set_pp_dpm_mclk(struct device *dev,
struct drm_device *ddev = dev_get_drvdata(dev);
struct amdgpu_device *adev = ddev->dev_private;
int ret;
- long level;
uint32_t mask = 0;
- char *sub_str = NULL;
- char *tmp;
- char buf_cpy[count];
- const char delimiter[3] = {' ', '\n', '\0'};
- memcpy(buf_cpy, buf, count+1);
- tmp = buf_cpy;
- while (tmp[0]) {
- sub_str = strsep(, delimiter);
- if (strlen(sub_str)) {
- ret = kstrtol(sub_str, 0, );
+ ret = amdgpu_read_mask(buf, count, );
+ if (ret)
+ return ret;
- if (ret) {
- count = -EINVAL;
- goto fail;
- }
- mask |= 1 << level;
- } else
- break;
- }
if (adev->powerplay.pp_funcs->force_clock_level)
amdgpu_dpm_force_clock_level(adev, PP_MCLK, mask);
-fail:
return count;
}
@@ -701,33 +703,15 @@ static ssize_t amdgpu_set_pp_dpm_pcie(struct device *dev,
struct drm_device *ddev = dev_get_drvdata(dev);
struct amdgpu_device *adev = ddev->dev_private;
int ret;
- long level;
uint32_t mask = 0;
- char *sub_str = NULL;
- char *tmp;
- char buf_cpy[count];
- const char delimiter[3] = {' ', '\n', '\0'};
-
- memcpy(buf_cpy, buf, count+1);
- tmp = buf_cpy;
- while (tmp[0]) {
- sub_str = strsep(, delimiter);
- if (strlen(sub_str)) {
- ret = kstrtol(sub_str, 0, );
+ ret = amdgpu_read_mask(buf, count, );
+ if (ret)
+ return ret;
- if (ret) {
- count = -EINVAL;
- goto fail;
-
[PATCH] drm/i2c: tda9950: Remove VLA usage
In the quest to remove all stack VLA usage from the kernel[1], this
sets the buffer to maximum size and adds a sanity check.
[1]
https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
Signed-off-by: Kees Cook
---
drivers/gpu/drm/i2c/tda9950.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda9950.c b/drivers/gpu/drm/i2c/tda9950.c
index 3f7396caad48..28314433c351 100644
--- a/drivers/gpu/drm/i2c/tda9950.c
+++ b/drivers/gpu/drm/i2c/tda9950.c
@@ -76,9 +76,12 @@ struct tda9950_priv {
static int tda9950_write_range(struct i2c_client *client, u8 addr, u8 *p, int
cnt)
{
struct i2c_msg msg;
- u8 buf[cnt + 1];
+ u8 buf[CEC_MAX_MSG_SIZE + 3];
int ret;
+ if (WARN_ON(cnt > sizeof(buf)))
+ return -EINVAL;
+
buf[0] = addr;
memcpy(buf + 1, p, cnt);
--
2.17.1
--
Kees Cook
Pixel Security
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/nouveau/secboot/acr: Remove VLA usage
On Thu, May 24, 2018 at 10:24 AM, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> allocates the working buffers before starting the writing so it won't
> abort in the middle. This needs an initial walk of the lists to figure
> out how large the buffer should be.
>
> [1]
> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>
> Signed-off-by: Kees Cook
Friendly ping. Who is best to take this patch?
Thanks!
-Kees
> ---
> .../nouveau/nvkm/subdev/secboot/acr_r352.c| 25 ---
> .../nouveau/nvkm/subdev/secboot/acr_r367.c| 16 +++-
> 2 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
> index a721354249ce..d02e183717dc 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r352.c
> @@ -414,6 +414,20 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
> list_head *imgs,
> {
> struct ls_ucode_img *_img;
> u32 pos = 0;
> + u32 max_desc_size = 0;
> + u8 *gdesc;
> +
> + /* Figure out how large we need gdesc to be. */
> + list_for_each_entry(_img, imgs, node) {
> + const struct acr_r352_ls_func *ls_func =
> +
> acr->func->ls_func[_img->falcon_id];
> +
> + max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
> + }
> +
> + gdesc = kmalloc(max_desc_size, GFP_KERNEL);
> + if (!gdesc)
> + return -ENOMEM;
>
> nvkm_kmap(wpr_blob);
>
> @@ -421,7 +435,6 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
> list_head *imgs,
> struct ls_ucode_img_r352 *img = ls_ucode_img_r352(_img);
> const struct acr_r352_ls_func *ls_func =
>
> acr->func->ls_func[_img->falcon_id];
> - u8 gdesc[ls_func->bl_desc_size];
>
> nvkm_gpuobj_memcpy_to(wpr_blob, pos, >wpr_header,
> sizeof(img->wpr_header));
> @@ -447,6 +460,8 @@ acr_r352_ls_write_wpr(struct acr_r352 *acr, struct
> list_head *imgs,
>
> nvkm_done(wpr_blob);
>
> + kfree(gdesc);
> +
> return 0;
> }
>
> @@ -771,7 +786,11 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
> *falcon,
> struct fw_bl_desc *hsbl_desc;
> void *bl, *blob_data, *hsbl_code, *hsbl_data;
> u32 code_size;
> - u8 bl_desc[bl_desc_size];
> + u8 *bl_desc;
> +
> + bl_desc = kzalloc(bl_desc_size, GFP_KERNEL);
> + if (!bl_desc)
> + return -ENOMEM;
>
> /* Find the bootloader descriptor for our blob and copy it */
> if (blob == acr->load_blob) {
> @@ -802,7 +821,6 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
> *falcon,
> code_size, hsbl_desc->start_tag, 0, false);
>
> /* Generate the BL header */
> - memset(bl_desc, 0, bl_desc_size);
> acr->func->generate_hs_bl_desc(load_hdr, bl_desc, offset);
>
> /*
> @@ -811,6 +829,7 @@ acr_r352_load(struct nvkm_acr *_acr, struct nvkm_falcon
> *falcon,
> nvkm_falcon_load_dmem(falcon, bl_desc, hsbl_desc->dmem_load_off,
> bl_desc_size, 0);
>
> + kfree(bl_desc);
> return hsbl_desc->start_tag << 8;
> }
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
> index 866877b88797..978ad0790367 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/acr_r367.c
> @@ -265,6 +265,19 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct
> list_head *imgs,
> {
> struct ls_ucode_img *_img;
> u32 pos = 0;
> + u32 max_desc_size = 0;
> + u8 *gdesc;
> +
> + list_for_each_entry(_img, imgs, node) {
> + const struct acr_r352_ls_func *ls_func =
> +
> acr->func->ls_func[_img->falcon_id];
> +
> + max_desc_size = max(max_desc_size, ls_func->bl_desc_size);
> + }
> +
> + gdesc = kmalloc(max_desc_size, GFP_KERNEL);
> + if (!gdesc)
> + return -ENOMEM;
>
> nvkm_kmap(wpr_blob);
>
> @@ -272,7 +285,6 @@ acr_r367_ls_write_wpr(struct acr_r352 *acr, struct
>
Re: [RFC][PATCH] Makefile: globally enable VLA warning
On Tue, Jun 26, 2018 at 1:21 PM, Joe Perches wrote: > On Tue, 2018-06-26 at 10:40 -0700, Kees Cook wrote: >> This is the patch I've got prepared now that fixes for all VLAs have been >> sent to maintainers (some are still under review/adjustment, but there >> aren't any unexplored cases left). My intention would be to have this land >> at the end of the next merge window after all the pending VLA patches >> have landed. I just wanted to get any feedback here, since it touches >> a couple areas in the process and I didn't want anyone to be surprised. :) > [] >> diff --git a/Makefile b/Makefile > [] >> @@ -778,6 +778,9 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) >> -print-file-name=include) >> # warn about C99 declaration after statement >> KBUILD_CFLAGS += $(call cc-option,-Wdeclaration-after-statement,) >> >> +# VLAs should not be used anywhere in the kernel >> +KBUILD_CFLAGS += $(call cc-option,-Wvla) > > I'd probably spell out what a VLA is here. > # VLAs (Variable Length Arrays) should not be used anywhere in the kernel > > Beyond that, seems sensible, thanks. Ah yes, good idea. I've made that change locally now. Thanks! -Kees -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC][PATCH] Makefile: globally enable VLA warning
This is the patch I've got prepared now that fixes for all VLAs have been sent to maintainers (some are still under review/adjustment, but there aren't any unexplored cases left). My intention would be to have this land at the end of the next merge window after all the pending VLA patches have landed. I just wanted to get any feedback here, since it touches a couple areas in the process and I didn't want anyone to be surprised. :) Thanks! -Kees Now that VLAs have been removed from the kernel, enable the VLA warning globally. The only exceptions to this are the KASan an UBSan tests which are explicitly checking that VLAs trigger their respective tests. Signed-off-by: Kees Cook --- Makefile | 3 +++ drivers/gpu/drm/i915/Makefile | 2 +- lib/Makefile | 2 ++ scripts/Makefile.extrawarn| 1 - 4 files changed, 6 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index c9132594860b..3d5013ec4116 100644 --- a/Makefile +++ b/Makefile @@ -778,6 +778,9 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include) # warn about C99 declaration after statement KBUILD_CFLAGS += $(call cc-option,-Wdeclaration-after-statement,) +# VLAs should not be used anywhere in the kernel +KBUILD_CFLAGS += $(call cc-option,-Wvla) + # disable pointer signed / unsigned warnings in gcc 4.0 KBUILD_CFLAGS += $(call cc-disable-warning, pointer-sign) diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 4c6adae23e18..289ab5dc5712 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -12,7 +12,7 @@ # Note the danger in using -Wall -Wextra is that when CI updates gcc we # will most likely get a sudden build breakage... Hopefully we will fix # new warnings before CI updates! -subdir-ccflags-y := -Wall -Wextra -Wvla +subdir-ccflags-y := -Wall -Wextra subdir-ccflags-y += $(call cc-disable-warning, unused-parameter) subdir-ccflags-y += $(call cc-disable-warning, type-limits) subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers) diff --git a/lib/Makefile b/lib/Makefile index 90dc5520b784..4720e276232e 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -52,7 +52,9 @@ obj-$(CONFIG_TEST_SYSCTL) += test_sysctl.o obj-$(CONFIG_TEST_HASH) += test_hash.o test_siphash.o obj-$(CONFIG_TEST_KASAN) += test_kasan.o CFLAGS_test_kasan.o += -fno-builtin +CFLAGS_test_kasan.o += $(call cc-disable-warning, vla) obj-$(CONFIG_TEST_UBSAN) += test_ubsan.o +CFLAGS_test_ubsan.o += $(call cc-disable-warning, vla) UBSAN_SANITIZE_test_ubsan.o := y obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o obj-$(CONFIG_TEST_LIST_SORT) += test_list_sort.o diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index 8d5357053f86..24b2fb1d1297 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -52,7 +52,6 @@ warning-3 += -Wpointer-arith warning-3 += -Wredundant-decls warning-3 += -Wswitch-default warning-3 += $(call cc-option, -Wpacked-bitfield-compat) -warning-3 += $(call cc-option, -Wvla) warning := $(warning-$(findstring 1, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS))) warning += $(warning-$(findstring 2, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS))) -- 2.17.1 -- Kees Cook Pixel Security ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/msm/adreno: Remove VLA usage
In the quest to remove all stack VLA usage from the kernel[1], this
switches to using a kasprintf()ed buffer. Return paths are updated
to free the allocation.
[1]
https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
Signed-off-by: Kees Cook
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 7 +--
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 28 +
2 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index d39400e5bc42..f5f76f8151f9 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -11,6 +11,7 @@
*
*/
+#include
#include
#include
#include
@@ -19,6 +20,7 @@
#include
#include
#include
+#include
#include "msm_gem.h"
#include "msm_mmu.h"
#include "a5xx_gpu.h"
@@ -91,12 +93,13 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const
char *fwname)
ret = qcom_mdt_load(dev, fw, fwname, GPU_PAS_ID,
mem_region, mem_phys, mem_size, NULL);
} else {
- char newname[strlen("qcom/") + strlen(fwname) + 1];
+ char *newname;
- sprintf(newname, "qcom/%s", fwname);
+ newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname);
ret = qcom_mdt_load(dev, fw, newname, GPU_PAS_ID,
mem_region, mem_phys, mem_size, NULL);
+ kfree(newname);
}
if (ret)
goto out;
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index bcbf9f2a29f9..bfaafdfc7eb2 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -17,7 +17,9 @@
* this program. If not, see <http://www.gnu.org/licenses/>.
*/
+#include
#include
+#include
#include "adreno_gpu.h"
#include "msm_gem.h"
#include "msm_mmu.h"
@@ -70,10 +72,12 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const char
*fwname)
{
struct drm_device *drm = adreno_gpu->base.dev;
const struct firmware *fw = NULL;
- char newname[strlen("qcom/") + strlen(fwname) + 1];
+ char *newname;
int ret;
- sprintf(newname, "qcom/%s", fwname);
+ newname = kasprintf(GFP_KERNEL, "qcom/%s", fwname);
+ if (!newname)
+ return ERR_PTR(-ENOMEM);
/*
* Try first to load from qcom/$fwfile using a direct load (to avoid
@@ -87,11 +91,12 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const char
*fwname)
dev_info(drm->dev, "loaded %s from new location\n",
newname);
adreno_gpu->fwloc = FW_LOCATION_NEW;
- return fw;
+ goto out;
} else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) {
dev_err(drm->dev, "failed to load %s: %d\n",
newname, ret);
- return ERR_PTR(ret);
+ fw = ERR_PTR(ret);
+ goto out;
}
}
@@ -106,11 +111,12 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const
char *fwname)
dev_info(drm->dev, "loaded %s from legacy location\n",
newname);
adreno_gpu->fwloc = FW_LOCATION_LEGACY;
- return fw;
+ goto out;
} else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) {
dev_err(drm->dev, "failed to load %s: %d\n",
fwname, ret);
- return ERR_PTR(ret);
+ fw = ERR_PTR(ret);
+ goto out;
}
}
@@ -126,16 +132,20 @@ adreno_request_fw(struct adreno_gpu *adreno_gpu, const
char *fwname)
dev_info(drm->dev, "loaded %s with helper\n",
newname);
adreno_gpu->fwloc = FW_LOCATION_HELPER;
- return fw;
+ goto out;
} else if (adreno_gpu->fwloc != FW_LOCATION_UNKNOWN) {
dev_err(drm->dev, "failed to load %s: %d\n",
newname, ret);
- return ERR_PTR(ret);
+ fw = ERR_PTR(ret);
+ goto out;
}
}
dev_err(drm->dev, "failed to load %s\n", fwname);
- return ERR_PTR(-ENOENT);
+ fw = ERR_PTR(-ENOENT);
+out:
+ kfree(newname);
+ return fw;
