Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made the key const, since it could be in ROM. Made 'Multiply' a function... #3

Closed
wants to merge 1 commit into from

Conversation

jcallan
Copy link

@jcallan jcallan commented Dec 4, 2014

Hi Kokke, thanks for writing such a small AES! I have made a few little changes which (hopefully) improve it even more. When I compile it using the ARM Keil compiler it now only uses 1268 bytes total.
Jon.

…ion instead of a macro, this reduces the code size. Corrected the test vectors in the comments.
@kokke
Copy link
Owner

kokke commented Dec 4, 2014

Hey dude,

I'm glad you like the code! And thanks for improving on it :)
I'll check out your changes and merge once I've reviewed them.

From ~2K to 1268 bytes is really a big improvement!
I'm looking forward to checking it out!

Kind regards

/Mikkel

On 04/12/14 13:28, jcallan wrote:

Hi Kokke, thanks for writing such a small AES! I have made a few
little changes which (hopefully) improve it even more. When I compile
it using the ARM Keil compiler it now only uses 1268 bytes total.
Jon.


    You can merge this Pull Request by running

git pull https://github.com/jcallan/tiny-AES128-C master

Or view, comment on, or merge it at:

#3

    Commit Summary


Reply to this email directly or view it on GitHub
#3.

@kokke
Copy link
Owner

kokke commented Dec 5, 2014

I can't reproduce the size improvements you've made with your additions. Actually, moving Multiply into a function increases code size here, from 2087 bytes to 2131 bytes.

$ size aes_jcallan.o
   text    data     bss     dec     hex filename
   1927       0     204    2131     853 aes_jcallan.o

$ size aes_kokke.o
   text    data     bss     dec     hex filename
   1883       0     204    2087     827 aes_kokke.o

I know the IAR compiler has a rep for generating tight code. Which version are you using?
Would you mind compiling the aes-module as-is and then with your additions and doing a size comparison like I've done above ?

@jcallan
Copy link
Author

jcallan commented Dec 5, 2014

Interesting! I am using Keil armcc.exe v4.1.0. Which is quite old now (2011).
It looks as if IAR is including the RO Data in 'text', if so it is producing smaller code than Keil, which I find quite impressive... maybe time to switch ;-)
I think IAR must be clever enough to spot the common code sequences and make Multiply into a function anyway. You could include a #define to choose whether to have Multiply as a macro or a function, perhaps?

My version:

Code (inc. data)   RO Data    RW Data    ZI Data      Debug   Object Name
1236         26        767         12        192       9884   aes.o

Your version:

Code (inc. data)   RO Data    RW Data    ZI Data      Debug   Object Name
1856         26        767         12        192       9832   aes.o

@kokke
Copy link
Owner

kokke commented Dec 5, 2014

I am really impressed by the output of your compiler! :)

What about you add a #define and a comment with a link to this issue so people can make a qualified decision. What do you think about that?

Then I'll merge.

Oh and by the way, I misread Keil as IAR when I read your first post. So when I was referencing IAR, I was talking about your Keil compiler. For the record I am using the free Mentor Graphics / GCC ARM toolchain. Sorry about the misunderstanding.

@jcallan
Copy link
Author

jcallan commented Dec 5, 2014

Yes, perfect!

@kokke
Copy link
Owner

kokke commented Dec 6, 2014

@jcallan Okay since I have a few hours on my hand tonight, I became impatient and decided to add some of the revisions you've proposed. I'll commit them in a few minutes.
I'm sorry if this causes a conflict, I'm not yet a very experienced git user and I edit files through the browser-UI I'm embarrassed to admit.
I'll of course accept the correction of the test vector in the top-file comment in aes.c.
I will however not accept adding a const-qualifier to the key without discussion :)
I have a bad feeling about declaring an uninitialized variable const. I know most compilers will accept assigning to a const variable, but you may be using different keys between invocations and so I don't think key should be const.

@jcallan
Copy link
Author

jcallan commented Dec 7, 2014

Mikkel, I am not declaring key as const - it is a pointer to something that is const. So I am saying to the compiler "don't allow writes to the data that key points to". A constant pointer would be
uint8_t *const key;
and in that case, you're right, it would have to be initialised and you wouldn't be able to change keys between invocations.
Ha, I'm not a very experienced git user either - I have used an old email address on the commit by accident... ah well...
Jon.

@kokke
Copy link
Owner

kokke commented Dec 8, 2014

Okay, that makes sense. The pointer points to const data, but is not const itself.
I tried to read through my K&R, but couldn't convince myself that it would be "correct" to const-qualify key.
Your comment helps clear it up, so I'll accept the const-qualifier as well :)

Will add it right away.

Thanks for improving the code base :)

@kokke
Copy link
Owner

kokke commented Dec 10, 2014

@jcallan I've added your proposed edits, so unless you have any objections, I'll close this issue.

@kokke kokke closed this Dec 10, 2014
@jcallan
Copy link
Author

jcallan commented Dec 11, 2014

That's great, thanks.

@bnorbs
Copy link

bnorbs commented Jun 4, 2015

Hi Kokke,
thanks for the good work.
I am going to use it in energia and for msp430G2553. This device has only 512 Bytes RAM. Therefore I had to try using the same buffer for input and output (encrypt // decrypt still to test) and it worked. I saw that you anyway copy input to output at an early stage. I wonder if it would be possible to change the code and everywhere use only one buffer. Perhaps one could also eliminate the static array state and save 16 more bytes ( :-) )?

Btw, I only use cbc, because ebc is almost every time prone to replay or chosen plaintext (and more) attacks. Therefore I was disappointed by TI's solution (which was mentioned above).

Thanks again, for the good job.

@MorgothCreator
Copy link

Great work, I compiled for atmega, atxmega E5 series, it works perfectly.

@skater-boy
Copy link

Just a note as reference for others.
Added a #define MULTIPLY_AS_A_FUNCTION 1 and tested on Cortex M0+, gcc compiler and -Os code optimization.
Code reduction is negligible.

@tecsantoshkumar
Copy link

hello friends,

any one help me for tiny-AES128CBC library how to used in this library salt and iteration count.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants