Avoid unaligned data access for grasshopper cipher. #376

shanechko · 2021-12-15T22:02:07Z

Compiler can produce incorrect instructions for arch with strict memory
access like ARM. Root case is compiler assuming some data type is always
aligned properly.

Pointer cast from unaligned pointer to this type `grasshopper_w128_t`
can trigger segmentation fault.

Compiler can produce incorrect instructions for arch with strict memory access like ARM. Root case is compiler assuming some data type is always aligned properly. Pointer cast from unaligned pointer to this type `grasshopper_w128_t` can trigger segmentation fault.

vt-alt · 2021-12-16T15:30:40Z

test_derive.c

@@ -183,7 +192,7 @@ static EVP_PKEY *load_private_key(int key_nid, int param_nid, const char *pk,
        BN_free(y);
        if (EC_POINT_cmp(group, pkey, xy, bc) == 0)
            printf("Public key %08x matches private key %08x\n",
-                   *(int *)pub, *(int *)pk);
+                   charbuf_to_uint(pub), charbuf_to_uint(pk));


Зачем это изменение - как вы поняли, что там могут быть unaligned данные?

Это исправление предупреждения компилятора.
Компилятор добавляет предупреждения при касте указателя с малого типа (char) в больший (int).

На архитектурах типа arm не каждый char* может быть конвертирован в int*.

Так как char* входящий параметр он не обязан быть выровнен по int.

vt-alt · 2021-12-16T15:33:20Z

gost_grasshopper_cipher.c

@@ -667,29 +655,36 @@ static int gost_grasshopper_cipher_do_cfb(EVP_CIPHER_CTX *ctx, unsigned char *ou
        (gost_grasshopper_cipher_ctx *) EVP_CIPHER_CTX_get_cipher_data(ctx);
    const unsigned char *in_ptr = in;
    unsigned char *out_ptr = out;
-    unsigned char *buf = EVP_CIPHER_CTX_buf_noconst(ctx);


Почему вы решили, что этот буфер unaligned?

скорее всего потому, что он выделяется изнутри openssl. Коллегам наверняка нужно выравнивание по 16 байтам, а компилятор по умолчанию выравнивает по 8. Компилятор openssl при этом ничего не знает про требования engine к выравниванию.

Спасибо. Но, почему должно быть выравнивание по 16 байт?
Особенно в том примере где просто доступ к (int *)?

Почему вы решили, что этот буфер unaligned?

Я исправил предупреждение компилятора. Компилятор рассматривает наихудший допустимый случай на основе типа данных.

@beldmit у компилятора (и языка С) есть ABI и который устанавливает обязательное выравнивание типов и структур. Проблема в том что когда метод возвращает указатель на char из поля структуры, информация о выравнивании теряется, и компилятор считает что это произвольный допустимый указатель на char.

Спасибо. Но, почему должно быть выравнивание по 16 байт? Особенно в том примере где просто доступ к (int *)?

Нет требования про 16 байт, это недопонимание.

Изменения в этом MR направлены на избавления от одного UB: Каста указателя меньшего типа к большему. Так делать нельзя, потому что это приводит к ошибкам на архитектурах с инструкциями требующими выровненного обращению к памяти.

vt-alt · 2021-12-16T16:41:59Z

Кстати, по поводу остальных мест - скорее всего надо просто убрать ALIGN(16), чтоб компилятор не думал, что там есть выравнивание.

vt-alt · 2021-12-16T23:33:38Z

Кстати, по поводу остальных мест - скорее всего надо просто убрать ALIGN(16), чтоб компилятор не думал, что там есть выравнивание.

В общем, моё мнение такое — я не одобряю этого PR, так как он испортит, а не улучшит код.

Лучше вообще не использовать align лишний раз (как и cast'ы к большему размеру) чем заниматься таким с выравниванием по 16 байт (которое ниоткуда логически не следует, и это уже магия (в плохом смысле), которую нужно помнить, а значит будущий код не защищен от таких "ошибок" (так как они не рациональны) — с моей точки зрения, это просто случайные "фиксы"). Не нужно сообщать компилятору ложную информацию (про align(16)) и он не сгенерирует unaligned код.
Лучше фиксить реальные баги — где есть реальный segmentation fault — (пример с charbuf_to_uint показывает, что тут это не так) — иначе, это просто нагромождение исправлений того что не сломано. Так же я рекомендую написать тесты, которые падают до фикса и работают после, доказывающие как наличие бага, так и эффективность фикса.
Такое нагромождение кода невозможно разевьювить.

vt-alt · 2021-12-16T23:43:49Z

Если принять этот PR, то и дальше и во всем коде надо будет выравнивать всё по 16 байт, такой тренд задан. Мы готовы и хотим это делать? Почему нигде люди таким не занимаются?

shanechko · 2021-12-17T06:42:09Z

Если принять этот PR, то и дальше и во всем коде надо будет выравнивать всё по 16 байт, такой тренд задан. Мы готовы и хотим это делать? Почему нигде люди таким не занимаются?

Я повторюсь это недопонимание. Не нужно ничего явно выравнивать, это работа компилятора правильно размещать данные. Нужно не мешать компилятору несовместимыми кастами указателей.

Посмотрите как реализованы другие блочные шифры.

shanechko · 2021-12-17T06:55:24Z

Кстати, по поводу остальных мест - скорее всего надо просто убрать ALIGN(16), чтоб компилятор не думал, что там есть выравнивание.

Это так не работает. Выравнивание есть всегда, на основе типа данных и ABI. И компилятор ожидает этого выравнивания.

Например если функция получает int* компилятор уверен что указатель кратен четырём. Если это SSE тип то компилятор знает что он выровнен по 16 байтам, и использует инструкции для работы с выровненными данными.

ALIGN(16) добавлен потому что на x86 архитектуре SSE требует выровненных данных. На ARM архитектуре работа с int требует выровненных данных.

shanechko · 2021-12-17T07:21:48Z

В общем, моё мнение такое — я не одобряю этого PR, так как он испортит, а не улучшит код.

Это не улучшение, а исправление реальных и возможных сбоев. Эти сбои никогда не воспроизведутся на x86 архитектуре.

1. Лучше вообще не использовать align лишний раз (как и cast'ы к большему размеру) чем заниматься таким с выравниванием по 16 байт (которое ниоткуда логически не следует, и это уже магия (в плохом смысле), которую нужно помнить, а значит будущий код не защищен от таких "ошибок" (так как они не рациональны) — с моей точки зрения, это просто случайные "фиксы"). Не нужно сообщать компилятору ложную информацию (про align(16)) и он не сгенерирует unaligned код.

Позвольте объяснить для чего используется ALIGN. Для переносимого кода. Если бы писалось для компьютеров с SSE то можно было бы просто использовать SSE тип данных. Но используются типы данных универсальные для всех архитектур, однако, чтобы с ними можно было работать как с SSE их нужно ДОПОЛНИТЕЛЬНО выровнить, чтобы был безопасный cast. Для архитектур без SSE это НЕНУЖНОЕ выравнивание, но оно безвредное.

Абсолютно согласен что касты к большему типу не нужны. Я расскажу как делал этот MR
Добавил флаг компилятору показывающий все неправильные касты из меньшего типа к большему, и исправил предупреждения.

Компилятор всегда генерирует aligned код. Потому что так работает процессор.

2. Лучше фиксить реальные баги — где есть реальный segmentation fault  — (пример с `charbuf_to_uint` показывает, что тут это не так) — иначе, это просто нагромождение исправлений того что не сломано. Так же я рекомендую написать тесты, которые падают до фикса и работают после, доказывающие как наличие бага, так и эффективность фикса.

charbuf_to_uint это исправление недопустимого каста.
про тесты согласен, только проблема в том что для их запуска нужен arm.

3. Такое нагромождение кода невозможно разевьювить.

В каждом коммите менятся только одна функция, и не более 20 изменений строк. Подскажите какие требования к изменениям?

beldmit · 2021-12-17T15:21:27Z

Посмотрю на той неделе, сегодня и на выходных недееспособен, извините.

vt-alt · 2021-12-17T17:00:02Z

@shanechko, спасибо аз ответ. Извиняюсь, что не внимательно посмотрел ваш PR.

У меня есть доступ к разным архитектурам, так что если вы напишете тесты, которые воспроизводят эти проблемы мы, вероятно, сможем их запустить. Даже если это будет не часто — это возможность тестирования на будущее.

Ранее, мы специально делали тесты такими (есть даже отключение MIPS_FIXADE для mipsel), и гоняли их да других архитектурах, чтоб всплывали проблемы с alignment.

ps. В теории, есть возможность вернуть CI на ppc64le, s390x и arm64 на Travis-CI (там это всё ещё доступно для Open Source repositories).

vt-alt · 2021-12-17T17:54:39Z

CMakeLists.txt

 elseif(CMAKE_C_COMPILER_ID MATCHES "GNU")
  set(CMAKE_C_FLAGS_RELEASE -O2)
  set(CMAKE_C_FLAGS_DEBUG "-O0 -ggdb")
  set(CMAKE_C_FLAGS_RELWITHDEBINFO "-O2 -ggdb")
-  add_compile_options(-Werror -Wall -Wno-unused-parameter -Wno-unused-function -Wno-missing-braces -Wno-error=unknown-pragmas -Wno-error=pragmas -Wno-deprecated-declarations)
+  add_compile_options(-Werror -Wall -Wcast-align -Wno-unused-parameter -Wno-unused-function -Wno-missing-braces -Wno-error=unknown-pragmas -Wno-error=pragmas -Wno-deprecated-declarations)


Если мы хотим это фиксить для всех архитектур, то, видимо, надо -Wcast-align=strict. Чтоб и на x86 вылазили эти варнинги.

vt-alt · 2021-12-17T18:14:04Z

7499536 2021-12-16 Fix align-cast for test_derive.c (Mikhail Labiuk)

[Теперь я понял что] тут всё ОК. -Wcast-align=strict вывел этот варнинг, который фиксится. (Как я понял, в этом PR фиксятся в основном/только варнинги, а не реальные segmentation faults на которые наткнулись. Это ОК. Но можно было это написать в commit message, Так как я этого сразу не понял.) В этом коммите поменяется вывод key "id" на разных архитектурах (в зависимости от endianness), но тут это и не важно, так как главное там показать в дебаге, что ключи одинаковые. Спасибо.

vt-alt · 2021-12-17T18:40:49Z

Как я понял, в этом PR фиксятся в основном/только варнинги, а не реальные segmentation faults на которые наткнулись.

В таком случае тесты (для этих багов) не нужны! Я изначально не понял, что это фиксы варнингов и говорит про тесты и вообще не правильно понимал что тут фиксится.

vt-alt · 2021-12-19T19:14:23Z

На ARM архитектуре работа с int требует выровненных данных.

Ранее код был протестирован на sparc64 м mipsel (c MIPS_FIXADE 0) где доступ к int реально требует выровненных данных. А так же на armv7hf. Те segmentation faults, которые возникали были исправлены.

Далее, если вопрос сводится к фиксу варнинга -Wcast-align. Этот варнинг не входит ни в -Wall ни даже в -Wextra. Если включить -Wcast-align то при сборке openssl этот варнинг возникает 175 раз. Почему не внести сначала в openssl исправление реальных и возможных сбоев?

ps. И, на сколько я понимаю, на ARM ядро линукс умет исправлять unaligned access (как у mipsel).

shanechko added 9 commits December 6, 2021 00:01

Fix align-cast for gost_grasshopper_cipher_key.

8c5ee08

Fix align-cast for gost_grasshopper_master_key.

55bae42

Fix align-cast for gost_grasshopper_cipher_do_ecb.

b86224f

Fix align-cast for gost_grasshopper_cipher_do_cbc

ac04583

Fix align-cast for acpkm_next.

28369bb

Fix align-cast for gost_grasshopper_cipher_do_ctr.

3fb4376

Fix align-cast for gost_grasshopper_cipher_do_ofb.

f1a0f82

Fix align-cast for gost_grasshopper_cipher_do_cfb.

03e3cdf

shanechko force-pushed the align_fix branch from 616c6ee to ff220fe Compare December 16, 2021 10:58

shanechko added 3 commits December 16, 2021 15:15

Ensure data alignment for uint512_u.

49d62ca

Fix align-cast for test_derive.c

7499536

Add compilation option to emit warning about invalid cast-align.

600af30

shanechko force-pushed the align_fix branch from ff220fe to 600af30 Compare December 16, 2021 12:16

vt-alt reviewed Dec 16, 2021

View reviewed changes

shanechko requested review from beldmit and vt-alt December 17, 2021 14:11

vt-alt reviewed Dec 17, 2021

View reviewed changes

vt-alt force-pushed the master branch from 456e71b to ede3886 Compare March 22, 2024 02:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid unaligned data access for grasshopper cipher. #376

Avoid unaligned data access for grasshopper cipher. #376

shanechko commented Dec 15, 2021

vt-alt Dec 16, 2021

shanechko Dec 17, 2021

vt-alt Dec 16, 2021

beldmit Dec 16, 2021

vt-alt Dec 16, 2021

shanechko Dec 17, 2021

shanechko Dec 17, 2021

vt-alt commented Dec 16, 2021

vt-alt commented Dec 16, 2021

vt-alt commented Dec 16, 2021

shanechko commented Dec 17, 2021

shanechko commented Dec 17, 2021

shanechko commented Dec 17, 2021

beldmit commented Dec 17, 2021

vt-alt commented Dec 17, 2021 •

edited

Loading

vt-alt Dec 17, 2021 •

edited

Loading

vt-alt commented Dec 17, 2021

vt-alt commented Dec 17, 2021 •

edited

Loading

vt-alt commented Dec 19, 2021 •

edited

Loading

Avoid unaligned data access for grasshopper cipher. #376

Are you sure you want to change the base?

Avoid unaligned data access for grasshopper cipher. #376

Conversation

shanechko commented Dec 15, 2021

vt-alt Dec 16, 2021

Choose a reason for hiding this comment

shanechko Dec 17, 2021

Choose a reason for hiding this comment

vt-alt Dec 16, 2021

Choose a reason for hiding this comment

beldmit Dec 16, 2021

Choose a reason for hiding this comment

vt-alt Dec 16, 2021

Choose a reason for hiding this comment

shanechko Dec 17, 2021

Choose a reason for hiding this comment

shanechko Dec 17, 2021

Choose a reason for hiding this comment

vt-alt commented Dec 16, 2021

vt-alt commented Dec 16, 2021

vt-alt commented Dec 16, 2021

shanechko commented Dec 17, 2021

shanechko commented Dec 17, 2021

shanechko commented Dec 17, 2021

beldmit commented Dec 17, 2021

vt-alt commented Dec 17, 2021 • edited Loading

vt-alt Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

vt-alt commented Dec 17, 2021

vt-alt commented Dec 17, 2021 • edited Loading

vt-alt commented Dec 19, 2021 • edited Loading

vt-alt commented Dec 17, 2021 •

edited

Loading

vt-alt Dec 17, 2021 •

edited

Loading

vt-alt commented Dec 17, 2021 •

edited

Loading

vt-alt commented Dec 19, 2021 •

edited

Loading