From mboxrd@z Thu Jan 1 00:00:00 1970 From: Corey Minyard Subject: Re: [PATCH v3 23/25] hw/ipmi: Assert outlen > outpos Date: Wed, 20 Feb 2019 07:36:08 -0600 Message-ID: <20190220133608.GB5504@minyard.net> References: <20190220010232.18731-1-philmd@redhat.com> <20190220010232.18731-24-philmd@redhat.com> Reply-To: minyard@acm.org Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from us1-rack-dfw2.inumbo.com ([104.130.134.6]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1gwS2o-00023z-Ui for xen-devel@lists.xenproject.org; Wed, 20 Feb 2019 13:36:14 +0000 Received: by mail-ot1-x341.google.com with SMTP id 98so40229665oty.1 for ; Wed, 20 Feb 2019 05:36:13 -0800 (PST) Content-Disposition: inline In-Reply-To: <20190220010232.18731-24-philmd@redhat.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" To: Philippe =?utf-8?Q?Mathieu-Daud=C3=A9?= Cc: Li Zhijian , "Michael S. Tsirkin" , Jason Wang , qemu-devel@nongnu.org, Gerd Hoffmann , Stefano Stabellini , Samuel Thibault , Halil Pasic , Christian Borntraeger , Anthony Perard , xen-devel@lists.xenproject.org, Amit Shah , =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , qemu-s390x@nongnu.org, Paul Durrant , Pavel Dovgalyuk , Zhang Chen , David Gibson , Prasad J Pandit , Cornelia Huck , qemu-ppc@nongnu.org, Paolo Bonzini , Stefan Berger List-Id: xen-devel@lists.xenproject.org T24gV2VkLCBGZWIgMjAsIDIwMTkgYXQgMDI6MDI6MzBBTSArMDEwMCwgUGhpbGlwcGUgTWF0aGll dS1EYXVkw6kgd3JvdGU6Cj4gQSB0aHJvdWdmdWxsIGF1ZGl0IHNob3cgdGhhdCBhbGwgdGltZSBk YXRhIGlzIGFkZGVkIHRvIG91dGJ1ZltdLAo+ICdvdXRsZW4nIGlzIGluY3JlbWVudGVkLiAgVGhl biBhdCBjcmVhdGlvbiBhbmQgZWFjaCB0aW1lCj4gY29udGludWVfc2VuZCgpIHJldHVybnMgaXQg cGFzcyB0aHJ1IGNoZWNrX3Jlc2V0IHdoaWNoIHJlc2V0cwo+ICdvdXRwb3MnLCB0aHVzIHdlIGFs d2F5cyBoYXZlICdvdXRsZW4gPj0gb3V0cG9zJy4KClBlcmhhcHM6ICJBIHRob3JvdWdoIGF1ZGl0 IHNob3dzIHRoYXQgb3V0bGVuIGlzIGFsd2F5cyBpbmNyZW1lbnRlZAp3aGVuIGRhdGEgaXMgYWx3 YXlzIGFkZGVkIHRvIG91dGJ1ZltdLiAgVGhlbiBhdCBjcmVhdGlvbiBhbmQgZWFjaAp0aW1lIGNv bnRpbnVzX3NlbmQoKSByZXR1cm5zIGl0IGFzc3VyZXMgaWYgb3V0cG9zIHJlYWNoZXMgb3V0bGVu LApib3RoIHZhbHVlcyBhcmUgcmVzZXQgdG8gemVybywgZXhjZXB0IGluIHRoZSBjYXNlIG9mIHNl bmRpbmcKYSByZXNldCB3aGVyZSBhIG5ldyBjb21tYW5kIGlzIGFkZGVkLiIKClRoaXMgaXMgY2Vy dGFpbmx5IHRoZSBkZXNpZ24gaW50ZW50LCB0aGFuayB5b3UgZm9yIHRoZSB0aG9yb3VnaAphdWRp dC4KCj4gQWxzbyBkdWUgdG8gdGhlIGNoZWNrIG9uIGVudHJ5LCB3ZSBrbm93IG91dGxlbiAhPSAw Lgo+IFdlIGNhbiB0aGVuIGFkZCBhbiBhc3NlcnRpb24gb24gJ291dGxlbiA+IG91dHBvcycsIHdo aWNoIHdpbGwKPiBoZWxwcyB0aGUgbmV4dCBwYXRjaCB0byBzYWZlbHkgY29udmVydCAnb3V0bGVu IC0gb3V0cG9zJyBhcyBhbgoKSSB3YXMgYSBsaXR0bGUgY29uZnVzZWQgYnkgIm5leHQgcGF0Y2gi LCB0aGVyZSBpcyBubyBmb2xsb3dpbmcKcGF0Y2ggaW4gdGhlIHNlcmllcyBmb3IgdGhpcy4gIE1h eWJlICJuZXh0IHBhcnQgb2YgdGhlIHBhdGNoIj8KCj4gdW5zaWduZWQgdHlwZSAoc2l6ZV90KS4K PiAKPiBNYWtlIHRoaXMgYXNzZXJ0aW9uIGV4cGxpY2l0IGJ5IGNhc3RpbmcgJ291dGxlbiAtIG91 dHBvcycgc2l6ZV90Lgo+IAo+IFNpZ25lZC1vZmYtYnk6IFBoaWxpcHBlIE1hdGhpZXUtRGF1ZMOp IDxwaGlsbWRAcmVkaGF0LmNvbT4KCk91dHNpZGUgb2YgdGhlIG1pbm9yIGdyYW1tZXIgaXNzdWVz LCB0aGlzIGxvb2tzIGdvb2QuICBJIGhhdmUKbm90aWNlZCB0aGUgaW5jb25zaXN0ZW50IHNpZ25l ZC91bnNpZ25lZCB1c2FnZSBpbiBxZW11IGFuZCBJTUhPCml0J3MgbGlrZWx5IHRvIGxlYWQgdG8g dmVyeSBiYWQgYnVncyBhdCBzb21lIHBvaW50LiAgVGhlcmUgaGF2ZQpiZWVuIHN0dWRpZXMgdGhh dCBzaG93IHRoYXQgdW5zaWduZWQgdmFsdWVzIHRlbmQgdG8gYmUgbW9yZQpidWdneSBpbiB1c2Fn ZSBkdWUgdG8gdW5kZXJmbG93cywgYnV0IGZvciBhIGxlbmd0aCB2YWx1ZSB0aGF0CndpbGwgZXZl bnR1YWxseSBiZSBjb252ZXJ0ZWQgdG8gYW4gdW5zaWduZWQgdmFsdWUsIHdoYXQgaXMKaGVyZSBp cyBiZXR0ZXIsIEkgdGhpbmsuCgpCb3RoIG91dHBvcyBhbmQgb3V0bGVuIGFyZSB1bnNpZ25lZCwg c28gdGhlIHNpemVfdCgpIGNhc3QgaXMKbm90IHJlYWxseSBuZWNlc3NhcnksIGJ1dCBJIGd1ZXNz IGl0IG1ha2VzIGl0IGNsZWFyLgoKUmV2aWV3ZWQtYnk6IENvcmV5IE1pbnlhcmQgPGNtaW55YXJk QG12aXN0YS5jb20+Cgo+IC0tLQo+ICBody9pcG1pL2lwbWlfYm1jX2V4dGVybi5jIHwgMyArKy0K PiAgMSBmaWxlIGNoYW5nZWQsIDIgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQo+IAo+IGRp ZmYgLS1naXQgYS9ody9pcG1pL2lwbWlfYm1jX2V4dGVybi5jIGIvaHcvaXBtaS9pcG1pX2JtY19l eHRlcm4uYwo+IGluZGV4IGJmMGI3ZWUwZjUuLmNhNjFiMDQ5NDIgMTAwNjQ0Cj4gLS0tIGEvaHcv aXBtaS9pcG1pX2JtY19leHRlcm4uYwo+ICsrKyBiL2h3L2lwbWkvaXBtaV9ibWNfZXh0ZXJuLmMK PiBAQCAtMTA3LDggKzEwNyw5IEBAIHN0YXRpYyB2b2lkIGNvbnRpbnVlX3NlbmQoSVBNSUJtY0V4 dGVybiAqaWJlKQo+ICAgICAgICAgIGdvdG8gY2hlY2tfcmVzZXQ7Cj4gICAgICB9Cj4gICBzZW5k Ogo+ICsgICAgYXNzZXJ0KGliZS0+b3V0bGVuID4gaWJlLT5vdXRwb3MpOwo+ICAgICAgcmV0ID0g cWVtdV9jaHJfZmVfd3JpdGUoJmliZS0+Y2hyLCBpYmUtPm91dGJ1ZiArIGliZS0+b3V0cG9zLAo+ IC0gICAgICAgICAgICAgICAgICAgICAgICAgICAgaWJlLT5vdXRsZW4gLSBpYmUtPm91dHBvcyk7 Cj4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAoc2l6ZV90KShpYmUtPm91dGxlbiAtIGli ZS0+b3V0cG9zKSk7Cj4gICAgICBpZiAocmV0ID4gMCkgewo+ICAgICAgICAgIGliZS0+b3V0cG9z ICs9IHJldDsKPiAgICAgIH0KPiAtLSAKPiAyLjIwLjEKPiAKCl9fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fClhlbi1kZXZlbCBtYWlsaW5nIGxpc3QKWGVuLWRl dmVsQGxpc3RzLnhlbnByb2plY3Qub3JnCmh0dHBzOi8vbGlzdHMueGVucHJvamVjdC5vcmcvbWFp bG1hbi9saXN0aW5mby94ZW4tZGV2ZWw= From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:59979) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gwU7N-0003Qs-Fq for qemu-devel@nongnu.org; Wed, 20 Feb 2019 10:49:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gwU7L-00059i-5W for qemu-devel@nongnu.org; Wed, 20 Feb 2019 10:49:05 -0500 Sender: Corey Minyard Date: Wed, 20 Feb 2019 07:36:08 -0600 From: Corey Minyard Message-ID: <20190220133608.GB5504@minyard.net> Reply-To: minyard@acm.org References: <20190220010232.18731-1-philmd@redhat.com> <20190220010232.18731-24-philmd@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20190220010232.18731-24-philmd@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 23/25] hw/ipmi: Assert outlen > outpos List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Philippe =?utf-8?Q?Mathieu-Daud=C3=A9?= Cc: qemu-devel@nongnu.org, Prasad J Pandit , =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , Paolo Bonzini , Jason Wang , Anthony Perard , qemu-ppc@nongnu.org, Stefan Berger , David Gibson , Gerd Hoffmann , Zhang Chen , xen-devel@lists.xenproject.org, Cornelia Huck , Samuel Thibault , Christian Borntraeger , Amit Shah , Li Zhijian , "Michael S. Tsirkin" , Paul Durrant , Halil Pasic , Stefano Stabellini , qemu-s390x@nongnu.org, Pavel Dovgalyuk On Wed, Feb 20, 2019 at 02:02:30AM +0100, Philippe Mathieu-Daudé wrote: > A througfull audit show that all time data is added to outbuf[], > 'outlen' is incremented. Then at creation and each time > continue_send() returns it pass thru check_reset which resets > 'outpos', thus we always have 'outlen >= outpos'. Perhaps: "A thorough audit shows that outlen is always incremented when data is always added to outbuf[]. Then at creation and each time continus_send() returns it assures if outpos reaches outlen, both values are reset to zero, except in the case of sending a reset where a new command is added." This is certainly the design intent, thank you for the thorough audit. > Also due to the check on entry, we know outlen != 0. > We can then add an assertion on 'outlen > outpos', which will > helps the next patch to safely convert 'outlen - outpos' as an I was a little confused by "next patch", there is no following patch in the series for this. Maybe "next part of the patch"? > unsigned type (size_t). > > Make this assertion explicit by casting 'outlen - outpos' size_t. > > Signed-off-by: Philippe Mathieu-Daudé Outside of the minor grammer issues, this looks good. I have noticed the inconsistent signed/unsigned usage in qemu and IMHO it's likely to lead to very bad bugs at some point. There have been studies that show that unsigned values tend to be more buggy in usage due to underflows, but for a length value that will eventually be converted to an unsigned value, what is here is better, I think. Both outpos and outlen are unsigned, so the size_t() cast is not really necessary, but I guess it makes it clear. Reviewed-by: Corey Minyard > --- > hw/ipmi/ipmi_bmc_extern.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c > index bf0b7ee0f5..ca61b04942 100644 > --- a/hw/ipmi/ipmi_bmc_extern.c > +++ b/hw/ipmi/ipmi_bmc_extern.c > @@ -107,8 +107,9 @@ static void continue_send(IPMIBmcExtern *ibe) > goto check_reset; > } > send: > + assert(ibe->outlen > ibe->outpos); > ret = qemu_chr_fe_write(&ibe->chr, ibe->outbuf + ibe->outpos, > - ibe->outlen - ibe->outpos); > + (size_t)(ibe->outlen - ibe->outpos)); > if (ret > 0) { > ibe->outpos += ret; > } > -- > 2.20.1 >