Sommaire
- L'analyse du kernel panic
- Les pré-requis pour analyser le plantage
- L'analyse du problème
- Correction du problème et envoi du patch…
Bonjour tout le monde
Ma distribution, Debian sid, m'a proposé de passer sur le noyau 4.13. À l'occasion d'un reboot, je tente un démarrage sur ce nouveau noyau… et suis accueilli par un kernel panic. Expérience déplaisante s'il en est, je décide de repasser sur le 4.12, du travail m'attendant.
Ayant eu plus de temps disponible ce week-end, je décide d'essayer le noyau 4.14-rc7, et découvre que le bug est toujours présent. Il est donc temps pour une enquête et un patch pour que les nouveaux noyaux continuent de démarrer mon système… Et on verra que des rudiments de C peuvent être suffisants pour certains bugs.
L'analyse du kernel panic
J'ai été contraint de prendre une photo du kernel panic (ma carte mère n'a pas le nécessaire pour sauvegarder un kernel panic hélas).
Le kernel panic est visible ici : http://host.pinaraf.info/~pinaraf/kernel-panic-4.13-crypto.jpg (notez le cadrage excellent de la photo… je l'ai prise très rapidement car un autre message d'erreur non lié allait apparaître)
Les éléments intéressants sont les suivants :
- l'erreur en tant que telle
BUG: unable to handle kernel pointer dereference at 00000000000000xx
IP: ecc_gen_privkey+0xa9 [ecdh_generic]
- la call trace,
- les registres.
La call trace et l'IP (Instruction Pointer) montrent que l'erreur a lieu dans le module ecdh_generic, dans la fonction ecc_gen_privkey à l'adresse 0xa9 (relative au début de la fonction).
L'erreur est une déréférence de pointeur NULL.
Nous allons devoir regarder le code pour comprendre ce qu'il se passe dans ce module.
Les pré-requis pour analyser le plantage
J'ai installé les paquets suivants pour travailler à partir du noyau 4.13 : gdb, linux-image-4.13.0-1-amd64-dbg et linux-source-4.13.
Ces paquets sont TRÈS volumineux. Ils installent un ensemble de symboles de debug dans /usr/lib/debug pour le premier, et le second nous met à disposition dans /usr/src une archive du code du noyau avec les patchs correspondant de debian.
Je décompresse le code dans ma partition /data (les symboles de debug me prennent déjà 4GB, je peux pas beaucoup plus sur le SSD), et je dégaine gdb. Le module concerné est ecdh_generic, donc…
L'analyse du problème
Lançons GDB !
$ gdb /usr/lib/debug/lib/modules/4.13.0-1-amd64/kernel/crypto/ecdh_generic.ko
GNU gdb (Debian 7.12-6+b1) 7.12.0.20161007-git
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /usr/lib/debug/lib/modules/4.13.0-1-amd64/kernel/crypto/ecdh_generic.ko...done.
Il ne reste plus qu'à indiquer à gdb où se trouve le code, et lui demander de désassembler la fonction ecc_gen_privkey en indiquant le code source correspondant. On sait que l'erreur a lieu à l'offset 0xa9, soit 169 en décimal. Je ne vous mettrai donc que le code correspondant à cette partie ici.
(gdb) set directories /data/pierre/kernel-panic/linux-source-4.13/
(gdb) disassemble /s ecc_gen_privkey
Dump of assembler code for function ecc_gen_privkey:
./crypto/ecc.c:
…
949 int err;
950
951 /* Check that N is included in Table 1 of FIPS 186-4, section 6.1.1 */
952 if (nbits < 160)
0x000000000000226d <+141>: jbe 0x22c7 <ecc_gen_privkey+231>
0x000000000000226f <+143>: mov %r9,-0x38(%rbp)
954
955 /*
956 * FIPS 186-4 recommends that the private key should be obtained from a
957 * RBG with a security strength equal to or greater than the security
958 * strength associated with N.
959 *
960 * The maximum security strength identified by NIST SP800-57pt1r4 for
961 * ECC is 256 (N >= 512).
962 *
963 * This condition is met by the default RNG because it selects a favored
964 * DRBG with a security strength of 256.
965 */
966 if (crypto_get_default_rng())
0x0000000000002273 <+147>: callq 0x2278 <ecc_gen_privkey+152>
967 err = -EFAULT;
968
969 err = crypto_rng_get_bytes(crypto_default_rng, (u8 *)priv, nbytes);
0x0000000000002278 <+152>: mov 0x0(%rip),%rdi # 0x227f <ecc_gen_privkey+159>
./include/crypto/rng.h:
143 return crypto_rng_alg(tfm)->generate(tfm, src, slen, dst, dlen);
0x000000000000227f <+159>: mov %r15d,%r8d
0x0000000000002282 <+162>: xor %edx,%edx
0x0000000000002284 <+164>: xor %esi,%esi
0x0000000000002286 <+166>: mov %rbx,%rcx
0x0000000000002289 <+169>: mov 0x38(%rdi),%rax
0x000000000000228d <+173>: callq *-0x20(%rax)
0x0000000000002290 <+176>: mov %eax,%r15d
On voit bien à l'offset 169 une utilisation du registre RDI qui était à 0 dans le kernel panic. Le bug est donc assez évident : crypto_rng_alg(tfm) doit renvoyer un pointeur null, donc la méthode generate est à une adresse incorrecte, et la tentative d'appel échoue.
Le code d'ecc.c correspondant est le suivant :
if (crypto_get_default_rng())
err = -EFAULT;
err = crypto_rng_get_bytes(crypto_default_rng, (u8 *)priv, nbytes);On note là une erreur de logique : si l'appel à crypto_get_default_rng échoue, on stocke une valeur dans une variable sans l'utiliser par la suite ? Cela semble incorrect.
Voici le code de crypto_get_default_rng :
int crypto_get_default_rng(void)
{
struct crypto_rng *rng;
int err;
mutex_lock(&crypto_default_rng_lock);
if (!crypto_default_rng) {
rng = crypto_alloc_rng("stdrng", 0, 0);
err = PTR_ERR(rng);
if (IS_ERR(rng))
goto unlock;
err = crypto_rng_reset(rng, NULL, crypto_rng_seedsize(rng));
if (err) {
crypto_free_rng(rng);
goto unlock;
}
crypto_default_rng = rng;
}
crypto_default_rng_refcnt++;
err = 0;
unlock:
mutex_unlock(&crypto_default_rng_lock);
return err;
}Si cet appel échoue, la variable crypto_default_rng reste à NULL. Donc l'appel par la suite à crypto_rng_get_bytes avec un pointeur NULL déclenche la déréférence de pointeur NULL.
Le correctif est donc très simple : si crypto_get_default_rng renvoie une erreur, il faut renvoyer -EFAULT et pas juste ignorer l'erreur…
Correction du problème et envoi du patch…
Le bug a lieu dans la partie crypto du noyau. Regardons qui s'en occupe…
$ ./scripts/get_maintainer.pl crypto/ecc.c
Herbert Xu <herbert@gondor.apana.org.au> (maintainer:CRYPTO API)
"David S. Miller" <davem@davemloft.net> (maintainer:CRYPTO API)
linux-crypto@vger.kernel.org (open list:CRYPTO API)
linux-kernel@vger.kernel.org (open list)
Donc nous avons là la mailing list, les mainteneurs, et nous pouvons déduire de https://git.kernel.org/ qu'il faut cloner le dépôt https://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git/ pour disposer du dernier code de cette partie du noyau.
Après un long git clone (le noyau, c'est beaucoup), on peut voir que le bug est toujours présent dans le dernier noyau.
On fait donc un patch de la ligne err = -EFAULT; pour la changer en return -EFAULT;… Et il n'y a plus qu'à préparer le tout.
$ git commit -sa
# Je vous laisse imaginer un beau message
$ git format-patch origin/master -o ../patchs
On a dans le dossier ../patchs un beau fichier .patch, qu'il n'y a plus qu'à envoyer aux adresses précédemment citées. (Paquet git-email pour les debianneux)
git send-email --to=linux-crypto@vger.kernel.org -cc=davem@davemloft.net -cc=herbert@gondor.apana.org.au ../patchs/0001-Fix-NULL-pointer-deref.-on-no-default_rng.patch
Et voilà !
J'espère que le 4.14 sera corrigé avant sa sortie, je n'aime pas avoir une version plus maintenue du noyau…
# Bravo et merci
Posté par Dareg . Évalué à 10.
Merci pour ton travail, et merci pour ce journal l'expliquant clairement !
:)
Je note tout de même que tu sembles avoir bien plus que des rudiments de C, notamment de la motivation et une connaissance du processus de développement du noyau.
[^] # Re: Bravo et merci
Posté par Pinaraf . Évalué à 10.
J'ai pas dit que je n'avais que des rudiments de C, j'ai dit qu'il faut que des rudiments pour comprendre le code impacté :)
Pour le processus de développement du noyau, j'ai copié depuis moi-même y'a 7 ans… https://linuxfr.org/users/pied/journaux/de-l%C3%A9criture-dun-pilote-linux-pour-un-gadget-suite
# Mail thread
Posté par claudex . Évalué à 10.
Pour ceux qui veulent suivre la correction du patch, le lien vers l'archive de la mailing list: https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg29135.html.
Et merci pour le journal.
« Rappelez-vous toujours que si la Gestapo avait les moyens de vous faire parler, les politiciens ont, eux, les moyens de vous faire taire. » Coluche
# Mauvais timing...
Posté par cosmocat . Évalué à 6.
Le noyau 4.14 est sorti ( https://lwn.net/Articles/738810/rss ). Peut-être pour la prochaine version stable…
[^] # Re: Mauvais timing...
Posté par Cheuteumi . Évalué à 2.
Même pas dans des versions mineures ?
[^] # Re: Mauvais timing...
Posté par Renault (site web personnel) . Évalué à 4.
Cela est en effet possible vu qu'il semble que c'est juste une correction de bogue sans grand impact négatif (au contraire) sur les applications.
Cela dépend des mainteneurs. D'ailleurs un rétroportage par ce biais est possible aussi sur les versions antérieures.
[^] # Re: Mauvais timing...
Posté par Obsidian . Évalué à 5. Dernière modification le 15 novembre 2017 à 15:58.
Absolument, et cela est même très codifié. On peut lire tout cela dans le Kernel Development Process, que l'on trouve également au format texte (et maintenant *.rst) sous Documentation/process dans l'arborescence des sources du noyau.
Le texte commence à avoir de l'âge mais on y lit notamment :
Soit littéralement :
Donc, concrètement, ça veut dire que quoi qu'il se passe, ton patch doit suivre la procédure normale, en passant par la branche de développement linux-next, pour vérifier entre autre qu'il compile bien et qu'il ne provoque pas d'autres régressions (ce qui serait un comble). Et cela signifie aussi qu'il doit d'abord inclus dans le noyau courant avant d'être backporté aux mises à jour des versions précédentes. On n'y pense pas mais ça tombe effectivement sous le sens une fois qu'on le dit : ce serait idiot de corriger un bug dans la version qui vient de sortir pour voir réapparaître le bug dans le noyau suivant…
Tout cela est important parce que la phase d'intégration (de nouveautés ou autres) ne dure que deux semaines à chaque fois, suivies de SEPT semaines environ de phase de stabilisation : les fameuses -rc1 à -rc7 (le plus souvent car pour 4.14, on a eu une -rc8), chaque rc (release candidate) étant publiée par Linus le dimanche soir. Et quand on regarde l'arbre Git ainsi que les notes de publication sur la LKML, que l'on peut retrouver par exemple dans l'actuelle dépêche sur la sortie du noyau 4.14 en cours de rédaction, on s'aperçoit que nombreux sont les gens à soumettre des patches, voire des lignées entières de soumissions « urgent-pour-linus » le vendredi soir de la dernière semaine, ce qui a généralement le don de mettre le chef de mauvaise humeur. :)
Tout cela pour dire que c'est cuit pour le 4.14 proprement dit, mais qu'on a de bonnes chances de le voir apparaître dans le 4.14.1 ou .2 dans les prochaines semaines, avant d'attendre la stabilisation du 4.15, sachant qu'un kernel panic doit être considéré comme un bug non négligeable. :)
[^] # Re: Mauvais timing...
Posté par cosmocat . Évalué à 5.
Oui, tout à fait. Je me suis mal exprimé mais en disant "stable", je voulais dire la prochaine sortie corrective (c-à-d mineur) des noyaux stables.
# test du correctif
Posté par goeb . Évalué à 10. Dernière modification le 12 novembre 2017 à 22:42.
Tu n'en parles pas, mais je suppose que tu as testé ton correctif avant de proposer le patch, n'est-ce pas ?
("tester" signifiant ici : recompiler le noyau, booter dessus, et constater la correction)
[^] # Re: test du correctif
Posté par Pinaraf . Évalué à 4.
Yep, bien sûr. J'ai même été plus bourrin sur les nouveaux noyaux debian : flemme de recompiler, donc je patche le fichier à coup d'éditeur hexadécimal pour injecter un return -EFAULT; à la place de la fonction.
# Et donc, comment se passe ton boot, maintenant ?
Posté par windu.2b . Évalué à 10.
Merci pour cette explication très claire.
Ma question est dans le titre : comment se passe ton boot, maintenant que tu gères mieux l'erreur (mais il y a toujours l'erreur, non ?) ?
[^] # Re: Et donc, comment se passe ton boot, maintenant ?
Posté par Pinaraf . Évalué à 4.
Ha ben ça juste boote, c'est mieux, non ? :)
# git ?
Posté par Denis Dordoigne . Évalué à 2.
Aurait-il été possible d'éviter l'étape GDB en regardant les logs de l'arborescence concernée ? Je n'ai pas vérifié, je suis peu doué avec git
Membre de l'april, et vous ? https://april.org/adherer -- Infini, l'internet libre et non commercial : https://infini.fr
[^] # Re: git ?
Posté par Obsidian . Évalué à 5. Dernière modification le 16 novembre 2017 à 16:52.
Apparemment pas :
git blame ecc.c… et :
git log --format='%h %an %ad %s' ecc.cDonc, le fichier incriminé a été créé en 2016 et n'a fait l'objet que de six patches en tout, donc les quatre derniers au printemps dernier. La ligne fautive se trouve dans le tout dernier patch, qui lui même ne modifie pas l'existant mais déclare de nouvelles fonctions, qui n'ont donc encore jamais été amendées jusqu'ici.
C'est donc un authentique bug qui a passé tous les filtres jusqu'à présent.
# Questions
Posté par dud . Évalué à 10.
Merci pour cet article. 2 petites questions :
- qu'entends-tu exactement par "ma carte mère n'a pas le nécessaire pour sauvegarder un kernel panic hélas" ?
- par curiosité, quel est l'outil ou le composant matériel qui génère une clé au démarrage ?
[^] # Re: Questions
Posté par Pinaraf . Évalué à 3.
Désolé pour le temps de réponse, j'ai été très négligent avec ce journal et j'ai manqué de temps à y consacrer (boulots, maladie, tout ça tout ça)
«ma carte mère n'a pas le nécessaire pour sauvegarder un kernel panic hélas» : les cartes mères de serveurs ont un espace mémoire disponible, exposé en ACPI, qui permet au système de stocker des données en cas de crash.
Cf https://lwn.net/Articles/434821/
Sinon la clé est générée par le noyau pour vérifier sa compatibilité avec certaines normes de sécu (du FIPS…). Je n'ai pas creusé plus.
# Arborescences utiles
Posté par Obsidian . Évalué à 6.
Juste une petite précision, parce cette page est effectivement un point d'entrée mais qu'elle ne contient pas moins de 800 arborescences, chacune contenant au moins une branche, souvent plusieurs.
D'abord la branche de Linus, qui recèle le noyau tel qu'il sera effectivement publié à la fin est «
kernel/git/torvalds/linux.git». C'est utile de le savoir parce qu'elle n'arrive qu'en 693 position sur 800 et que de nombreuses autres arborescences, dans les répertoires de leurs propriétaires respectifs, s'appellent égalementlinux.git.Ensuite, toutes ces branches sont faites pour être mergées, donc in fine être intégrées dans l'arborescence principale, et elles-mêmes s'appuient sur l'existant, donc sont toujours un fork de cette même lignée au départ. Cela veut dire que si on suit une arborescence spécifique et que l'on remonte une de ses branches, on va toujours retomber tôt ou tard sur le noyau officiel. Donc, si on clone dans un dépôt local vide la branche citée ci-dessus, on télécharge en fait le noyau entier jusqu'au moment où la branche a forké pour la dernière fois, c'est-à-dire généralement à la dernière version du noyau, en gros. Et si on est sûr d'avoir la branche au complet, il nous manque justement les dernières modifications apportées au noyau officiel, alors qu'on a téléchargé le reste de l'historique jusqu'à 2005 pratiquement pour rien.
C'est important aussi parce que si on clone indépendamment plusieurs dépôts de la même façon, on va se retrouver à chaque fois avec un répertoire de 3 Go par arborescence. Et il se trouve que tous ces dépôts vont en fait contenir à 90% la même chose.
Ça va être le cas en particulier avec
linux-next, qui est la branche de travail à suivre dès lors qu'on veut faire des contributions au noyau sur la durée. Ça l'est encore plus quand on débute avec Git et qu'on a l'habitude de fairegit pullpour rapatrier le contenu d'une branche distante. Ça se passe toujours bien avec la branche de Linus, mais c'est une autre affaire aveclinux-nextqui est reconstruite à intervalles réguliers. Quand on a recloné le dépôt deux ou trois fois en re-téléchargeant à chaque fois 3 Go de données, on apprend vite à faire autrement. :)Le mieux à faire est donc de cloner à la base la branche de Linus, et d'utiliser
git remote addpour ajouter des références vers des dépôts distants à notre dépôt local. La base d'objet étant commune, seul ce qui n'a pas déjà été rapatrié le sera lors d'une mise à jour des branches. La bonne idée étant donc d'ajouterlinux-nextet les arborescences des sous-systèmes qui nous intéressent. Cela évite également de créer automatiquement une branche locale associée si ce n'est pas nécessaire (cas de linux-next).Ensuite, il suffit de faire
git remote updatepour mettre à jour toutes les branches d'un coup et sans impacter les branches locale.# Il est arrivé !
Posté par Obsidian . Évalué à 6.
Ça y est ! Le patch est arrivé mercredi dernier dev crypto-dev/master et linux-next/master sous le numéro 4c0e22c90510308433272d7ba281b1eb4eda8209
Si l'importance du bug est bien relayée aux mainteneurs successifs, on a des chances de le voir arriver dans les prochaines rc. Peut-être pas la rc2 à venir demain soir mais encore que… les gens sont invités pendant cette période à tester intensivement le noyau et c'est le genre de correctif qu'on aime voir arriver assez vite. Surtout qu'étant très concis, il peut se permettre d'être examiné rapidement.
Donc, j'imagine qu'il va d'abord être publié avec le noyau 4.15, puis rétroporté. Les règles disent que le patch doit se trouver dans la branche de Linus pour pouvoir être ajouté à -stable, mais ne précisent pas si un noyau officiel doit être sorti ou si les -rc suffisent (sachant qu'elles servent à cela).
Autre ambiguïté : il y a aussi les règles de soumissions des patches à la branche -stable, mais impossible de savoir si cela doit être fait à l'initiative du développeur, si ces règles ne s'adressent qu'aux mainteneurs, ou si c'est ouvert aux deux. Elles précisent aussi que le patch concerné — ou un patch équivalent – doit se trouver dans la branche principale. C'est évidemment utile quand on veut rajouter un correctif au noyaux LTS et il se trouve que le dernier en date (4.14) en est un ! Mais évidemment, on peut faire des rétroportages sur de très anciens noyaux (branche 3.x par exemple) et dans ce cas, il se peut que les patches à appliquer pour corriger la même chose soient très différents.
Tout cela pour dire que c'est bon pour le noyau 4.15 à sortir le mois prochain, mais impossible de dire s'il va être automatiquement rétroporté.
[^] # Re: Il est arrivé !
Posté par Pinaraf . Évalué à 3.
Yep, j'ai hâte, pour le moment je démarre ma machine en patchant à l'éditeur hexadécimal le .ko des derniers noyaux debian (c'est plus rapide d'écrire au bon endroit b8f2ffffffc3 que de recompiler le noyau, quand même).
[^] # Re: Il est arrivé !
Posté par Obsidian . Évalué à 3.
Hmm. J'avais répondu à ce commentaire et il semblerait qu'il ait disparu. Bizarre…
Je crois bien que j'ai parlé trop vite : ton patch, comme ton journal, est daté du 12 novembre 2017. Je ne sais pas si cela correspond à ton propre commit en local où si c'est la date à laquelle tu l'as soumis, mais toujours est-il que c'est aussi exactement le jour de la sortie du noyau 4.14.
Donc, normalement, c'était aussi le début de la fenêtre d'intégration des patches qui dure typiquement 14 jours (deux semaines, du lundi au… lundi). Bon, il est entendu qu'en principe, un patch doit d'abord passer par linux-next et y être intensivement testé mais étant donné la chose, on aurait pu faire ça en accéléré et faire tenir toute la procédure dans les deux semaines critiques.
Normalement, la fenêtre a dû se refermer le 25 au soir et la commit date de ton patch est marquée du… 29 :-\ Je ne sais pas si c'est délibéré (dans le sens où le mainteneur doit se charger de faire intégrer les patches déjà en attente, puis s'occuper de préparer les nouveaux une fois la fenêtre refermée) ou si c'est parce que ça a traîné mais quoi qu'il en soit, le noyau 4.15 doit sortir soit lundi prochain, soit dans dix jours. Donc, ce sera pour l'intégration du 4.16 à venir incessamment, mais il faudra bien te faire rappeler à leur bon souvenir pour être sûr que ce soit fait…
Reste à savoir si ce correctif critique va pouvoir être immédiatement rétroporté comme indiqué dans les directives présentées ailleurs, ou s'il va falloir attendre deux mois que le 4.16 sorte officiellement avant d'envisager la chose… et d'attendre ensuite la publication d'une nouvelle branche stable.
# Petite question technique : un logiciel de vérification n'aurait il pas alerté de cette erreur ?
Posté par trancheX . Évalué à 1.
Tout d'abord merci le patch et surtout les explications. C'est bien d'avoir pris le temps de faire un fix + pris le temps d'expliquer le cheminement, le bémol c'est que maintenant si jamais ça m'arrive j'aurai plus trop d'excuses pour ne pas chercher de même.
Je me demande juste si un outils d'analyse/validation de code genre coverity* ou autre n'aurait pas pu lever une alerte sur ce genre d'erreur ?
Et il me semblait que coverity était utilisé sur le kernel, c'est donc que ce genre d'outil n'est pas capable d'avertir sur ce problème (ce qui me parait pourtant faisable) ?
*: (le genre de chose que j'ai jamais utilisé, donc je parle d'un truc que je maitrise pas : pas taper)
[^] # Re: Petite question technique : un logiciel de vérification n'aurait il pas alerté de cette erreur ?
Posté par Pinaraf . Évalué à 3.
Oui, ce type d'outil devrait lever une alerte. Mais le compilateur aussi devrait dire qu'il se passe quelque chose d'étrange.
Mais pour la défense des développeurs, les outils comme coverity, ou certains avertissements du compilateurs, sont difficiles à suivre :
- ils ont tendance à produire des faux négatifs
- les développeurs n'ont pas nécessairement tous les accès aux outils quand ils sont propriétaires (coverity) ou non officiellement supportés (les avertissements de clang diffèrent de ceux de gcc)
- sur le nombre de lignes de code du noyau, il reste beaucoup de rapports à trier
- comment les développeurs pourraient-ils tester ces outils sur leurs dépôts tiers ? A priori coverity ne teste que le noyau 'officiel' de Linus, ils ne peuvent pas savoir avant d'intégrer un commit s'il va augmenter le nombre de rapports. Et même avec un outil d'analyse libre que les devs pourraient lancer eux-même, ces outils sont souvent assez longs, surtout sur une telle base de code…
Pour le coup, je pense que c'est au compilateur de faire un avertissement devant un tel code. Mais il se mettrait sûrement à faire des tas d'avertissements sur par exemple du code généré… Équilibre délicat, je souhaite ne jamais bosser sur un compilateur existant pour ne pas avoir à gérer ce genre de choix entre la peste (ne pas avoir l'avertissement) et le choléra (casser du code existant).
[^] # Re: Petite question technique : un logiciel de vérification n'aurait il pas alerté de cette erreur ?
Posté par Julien HENRY . Évalué à 4.
Pour info, l'analyseur C/C++ de SonarQube rapporte bien un problème sur l'assignation de la variable err. Ce n'est pas identifié comme un bug cependant, "juste" du code douteux.
(Note : je travaille pour l'éditeur de SonarQube)
[^] # Re: Petite question technique : un logiciel de vérification n'aurait il pas alerté de cette erreur ?
Posté par Pinaraf . Évalué à 5.
Idem pour scan-build (l'analyseur statique de LLVM) qui le classe également en warning…
[^] # Re: Petite question technique : un logiciel de vérification n'aurait il pas alerté de cette erreur ?
Posté par trancheX . Évalué à 1.
Super, voilà 2 outils que je ne connaissais pas (scan-build et SonarQube). La prochaine fois que je ferais du c++ je testerai (j'ai plein de projets JS et Go à finir d'abord).
Mais du coup, simple curiosité + flem de le faire moi même : quand on lance un scan-build ou SonarQube sur un kernel ça donne quoi comme nombre d'alerte de ce genre : c'est monstrueux/énorme/pas-si-pire/ça-va/RAS/c'est-pas-si-simple ?
C'est vrai que c'est dommage que ce genre de chose ne soit qu'un warning mais ça doit être complexe de faire mieux comme ce n'est crypto_get_default_rng() ne sert "que" à initialiser une structure et c'est suivant la valeur retourné que l'ont sait si tous c'est bien passé… Je ne sais pas si les analyseurs sont capable de "comprendre" ça auquel cas ils pourraient dire que c'est une erreur et pas un warning.
D’ailleurs question technique pour SonarQube : le warning est levé tout simplement parce que la variable err est écrasé sans être utilisé ou bien il y a des mécanismes en plus ?
[^] # Re: Petite question technique : un logiciel de vérification n'aurait il pas alerté de cette erreur ?
Posté par Pinaraf . Évalué à 3.
Il faudrait que le noyau compile avec Clang/LLVM pour scan-build. C'est un morceau de code quand même bien complexe, il a des dépendances légitimes à des extensions de GCC qui ne sont pas nécessairement implémentées par clang. Le projet LLVMLinux étant semble-t-il en sommeil pour le moment, les patchs ne sont pas à jour de ce côté.
Je ne sais pas pour SonarQube, mais pour LLVM c'est parce que err est écrasé. C'est le seul warning levé pour le code suivant, qui correspond globalement à ce que fait au final le noyau pour le compilateur (j'ai pris stat comme fonction dont le résultat ne peut être déterminé par le compilateur)
[^] # Re: Petite question technique : un logiciel de vérification n'aurait il pas alerté de cette erreur ?
Posté par trancheX . Évalué à 1.
Je croyais que le projet LLVM-Linux avait aboutit … visiblement c'est au point mort. J'ai du mélanger avec freeBSD ou autre.
Effectivement les analyseurs n'ont pas l'air de faire trop d'intelligence sur ce genre de problème : ils avertissent juste que la variable est écrasée sans avoir été utilisée.
Et du coup GCC n'a aucun flag qui permettrait de lever un warning la dessus (variable écrasé bêtement) : j'ai testé ton code avec -Wall -Wextra et -Wpedantic (ouai -Wpedantic parcequ'on sait jamais, et à défaut de -Wbrico3000), RAS pour lui.
J'aurai bien vu un flag pour ça parmis tous les flags de GCC mais visiblement non… dommage pour les étourdis comme moi.
[^] # Re: Petite question technique : un logiciel de vérification n'aurait il pas alerté de cette erreur ?
Posté par Pinaraf . Évalué à 4.
Je viens de faire quelques recherches, le gros du noyau est quasiment compilable avec LLVM/Clang. Il reste quelques patchs, mais par contre les pilotes sont loin d'être tous testés… Donc pour quelques systèmes embarqués ça doit être jouable, pour un noyau générique par contre c'est pas encore ça…
# un petit détail
Posté par Selso (site web personnel) . Évalué à 2.
Bonjour,
C'est le genre d'article que j'aime : efficace et agréable à lire.
Par contre je me pose quelques questions :
- n'aurait-il pas mieux valu débogguer sur la version 4.14-rc7 puisqu'elle était disponible. (mais par la suite je vois que tu vérifies quand même que dans le dépôt de dev le bug est toujours présent).
- lorsque tu as chargé le module avec gdb, tu étais dans quel environnement ? une 4.12 ? Et le noyau ne s'est pas plaint que le module n'était pas compilé pour la même version ?
[^] # Re: un petit détail
Posté par Pinaraf . Évalué à 5.
Bonjour
Je n'ai pas fait de debug interactif (c'est loin d'être facile sur un noyau et mériterait un journal à part entière). J'ai uniquement utilisé gdb, sans être root, comme un joli désassembleur affichant le code C correspondant en même temps. Le code n'a pas été exécuté par le noyau.
Et j'ai préféré travailler sur la 4.13 parce que je savais que la régression y avait été introduite, j'ai préféré éviter un éventuel «empilement» de problèmes au cas où la 4.14 introduisait d'autres bugs.
[^] # Re: un petit détail
Posté par Selso (site web personnel) . Évalué à 1.
Je suis passé à côté de cette subtilité.
Plutôt bonne la démarche !
# Not in 4.15-RC5 yet
Posté par razorb . Évalué à 2.
It seems this patch still hasn't been applied in 4.15-RC5, the most recent release candidate.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/crypto/ecc.c?id=v4.15-rc5#n943
And the Debian compiled version confirms it, still gives a kernel oops. It's really disappointing, as I've been waiting for this patch since 4.13 came out… I wish there was a way to tell the Linux developers this is an important one!
[^] # Re: Not in 4.15-RC5 yet
Posté par Pinaraf . Évalué à 4.
Why in english ? This is a french website, quite unusual to see an english comment here.
I contacted the crypto subsystem maintainers a few days ago to know how to boost this. Do you have any Debian bug report regarding this ?
FR : J'ai contacté les mainteneurs de la partie crypto il y a quelques jours pour savoir comment pousser le patch… As-tu un rapport de bug debian à propos de ce soucis ?
[^] # Re: Not in 4.15-RC5 yet
Posté par razorb . Évalué à 3.
Thank you, and sorry about using English! (I can read some French, but not really write/speak anything complex… I used Google Translate for your article, but thought it might be rude to reply using it…)
I haven't made a Debian bug report yet, but I could – would it be helpful? I can contribute some screenshots from my system with 4.15-RC5: https://imgur.com/a/TwLPi
Thank you again for your work!
[^] # Re: Not in 4.15-RC5 yet
Posté par Pinaraf . Évalué à 5.
Ok, now we have something :)
I did not push strongly my patch because I recently had an hard time trusting my computer (I fear my motherboard may have an issue). Since you have exactly the same bug, yes, a debian bug report would help since the debian team could backport the patch in their kernel, waiting for it to be upstreamed.
Otherwise, well… I'm manually patching my .ko file using an hex editor. That's a weird way to work around the bug, but it's faster than recompiling :)
Just so I can have an idea what may be wrong here, what is your motherboard and what is your CPU ?
[^] # Re: Not in 4.15-RC5 yet
Posté par razorb . Évalué à 1.
Alright! I've submitted a Debian bug report: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=886556. Hopefully it'll help give the patch an extra push, in Debian at least.
My laptop's a Lenovo ThinkPad T430 with an i7-3520M CPU.
I'd love to do the same hex edit you're using, but it's above my knowledge level: I don't know how to work out the address and raw values that need to be set… Comparing disassemblies of 4.13, 4.14 and 4.15-rc5 , there seem to be differences in addresses. But if I understand correctly it's somewhere between 0x2273 - 0x2278 (in 4.13), or 0x2458 - 0x245d (in 4.15-rc5)?
I'm having trouble understanding the gdb disassembly output, but it also looks like the compiler optimized out the "err = -EFAULT" line? If so, how do you insert a return call without shifting other addresses and breaking the .ko?
And to work out what "return -EFAULT" looks like in hex, I suppose you copy bytes from one of the other places it's used?
Thank you for reading this, and don't feel bad if you don't have the time to explain!
[^] # Re: Not in 4.15-RC5 yet
Posté par Pinaraf . Évalué à 5.
Ok, you want the nasty one :)
You have to locate the buggy function with objdump, and replace the whole function with a simple «return -EFAULT;» aka 0xb8f2ffffffc3.
Using objdump -j text on ecdh_generic.ko, there will be two interesting lines :
And
You will have to patch at address 0x70 + 0x23e0. And just overwrite the function first bytes with 0xb8f2ffffffc3. Then you will have a patched .ko file.
It's a really really really nasty way of patching this. But it works…
[^] # Re: Not in 4.15-RC5 yet
Posté par razorb . Évalué à 2.
Ok, after much tribulation I finally did the manual patch. I didn't realise we can excise the whole ecc_gen_privkey()—that is quite drastic, haha!
I tried it on 4.15-rc5, where the address is 0x70+0x23a0. No sign of the oops afterwards! It looks like a clean boot but unfortunately ethernet and bluetooth still didn't come up on my system—it might need some of the things ecc_gen_privkey() was doing, or maybe there's a separate problem for those.
In any case, thank you very much again for teaching me new tricks! As some good news, although it looks like your correction still hasn't been included in RC8, I can see it in the next tree now. So maybe RC9 will have it (or 4.16, at the latest).
[^] # Re: Not in 4.15-RC5 yet
Posté par Pinaraf . Évalué à 3.
Ok, good news for your boot.
AFAIK, crypto should not prevent ethernet nor bluetooth from coming up. Maybe another issue. I suppose the drivers are properly loading, with no warning ?
If you need any help, you can try contacting me on IRC (Pinaraf on freenode and oftc.net), or send me an email…
[^] # Re: Not in 4.15-RC5 yet
Posté par razorb . Évalué à 1.
No error messages that I could find (except one, below). I wouldn't have expected ethernet to be correlated either, but bluetooth definitely is—it's the only thing that requires ecdh_generic on my system (according to lsmod, at least). If bluetooth is turned off (either from BIOS or the wireless hardware switch) the system boots without the kernel oops even with the buggy kernels. And it doesn't seem to mind bluetooth being turned on later, after boot either. It feels like a sort of race condition, and a delay to loading ecdh_generic is sufficient to fix it.
I ended up applying your official (non-nasty) patch to the 4.14 kernel sources and recompiling the module, in the end. And it works great! There's now one cryptic error message during boot, that says
alg: ecdh: test failed on vector 3, err=-14
where -14 is -EFAULT, of course. I imagine this error message is what should have happened all along instead of the kernel oops, so I view it as a good thing!
The mysterious ethernet annoyances still remain, but they're intermittent and work-aroundable, and I agree they are a completely different issue…
Merci beaucoup encore une fois! I won't load up the comment section anymore, I'll try to contact you on email/irc if anything interesting develops!
Suivre le flux des commentaires
Note : les commentaires appartiennent à celles et ceux qui les ont postés. Nous n’en sommes pas responsables.