bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[patch #6088] Entropy Patch


From: Michael Banck
Subject: [patch #6088] Entropy Patch
Date: Fri, 18 Jul 2008 13:53:37 +0000
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20070113 Epiphany/1.4.8 (Debian)

Follow-up Comment #1, patch #6088 (project hurd):

Comments on the 2007/08/28 version of the patch by Samuel Thibault:

> Index: device/blkio.c
> @@ -149,6 +151,10 @@
>       do {
>           prev = next;
>           next = prev->io_next;
> +#ifdef MACH_ENTROPY
> +         /* Let's grab the cylinder numbers for entropy.  */
> +         entropy_putdata (ior, sizeof(ior), ENTROPY_HIGH_QUALITY,
ENTROPY_HIGH_PARANOIA);

This is bogus: sizeof(ior) gives the size of the pointer, not the size
of the pointed data.

> Index: device/cons.c
>
>  #ifdef MACH_KERNEL
>  int
>  cnmaygetc()
> -{
> + {

Particularly useless modification :)

> @@ -677,6 +680,13 @@
>       moved.mm_deltaX = (char)mousebuf[1] + (char)mousebuf[3];
>       moved.mm_deltaY = (char)mousebuf[2] + (char)mousebuf[4];
>
> +#ifdef MACH_ENTROPY
> +     /* Kick some mouse data to the entropy driver.  */
> +     entropy_putchar((buttonchanges + moved.mm_deltaX
> +                      + moved.mm_deltaY), ENTROPY_HIGH_QUALITY,
ENTROPY_MEDIUM_PARANOIA);

Shouldn't mm_deltaX and mm_deltaY rather get shifted left and ORed?

> Index: linux/dev/glue/net.c
> @@ -479,6 +488,12 @@
>    skb->reply = reply_port;
>    skb->reply_type = reply_port_type;
>
> +#ifdef MACH_ENTROPY
> +  /* Grab the packet for entropy purposes.  */
> +  entropy_putdata(skb, sizeof(skb), ENTROPY_HIGH_QUALITY,
ENTROPY_LOW_PARANOIA);

Again, sizeof(skb) gives you the size of the pointer, not the size of
the skb structure... Also, putting the content of the skb in the entropy
pool won't put the network data in the pool, you need to put skb->data,
skb->len for that.

> +void
> +entropyinit()
> +{
...
> +
> +  /* Zero out array's */
> +  memset(entropy_qualities, 0, ENTROPYBUFSIZE);
> +  memset(entropy_buffer, 0, ENTROPYBUFSIZE);

Isn't entropyinit() supposed to be called only once?  If yes, then this
is useless: static data is always already initialized to zero by
default.

> +io_return_t
> +entropyopen(dev_t dev, int flag, io_req_t ior)
> +{
> +  /* Check to see if we've initalized entropy.   */
> +  if (entropy_init_done == 0)
> +    {
> +       entropyinit();
> +    }

I'd rather see this chick inside entropyinit() itself, but even better,
you should manage to have entropyinit() called just once early enough
during the startup instead of calling it all the time.

> +entropy_read(io_req_t ior)
> +  amt = ior->io_count;
> +  if (amt > len)

Shouldn't that be if (amt >= len), since that case will deplete the pool
too?

> +    {
> +      /* This read will deplete the pool, mark it empty */
> +      amt = len;
> +      is_entropy_empty = 1;
> +    }

Instead of a global variable which you have to keep up to date, can't
you just read entropy_amount instead of reading is_entropy_empty?

> +      /* We need to wrap around so do it in two copies.  */
> +      int cnt;
> +      cnt = ENTROPYBUFSIZE - entropy_read_offset;
> +      memcpy(ior->io_data, entropy_buffer + entropy_read_offset, cnt);
> +      memcpy(ior->io_data, entropy_buffer, amt - cnt);

Bogus: you're putting data twice at ior->io_data. You need to add cnt
for the second memcpy.

> +entropy_read_done(io_req_t ior)
> +{
> +  if (amt > len)
> +    {
> +      /* This read will depelete the pool, mark it empty */
> +      is_entropy_empty = 1;
> +      amt = len;
> +    }

Same as above.

> +      /* The buffer needs to wrap around, so copy in two
> +      installments.  */
> +      int cnt;
> +
> +      cnt = ENTROPYBUFSIZE - entropy_read_offset;
> +      memcpy (ior->io_data, entropy_buffer + entropy_read_offset, cnt);
> +      memcpy (ior->io_data + cnt, entropy_buffer, amt - cnt);
> +    }

This time it's correct. Shouldn't both functions share such code instead
of duplicating potential bugs?

> +entropy_putchar(char c, enum entropy_quality quality, enum
entropy_paranoia paranoia)
> +  /* Alright, we can do one of three things
> +     1. Uncondtionally accept incoming entropy
> +        (done if the buffer is not full)

Please take care of spelling :) (there are a few other such typos in
comments in several places)

> +      would be most appericated */

Really please take care :)


> +      if (entropy_write_offset == ENTROPYBUFSIZE)
> +      entropy_write_offset = 0;

missing indentation.

> +      /* If the buffer been filled, start generating averages */
> +      if (entropy_amount == ENTROPYBUFSIZE)
> +     use_entropic_average = 1;

Same remark as for is_entropy_empty: I don't see the point of keeping
a separate coherency-bug-prone global state variable rather than just
reading entropy_amount instead of reading use_entropic_average.

> +      for (i = 0; i != ENTROPYBUFSIZE; i++)
> +       total = total + entropy_qualities[i];

This total, however, could be cached so that you don't have to recompute
it.

> +  /* Deep magic (tells any read functions more entropy is
> +     available) */
> +
> +  while ((ior = (io_req_t) dequeue_head (&entropy_read_queue)) != NULL)
> +    iodone (ior);

IIRC I already said that this should really not be a while loop: this
function only provides _one_ character of entropy, so it's only useful
to wake _one_ reader.

> +void
> +entropy_put_timestamp(enum entropy_quality quality, enum entropy_paranoia
paranoia)
> +{
> +  /* elasped ticks is a global from mach_clock.h */
> +  entropy_putchar(elapsed_ticks, quality, paranoia);
> +}

We should probably have an architecture-dependant macro for more precise
timestamps: starting from the pentium we could use rdtsc for instance.
In such case we could announce a yet better entropy quality.

> +/* configure should give us a paranoia level
> +   but if not, we'll default to none */
> +
> +/* FIXME: Make this changable at runtime */
> +#ifndef ENTROPY_PARANOIA_LEVEL
> +#define ENTROPY_PARANOIA_LEVEL 0
> +#endif /* ENTROPY_PARANOIA_LEVEL */

Default values usually belong to configure scripts (so that --help can
give it, for instance), so these lines should go there.


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/patch/?6088>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/





reply via email to

[Prev in Thread] Current Thread [Next in Thread]