Various static checker fixes

Various static checker fixes

[Andi Kleen]

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

[PATCH 01/11] random: don't feed stack data into pool when interrupt regs NULL

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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: tony.luck@intel.com
Cc: tytso@mit.edu
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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

[PATCH 02/11] Disable initialized_var for clang

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

LLVM clang doesn't understand uninitialized_var and always throws a
warning. Disable the macro for this case.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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

[PATCH 03/11] posix-timers: Initialize timer value to 0 for invalid timers.

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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: tglx@linutronix.de
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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

[PATCH 04/11] block: Return error code of elevator init function

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

The error code was ignored, which I assume is a mistake.

Cc: axboe@kernel.dk
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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

[PATCH 05/11] seq_file: Handle ->next error in seq_read

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

When ->next fails the error would not be returned to user space.
Add the missing check.

Cc: viro@zeniv.linux.org.uk
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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

[PATCH 06/11] sysctl: remove unnecessary variable initialization

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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 <ak@linux.intel.com>
---
 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

[PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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: jeffrey.t.kirsher@intel.com
Cc: netdev@vger.kernel.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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

[PATCH 08/11] ext4: Fix end of group handling in ext4_mb_init_cache

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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: tytso@mit.edu
Cc: linux-ext4@vger.kernel.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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

[PATCH 09/11] epoll: Remove unnecessary error path

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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: viro@zeniv.linux.org.uk
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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

[PATCH 10/11] tcp: Always set options to 0 before calling tcp_established_options

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

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: netdev@vger.kernel.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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

[PATCH 11/11] perf: Avoid uninitialized sample type reference in __perf_event__output_id_sample

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

__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: mingo@kernel.org
Cc: eranian@google.com
Cc: peterz@infradead.org
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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

RE: [PATCH 01/11] random: don't feed stack data into pool when interrupt regs NULL

[Luck, Tony]

> 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 <tony.luck@intel.com>

---

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))

Re: [PATCH 09/11] epoll: Remove unnecessary error path

[Eric Wong]

Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> 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: viro@zeniv.linux.org.uk
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  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);
>  
>  	/*

Re: [PATCH 09/11] epoll: Remove unnecessary error path

[Andi Kleen]

> > @@ -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

Re: [PATCH 04/11] block: Return error code of elevator init function

[Jeff Moyer]

Andi Kleen <andi@firstfloor.org> writes:

> From: Andi Kleen <ak@linux.intel.com>
>
> 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 <jmoyer@redhat.com>

> Cc: axboe@kernel.dk
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  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);

Re: [PATCH 01/11] random: don't feed stack data into pool when interrupt regs NULL

[Theodore Ts'o]

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

Re: [PATCH 08/11] ext4: Fix end of group handling in ext4_mb_init_cache

[Theodore Ts'o]

On Mon, Sep 30, 2013 at 01:29:09PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> 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: tytso@mit.edu
> Cc: linux-ext4@vger.kernel.org
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  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

Re: [PATCH 08/11] ext4: Fix end of group handling in ext4_mb_init_cache

[Andi Kleen]

> 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

RE: [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur

[Wyborny, Carolyn]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Andi Kleen
> Sent: Monday, September 30, 2013 1:29 PM
> To: linux-kernel@vger.kernel.org
> Cc: Andi Kleen; Kirsher, Jeffrey T; netdev@vger.kernel.org
> Subject: [PATCH 07/11] igb: Avoid uninitialized advertised variable in
> eee_set_cur
> 
> From: Andi Kleen <ak@linux.intel.com>
> 
> 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: jeffrey.t.kirsher@intel.com
> Cc: netdev@vger.kernel.org
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  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 

Re: [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

On Mon, 2013-09-30 at 13:29 -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> 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: jeffrey.t.kirsher@intel.com
> Cc: netdev@vger.kernel.org
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 11/11] perf: Avoid uninitialized sample type reference in __perf_event__output_id_sample

[Peter Zijlstra]

On Mon, Sep 30, 2013 at 01:29:12PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> __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?

Re: [PATCH 11/11] perf: Avoid uninitialized sample type reference in __perf_event__output_id_sample

[Andi Kleen]

> 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

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 11/11] perf: Avoid uninitialized sample type reference in __perf_event__output_id_sample

[Peter Zijlstra]

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.

Re: [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Mon, 30 Sep 2013 13:29:08 -0700

> From: Andi Kleen <ak@linux.intel.com>
> 
> 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: jeffrey.t.kirsher@intel.com
> Cc: netdev@vger.kernel.org
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Applied.

Re: [PATCH 10/11] tcp: Always set options to 0 before calling tcp_established_options

[David Miller]

From: Andi Kleen <andi@firstfloor.org>
Date: Mon, 30 Sep 2013 13:29:11 -0700

> From: Andi Kleen <ak@linux.intel.com>
> 
> 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: netdev@vger.kernel.org
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Applied.

Re: [PATCH 11/11] perf: Avoid uninitialized sample type reference in __perf_event__output_id_sample

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> 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

Re: [PATCH 11/11] perf: Avoid uninitialized sample type reference in __perf_event__output_id_sample

[Andi Kleen]

> 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

Re: [PATCH 11/11] perf: Avoid uninitialized sample type reference in __perf_event__output_id_sample

[Ingo Molnar]

* Andi Kleen <ak@linux.intel.com> 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

Re: [PATCH 01/11] random: don't feed stack data into pool when interrupt regs NULL

[Sebastian Andrzej Siewior]

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 <bigeasy@linutronix.de>
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 <bigeasy@linutronix.de>
---
 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

Re: [PATCH 01/11] random: don't feed stack data into pool when interrupt regs NULL

[Theodore Ts'o]

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.  :-/

Re: [PATCH 01/11] random: don't feed stack data into pool when interrupt regs NULL

[Sebastian Andrzej Siewior]

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 <bigeasy@linutronix.de>
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 <bigeasy@linutronix.de>
---
 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

Re: [PATCH 01/11] random: don't feed stack data into pool when interrupt regs NULL

[Theodore Ts'o]

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

Re: [PATCH 01/11] random: don't feed stack data into pool when interrupt regs NULL

[rkuo]

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