* 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).