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(®0, ®1, ®2, ®3);
> 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(®0, ®1, ®2, ®3);
> 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(®0, ®1, ®2, ®3);
> 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
