* is size correct in ecryptfs_parse_packet_length()
@ 2014-10-21 12:04 Dan Carpenter
2014-10-21 21:29 ` Tyler Hicks
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2014-10-21 12:04 UTC (permalink / raw)
To: ecryptfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: is size correct in ecryptfs_parse_packet_length()
2014-10-21 12:04 is size correct in ecryptfs_parse_packet_length() Dan Carpenter
@ 2014-10-21 21:29 ` Tyler Hicks
2014-10-22 7:58 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Tyler Hicks @ 2014-10-21 21:29 UTC (permalink / raw)
To: Dan Carpenter; +Cc: ecryptfs
[-- 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 --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: is size correct in ecryptfs_parse_packet_length()
2014-10-21 21:29 ` Tyler Hicks
@ 2014-10-22 7:58 ` Dan Carpenter
2014-10-23 14:38 ` [PATCH] eCryptfs: Remove unnecessary casts when parsing packet lengths Tyler Hicks
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2014-10-22 7:58 UTC (permalink / raw)
To: Tyler Hicks; +Cc: ecryptfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] eCryptfs: Remove unnecessary casts when parsing packet lengths
2014-10-22 7:58 ` Dan Carpenter
@ 2014-10-23 14:38 ` Tyler Hicks
0 siblings, 0 replies; 4+ messages in thread
From: Tyler Hicks @ 2014-10-23 14:38 UTC (permalink / raw)
To: ecryptfs; +Cc: Dan Carpenter
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-10-23 14:39 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-21 12:04 is size correct in ecryptfs_parse_packet_length() Dan Carpenter
2014-10-21 21:29 ` Tyler Hicks
2014-10-22 7:58 ` Dan Carpenter
2014-10-23 14:38 ` [PATCH] eCryptfs: Remove unnecessary casts when parsing packet lengths Tyler Hicks
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.