I was playing around with LLVM's static checker. Here are some fixes
from reviewing the output on a kernel build.
Some merely shut up the checker -- it is technically right, but
the problem is not a real bug as far as I can tell. Still it's
cleaner/clearer to avoid uninitialized variables and similar.
A few others are real bugs.
-Andi
From: Andi Kleen <[email protected]>
LLVM clang doesn't understand uninitialized_var and always throws a
warning. Disable the macro for this case.
Signed-off-by: Andi Kleen <[email protected]>
---
include/linux/compiler-gcc.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 24545cd..fa93722 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -110,6 +110,10 @@
* A trick to suppress uninitialized variable warning without generating any
* code
*/
+#ifdef __clang__
+#define uninitialized_var(x) x
+#else
#define uninitialized_var(x) x = x
+#endif
#define __always_inline inline __attribute__((always_inline))
--
1.8.3.1
From: Andi Kleen <[email protected]>
The error code was ignored, which I assume is a mistake.
Cc: [email protected]
Signed-off-by: Andi Kleen <[email protected]>
---
block/elevator.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/block/elevator.c b/block/elevator.c
index 2bcbd8c..6c2a8ee 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -184,7 +184,6 @@ static void elevator_release(struct kobject *kobj)
int elevator_init(struct request_queue *q, char *name)
{
struct elevator_type *e = NULL;
- int err;
if (unlikely(q->elevator))
return 0;
@@ -222,8 +221,7 @@ int elevator_init(struct request_queue *q, char *name)
}
}
- err = e->ops.elevator_init_fn(q, e);
- return 0;
+ return e->ops.elevator_init_fn(q, e);
}
EXPORT_SYMBOL(elevator_init);
--
1.8.3.1
From: Andi Kleen <[email protected]>
In some cases, e.g. after this
arch/ia64/kernel/irq.c:185: struct pt_regs *old_regs = set_irq_regs(NULL);
arch/ia64/kernel/irq_ia64.c:560: struct pt_regs *old_regs = set_irq_regs(NULL);
the regs passed to add_interrupt_randomness() could be NULL.
In this case fast_mix would use two uninitialized ints from the stack
and mix it into the pool.
In this case set the input to 0.
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Andi Kleen <[email protected]>
---
drivers/char/random.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 7737b5b..25ed2dc 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -752,6 +752,8 @@ void add_interrupt_randomness(int irq, int irq_flags)
__u64 ip = instruction_pointer(regs);
input[2] = ip;
input[3] = ip >> 32;
+ } else {
+ input[2] = input[3] = 0;
}
fast_mix(fast_pool, input, sizeof(input));
--
1.8.3.1
From: Andi Kleen <[email protected]>
In this if () branch first is not needed, so no need to set it.
Fixes a set-but-not-used warning.
Signed-off-by: Andi Kleen <[email protected]>
---
kernel/sysctl.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b2f06f3..7f82d6d 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2549,7 +2549,6 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
}
bitmap_set(tmp_bitmap, val_a, val_b - val_a + 1);
- first = 0;
proc_skip_char(&kbuf, &left, '\n');
}
free_page(page);
--
1.8.3.1
From: Andi Kleen <[email protected]>
The first loop in ext4_mb_init_cache can bail out when the end of
all groups is reached. Unfortunately the later loops did not
have that check and could access uninitialized buffer pointers
in bh[]. Add the end of group check everywhere.
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Andi Kleen <[email protected]>
---
Makefile | 6 +++---
fs/ext4/mballoc.c | 6 +++++-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/Makefile b/Makefile
index 8d0668f..be3ef83 100644
--- a/Makefile
+++ b/Makefile
@@ -663,9 +663,9 @@ KBUILD_CFLAGS += $(call cc-option,-fconserve-stack)
KBUILD_ARFLAGS := $(call ar-option,D)
# check for 'asm goto'
-ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC)), y)
- KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
-endif
+#ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC)), y)
+# KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
+#endif
# Add user supplied CPPFLAGS, AFLAGS and CFLAGS as the last assignments
KBUILD_CPPFLAGS += $(KCPPFLAGS)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index a41e3ba..619d8ed 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -878,6 +878,8 @@ static int ext4_mb_init_cache(struct page *page, char *incore)
/* wait for I/O completion */
for (i = 0, group = first_group; i < groups_per_page; i++, group++) {
+ if (group >= ngroups)
+ break;
if (bh[i] && ext4_wait_block_bitmap(sb, group, bh[i])) {
err = -EIO;
goto out;
@@ -953,7 +955,9 @@ static int ext4_mb_init_cache(struct page *page, char *incore)
out:
if (bh) {
- for (i = 0; i < groups_per_page; i++)
+ for (i = 0, group = first_group;
+ i < groups_per_page && group < ngroups;
+ i++, group++)
brelse(bh[i]);
if (bh != &bhs)
kfree(bh);
--
1.8.3.1
From: Andi Kleen <[email protected]>
__perf_event__output_id_sample looks at data->type to decide
what to output.
A lot of of the custom output functions, for example perf_log_throttle
start with perf_event_header__init_id, which only initializes
the header when event->attr.sample_id_all is true.
But when this is false the output function is still called,
and will look at an uninitialized header.
I changed all the callers to perf_event_header__init_id
to __perf_event_header__init_id which unconditionally
initializes the header.
FWIW I'm not fully sure this is the correct fix and what the
exact semantics of sample_id_all are supposed to be.
Should it disable all throttling etc. messages?
Please review carefully.
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Andi Kleen <[email protected]>
---
kernel/events/core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index cb4238e..0b15209 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4732,7 +4732,7 @@ perf_event_read_event(struct perf_event *event,
};
int ret;
- perf_event_header__init_id(&read_event.header, &sample, event);
+ __perf_event_header__init_id(&read_event.header, &sample, event);
ret = perf_output_begin(&handle, event, read_event.header.size);
if (ret)
return;
@@ -4837,7 +4837,7 @@ static void perf_event_task_output(struct perf_event *event,
if (!perf_event_task_match(event))
return;
- perf_event_header__init_id(&task_event->event_id.header, &sample, event);
+ __perf_event_header__init_id(&task_event->event_id.header, &sample, event);
ret = perf_output_begin(&handle, event,
task_event->event_id.header.size);
@@ -4931,7 +4931,7 @@ static void perf_event_comm_output(struct perf_event *event,
if (!perf_event_comm_match(event))
return;
- perf_event_header__init_id(&comm_event->event_id.header, &sample, event);
+ __perf_event_header__init_id(&comm_event->event_id.header, &sample, event);
ret = perf_output_begin(&handle, event,
comm_event->event_id.header.size);
@@ -5063,7 +5063,7 @@ static void perf_event_mmap_output(struct perf_event *event,
mmap_event->event_id.header.size += sizeof(mmap_event->ino_generation);
}
- perf_event_header__init_id(&mmap_event->event_id.header, &sample, event);
+ __perf_event_header__init_id(&mmap_event->event_id.header, &sample, event);
ret = perf_output_begin(&handle, event,
mmap_event->event_id.header.size);
if (ret)
@@ -5237,7 +5237,7 @@ static void perf_log_throttle(struct perf_event *event, int enable)
if (enable)
throttle_event.header.type = PERF_RECORD_UNTHROTTLE;
- perf_event_header__init_id(&throttle_event.header, &sample, event);
+ __perf_event_header__init_id(&throttle_event.header, &sample, event);
ret = perf_output_begin(&handle, event,
throttle_event.header.size);
--
1.8.3.1
From: Andi Kleen <[email protected]>
A static checker was pointing out that nothing can possible set
nwait < 0 in this path. The comment and the check appears to be
outdated. Remove it.
Cc: [email protected]
Signed-off-by: Andi Kleen <[email protected]>
---
fs/eventpoll.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 473e09d..f72bf55 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1276,15 +1276,6 @@ static int ep_insert(struct eventpoll *ep, struct epoll_event *event,
*/
revents = ep_item_poll(epi, &epq.pt);
- /*
- * We have to check if something went wrong during the poll wait queue
- * install process. Namely an allocation for a wait queue failed due
- * high memory pressure.
- */
- error = -ENOMEM;
- if (epi->nwait < 0)
- goto error_unregister;
-
/* Add the current item to the list of active epoll hook for this file */
spin_lock(&tfile->f_lock);
list_add_tail(&epi->fllink, &tfile->f_ep_links);
@@ -1334,7 +1325,6 @@ error_remove_epi:
rb_erase(&epi->rbn, &ep->rbr);
-error_unregister:
ep_unregister_pollwait(ep, epi);
/*
--
1.8.3.1
From: Andi Kleen <[email protected]>
tcp_established_options assumes opts->options is 0 before calling,
as it read modify writes it.
For the tcp_current_mss() case the opts structure is not zeroed,
so this can be done with uninitialized values.
This is ok, because ->options is not read in this path.
But it's still better to avoid the operation on the uninitialized
field. This shuts up a static code analyzer, and presumably
may help the optimizer.
Cc: [email protected]
Signed-off-by: Andi Kleen <[email protected]>
---
net/ipv4/tcp_output.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 7c83cb8..f3ed78d 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -637,6 +637,8 @@ static unsigned int tcp_established_options(struct sock *sk, struct sk_buff *skb
unsigned int size = 0;
unsigned int eff_sacks;
+ opts->options = 0;
+
#ifdef CONFIG_TCP_MD5SIG
*md5 = tp->af_specific->md5_lookup(sk, sk);
if (unlikely(*md5)) {
--
1.8.3.1
From: Andi Kleen <[email protected]>
cpu_clock_sample et.al. can fail with an invalid timer type.
For most callers this cannot happen because the timer has been
successfully initialized earlier, so they don't check
the return value, but still use the val output.
Unfortunately static checkers for uninitialied variables
don't understand this and always throw a warning in this case.
Initialize the timer value to 0 for the error case to
shut it up.
This doesn't fix any real bug, just reduces warning noise.
Cc: [email protected]
Signed-off-by: Andi Kleen <[email protected]>
---
kernel/posix-cpu-timers.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index c7f31aa..cc3c3f2 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -182,6 +182,7 @@ static int cpu_clock_sample(const clockid_t which_clock, struct task_struct *p,
{
switch (CPUCLOCK_WHICH(which_clock)) {
default:
+ *sample = 0;
return -EINVAL;
case CPUCLOCK_PROF:
*sample = prof_ticks(p);
@@ -243,6 +244,7 @@ static int cpu_clock_sample_group(const clockid_t which_clock,
switch (CPUCLOCK_WHICH(which_clock)) {
default:
+ *sample = 0;
return -EINVAL;
case CPUCLOCK_PROF:
thread_group_cputime(p, &cputime);
--
1.8.3.1
From: Andi Kleen <[email protected]>
eee_get_cur assumes that the output data is already zeroed. It can
read-modify-write the advertised field:
if (ipcnfg & E1000_IPCNFG_EEE_100M_AN)
2594 edata->advertised |= ADVERTISED_100baseT_Full;
This is ok for the normal ethtool eee_get call, which always
zeroes the input data before.
But eee_set_cur also calls eee_get_cur and it did not zero the input
field. Later on it then compares agsinst the field, which can contain partial
stack garbage.
Zero the input field in eee_set_cur() too.
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Andi Kleen <[email protected]>
---
drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 48cbc83..41e37ff 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2652,6 +2652,8 @@ static int igb_set_eee(struct net_device *netdev,
(hw->phy.media_type != e1000_media_type_copper))
return -EOPNOTSUPP;
+ memset(&eee_curr, 0, sizeof(struct ethtool_eee));
+
ret_val = igb_get_eee(netdev, &eee_curr);
if (ret_val)
return ret_val;
--
1.8.3.1
From: Andi Kleen <[email protected]>
When ->next fails the error would not be returned to user space.
Add the missing check.
Cc: [email protected]
Signed-off-by: Andi Kleen <[email protected]>
---
fs/seq_file.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 3135c25..e16b4a8 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -262,6 +262,8 @@ Fill:
pos = next;
}
m->op->stop(m, p);
+ if (err)
+ goto Done;
n = min(m->count, size);
err = copy_to_user(buf, m->buf, n);
if (err)
--
1.8.3.1
> In this case fast_mix would use two uninitialized ints from the stack
> and mix it into the pool.
Is the concern here is that an attacker might know (or be able to control) what is on
the stack - and so get knowledge of what is being mixed into the pool?
> In this case set the input to 0.
And the fix is to guarantee that everyone knows what is being mixed in? (!)
Wouldn't it be better to adjust the "nbytes" parameter to
fast_mix(..., ..., sizeof (input));
to only mix in the part of input[] that we successfully initialized?
Untested patch below.
Signed-off-by: Tony Luck <[email protected]>
---
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 7737b5bd26af..5c4ec0abb702 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -745,16 +745,19 @@ void add_interrupt_randomness(int irq, int irq_flags)
struct pt_regs *regs = get_irq_regs();
unsigned long now = jiffies;
__u32 input[4], cycles = get_cycles();
+ int nbytes;
input[0] = cycles ^ jiffies;
input[1] = irq;
+ nbytes = 2 * sizeof(input[0]);
if (regs) {
__u64 ip = instruction_pointer(regs);
input[2] = ip;
input[3] = ip >> 32;
+ nbytes += 2 * sizeof(input[0]);
}
- fast_mix(fast_pool, input, sizeof(input));
+ fast_mix(fast_pool, input, nbytes);
if ((fast_pool->count & 1023) &&
!time_after(now, fast_pool->last + HZ))
Andi Kleen <[email protected]> wrote:
> From: Andi Kleen <[email protected]>
>
> A static checker was pointing out that nothing can possible set
> nwait < 0 in this path. The comment and the check appears to be
> outdated. Remove it.
I don't think so...
> Cc: [email protected]
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> fs/eventpoll.c | 10 ----------
> 1 file changed, 10 deletions(-)
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 473e09d..f72bf55 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -1276,15 +1276,6 @@ static int ep_insert(struct eventpoll *ep, struct epoll_event *event,
> */
> revents = ep_item_poll(epi, &epq.pt);
ep_item_poll calls f_op->poll, which calls poll_wait().
poll_wait() will call ep_ptable_queue_proc.
> - /*
> - * We have to check if something went wrong during the poll wait queue
> - * install process. Namely an allocation for a wait queue failed due
> - * high memory pressure.
> - */
> - error = -ENOMEM;
> - if (epi->nwait < 0)
> - goto error_unregister;
> -
> /* Add the current item to the list of active epoll hook for this file */
> spin_lock(&tfile->f_lock);
> list_add_tail(&epi->fllink, &tfile->f_ep_links);
> @@ -1334,7 +1325,6 @@ error_remove_epi:
>
> rb_erase(&epi->rbn, &ep->rbr);
>
> -error_unregister:
> ep_unregister_pollwait(ep, epi);
>
> /*
> > @@ -1276,15 +1276,6 @@ static int ep_insert(struct eventpoll *ep, struct epoll_event *event,
> > */
> > revents = ep_item_poll(epi, &epq.pt);
>
> ep_item_poll calls f_op->poll, which calls poll_wait().
> poll_wait() will call ep_ptable_queue_proc.
Thanks. Good point.
-Andi
Andi Kleen <[email protected]> writes:
> From: Andi Kleen <[email protected]>
>
> The error code was ignored, which I assume is a mistake.
Yeah, introduced in d50235b7bc3ee0a0427984d763ea7534149531b4, so I cc'd
Jianpeng Ma, though it looks pretty obvious.
Reviewed-by: Jeff Moyer <[email protected]>
> Cc: [email protected]
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> block/elevator.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/block/elevator.c b/block/elevator.c
> index 2bcbd8c..6c2a8ee 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -184,7 +184,6 @@ static void elevator_release(struct kobject *kobj)
> int elevator_init(struct request_queue *q, char *name)
> {
> struct elevator_type *e = NULL;
> - int err;
>
> if (unlikely(q->elevator))
> return 0;
> @@ -222,8 +221,7 @@ int elevator_init(struct request_queue *q, char *name)
> }
> }
>
> - err = e->ops.elevator_init_fn(q, e);
> - return 0;
> + return e->ops.elevator_init_fn(q, e);
> }
> EXPORT_SYMBOL(elevator_init);
On Mon, Sep 30, 2013 at 08:51:43PM +0000, Luck, Tony wrote:
> > In this case fast_mix would use two uninitialized ints from the stack
> > and mix it into the pool.
>
> Is the concern here is that an attacker might know (or be able to control) what is on
> the stack - and so get knowledge of what is being mixed into the pool?
Yes, this is a bogus complaint.
> > In this case set the input to 0.
>
> And the fix is to guarantee that everyone knows what is being mixed in? (!)
>
> Wouldn't it be better to adjust the "nbytes" parameter to
>
> fast_mix(..., ..., sizeof (input));
>
> to only mix in the part of input[] that we successfully initialized?
The changes queued for the next merge window in the random tree solve
this problem slightly differently:
...
input[0] = cycles ^ j_high ^ irq;
input[1] = now ^ c_high;
ip = regs ? instruction_pointer(regs) : _RET_IP_;
input[2] = ip;
input[3] = ip >> 32;
fast_mix(fast_pool, input);
...
(Note the lack of nbytes parameter in fast_mix --- there are some
optimizations so that we mix in the changes by 32-bit words, instead
of bytes, and the number of 32-bit words is fixed to 4, since it's the
only way fast_mix is called).
_RET_IP_ isn't that much better than 0, but it's at least kernel
specific, and I figured it was better to shut up bogus warnings, as
opposed to trying to depend on stack garbage (which would likely be
kernel specific anyway).
Cheers,
- Ted
On Mon, Sep 30, 2013 at 01:29:09PM -0700, Andi Kleen wrote:
> From: Andi Kleen <[email protected]>
>
> The first loop in ext4_mb_init_cache can bail out when the end of
> all groups is reached. Unfortunately the later loops did not
> have that check and could access uninitialized buffer pointers
> in bh[]. Add the end of group check everywhere.
>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> Makefile | 6 +++---
> fs/ext4/mballoc.c | 6 +++++-
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 8d0668f..be3ef83 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -663,9 +663,9 @@ KBUILD_CFLAGS += $(call cc-option,-fconserve-stack)
> KBUILD_ARFLAGS := $(call ar-option,D)
>
> # check for 'asm goto'
> -ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC)), y)
> - KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
> -endif
> +#ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC)), y)
> +# KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
> +#endif
>
> # Add user supplied CPPFLAGS, AFLAGS and CFLAGS as the last assignments
> KBUILD_CPPFLAGS += $(KCPPFLAGS)
What's this change all about, and why is it included in this
patch?
- Ted
> What's this change all about, and why is it included in this
> patch?
Sorry that was me fat-fingering git add. Ignore that hunk.
I needed it for the static analyzer, which does not understand asm goto.
-Andi
> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> On Behalf Of Andi Kleen
> Sent: Monday, September 30, 2013 1:29 PM
> To: [email protected]
> Cc: Andi Kleen; Kirsher, Jeffrey T; [email protected]
> Subject: [PATCH 07/11] igb: Avoid uninitialized advertised variable in
> eee_set_cur
>
> From: Andi Kleen <[email protected]>
>
> eee_get_cur assumes that the output data is already zeroed. It can read-modify-
> write the advertised field:
>
> if (ipcnfg & E1000_IPCNFG_EEE_100M_AN)
> 2594 edata->advertised |= ADVERTISED_100baseT_Full;
>
> This is ok for the normal ethtool eee_get call, which always zeroes the input
> data before.
>
> But eee_set_cur also calls eee_get_cur and it did not zero the input field. Later
> on it then compares agsinst the field, which can contain partial stack garbage.
>
> Zero the input field in eee_set_cur() too.
>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 48cbc83..41e37ff 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -2652,6 +2652,8 @@ static int igb_set_eee(struct net_device *netdev,
> (hw->phy.media_type != e1000_media_type_copper))
> return -EOPNOTSUPP;
>
> + memset(&eee_curr, 0, sizeof(struct ethtool_eee));
> +
> ret_val = igb_get_eee(netdev, &eee_curr);
> if (ret_val)
> return ret_val;
> --
> 1.8.3.1
>
ACK
Thanks,
Carolyn
On Mon, 2013-09-30 at 13:29 -0700, Andi Kleen wrote:
> From: Andi Kleen <[email protected]>
>
> eee_get_cur assumes that the output data is already zeroed. It can
> read-modify-write the advertised field:
>
> if (ipcnfg & E1000_IPCNFG_EEE_100M_AN)
> 2594 edata->advertised |= ADVERTISED_100baseT_Full;
>
> This is ok for the normal ethtool eee_get call, which always
> zeroes the input data before.
>
> But eee_set_cur also calls eee_get_cur and it did not zero the input
> field. Later on it then compares agsinst the field, which can contain
> partial
> stack garbage.
>
> Zero the input field in eee_set_cur() too.
>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Andi Kleen <[email protected]>
> ---
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 ++
> 1 file changed, 2 insertions(+)
Acked-by: Jeff Kirsher <[email protected]>
On Mon, Sep 30, 2013 at 01:29:12PM -0700, Andi Kleen wrote:
> From: Andi Kleen <[email protected]>
>
> __perf_event__output_id_sample looks at data->type to decide
> what to output.
>
> A lot of of the custom output functions, for example perf_log_throttle
> start with perf_event_header__init_id, which only initializes
> the header when event->attr.sample_id_all is true.
>
> But when this is false the output function is still called,
> and will look at an uninitialized header.
>
> I changed all the callers to perf_event_header__init_id
> to __perf_event_header__init_id which unconditionally
> initializes the header.
>
> FWIW I'm not fully sure this is the correct fix and what the
> exact semantics of sample_id_all are supposed to be.
> Should it disable all throttling etc. messages?
> Please review carefully.
Why are you doing this; also what's up with 11/11?
> Why are you doing this; also what's up with 11/11?
I was playing with a static checker, and posted some
of the fixes from looking at the warnings.
There were 11 patches in the series
Only one is perf related.
-Andi
--
[email protected] -- Speaking for myself only
On Wed, Oct 02, 2013 at 10:25:07AM -0700, Andi Kleen wrote:
> > Why are you doing this; also what's up with 11/11?
>
> I was playing with a static checker, and posted some
> of the fixes from looking at the warnings.
> There were 11 patches in the series
>
> Only one is perf related.
It would help to know what the reported 'error' was. It wasn't at all
clear to me what we were trying to do and why there would be a problem.
From: Andi Kleen <[email protected]>
Date: Mon, 30 Sep 2013 13:29:08 -0700
> From: Andi Kleen <[email protected]>
>
> eee_get_cur assumes that the output data is already zeroed. It can
> read-modify-write the advertised field:
>
> if (ipcnfg & E1000_IPCNFG_EEE_100M_AN)
> 2594 edata->advertised |= ADVERTISED_100baseT_Full;
>
> This is ok for the normal ethtool eee_get call, which always
> zeroes the input data before.
>
> But eee_set_cur also calls eee_get_cur and it did not zero the input
> field. Later on it then compares agsinst the field, which can contain partial
> stack garbage.
>
> Zero the input field in eee_set_cur() too.
>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Andi Kleen <[email protected]>
Applied.
From: Andi Kleen <[email protected]>
Date: Mon, 30 Sep 2013 13:29:11 -0700
> From: Andi Kleen <[email protected]>
>
> tcp_established_options assumes opts->options is 0 before calling,
> as it read modify writes it.
>
> For the tcp_current_mss() case the opts structure is not zeroed,
> so this can be done with uninitialized values.
>
> This is ok, because ->options is not read in this path.
> But it's still better to avoid the operation on the uninitialized
> field. This shuts up a static code analyzer, and presumably
> may help the optimizer.
>
> Cc: [email protected]
> Signed-off-by: Andi Kleen <[email protected]>
Applied.
* Peter Zijlstra <[email protected]> wrote:
> On Wed, Oct 02, 2013 at 10:25:07AM -0700, Andi Kleen wrote:
> > > Why are you doing this; also what's up with 11/11?
> >
> > I was playing with a static checker, and posted some
> > of the fixes from looking at the warnings.
> > There were 11 patches in the series
> >
> > Only one is perf related.
>
> It would help to know what the reported 'error' was. [...]
Forcing multiple, unnecessary rounds of emails instead of clearly
volunteering all technical information that is related to the matter is
something Andi is still doing routinely.
To deal with this repeat anomaly of Andi's communication patterns I'll
just reply with a form letter in the future:
" Your mail or patch is intentionally or accidentally missing key bits of
technical information. The kernel list is an open technical forum where
volunteering as much useful information as possible about a specific
technical matter _before_ a maintainer asks for it is strongly favored.
The kernel is not a need-to-know security agency where people are
routinely faking superior expertise by holding back and hoarding
information, forcing others to request that information piecemail wise. "
Thanks,
Ingo
> Forcing multiple, unnecessary rounds of emails instead of clearly
> volunteering all technical information that is related to the matter is
> something Andi is still doing routinely.
Sorry all the information was in the full email thread
(including the intro). And the patch description
clearly described the problem in your code.
As the patches spanned many subsystems you were only copied
on the patches that affect your subsystem, and not on
the intro (as git send-email doesn't seem to support that)
Here are possible alternatives. Please let me know which ones
you prefer and I'll try to adapt to your specific preferences
in the future:
[ ] Always copy you guys on all patches of
subsystem spanning patch kits that are mostly of no relevance
to you.
[ ] Repeat the complete intro in every patch.
[ ] You think uninitialized are not a problem and you
don't want to see any patches related to that.
[ ] You are not interested in fixing static checker problems in your
subsystem and don't want to see any patches related to that.
Thanks.
-Andi
* Andi Kleen <[email protected]> wrote:
> > Forcing multiple, unnecessary rounds of emails instead of clearly
> > volunteering all technical information that is related to the matter
> > is something Andi is still doing routinely.
>
> Sorry all the information was in the full email thread (including the
> intro). And the patch description clearly described the problem in your
> code.
>
> As the patches spanned many subsystems you were only copied on the
> patches that affect your subsystem, and not on the intro (as git
> send-email doesn't seem to support that)
>
> Here are possible alternatives. Please let me know which ones you prefer
> and I'll try to adapt to your specific preferences in the future:
>
> [ ] Always copy you guys on all patches of
> subsystem spanning patch kits that are mostly of no relevance
> to you.
>
> [ ] Repeat the complete intro in every patch.
>
> [ ] You think uninitialized are not a problem and you
> don't want to see any patches related to that.
>
> [ ] You are not interested in fixing static checker problems in your
> subsystem and don't want to see any patches related to that.
[x] Fix your changelog. Any commit log that is not self-describing to a
maintainer like PeterZ is faulty by definition - regardless of any intro
somewhere else. Changelogs ultimately detach from intros, they will be
read by others in the kernel repo, reviewing changes, looking for bugs,
following development, etc., and if the relevant context is lost then
that changelog is _BROKEN_. This is kernel development 101.
[x] You should stop making idiotic excuses for broken changelogs. The
fact that we had to complain about the quality of your patches and
changelogs for years should give you a big, huge hint that the problem
is still on your side. Writing snarky responses to maintainer questions
does not improve your changelogs, it only wastes PeterZ's, my and other
people's time.
Thanks,
Ingo
On 2013-10-01 08:44:24 [-0400], Theodore Ts'o wrote:
> The changes queued for the next merge window in the random tree solve
> this problem slightly differently:
>
> ...
> input[0] = cycles ^ j_high ^ irq;
> input[1] = now ^ c_high;
> ip = regs ? instruction_pointer(regs) : _RET_IP_;
> input[2] = ip;
> input[3] = ip >> 32;
>
> fast_mix(fast_pool, input);
> ...
>
> (Note the lack of nbytes parameter in fast_mix --- there are some
> optimizations so that we mix in the changes by 32-bit words, instead
> of bytes, and the number of 32-bit words is fixed to 4, since it's the
> only way fast_mix is called).
>
> _RET_IP_ isn't that much better than 0, but it's at least kernel
> specific, and I figured it was better to shut up bogus warnings, as
> opposed to trying to depend on stack garbage (which would likely be
> kernel specific anyway).
That ip pointer was earlier optional. Now with _RET_IP_ it is a
constant since there is _one_ caller. Plus on 32bit the upper bits are
always zero. It probably didn't get worse because the four bytes on
stack were mostlikly constant as well. [2] is constant if _RET_IP_ is
used. The IP is kind of random. The lower bits are mostlikely 0 due to
32bit alignment (not on x86, okay).
Lets look further. c_high is != 0 only if cycles is larger than 4 bytes.
This is in most cases a long which makes 4 bytes on all 32bit arches.
This makes [1] the lower bytes of jiffies. And you can imagine how often
the upper 16bit change.
Which brings me to [0]. The irq number changes now and then and mostlikely
only the lower 8 bit. j_high is 0 on 32bit platforms. Even on 64bit
with HZ=250 the lower 32bit overflows every ~198 days unless I miscalculated.
Doesn't this make it a constant?
And finally cycles which is random_get_entropy(). On ARM (as previously on
MIPS) this returns 0. Well not always but for all platforms which do not
implement register_current_timer_delay() which makes a lot of them.
This makes
[0] = irq (8 bit)
[1] = jiffies
[2] = a constant unless regs is available and
[3] = 0
from looking at the code it reads like 16 random bytes are fed but now it
looks a little different.
May I kill this and save a few cycles in irq context?
Why don't you take a small amount of randomness and use that as a key and
feed into a block cipher in CTR mode instead of giving it to user right
away? This _can_ work in parallell and should provide *good* pseudo random
numbers on demand with a high performance.
But seriously. What about this:
>From 082dab5c482728a9ef695aa5b42217dcec8e3dd5 Mon Sep 17 00:00:00 2001
From: Sebastian Andrzej Siewior <[email protected]>
Date: Fri, 4 Apr 2014 18:49:29 +0200
Subject: [PATCH] random: yell if random_get_entropy() returns a constant
A few architectures still return 0 (i.e. a constant) if
random_get_entropy() is invoked. Make them aware of this so they may fix
this.
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
drivers/char/random.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 429b75b..48775f8 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -241,6 +241,7 @@
#include <linux/major.h>
#include <linux/string.h>
#include <linux/fcntl.h>
+#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/random.h>
#include <linux/poll.h>
@@ -1675,3 +1676,23 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len)
return 0;
return PAGE_ALIGN(get_random_int() % range + start);
}
+
+static int check_random_get_entropy(void)
+{
+ cycles_t cyc1;
+ cycles_t cyc2;
+
+ cyc1 = random_get_entropy();
+ cyc2 = random_get_entropy();
+ if (cyc1 != cyc2)
+ return 0;
+ udelay(1);
+ cyc2 = random_get_entropy();
+
+ if (cyc1 != cyc2)
+ return 0;
+ pr_err("Error: random_get_entropy() does not return anything random\n");
+ WARN_ON(1);
+ return -EINVAL;
+}
+late_initcall(check_random_get_entropy);
--
1.9.1
> Cheers,
> - Ted
Sebastian
On Fri, Apr 04, 2014 at 06:54:47PM +0200, Sebastian Andrzej Siewior wrote:
> And finally cycles which is random_get_entropy(). On ARM (as previously on
> MIPS) this returns 0. Well not always but for all platforms which do not
> implement register_current_timer_delay() which makes a lot of them.
Yes, ARM sucks for not implement random_get_entropy() on all
platforms. Film at 11. I'm told that Cortex ARM systms are supposed
to have a cycle counter. I'm not sure why it hasn't been wired up,
but it really should be.
> [0] = irq (8 bit)
> [1] = jiffies
> [2] = a constant unless regs is available and
> [3] = 0
Actually regs should be available nearly all of the time. So the
situation isn't quite as dire as you describe, but agreed, it is
pretty bad.
Still, it would be nice if random_get_entropy() could be wired up on
as many platforms as possible.
> +static int check_random_get_entropy(void)
> +{
> + cycles_t cyc1;
> + cycles_t cyc2;
> +
> + cyc1 = random_get_entropy();
> + cyc2 = random_get_entropy();
> + if (cyc1 != cyc2)
> + return 0;
> + udelay(1);
> + cyc2 = random_get_entropy();
> +
> + if (cyc1 != cyc2)
> + return 0;
> + pr_err("Error: random_get_entropy() does not return anything random\n");
> + WARN_ON(1);
> + return -EINVAL;
> +}
> +late_initcall(check_random_get_entropy);
This is lacking in subtly, and while I'm sympathetic with your
frustration, and unfortunately, there are a huge number of CPU's/SOC's
that don't implement a cycle counter or some other kind of cheap, high
resolution counter. I'm not against putting in a warning printk, but
issuing a WARN_ON(1) would be really be too obnoxious. Maybe
something like, "you're running on a crap CPU architecture and so
/dev/random may very well be insecure", but I think that's about as
blunt as we can really afford to be at this point.
- Ted
P.S. Maybe if Intel started a marketing campaign on G+ explaining why
ChromeOS on Intel machines is much more secure than ChromeOS on ARM,
we could finally get some action out of the !@#!?! ARM chip
manufacturers. :-/
On 2014-04-07 00:01:37 [-0400], Theodore Ts'o wrote:
> Yes, ARM sucks for not implement random_get_entropy() on all
> platforms. Film at 11. I'm told that Cortex ARM systms are supposed
> to have a cycle counter. I'm not sure why it hasn't been wired up,
> but it really should be.
I see.
> Actually regs should be available nearly all of the time. So the
> situation isn't quite as dire as you describe, but agreed, it is
> pretty bad.
You dropped that part where I suggested to use something like AES+CTR
and create the numbers on demand and dropping that attempt to create as
much random data with custom functions as possible. You completly dislike
that approach? And if so, why?
> Still, it would be nice if random_get_entropy() could be wired up on
> as many platforms as possible.
Yes. Usually there is generic function doing something sane but not as
good as it could do with arch specific code. Or the code is completly
disabled unless the architecture wires it up. Dropping a new function and
hoping everyone will wire it up in no time is, ehm, brave. Nobody implemented
random_get_entropy(), everyone falls back to get_cycles. From a quick
grep I can see that atleast Hexagon, Cris, Frv, m32r and m68k return 0. I
put some of the maintainers Cc, I am curious if they know about the side
effects.
> This is lacking in subtly, and while I'm sympathetic with your
> frustration, and unfortunately, there are a huge number of CPU's/SOC's
> that don't implement a cycle counter or some other kind of cheap, high
> resolution counter. I'm not against putting in a warning printk, but
Cheap? I know there a few platforms running on a 16bit counter. It is
enough for a high resolution timer, it overflows quite often, too (i.e.
in less than two seconds). However it is the only thing as time base that
they have and from clocksource point of view it is enough.
> issuing a WARN_ON(1) would be really be too obnoxious. Maybe
> something like, "you're running on a crap CPU architecture and so
> /dev/random may very well be insecure", but I think that's about as
> blunt as we can really afford to be at this point.
A backtrace is something you catch during boot up. A single line may get
lost in the mass and I think this is important.
However, as you wish, here are the changes.
>
> - Ted
>
> P.S. Maybe if Intel started a marketing campaign on G+ explaining why
> ChromeOS on Intel machines is much more secure than ChromeOS on ARM,
> we could finally get some action out of the !@#!?! ARM chip
> manufacturers. :-/
Maybe post people are not aware what is going on here. A huge campaing
with ballons is one way to get attention. Just drop /dev/random if the
missing infrastucture isn't available and it shouldn't need G+ to get
the infrastrucure. And the CTR thingy might be a good one because it
would require *one* thing to get done and not each arch/subarch.
>From 1d2d3ef3c411af7fc6b1beb0db7a2c35c1a72702 Mon Sep 17 00:00:00 2001
From: Sebastian Andrzej Siewior <[email protected]>
Date: Fri, 4 Apr 2014 18:49:29 +0200
Subject: [PATCH] random: yell if random_get_entropy() returns a constant
A few architectures still return 0 (i.e. a constant) if
random_get_entropy() is invoked. Make them aware of this so they may fix
this.
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
drivers/char/random.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 429b75b..33508f9 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -241,6 +241,7 @@
#include <linux/major.h>
#include <linux/string.h>
#include <linux/fcntl.h>
+#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/random.h>
#include <linux/poll.h>
@@ -1675,3 +1676,22 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len)
return 0;
return PAGE_ALIGN(get_random_int() % range + start);
}
+
+static int check_random_get_entropy(void)
+{
+ cycles_t cyc1;
+ cycles_t cyc2;
+
+ cyc1 = random_get_entropy();
+ cyc2 = random_get_entropy();
+ if (cyc1 != cyc2)
+ return 0;
+ udelay(1);
+ cyc2 = random_get_entropy();
+ if (cyc1 != cyc2)
+ return 0;
+
+ pr_err("Error: you're running on a crap CPU architecture and so /dev/random may very well be insecure");
+ return -EINVAL;
+}
+late_initcall(check_random_get_entropy);
--
1.9.1
On Mon, Apr 07, 2014 at 09:30:57PM +0200, Sebastian Andrzej Siewior wrote:
>
> You dropped that part where I suggested to use something like AES+CTR
> and create the numbers on demand and dropping that attempt to create as
> much random data with custom functions as possible. You completly dislike
> that approach? And if so, why?
Where are you going to get the "few random bits" from? Which crypto
primitive you use and how you gather the entropy are two completely
orothognal issue. If we can get at least 128 bits of secure
randomness before the embedded platform trying to generate RSA private
keys or otherwise depending on the RNG, we're fine. This is true
regardless of whether we use the current /dev/random machinery or
AES+CTR.
The reason why we are grabbing lots of bits from the interrupt handler
is that we're hoping that *some* of them will not be guessable by the
attacker. If we knew which ones were random, we wouldn't have to do
this, yes. But that's like say, "playing the stock market is easy;
all you have to do is buy low and sell high!"
> Yes. Usually there is generic function doing something sane but not as
> good as it could do with arch specific code. Or the code is completly
> disabled unless the architecture wires it up. Dropping a new function and
> hoping everyone will wire it up in no time is, ehm, brave. Nobody implemented
> random_get_entropy(), everyone falls back to get_cycles. From a quick
> grep I can see that atleast Hexagon, Cris, Frv, m32r and m68k return 0. I
> put some of the maintainers Cc, I am curious if they know about the side
> effects.
What we have right now is now worse than what we had before. We
introduced random_get_entryop() done because MIPS had a register which
wouldn't qualify for get_cycles(), but was good enough for what the
random driver had, so it allowed MIPS to be able to do a better job.
Basically, I had a MIPS developer who was highly motiviated to improve
security for home routers (which typically us MIPS), and I worked with
him.
If there is some ARM developer who is interested in woring with me,
that's great. I would love to have that. I've reached out to a few
people in Linaro about this over the past couple of months, but
nothing has happened yet.
- Ted
On Mon, Apr 07, 2014 at 09:30:57PM +0200, Sebastian Andrzej Siewior wrote:
> Yes. Usually there is generic function doing something sane but not as
> good as it could do with arch specific code. Or the code is completly
> disabled unless the architecture wires it up. Dropping a new function and
> hoping everyone will wire it up in no time is, ehm, brave. Nobody implemented
> random_get_entropy(), everyone falls back to get_cycles. From a quick
> grep I can see that atleast Hexagon, Cris, Frv, m32r and m68k return 0. I
> put some of the maintainers Cc, I am curious if they know about the side
> effects.
Thanks for the CC; I was not aware of the side effects. Hexagon does have a
pcycles mechanism, so I will hook that up in our arch.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation