patchThe GNU Hurd - Patches: patch #6088, Entropy Patch

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #6088: Entropy Patch

Submitter:  None
Submitted:  Thu 12 Jul 2007 07:09:52 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Originator Email:  -email is unavailable-
Open/Closed:  Open Planned Release:  None
Wiki-like text discussion box: 


Fri 18 Jul 2008 01:53:37 PM UTC, comment #1: 

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.

Michael Banck <mbanck>
Thu 12 Jul 2007 07:09:52 PM UTC, original submission:  

I'm just submitting the patch here, it's been wrapped up all into a single file.

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #16141:  entropy_patch.diff added by mbanck (29KiB - text/x-patch - last known version of patch from 2007/08/28)
file #13316:  entropy_patch.diff added by None (21KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mbanck (Updated the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-07-18 mbanck Attached File- Added entropy_patch.diff, #16141
    2007-07-12 None Attached File- Added entropy_patch.diff, #13316

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code