| Message ID | jpga7swygak.fsf_-_@linux.bootlegged.copy |
|---|---|
| State | New |
| Headers | show |
| Series | usb-mtp: Assert on suspicious TYPE_DATA packet from initiator | expand |
On 18 May 2018 at 19:22, Bandan Das <bsd@redhat.com> wrote: > > CID 1390604 > If the initiator sends a packet with TYPE_DATA set without > initiating a CMD_GET_OBJECT_INFO first, then usb_mtp_get_data > can trip on a null s->data_out. > > Signed-off-by: Bandan Das <bsd@redhat.com> I think you said this can be provoked by the guest? Misbehaving or malicious guests should never be able to provoke assertions. > --- > hw/usb/dev-mtp.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c > index 3d59fe4944..905e025d7f 100644 > --- a/hw/usb/dev-mtp.c > +++ b/hw/usb/dev-mtp.c > @@ -1696,6 +1696,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container, > uint64_t dlen; > uint32_t data_len = p->iov.size; > > + assert(d != NULL); > if (d->first) { > /* Total length of incoming data */ > d->length = cpu_to_le32(container->length) - sizeof(mtp_container); > -- > 2.14.3 thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 18 May 2018 at 19:22, Bandan Das <bsd@redhat.com> wrote: >> >> CID 1390604 >> If the initiator sends a packet with TYPE_DATA set without >> initiating a CMD_GET_OBJECT_INFO first, then usb_mtp_get_data >> can trip on a null s->data_out. >> >> Signed-off-by: Bandan Das <bsd@redhat.com> > > I think you said this can be provoked by the guest? Yes, this can only be initated by the guest as far as I understand. > Misbehaving or malicious guests should never be able > to provoke assertions. I am not sure, I thought it's better to kill a misbehaving guest rather than silently letting it run. Anyway, it's possible to send a No_Valid_ObjectInfo as well and we wouldn't have to mark it as a false positive either. Bandan >> --- >> hw/usb/dev-mtp.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c >> index 3d59fe4944..905e025d7f 100644 >> --- a/hw/usb/dev-mtp.c >> +++ b/hw/usb/dev-mtp.c >> @@ -1696,6 +1696,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container, >> uint64_t dlen; >> uint32_t data_len = p->iov.size; >> >> + assert(d != NULL); >> if (d->first) { >> /* Total length of incoming data */ >> d->length = cpu_to_le32(container->length) - sizeof(mtp_container); >> -- >> 2.14.3 > > thanks > -- PMM
On 18 May 2018 at 19:38, Bandan Das <bsd@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On 18 May 2018 at 19:22, Bandan Das <bsd@redhat.com> wrote: >>> >>> CID 1390604 >>> If the initiator sends a packet with TYPE_DATA set without >>> initiating a CMD_GET_OBJECT_INFO first, then usb_mtp_get_data >>> can trip on a null s->data_out. >>> >>> Signed-off-by: Bandan Das <bsd@redhat.com> >> >> I think you said this can be provoked by the guest? > > Yes, this can only be initated by the guest as far as I > understand. > >> Misbehaving or malicious guests should never be able >> to provoke assertions. > > I am not sure, I thought it's better to kill a misbehaving guest rather > than silently letting it run. Anyway, it's possible to send a > No_Valid_ObjectInfo as well and we wouldn't have to mark it as a > false positive either. Broadly speaking, where we're emulating hardware we should do what the hardware does when the guest does wrong things to it. A real server doesn't suddenly vanish leaving behind a message saying "assertion failed" :-) thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 18 May 2018 at 19:38, Bandan Das <bsd@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On 18 May 2018 at 19:22, Bandan Das <bsd@redhat.com> wrote: >>>> >>>> CID 1390604 >>>> If the initiator sends a packet with TYPE_DATA set without >>>> initiating a CMD_GET_OBJECT_INFO first, then usb_mtp_get_data >>>> can trip on a null s->data_out. >>>> >>>> Signed-off-by: Bandan Das <bsd@redhat.com> >>> >>> I think you said this can be provoked by the guest? >> >> Yes, this can only be initated by the guest as far as I >> understand. >> >>> Misbehaving or malicious guests should never be able >>> to provoke assertions. >> >> I am not sure, I thought it's better to kill a misbehaving guest rather >> than silently letting it run. Anyway, it's possible to send a >> No_Valid_ObjectInfo as well and we wouldn't have to mark it as a >> false positive either. > > Broadly speaking, where we're emulating hardware we should do > what the hardware does when the guest does wrong things to it. > A real server doesn't suddenly vanish leaving behind a > message saying "assertion failed" :-) Posted v2 and agree with you! Especially, since the protocol does specify the case where something like this can happen. Thanks for reviewing, Bandan > thanks > -- PMM
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c index 3d59fe4944..905e025d7f 100644 --- a/hw/usb/dev-mtp.c +++ b/hw/usb/dev-mtp.c @@ -1696,6 +1696,7 @@ static void usb_mtp_get_data(MTPState *s, mtp_container *container, uint64_t dlen; uint32_t data_len = p->iov.size; + assert(d != NULL); if (d->first) { /* Total length of incoming data */ d->length = cpu_to_le32(container->length) - sizeof(mtp_container);
CID 1390604 If the initiator sends a packet with TYPE_DATA set without initiating a CMD_GET_OBJECT_INFO first, then usb_mtp_get_data can trip on a null s->data_out. Signed-off-by: Bandan Das <bsd@redhat.com> --- hw/usb/dev-mtp.c | 1 + 1 file changed, 1 insertion(+)