linux-kernel - Re: virtio_blk: Less function calls in init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 13 Sep 2016 19:30:27 +0200
From:   SF Markus Elfring <elfring@...rs.sourceforge.net>
To:     Christian Bornträger <borntraeger@...ibm.com>,
        virtualization@...ts.linux-foundation.org,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Minfei Huang <minfei.hmf@...baba-inc.com>,
        Cornelia Huck <cornelia.huck@...ibm.com>,
        Stefan Hajnoczi <stefanha@...hat.com>
Cc:     Julia Lawall <julia.lawall@...6.fr>,
        kernel-janitors@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        Chao Fan <fanc.fnst@...fujitsu.com>
Subject: Re: virtio_blk: Less function calls in init_vq() after error
 detection

> In addition, please have a look at commit 347a529398e8e723338cca5d8a8ae2d9e7e93448
>     virtio_blk: Fix a slient kernel panic

I would like to add another view on the implementation details in this software update.

> which did the opposite of your patch.

This update contained a different approach for error detection and corresponding
exception handling.

> And in fact it fixed a bug.

This is great in principle according to an information in the commit description.

"…
To fix this bug, we should take care of allocation failure,
and return correct value to let caller know what happen.
…"

> Quite obviously multiple labels are harder to read and harder to get right.
> For error handling with just kfree one label is just the right thing to.

Unfortunately, I get an other impression here after a closer look.

Can it be that the discussed commit from 2016-08-09 accepted (or tolerated)
two weaknesses at least?

1. Commit title:
   Is the word "slient" a typo?
   Would you like to read "silent" there instead?

2. Source code:
   Why would another memory allocation be attempted if it could be determined quicker
   that a previous one failed and this function implementation can not succeed then?

   How much will it matter in general that two function calls are performed
   in this use case without checking their return values immediately?
   https://cwe.mitre.org/data/definitions/252.html

	if (!names || !callbacks || !vqs) { …

   https://cwe.mitre.org/data/definitions/754.html

Was the software development attention a bit too low as it happens occasionally?

I hope that my suggestions can improve the affected situation a bit more
also for this software module.

Regards,
Markus