| Message ID | 20190719150313.29198-1-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: Initialize reply on failure | expand |
On 19/07/2019 18:03, Eric Blake wrote: > We've had two separate reports of a caller running into use of > uninitialized data if s->quit is set (one detected by gcc -O3, another > by valgrind), due to checking 'nbd_reply_is_simple(reply) || s->quit' > in the wrong order. Rather than chasing down which callers need to > pre-initialize reply, it's easier to guarantee that reply will always > be set by nbd_co_receive_one_chunk() even on failure. > > Reported-by: Thomas Huth <thuth@redhat.com> > Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > Replaces: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg04477.html > Replaces: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html > > block/nbd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/block/nbd.c b/block/nbd.c > index 8d565cc624ec..f751a8e633e5 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk( > request_ret, qiov, payload, errp); > > if (ret < 0) { > + memset(reply, 0, sizeof *reply); > s->quit = true; > } else { > /* For assert at loop start in nbd_connection_entry */ > Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
On 7/19/19 5:03 PM, Eric Blake wrote: > We've had two separate reports of a caller running into use of > uninitialized data if s->quit is set (one detected by gcc -O3, another > by valgrind), due to checking 'nbd_reply_is_simple(reply) || s->quit' > in the wrong order. Rather than chasing down which callers need to > pre-initialize reply, it's easier to guarantee that reply will always > be set by nbd_co_receive_one_chunk() even on failure. > > Reported-by: Thomas Huth <thuth@redhat.com> > Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > Replaces: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg04477.html > Replaces: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html > > block/nbd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/block/nbd.c b/block/nbd.c > index 8d565cc624ec..f751a8e633e5 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk( > request_ret, qiov, payload, errp); > > if (ret < 0) { > + memset(reply, 0, sizeof *reply); I never had problem with sizeof without parenthesis, but here I find it not easy to review. Anyhow, this definitively looks like 4.1 material. Preferently with sizeof(), but I don't mind, so: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > s->quit = true; > } else { > /* For assert at loop start in nbd_connection_entry */ >
On 7/19/19 10:53 AM, Philippe Mathieu-Daudé wrote: >> if (ret < 0) { >> + memset(reply, 0, sizeof *reply); > > I never had problem with sizeof without parenthesis, but here I find it > not easy to review. Holdover from my work on GNU coding style projects: the rationale is that you can tell 'sizeof(type)' apart from 'sizeof expr' if you always omit the () in the latter case, which further makes it possible to tell at a glance when you are using the expr form (preferred, because the type of the expression can change and the sizeof is still correct) or a type name (harder to audit, since changing a variable's type means you also have to change any associated sizeof in later code using that variable). But I will readily admit qemu is not GNU style: $ git grep 'sizeof ' | wc 394 2500 29756 $ git grep 'sizeof(' | wc 8899 46172 671537 so I've fixed it to use (). > > Anyhow, this definitively looks like 4.1 material. > > Preferently with sizeof(), but I don't mind, so: > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks. Queued on my NBD tree for -rc2.
On 7/19/19 10:03 AM, Eric Blake wrote: > We've had two separate reports of a caller running into use of > uninitialized data if s->quit is set (one detected by gcc -O3, another > by valgrind), due to checking 'nbd_reply_is_simple(reply) || s->quit' > in the wrong order. Rather than chasing down which callers need to > pre-initialize reply, it's easier to guarantee that reply will always > be set by nbd_co_receive_one_chunk() even on failure. > I'm adding: The bug is harmless (the only time uninitialized use is possible is if s->quit is set, so the conditional resolves to the same branch regardless of the contents of reply), but was introduced in commit 65e01d47.
On 7/19/19 10:03 AM, Eric Blake wrote: > We've had two separate reports of a caller running into use of > uninitialized data if s->quit is set (one detected by gcc -O3, another > by valgrind), due to checking 'nbd_reply_is_simple(reply) || s->quit' > in the wrong order. Rather than chasing down which callers need to > pre-initialize reply, it's easier to guarantee that reply will always > be set by nbd_co_receive_one_chunk() even on failure. > > Reported-by: Thomas Huth <thuth@redhat.com> > Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > Blech. Needs a v2. Expanding context: > +++ b/block/nbd.c > @@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk( > request_ret, qiov, payload, errp); > > if (ret < 0) { > + memset(reply, 0, sizeof *reply); > s->quit = true; > } else { > /* For assert at loop start in nbd_connection_entry */ if (reply) { *reply = s->reply; } either callers can pass in reply==NULL (in which case the memset() dereferences NULL, oops), or always pass in non-NULL reply (in which case the null check is dead code).
On 7/19/19 7:15 PM, Eric Blake wrote: > On 7/19/19 10:03 AM, Eric Blake wrote: >> We've had two separate reports of a caller running into use of >> uninitialized data if s->quit is set (one detected by gcc -O3, another >> by valgrind), due to checking 'nbd_reply_is_simple(reply) || s->quit' >> in the wrong order. Rather than chasing down which callers need to >> pre-initialize reply, it's easier to guarantee that reply will always >> be set by nbd_co_receive_one_chunk() even on failure. >> >> Reported-by: Thomas Huth <thuth@redhat.com> >> Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> > > Blech. Needs a v2. Expanding context: > > >> +++ b/block/nbd.c >> @@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk( >> request_ret, qiov, payload, errp); >> >> if (ret < 0) { >> + memset(reply, 0, sizeof *reply); >> s->quit = true; >> } else { >> /* For assert at loop start in nbd_connection_entry */ > if (reply) { > *reply = s->reply; > } > > either callers can pass in reply==NULL (in which case the memset() > dereferences NULL, oops), or always pass in non-NULL reply (in which Oh good catch... > case the null check is dead code). >
diff --git a/block/nbd.c b/block/nbd.c index 8d565cc624ec..f751a8e633e5 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -640,6 +640,7 @@ static coroutine_fn int nbd_co_receive_one_chunk( request_ret, qiov, payload, errp); if (ret < 0) { + memset(reply, 0, sizeof *reply); s->quit = true; } else { /* For assert at loop start in nbd_connection_entry */
We've had two separate reports of a caller running into use of uninitialized data if s->quit is set (one detected by gcc -O3, another by valgrind), due to checking 'nbd_reply_is_simple(reply) || s->quit' in the wrong order. Rather than chasing down which callers need to pre-initialize reply, it's easier to guarantee that reply will always be set by nbd_co_receive_one_chunk() even on failure. Reported-by: Thomas Huth <thuth@redhat.com> Reported-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- Replaces: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg04477.html Replaces: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03712.html block/nbd.c | 1 + 1 file changed, 1 insertion(+)