is size correct in ecryptfs_parse_packet_length()

is size correct in ecryptfs_parse_packet_length()

[Dan Carpenter]

fs/ecryptfs/keystore.c +93
    85  /**
    86   * ecryptfs_parse_packet_length
    87   * @data: Pointer to memory containing length at offset
    88   * @size: This function writes the decoded size to this memory
    89   *        address; zero on error
    90   * @length_size: The number of bytes occupied by the encoded length
    91   *
    92   * Returns zero on success; non-zero on error
    93   */
    94  int ecryptfs_parse_packet_length(unsigned char *data, size_t *size,
    95                                   size_t *length_size)
    96  {
    97          int rc = 0;
    98  
    99          (*length_size) = 0;
   100          (*size) = 0;
   101          if (data[0] < 192) {
   102                  /* One-byte length */
   103                  (*size) = (unsigned char)data[0];
   104                  (*length_size) = 1;
   105          } else if (data[0] < 224) {
   106                  /* Two-byte length */
   107                  (*size) = (((unsigned char)(data[0]) - 192) * 256);
                                    ^^^^^^^^^^^^^^^
   108                  (*size) += ((unsigned char)(data[1]) + 192);
                                    ^^^^^^^^^^^^^^^
These casts are no-ops because they are "data" is an unsigned char
pointer already.  Then the value is type promoted to int, we subtract
192 giving a negative number and we multiply by 256 giving a slightly
larger negative then we save it as a very large positive.

I don't know this well enough to say what the intent was.

   109                  (*length_size) = 2;
   110          } else if (data[0] == 255) {
   111                  /* If support is added, adjust ECRYPTFS_MAX_PKT_LEN_SIZE */
   112                  ecryptfs_printk(KERN_ERR, "Five-byte packet length not "
   113                                  "supported\n");
   114                  rc = -EINVAL;
   115                  goto out;
   116          } else {
   117                  ecryptfs_printk(KERN_ERR, "Error parsing packet length\n");
   118                  rc = -EINVAL;
   119                  goto out;
   120          }
   121  out:
   122          return rc;
   123  }

regards,
dan carpenter

Re: is size correct in ecryptfs_parse_packet_length()

[Tyler Hicks]

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

Hi Dan - Thanks for taking a look at the code!

On 2014-10-21 15:04:35, Dan Carpenter wrote:
> fs/ecryptfs/keystore.c +93
>     85  /**
>     86   * ecryptfs_parse_packet_length
>     87   * @data: Pointer to memory containing length at offset
>     88   * @size: This function writes the decoded size to this memory
>     89   *        address; zero on error
>     90   * @length_size: The number of bytes occupied by the encoded length
>     91   *
>     92   * Returns zero on success; non-zero on error
>     93   */
>     94  int ecryptfs_parse_packet_length(unsigned char *data, size_t *size,
>     95                                   size_t *length_size)
>     96  {
>     97          int rc = 0;
>     98  
>     99          (*length_size) = 0;
>    100          (*size) = 0;
>    101          if (data[0] < 192) {
>    102                  /* One-byte length */
>    103                  (*size) = (unsigned char)data[0];
>    104                  (*length_size) = 1;
>    105          } else if (data[0] < 224) {
>    106                  /* Two-byte length */
>    107                  (*size) = (((unsigned char)(data[0]) - 192) * 256);
>                                     ^^^^^^^^^^^^^^^
>    108                  (*size) += ((unsigned char)(data[1]) + 192);
>                                     ^^^^^^^^^^^^^^^
> These casts are no-ops because they are "data" is an unsigned char
> pointer already.  Then the value is type promoted to int, we subtract
> 192 giving a negative number and we multiply by 256 giving a slightly
> larger negative then we save it as a very large positive.

Subtracting 192 from data[0] should never result in a negative number.
We know that data[0] is greater than or equal to 192 (and less than 224)
because the previous conditional was false.

> 
> I don't know this well enough to say what the intent was.

This method of specifying packet sizes came from RFC2440 "OpenPGP
Message Format" (https://tools.ietf.org/html/rfc2440#section-4.2.2).

Tyler

> 
>    109                  (*length_size) = 2;
>    110          } else if (data[0] == 255) {
>    111                  /* If support is added, adjust ECRYPTFS_MAX_PKT_LEN_SIZE */
>    112                  ecryptfs_printk(KERN_ERR, "Five-byte packet length not "
>    113                                  "supported\n");
>    114                  rc = -EINVAL;
>    115                  goto out;
>    116          } else {
>    117                  ecryptfs_printk(KERN_ERR, "Error parsing packet length\n");
>    118                  rc = -EINVAL;
>    119                  goto out;
>    120          }
>    121  out:
>    122          return rc;
>    123  }
> 
> regards,
> dan carpenter
> --
> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: is size correct in ecryptfs_parse_packet_length()

[Dan Carpenter]

On Tue, Oct 21, 2014 at 05:29:53PM -0400, Tyler Hicks wrote:
> >    101          if (data[0] < 192) {
> >    102                  /* One-byte length */
> >    103                  (*size) = (unsigned char)data[0];
> >    104                  (*length_size) = 1;
> >    105          } else if (data[0] < 224) {
> >    106                  /* Two-byte length */
> >    107                  (*size) = (((unsigned char)(data[0]) - 192) * 256);
> >                                     ^^^^^^^^^^^^^^^
> >    108                  (*size) += ((unsigned char)(data[1]) + 192);
> >                                     ^^^^^^^^^^^^^^^
> > These casts are no-ops because they are "data" is an unsigned char
> > pointer already.  Then the value is type promoted to int, we subtract
> > 192 giving a negative number and we multiply by 256 giving a slightly
> > larger negative then we save it as a very large positive.
> 
> Subtracting 192 from data[0] should never result in a negative number.
> We know that data[0] is greater than or equal to 192 (and less than 224)
> because the previous conditional was false.
> 

Oh right.  Duh...  Thanks.

We could remove the casting though because it's a no-op?

regards,
dan carpenter

[PATCH] eCryptfs: Remove unnecessary casts when parsing packet lengths

[Tyler Hicks]

The elements in the data array are already unsigned chars and do not
need to be casted.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 fs/ecryptfs/keystore.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
index 635e8e1..917bd5c 100644
--- a/fs/ecryptfs/keystore.c
+++ b/fs/ecryptfs/keystore.c
@@ -100,12 +100,12 @@ int ecryptfs_parse_packet_length(unsigned char *data, size_t *size,
 	(*size) = 0;
 	if (data[0] < 192) {
 		/* One-byte length */
-		(*size) = (unsigned char)data[0];
+		(*size) = data[0];
 		(*length_size) = 1;
 	} else if (data[0] < 224) {
 		/* Two-byte length */
-		(*size) = (((unsigned char)(data[0]) - 192) * 256);
-		(*size) += ((unsigned char)(data[1]) + 192);
+		(*size) = (data[0] - 192) * 256;
+		(*size) += data[1] + 192;
 		(*length_size) = 2;
 	} else if (data[0] == 255) {
 		/* If support is added, adjust ECRYPTFS_MAX_PKT_LEN_SIZE */
-- 
2.1.0