| Message ID | 20230530203116.2008-16-demi@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Diskseq support in loop, device-mapper, and blkback | expand |
On Tue, May 30, 2023 at 04:31:15PM -0400, Demi Marie Obenour wrote: > This adds a couple of BUILD_BUG_ON()s and moves some arithmetic after > the validation code that checks the arithmetic’s preconditions. The > previous code was correct but could potentially trip sanitizers that > check for unsigned integer wraparound. > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> > --- > drivers/block/xen-blkback/blkback.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c > index c362f4ad80ab07bfb58caff0ed7da37dc1484fc5..ac760a08d559085ab875784f1c58cdf2ead95a43 100644 > --- a/drivers/block/xen-blkback/blkback.c > +++ b/drivers/block/xen-blkback/blkback.c > @@ -1342,6 +1342,8 @@ static int dispatch_rw_block_io(struct xen_blkif_ring *ring, > nseg = req->operation == BLKIF_OP_INDIRECT ? > req->u.indirect.nr_segments : req->u.rw.nr_segments; > > + BUILD_BUG_ON(offsetof(struct blkif_request, u.rw.id) != 8); > + BUILD_BUG_ON(offsetof(struct blkif_request, u.indirect.id) != 8); Won't it be clearer as: offsetof(struct blkif_request, u.rw.id) != offsetof(struct blkif_request, u.indirect.id) We don't really care about the specific offset value, but both layouts must match. Also, you likely want to check for all requests types, not just rw and indirect (discard and other). > if (unlikely(nseg == 0 && operation_flags != REQ_PREFLUSH) || > unlikely((req->operation != BLKIF_OP_INDIRECT) && > (nseg > BLKIF_MAX_SEGMENTS_PER_REQUEST)) || > @@ -1365,13 +1367,13 @@ static int dispatch_rw_block_io(struct xen_blkif_ring *ring, > preq.sector_number = req->u.rw.sector_number; > for (i = 0; i < nseg; i++) { > pages[i]->gref = req->u.rw.seg[i].gref; > - seg[i].nsec = req->u.rw.seg[i].last_sect - > - req->u.rw.seg[i].first_sect + 1; > - seg[i].offset = (req->u.rw.seg[i].first_sect << 9); > if ((req->u.rw.seg[i].last_sect >= (XEN_PAGE_SIZE >> 9)) || > (req->u.rw.seg[i].last_sect < > req->u.rw.seg[i].first_sect)) > goto fail_response; > + seg[i].nsec = req->u.rw.seg[i].last_sect - > + req->u.rw.seg[i].first_sect + 1; > + seg[i].offset = (req->u.rw.seg[i].first_sect << 9); Parentheses here are unneeded. If we do that move, we might as well move the assignation of pages[i]->gref as well, just to avoid assigning to gref to take the failure path. I do think however wraparound is not an issue here, as we will discard the result. Thanks, Roger.
diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c index c362f4ad80ab07bfb58caff0ed7da37dc1484fc5..ac760a08d559085ab875784f1c58cdf2ead95a43 100644 --- a/drivers/block/xen-blkback/blkback.c +++ b/drivers/block/xen-blkback/blkback.c @@ -1342,6 +1342,8 @@ static int dispatch_rw_block_io(struct xen_blkif_ring *ring, nseg = req->operation == BLKIF_OP_INDIRECT ? req->u.indirect.nr_segments : req->u.rw.nr_segments; + BUILD_BUG_ON(offsetof(struct blkif_request, u.rw.id) != 8); + BUILD_BUG_ON(offsetof(struct blkif_request, u.indirect.id) != 8); if (unlikely(nseg == 0 && operation_flags != REQ_PREFLUSH) || unlikely((req->operation != BLKIF_OP_INDIRECT) && (nseg > BLKIF_MAX_SEGMENTS_PER_REQUEST)) || @@ -1365,13 +1367,13 @@ static int dispatch_rw_block_io(struct xen_blkif_ring *ring, preq.sector_number = req->u.rw.sector_number; for (i = 0; i < nseg; i++) { pages[i]->gref = req->u.rw.seg[i].gref; - seg[i].nsec = req->u.rw.seg[i].last_sect - - req->u.rw.seg[i].first_sect + 1; - seg[i].offset = (req->u.rw.seg[i].first_sect << 9); if ((req->u.rw.seg[i].last_sect >= (XEN_PAGE_SIZE >> 9)) || (req->u.rw.seg[i].last_sect < req->u.rw.seg[i].first_sect)) goto fail_response; + seg[i].nsec = req->u.rw.seg[i].last_sect - + req->u.rw.seg[i].first_sect + 1; + seg[i].offset = (req->u.rw.seg[i].first_sect << 9); preq.nr_sects += seg[i].nsec; } } else {
This adds a couple of BUILD_BUG_ON()s and moves some arithmetic after the validation code that checks the arithmetic’s preconditions. The previous code was correct but could potentially trip sanitizers that check for unsigned integer wraparound. Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> --- drivers/block/xen-blkback/blkback.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)