Re: [PATCH 05/12] hw/virtio: Add support for apple virtio-blk

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

Am 20.06.2023 um 16:35 hat Stefan Hajnoczi geschrieben:
> On Wed, Jun 14, 2023 at 10:56:22PM +0000, Alexander Graf wrote:
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 39e7f23fab..76b85bb3cb 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -1120,6 +1120,20 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
> >  
> >          break;
> >      }
> > +    case VIRTIO_BLK_T_APPLE1:
> > +    {
> > +        if (s->conf.x_apple_type) {
> > +            /* Only valid on Apple Virtio */
> > +            char buf[iov_size(in_iov, in_num)];
> 
> I'm concerned that a variable-sized stack buffer could be abused by a
> malicious guest. Even if it's harmless in the Apple use case, someone
> else might copy this approach and use it where it creates a security
> problem. Please either implement iov_memset() or allocate the temporary
> buffer using bdrv_blockalign() (and free it with qemu_vfree()).
> 
> > +            memset(buf, 0, sizeof(buf));
> > +            iov_from_buf(in_iov, in_num, 0, buf, sizeof(buf));
> > +            virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);

Good point, even more so when iov_memset() should do the job with
simpler code.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]