| Message ID | 20181019094604.14422-2-kleber.souza@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix for CVE-2018-15471 | expand |
On 19.10.18 11:46, Kleber Sacilotto de Souza wrote: > From: Jan Beulich <JBeulich@suse.com> > > Both len and off are frontend specified values, so we need to make > sure there's no overflow when adding the two for the bounds check. We > also want to avoid undefined behavior and hence use off to index into > ->hash.mapping[] only after bounds checking. This at the same time > allows to take care of not applying off twice for the bounds checking > against vif->num_queues. > > It is also insufficient to bounds check copy_op.len, as this is len > truncated to 16 bits. > > This is XSA-270 / CVE-2018-15471. > > Reported-by: Felix Wilhelm <fwilhelm@google.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > Tested-by: Paul Durrant <paul.durrant@citrix.com> > Cc: stable@vger.kernel.org [4.7 onwards] > Signed-off-by: David S. Miller <davem@davemloft.net> > > CVE-2018-15471 > (cherry picked from commit 780e83c259fc33e8959fed8dfdad17e378d72b62) > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > drivers/net/xen-netback/hash.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c > index 3c4c58b9fe76..3b6fb5b3bdb2 100644 > --- a/drivers/net/xen-netback/hash.c > +++ b/drivers/net/xen-netback/hash.c > @@ -332,20 +332,22 @@ u32 xenvif_set_hash_mapping_size(struct xenvif *vif, u32 size) > u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len, > u32 off) > { > - u32 *mapping = &vif->hash.mapping[off]; > + u32 *mapping = vif->hash.mapping; > struct gnttab_copy copy_op = { > .source.u.ref = gref, > .source.domid = vif->domid, > - .dest.u.gmfn = virt_to_gfn(mapping), > .dest.domid = DOMID_SELF, > - .dest.offset = xen_offset_in_page(mapping), > - .len = len * sizeof(u32), > + .len = len * sizeof(*mapping), > .flags = GNTCOPY_source_gref > }; > > - if ((off + len > vif->hash.size) || copy_op.len > XEN_PAGE_SIZE) > + if ((off + len < off) || (off + len > vif->hash.size) || > + len > XEN_PAGE_SIZE / sizeof(*mapping)) > return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER; > > + copy_op.dest.u.gmfn = virt_to_gfn(mapping + off); > + copy_op.dest.offset = xen_offset_in_page(mapping + off); > + > while (len-- != 0) > if (mapping[off++] >= vif->num_queues) > return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER; >
On 19/10/2018 10:46, Kleber Sacilotto de Souza wrote: > From: Jan Beulich <JBeulich@suse.com> > > Both len and off are frontend specified values, so we need to make > sure there's no overflow when adding the two for the bounds check. We > also want to avoid undefined behavior and hence use off to index into > ->hash.mapping[] only after bounds checking. This at the same time > allows to take care of not applying off twice for the bounds checking > against vif->num_queues. > > It is also insufficient to bounds check copy_op.len, as this is len > truncated to 16 bits. > > This is XSA-270 / CVE-2018-15471. > > Reported-by: Felix Wilhelm <fwilhelm@google.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > Tested-by: Paul Durrant <paul.durrant@citrix.com> > Cc: stable@vger.kernel.org [4.7 onwards] > Signed-off-by: David S. Miller <davem@davemloft.net> > > CVE-2018-15471 > (cherry picked from commit 780e83c259fc33e8959fed8dfdad17e378d72b62) > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > drivers/net/xen-netback/hash.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c > index 3c4c58b9fe76..3b6fb5b3bdb2 100644 > --- a/drivers/net/xen-netback/hash.c > +++ b/drivers/net/xen-netback/hash.c > @@ -332,20 +332,22 @@ u32 xenvif_set_hash_mapping_size(struct xenvif *vif, u32 size) > u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len, > u32 off) > { > - u32 *mapping = &vif->hash.mapping[off]; > + u32 *mapping = vif->hash.mapping; > struct gnttab_copy copy_op = { > .source.u.ref = gref, > .source.domid = vif->domid, > - .dest.u.gmfn = virt_to_gfn(mapping), > .dest.domid = DOMID_SELF, > - .dest.offset = xen_offset_in_page(mapping), > - .len = len * sizeof(u32), > + .len = len * sizeof(*mapping), > .flags = GNTCOPY_source_gref > }; > > - if ((off + len > vif->hash.size) || copy_op.len > XEN_PAGE_SIZE) > + if ((off + len < off) || (off + len > vif->hash.size) || > + len > XEN_PAGE_SIZE / sizeof(*mapping)) > return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER; > > + copy_op.dest.u.gmfn = virt_to_gfn(mapping + off); > + copy_op.dest.offset = xen_offset_in_page(mapping + off); > + > while (len-- != 0) > if (mapping[off++] >= vif->num_queues) > return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER; > OK, clean cherry pick, does extra bounds checking which looks sane. Acked-by: Colin Ian King <colin.king@canonical.com>
diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c index 3c4c58b9fe76..3b6fb5b3bdb2 100644 --- a/drivers/net/xen-netback/hash.c +++ b/drivers/net/xen-netback/hash.c @@ -332,20 +332,22 @@ u32 xenvif_set_hash_mapping_size(struct xenvif *vif, u32 size) u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len, u32 off) { - u32 *mapping = &vif->hash.mapping[off]; + u32 *mapping = vif->hash.mapping; struct gnttab_copy copy_op = { .source.u.ref = gref, .source.domid = vif->domid, - .dest.u.gmfn = virt_to_gfn(mapping), .dest.domid = DOMID_SELF, - .dest.offset = xen_offset_in_page(mapping), - .len = len * sizeof(u32), + .len = len * sizeof(*mapping), .flags = GNTCOPY_source_gref }; - if ((off + len > vif->hash.size) || copy_op.len > XEN_PAGE_SIZE) + if ((off + len < off) || (off + len > vif->hash.size) || + len > XEN_PAGE_SIZE / sizeof(*mapping)) return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER; + copy_op.dest.u.gmfn = virt_to_gfn(mapping + off); + copy_op.dest.offset = xen_offset_in_page(mapping + off); + while (len-- != 0) if (mapping[off++] >= vif->num_queues) return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;