Re: kern/44964: cgd seems to panic on unaligned writes instead of giving EINVAL

[Taylor R Campbell <campbell+netbsd%mumble.net@localhost>]

The following reply was made to PR kern/44964; it has been noted by GNATS.

From: Taylor R Campbell <campbell+netbsd%mumble.net@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: 
Subject: Re: kern/44964: cgd seems to panic on unaligned writes instead of 
giving EINVAL
Date: Sat, 14 May 2011 05:07:32 +0000

    Date: Sat, 14 May 2011 02:05:03 +0000 (UTC)
    From: christos%zoulas.com@localhost (Christos Zoulas)
 
    Should be fixed on revision rijndael-api-fst.c rev 1.21
 
 (1.22, presumably.)  I don't think this fully fixes the problem: other
 parts of cgd are not prepared to handle unaligned writes.  For
 example, cgd_cipher requires that len be an integral multiple of
 blocksize (by assertion) and an integral multiple of secsize (by
 subtracting secsize from the size_t len until len is zero).
 
 Also, I'm worried that using 8-bit operations rather than 32-bit
 operations will hurt performance; have you compared the performance
 before and after?
 
 How about the following patch to reject unaligned writes?  Untested,
 but I'll test it in the next couple days if I get a chance.
 
 Index: cgd.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/cgd.c,v
 retrieving revision 1.71
 diff -p -u -r1.71 cgd.c
 --- cgd.c      19 Nov 2010 06:44:39 -0000      1.71
 +++ cgd.c      14 May 2011 05:06:22 -0000
 @@ -293,6 +293,21 @@ cgdstrategy(struct buf *bp)
  
        DPRINTF_FOLLOW(("cgdstrategy(%p): b_bcount = %ld\n", bp,
            (long)bp->b_bcount));
 +
 +      /*
 +       * Reject unaligned writes.  We can encrypt and decrypt only
 +       * complete disk sectors, and we let the ciphers require their
 +       * buffers to be aligned to 32-bit boundaries.
 +       */
 +      if (bp->b_blkno < 0 ||
 +          (bp->b_bcount % DEV_BSIZE) != 0 ||
 +          ((uintptr_t)bp->b_data & 3) != 0) {
 +              bp->b_error = EINVAL;
 +              bp->b_resid = bp->b_bcount;
 +              biodone(bp);
 +              return;
 +      }
 +
        /* XXXrcd: Should we test for (cs != NULL)? */
        dk_strategy(di, &cs->sc_dksc, bp);
        return;