Journal D'un kernel panic à un patch…

Posté par . Licence CC by-sa
128
12
nov.
2017

Sommaire

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 . É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.

  • # Mail thread

    Posté par (page perso) . É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 . Évalué à 6.

    J'espère que le 4.14 sera corrigé avant sa sortie, je n'aime pas avoir une version plus maintenue du noyau…

    Le noyau 4.14 est sorti ( https://lwn.net/Articles/738810/rss ). Peut-être pour la prochaine version stable…

    • [^] # Re: Mauvais timing...

      Posté par . Évalué à 2.

      Même pas dans des versions mineures ?

      • [^] # Re: Mauvais timing...

        Posté par (page perso) . É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 . Évalué à 5. Dernière modification le 15/11/17 à 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 :

          Once a stable release is made, its ongoing maintenance is passed off to the
          "stable team," currently consisting of Greg Kroah-Hartman. The stable team
          will release occasional updates to the stable release using the 2.6.x.y
          numbering scheme. To be considered for an update release, a patch must (1)
          fix a significant bug, and (2) already be merged into the mainline for the
          next development kernel.

          Soit littéralement :

          Une fois la version stable publiée, son entretien est transmis à l'équipe « stable », actuellement formée d'un [seul] membre : Greg Kroah-Hartman. L'équipe stable publiera des mises à jour occasionnelles de la branche stable en utilisant la numérotation 2.6.x.y [aujourd'hui 4.x.y]. Pour envisager son intégration dans une mise à jour, un patch doit (1) corriger un bug non négligeable, et (2) avoir déjà été intégré dans la branche principale du développement du prochain noyau.

          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 . É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 . Évalué à 10. Dernière modification le 12/11/17 à 22:42.

    On fait donc un patch de la ligne err = -EFAULT; pour la changer en return -EFAULT;

    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 . É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 . É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 ?) ?

  • # git ?

    Posté par . É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 ? http://www.april.org/adherer

    • [^] # Re: git ?

      Posté par . Évalué à 5. Dernière modification le 16/11/17 à 16:52.

      Apparemment pas :

      git blame ecc.c

      6755fd269d5c1 (Tudor-Dan Ambarus   2017-05-30 17:52:48 +0300  963)       * This condition is met by the default RNG because it selects a favored
      6755fd269d5c1 (Tudor-Dan Ambarus   2017-05-30 17:52:48 +0300  964)       * DRBG with a security strength of 256.
      6755fd269d5c1 (Tudor-Dan Ambarus   2017-05-30 17:52:48 +0300  965)       */
      6755fd269d5c1 (Tudor-Dan Ambarus   2017-05-30 17:52:48 +0300  966)      if (crypto_get_default_rng())
      6755fd269d5c1 (Tudor-Dan Ambarus   2017-05-30 17:52:48 +0300  967)              err = -EFAULT;
      6755fd269d5c1 (Tudor-Dan Ambarus   2017-05-30 17:52:48 +0300  968) 
      6755fd269d5c1 (Tudor-Dan Ambarus   2017-05-30 17:52:48 +0300  969)      err = crypto_rng_get_bytes(crypto_default_rng, (u8 *)priv, nbytes);
      6755fd269d5c1 (Tudor-Dan Ambarus   2017-05-30 17:52:48 +0300  970)      crypto_put_default_rng();
      6755fd269d5c1 (Tudor-Dan Ambarus   2017-05-30 17:52:48 +0300  971)      if (err)
      6755fd269d5c1 (Tudor-Dan Ambarus   2017-05-30 17:52:48 +0300  972)              return err;
      

      … et :

      git log --format='%h %an %ad %s' ecc.c

      6755fd269d5c Tudor-Dan Ambarus Tue May 30 17:52:48 2017 +0300 crypto: ecdh - add privkey generation support
      7380c56d2fca Tudor-Dan Ambarus Tue May 30 15:37:56 2017 +0300 crypto: ecc - rename ecdh_make_pub_key()
      ad269597107e Tudor-Dan Ambarus Thu May 25 10:18:05 2017 +0300 crypto: ecc - remove unnecessary casts
      099054d735a5 Tudor-Dan Ambarus Thu May 25 10:18:04 2017 +0300 crypto: ecc - remove unused function arguments
      8f44df154de7 Stephen Rothwell Fri Jun 24 16:20:22 2016 +1000 crypto: ecdh - make ecdh_shared_secret unique
      3c4b23901a0c Salvatore Benedetto Wed Jun 22 17:49:15 2016 +0100 crypto: ecdh - Add ECDH software support
      (END)
      

      Donc, 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 . É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 . É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 . Évalué à 6.

    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.

    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 également linux.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 faire git pull pour rapatrier le contenu d'une branche distante. Ça se passe toujours bien avec la branche de Linus, mais c'est une autre affaire avec linux-next qui 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 add pour 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'ajouter linux-next et 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 update pour mettre à jour toutes les branches d'un coup et sans impacter les branches locale.

  • # Il est arrivé !

    Posté par . É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 . É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 . É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 . É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 . É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 . É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 . É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 . É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 . Évalué à 3.

            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 ?

            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é.

            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 ?

            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)

            #include <stdlib.h>
            #include <sys/types.h>
            #include <sys/stat.h>
            #include <unistd.h>
            
            
            int *toto = NULL;
            
            
            int foo (const char *p) {
                    struct stat statbuf;
                    if (stat(p, &statbuf) != 0) {
                            return -1;
                    }
                    toto = malloc(sizeof(int));
                    return 0;
            }
            
            int zaz (int *a) {
                    *a = 42;
                    return 0;
            }
            
            int bar () {
                    int err = 0;
                    if (foo("/tmp/patate") != 0)
                            err = 1;
                    err = zaz(toto);
                    return err;
            }
  • # un petit détail

    Posté par (page perso) . É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 . É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.

  • # Not in 4.15-RC5 yet

    Posté par . É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 . É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 . É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 . É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 . É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 . É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 :

              Idx Name          Size      VMA               LMA               File off  Algn
                1 .text         0000WXYZ  0000000000000000  0000000000000000  00000070  2**4
              

              And

              00000000000023e0 g     F .text  000000000000016f ecc_gen_privkey
              

              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 . É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 . É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 . É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 à ceux qui les ont postés. Nous n'en sommes pas responsables.