2022-07-19 08:15:39

by Dan Carpenter

Subject: [bug report] crypto: aria - Implement ARIA symmetric cipher algorithm

Hello Taehee Yoo,

The patch e4e712bbbd6d: "crypto: aria - Implement ARIA symmetric
cipher algorithm" from Jul 4, 2022, leads to the following Smatch
static checker warning:

crypto/aria.c:69 aria_set_encrypt_key() error: buffer overflow 'ck' 4 <= 4
crypto/aria.c:70 aria_set_encrypt_key() error: buffer overflow 'ck' 4 <= 5
crypto/aria.c:71 aria_set_encrypt_key() error: buffer overflow 'ck' 4 <= 6
crypto/aria.c:72 aria_set_encrypt_key() error: buffer overflow 'ck' 4 <= 7
crypto/aria.c:86 aria_set_encrypt_key() error: buffer overflow 'ck' 4 <= 8
crypto/aria.c:87 aria_set_encrypt_key() error: buffer overflow 'ck' 4 <= 9
crypto/aria.c:88 aria_set_encrypt_key() error: buffer overflow 'ck' 4 <= 10
crypto/aria.c:89 aria_set_encrypt_key() error: buffer overflow 'ck' 4 <= 11

crypto/aria.c
19 static void aria_set_encrypt_key(struct aria_ctx *ctx, const u8 *in_key,
20 unsigned int key_len)
21 {
22 const __be32 *key = (const __be32 *)in_key;
23 u32 w0[4], w1[4], w2[4], w3[4];
24 u32 reg0, reg1, reg2, reg3;
25 const u32 *ck;
26 int rkidx = 0;
27
28 ck = &key_rc[(key_len - 16) / 8][0];

key_rc is declared like this:

static const u32 key_rc[5][4] = {

29
30 w0[0] = be32_to_cpu(key[0]);
31 w0[1] = be32_to_cpu(key[1]);
32 w0[2] = be32_to_cpu(key[2]);
33 w0[3] = be32_to_cpu(key[3]);
34
35 reg0 = w0[0] ^ ck[0];
36 reg1 = w0[1] ^ ck[1];
37 reg2 = w0[2] ^ ck[2];
38 reg3 = w0[3] ^ ck[3];
39
40 aria_subst_diff_odd(&reg0, &reg1, &reg2, &reg3);
41
42 if (key_len > 16) {
43 w1[0] = be32_to_cpu(key[4]);
44 w1[1] = be32_to_cpu(key[5]);
45 if (key_len > 24) {
46 w1[2] = be32_to_cpu(key[6]);
47 w1[3] = be32_to_cpu(key[7]);
48 } else {
49 w1[2] = 0;
50 w1[3] = 0;
51 }
52 } else {
53 w1[0] = 0;
54 w1[1] = 0;
55 w1[2] = 0;
56 w1[3] = 0;
57 }
58
59 w1[0] ^= reg0;
60 w1[1] ^= reg1;
61 w1[2] ^= reg2;
62 w1[3] ^= reg3;
63
64 reg0 = w1[0];
65 reg1 = w1[1];
66 reg2 = w1[2];
67 reg3 = w1[3];
68
--> 69 reg0 ^= ck[4];

So 4 and above is out of bounds.

70 reg1 ^= ck[5];
71 reg2 ^= ck[6];
72 reg3 ^= ck[7];
73
74 aria_subst_diff_even(&reg0, &reg1, &reg2, &reg3);
75
76 reg0 ^= w0[0];
77 reg1 ^= w0[1];
78 reg2 ^= w0[2];
79 reg3 ^= w0[3];
80
81 w2[0] = reg0;
82 w2[1] = reg1;
83 w2[2] = reg2;
84 w2[3] = reg3;
85
86 reg0 ^= ck[8];
87 reg1 ^= ck[9];
88 reg2 ^= ck[10];
89 reg3 ^= ck[11];
90
91 aria_subst_diff_odd(&reg0, &reg1, &reg2, &reg3);
92
93 w3[0] = reg0 ^ w1[0];
94 w3[1] = reg1 ^ w1[1];
95 w3[2] = reg2 ^ w1[2];
96 w3[3] = reg3 ^ w1[3];
97
98 aria_gsrk(ctx->enc_key[rkidx], w0, w1, 19);
99 rkidx++;
100 aria_gsrk(ctx->enc_key[rkidx], w1, w2, 19);
101 rkidx++;
102 aria_gsrk(ctx->enc_key[rkidx], w2, w3, 19);
103 rkidx++;
104 aria_gsrk(ctx->enc_key[rkidx], w3, w0, 19);
105
106 rkidx++;
107 aria_gsrk(ctx->enc_key[rkidx], w0, w1, 31);
108 rkidx++;
109 aria_gsrk(ctx->enc_key[rkidx], w1, w2, 31);
110 rkidx++;
111 aria_gsrk(ctx->enc_key[rkidx], w2, w3, 31);
112 rkidx++;
113 aria_gsrk(ctx->enc_key[rkidx], w3, w0, 31);
114
115 rkidx++;
116 aria_gsrk(ctx->enc_key[rkidx], w0, w1, 67);
117 rkidx++;
118 aria_gsrk(ctx->enc_key[rkidx], w1, w2, 67);
119 rkidx++;
120 aria_gsrk(ctx->enc_key[rkidx], w2, w3, 67);
121 rkidx++;
122 aria_gsrk(ctx->enc_key[rkidx], w3, w0, 67);
123
124 rkidx++;
125 aria_gsrk(ctx->enc_key[rkidx], w0, w1, 97);
126 if (key_len > 16) {
127 rkidx++;
128 aria_gsrk(ctx->enc_key[rkidx], w1, w2, 97);
129 rkidx++;
130 aria_gsrk(ctx->enc_key[rkidx], w2, w3, 97);
131
132 if (key_len > 24) {
133 rkidx++;
134 aria_gsrk(ctx->enc_key[rkidx], w3, w0, 97);
135
136 rkidx++;
137 aria_gsrk(ctx->enc_key[rkidx], w0, w1, 109);
138 }
139 }
140 }

regards,
dan carpenter

2022-07-19 15:33:51

by Taehee Yoo

Subject: Re: [bug report] crypto: aria - Implement ARIA symmetric cipher algorithm

Hi Dan,
Thank you so much for your report!

On 7/19/22 17:14, Dan Carpenter wrote:
> Hello Taehee Yoo,
>
> The patch e4e712bbbd6d: "crypto: aria - Implement ARIA symmetric
> cipher algorithm" from Jul 4, 2022, leads to the following Smatch
> static checker warning:
>
> crypto/aria.c:69 aria_set_encrypt_key() error: buffer overflow 'ck' 4
<= 4
> crypto/aria.c:70 aria_set_encrypt_key() error: buffer overflow 'ck' 4
<= 5
> crypto/aria.c:71 aria_set_encrypt_key() error: buffer overflow 'ck' 4
<= 6
> crypto/aria.c:72 aria_set_encrypt_key() error: buffer overflow 'ck' 4
<= 7
> crypto/aria.c:86 aria_set_encrypt_key() error: buffer overflow 'ck' 4
<= 8
> crypto/aria.c:87 aria_set_encrypt_key() error: buffer overflow 'ck' 4
<= 9
> crypto/aria.c:88 aria_set_encrypt_key() error: buffer overflow 'ck' 4
<= 10
> crypto/aria.c:89 aria_set_encrypt_key() error: buffer overflow 'ck' 4
<= 11
>
> crypto/aria.c
> 19 static void aria_set_encrypt_key(struct aria_ctx *ctx, const
u8 *in_key,
> 20 unsigned int key_len)
> 21 {
> 22 const __be32 *key = (const __be32 *)in_key;
> 23 u32 w0[4], w1[4], w2[4], w3[4];
> 24 u32 reg0, reg1, reg2, reg3;
> 25 const u32 *ck;
> 26 int rkidx = 0;
> 27
> 28 ck = &key_rc[(key_len - 16) / 8][0];
>
> key_rc is declared like this:
>
> static const u32 key_rc[5][4] = {
>
> 29
> 30 w0[0] = be32_to_cpu(key[0]);
> 31 w0[1] = be32_to_cpu(key[1]);
> 32 w0[2] = be32_to_cpu(key[2]);
> 33 w0[3] = be32_to_cpu(key[3]);
> 34
> 35 reg0 = w0[0] ^ ck[0];
> 36 reg1 = w0[1] ^ ck[1];
> 37 reg2 = w0[2] ^ ck[2];
> 38 reg3 = w0[3] ^ ck[3];
> 39
> 40 aria_subst_diff_odd(&reg0, &reg1, &reg2, &reg3);
> 41
> 42 if (key_len > 16) {
> 43 w1[0] = be32_to_cpu(key[4]);
> 44 w1[1] = be32_to_cpu(key[5]);
> 45 if (key_len > 24) {
> 46 w1[2] = be32_to_cpu(key[6]);
> 47 w1[3] = be32_to_cpu(key[7]);
> 48 } else {
> 49 w1[2] = 0;
> 50 w1[3] = 0;
> 51 }
> 52 } else {
> 53 w1[0] = 0;
> 54 w1[1] = 0;
> 55 w1[2] = 0;
> 56 w1[3] = 0;
> 57 }
> 58
> 59 w1[0] ^= reg0;
> 60 w1[1] ^= reg1;
> 61 w1[2] ^= reg2;
> 62 w1[3] ^= reg3;
> 63
> 64 reg0 = w1[0];
> 65 reg1 = w1[1];
> 66 reg2 = w1[2];
> 67 reg3 = w1[3];
> 68
> --> 69 reg0 ^= ck[4];
>
> So 4 and above is out of bounds.
>
> 70 reg1 ^= ck[5];
> 71 reg2 ^= ck[6];
> 72 reg3 ^= ck[7];
> 73
> 74 aria_subst_diff_even(&reg0, &reg1, &reg2, &reg3);
> 75
> 76 reg0 ^= w0[0];
> 77 reg1 ^= w0[1];
> 78 reg2 ^= w0[2];
> 79 reg3 ^= w0[3];
> 80
> 81 w2[0] = reg0;
> 82 w2[1] = reg1;
> 83 w2[2] = reg2;
> 84 w2[3] = reg3;
> 85
> 86 reg0 ^= ck[8];
> 87 reg1 ^= ck[9];
> 88 reg2 ^= ck[10];
> 89 reg3 ^= ck[11];
> 90
> 91 aria_subst_diff_odd(&reg0, &reg1, &reg2, &reg3);
> 92
> 93 w3[0] = reg0 ^ w1[0];
> 94 w3[1] = reg1 ^ w1[1];
> 95 w3[2] = reg2 ^ w1[2];
> 96 w3[3] = reg3 ^ w1[3];
> 97
> 98 aria_gsrk(ctx->enc_key[rkidx], w0, w1, 19);
> 99 rkidx++;
> 100 aria_gsrk(ctx->enc_key[rkidx], w1, w2, 19);
> 101 rkidx++;
> 102 aria_gsrk(ctx->enc_key[rkidx], w2, w3, 19);
> 103 rkidx++;
> 104 aria_gsrk(ctx->enc_key[rkidx], w3, w0, 19);
> 105
> 106 rkidx++;
> 107 aria_gsrk(ctx->enc_key[rkidx], w0, w1, 31);
> 108 rkidx++;
> 109 aria_gsrk(ctx->enc_key[rkidx], w1, w2, 31);
> 110 rkidx++;
> 111 aria_gsrk(ctx->enc_key[rkidx], w2, w3, 31);
> 112 rkidx++;
> 113 aria_gsrk(ctx->enc_key[rkidx], w3, w0, 31);
> 114
> 115 rkidx++;
> 116 aria_gsrk(ctx->enc_key[rkidx], w0, w1, 67);
> 117 rkidx++;
> 118 aria_gsrk(ctx->enc_key[rkidx], w1, w2, 67);
> 119 rkidx++;
> 120 aria_gsrk(ctx->enc_key[rkidx], w2, w3, 67);
> 121 rkidx++;
> 122 aria_gsrk(ctx->enc_key[rkidx], w3, w0, 67);
> 123
> 124 rkidx++;
> 125 aria_gsrk(ctx->enc_key[rkidx], w0, w1, 97);
> 126 if (key_len > 16) {
> 127 rkidx++;
> 128 aria_gsrk(ctx->enc_key[rkidx], w1, w2, 97);
> 129 rkidx++;
> 130 aria_gsrk(ctx->enc_key[rkidx], w2, w3, 97);
> 131
> 132 if (key_len > 24) {
> 133 rkidx++;
> 134 aria_gsrk(ctx->enc_key[rkidx], w3,
w0, 97);
> 135
> 136 rkidx++;
> 137 aria_gsrk(ctx->enc_key[rkidx], w0,
w1, 109);
> 138 }
> 139 }
> 140 }
>
> regards,
> dan carpenter

I think this is a false positive of smatch.
ck is a pointer and it points key_rc, which is a double array.
But ck is used as a single array.
So, I think smatch warns it although there are actually no out-of-bounds.

I just tested changing key_rc to a single array.
There are no smatch warnings.
So, I will send a patch to avoid this smatch warning.

Thank you so much,
Taehee Yoo