Re: Entropy Patch with Linebreaks

[Samuel Thibault]

Hi,

Michael Casadevall, le Sun 12 Aug 2007 16:44:59 -0400, a écrit :
> >>@@ -149,6 +151,10 @@
> >>     do {
> >>         prev = next;
> >>         next = prev->io_next;
> >>+#ifdef MACH_ENTROPY
> >>+        /* Let's grab the cylinder numbers for entropy.  */
> >>+        entropy_putdata (prev, sizeof(io_req_t), ENTROPY_HIGH_QUALITY);
> >>+#endif
> >>     } while (next != 0 && prev->io_cylinder == next->io_cylinder);
> >>
> >>     if (next == 0) {
> >
> >Mmm, haven't you noticed that this function is actually never used? :)
> >Anyway, it may still be useful since disk drivers are supposed to use
> >it. However, I don't see why you're putting the whole list into the
> >entropy pool, should it be just the request that is to added? (i.e.  
> >just
> >ior).
> >
> 
> Eh, can't hurt to have the code there, I think it is used though for
> any device drivers used within mach itself.

Should be, yes. But that's not my real point: you've put the call in the
list scan, so it's not adding just the new request's cylinder, but the
cylinder of _all_ currently pending requests (and will do again next
time, etc). Move that out of the loop.

> I could rename the function to entropy_putwhatever :-)

Actually, now that I can read the prototype in the header ;), it should
probably be putint.

> >>+    if (romgetc)
> >>+      {
> >>+#if defined (MACH_KERNEL) && defined(MACH_ENTROPY)
> >>+        entropy_putchar(*romgetc, ENTROPY_MEDIUM_QUALITY);
> >>+#endif /* MACH_KERNEL && MACH_ENTROPY */
> >
> >That should be an entropy_putptr, shouldn't it? Or you can define a
> >generic entropy_put macro with typeof() & sizeof() which is able to
> >infer the size.
> >
> 
> I'm not adding the pointer itself.

You definitely are. What do you expect to be adding by writing this code?

> >>+    entropy_putchar(scancode | (up ? 0200 : 0), ENTROPY_LOW_QUALITY);
> >Why low quality?
> 
> I think the data itself is low quality,

As discussed on IRC, people often type random stuff (IRC chats, PhD
thesis, love mails...) so we agreed that it's high quality :)

> I'm not too worried about security, the mixing code is designed to
> make it that even if you were to constantly enter the same character
> over and over again, it would generate a different number. 

That still can be predicted.

> Also, the mixing code uses entropy from the pool to reseed itself,
> so even if it was possible to manipulate the pool that way, you'd
> also need to control all the other entropic sources

Which are not necessarily so many. Computers are sometimes _really_
idle...

> and have a super computer to work out what the input would have to be.

Some people have them.

Yes, I'm paranoid, probably because of the courses I've taught :)

> >>-void add_blkdev_randomness(int major);
> >>+extern void add_blkdev_randomness(int major);
> >
> >Well, it's just the same for the compiler :)
> >
> I think I changed this because it tossed an error the other way.

I can't even imagine that. Forward declarations of functions don't need
extern, since it's always extern.

> +       {
> +#if defined(MACH_KERNEL) && defined(MACH_ENTROPY)
> +         entropy_putchar (((*cn_tab->cn_getc)(cn_tab->cn_dev, 1)), 
> ENTROPY_MEDIUM_QUALITY);
> +#endif /* MACH_ENTROPY and MACH_ENTROPY */
> +         return ((*cn_tab->cn_getc)(cn_tab->cn_dev, 1));
> +       }     

This is bogus: you are calling cn_getc twice: one keypress goes to
entropy, the other goes to the console. Call only once and use the
result twice.

> +     if (romgetc) 
> +       {
> +#if defined (MACH_KERNEL) && defined(MACH_ENTROPY)
> +         entropy_putchar(((*romgetc)(1)), ENTROPY_MEDIUM_QUALITY);
> +#endif /* MACH_KERNEL && MACH_ENTROPY */ 
> +         return ((*romgetc)(1));
> +       }     
>       return (0);

Same problem here (but now at least you're not putting a pointer any
more)

>  extern int printf (const char *, ...);
> @@ -224,6 +228,15 @@
>  void
>  add_blkdev_randomness (int major)
>  {
> +#ifdef MACH_ENTROPY
> +  /* This is useless for good quality, so we'll only use if it nothing
> +     else is available - The problem is that mach only has 1 block
> +     device, floppy (major 3 corresponds to Ctrl C) so this is useless
> +     for entropic sources. If we ever get more block devices the
> +     quality should be upped for additional entropy. */
> +  
> +  entropy_putchar(major, ENTROPY_POOR_QUALITY);
> +#endif
>  }

A timestamp (that would probably be high quality btw) should be put
here.

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

Mmm, since you've enqueued only one bit of entropy, shouldn't you wake
up only one of them?

> + /* Try to grab a lock on the device, otherwise bail.  */
> +  if (!simple_lock_try(&entropy_lock))
> +    return;

On SMP, you're grabbing it in putdata and then calling putchar, which
itself tries to grab it, and will always fail (since putdata already has
it). You have to free the lock before calling putchar, which will make
the checks anyway.

> +  for (i = 0; i == size; i++) 
> +    entropy_putchar((int)data+i, quality);

You're not putting data, but pointers.

> +  /* Tell any reads that we have more entropy.  */
> +  while ((ior = (io_req_t) dequeue_head (&entropy_read_queue)) != NULL)
> +    iodone (ior);

This is already done by entropy_putchar, no need to do it again.

> +void
> +entropy_put_timestamp()
> +{
> +  /* elasped ticks is a global from mach_clock.h */
> +  entropy_putchar(elapsed_ticks, ENTROPY_HIGH_QUALITY);
> +}

I'd rather see the quality as a parameter. Some timestamps may be better
sources than others.

Samuel