| Message ID | 1465311225-26490-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, On 07/06/16 15:53, Bill Fischofer wrote: > Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2310 by copying the > user area as part of odp_packet_copy(). The copy fails if the user area > size of the destination pool is not large enough to hold the source packet > user area. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/include/odp_packet_internal.h | 2 +- > platform/linux-generic/odp_packet.c | 18 ++++-------------- > 2 files changed, 5 insertions(+), 15 deletions(-) > > diff --git a/platform/linux-generic/include/odp_packet_internal.h b/platform/linux-generic/include/odp_packet_internal.h > index d5ace12..5c74d97 100644 > --- a/platform/linux-generic/include/odp_packet_internal.h > +++ b/platform/linux-generic/include/odp_packet_internal.h > @@ -294,7 +294,7 @@ static inline int packet_parse_not_complete(odp_packet_hdr_t *pkt_hdr) > } > > /* Forward declarations */ > -void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt); > +int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt); > > odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int parse); > > diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-generic/odp_packet.c > index 2868736..f92c257 100644 > --- a/platform/linux-generic/odp_packet.c > +++ b/platform/linux-generic/odp_packet.c > @@ -756,22 +756,11 @@ odp_packet_t odp_packet_copy(odp_packet_t pkt, odp_pool_t pool) > { > odp_packet_hdr_t *srchdr = odp_packet_hdr(pkt); > uint32_t pktlen = srchdr->frame_len; > - uint32_t meta_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, buf_hdr); > odp_packet_t newpkt = odp_packet_alloc(pool, pktlen); > > if (newpkt != ODP_PACKET_INVALID) { > - odp_packet_hdr_t *newhdr = odp_packet_hdr(newpkt); > - uint8_t *newstart, *srcstart; > - > - /* Must copy metadata first, followed by packet data */ > - newstart = (uint8_t *)newhdr + meta_offset; > - srcstart = (uint8_t *)srchdr + meta_offset; > - > - memcpy(newstart, srcstart, > - sizeof(odp_packet_hdr_t) - meta_offset); > - > - if (odp_packet_copy_from_pkt(newpkt, 0, pkt, 0, > - pktlen) != 0) { > + if (_odp_packet_copy_md_to_packet(pkt, newpkt) || > + odp_packet_copy_from_pkt(newpkt, 0, pkt, 0, pktlen)) { > odp_packet_free(newpkt); > newpkt = ODP_PACKET_INVALID; > } > @@ -966,7 +955,7 @@ int odp_packet_is_valid(odp_packet_t pkt) > * > */ > > -void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt) > +int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt) There are other callers of this function, we should either check the return value there or make a comment at least about why it's not possible for an error to happen. > { > odp_packet_hdr_t *srchdr = odp_packet_hdr(srcpkt); > odp_packet_hdr_t *dsthdr = odp_packet_hdr(dstpkt); > @@ -986,6 +975,7 @@ void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt) > odp_atomic_load_u32( > &srchdr->buf_hdr.ref_count)); > copy_packet_parser_metadata(srchdr, dsthdr); > + return dsthdr->buf_hdr.uarea_size < srchdr->buf_hdr.uarea_size; I think we should do this check before we do any copy > } > > /** >
On Wed, Jun 8, 2016 at 8:23 AM, Zoltan Kiss <zoltan.kiss@linaro.org> wrote: > Hi, > > > On 07/06/16 15:53, Bill Fischofer wrote: > >> Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2310 by copying the >> user area as part of odp_packet_copy(). The copy fails if the user area >> size of the destination pool is not large enough to hold the source packet >> user area. >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >> --- >> platform/linux-generic/include/odp_packet_internal.h | 2 +- >> platform/linux-generic/odp_packet.c | 18 >> ++++-------------- >> 2 files changed, 5 insertions(+), 15 deletions(-) >> >> diff --git a/platform/linux-generic/include/odp_packet_internal.h >> b/platform/linux-generic/include/odp_packet_internal.h >> index d5ace12..5c74d97 100644 >> --- a/platform/linux-generic/include/odp_packet_internal.h >> +++ b/platform/linux-generic/include/odp_packet_internal.h >> @@ -294,7 +294,7 @@ static inline int >> packet_parse_not_complete(odp_packet_hdr_t *pkt_hdr) >> } >> >> /* Forward declarations */ >> -void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t >> dstpkt); >> +int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t >> dstpkt); >> >> odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int parse); >> >> diff --git a/platform/linux-generic/odp_packet.c >> b/platform/linux-generic/odp_packet.c >> index 2868736..f92c257 100644 >> --- a/platform/linux-generic/odp_packet.c >> +++ b/platform/linux-generic/odp_packet.c >> @@ -756,22 +756,11 @@ odp_packet_t odp_packet_copy(odp_packet_t pkt, >> odp_pool_t pool) >> { >> odp_packet_hdr_t *srchdr = odp_packet_hdr(pkt); >> uint32_t pktlen = srchdr->frame_len; >> - uint32_t meta_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, >> buf_hdr); >> odp_packet_t newpkt = odp_packet_alloc(pool, pktlen); >> >> if (newpkt != ODP_PACKET_INVALID) { >> - odp_packet_hdr_t *newhdr = odp_packet_hdr(newpkt); >> - uint8_t *newstart, *srcstart; >> - >> - /* Must copy metadata first, followed by packet data */ >> - newstart = (uint8_t *)newhdr + meta_offset; >> - srcstart = (uint8_t *)srchdr + meta_offset; >> - >> - memcpy(newstart, srcstart, >> - sizeof(odp_packet_hdr_t) - meta_offset); >> - >> - if (odp_packet_copy_from_pkt(newpkt, 0, pkt, 0, >> - pktlen) != 0) { >> + if (_odp_packet_copy_md_to_packet(pkt, newpkt) || >> + odp_packet_copy_from_pkt(newpkt, 0, pkt, 0, pktlen)) { >> odp_packet_free(newpkt); >> newpkt = ODP_PACKET_INVALID; >> } >> @@ -966,7 +955,7 @@ int odp_packet_is_valid(odp_packet_t pkt) >> * >> */ >> >> -void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t >> dstpkt) >> +int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t >> dstpkt) >> > > There are other callers of this function, we should either check the > return value there or make a comment at least about why it's not possible > for an error to happen. > > { >> odp_packet_hdr_t *srchdr = odp_packet_hdr(srcpkt); >> odp_packet_hdr_t *dsthdr = odp_packet_hdr(dstpkt); >> @@ -986,6 +975,7 @@ void _odp_packet_copy_md_to_packet(odp_packet_t >> srcpkt, odp_packet_t dstpkt) >> odp_atomic_load_u32( >> &srchdr->buf_hdr.ref_count)); >> copy_packet_parser_metadata(srchdr, dsthdr); >> + return dsthdr->buf_hdr.uarea_size < srchdr->buf_hdr.uarea_size; >> > > I think we should do this check before we do any copy > v2 posted with comments explaining this. I've retained this functionality because the copy is still needed and that's how it was performed before. It's up to the caller to determine whether user area truncation is an issue in that context. Currently that's only odp_packet_copy(). > } >> >> /** >> >>
diff --git a/platform/linux-generic/include/odp_packet_internal.h b/platform/linux-generic/include/odp_packet_internal.h index d5ace12..5c74d97 100644 --- a/platform/linux-generic/include/odp_packet_internal.h +++ b/platform/linux-generic/include/odp_packet_internal.h @@ -294,7 +294,7 @@ static inline int packet_parse_not_complete(odp_packet_hdr_t *pkt_hdr) } /* Forward declarations */ -void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt); +int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt); odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int parse); diff --git a/platform/linux-generic/odp_packet.c b/platform/linux-generic/odp_packet.c index 2868736..f92c257 100644 --- a/platform/linux-generic/odp_packet.c +++ b/platform/linux-generic/odp_packet.c @@ -756,22 +756,11 @@ odp_packet_t odp_packet_copy(odp_packet_t pkt, odp_pool_t pool) { odp_packet_hdr_t *srchdr = odp_packet_hdr(pkt); uint32_t pktlen = srchdr->frame_len; - uint32_t meta_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, buf_hdr); odp_packet_t newpkt = odp_packet_alloc(pool, pktlen); if (newpkt != ODP_PACKET_INVALID) { - odp_packet_hdr_t *newhdr = odp_packet_hdr(newpkt); - uint8_t *newstart, *srcstart; - - /* Must copy metadata first, followed by packet data */ - newstart = (uint8_t *)newhdr + meta_offset; - srcstart = (uint8_t *)srchdr + meta_offset; - - memcpy(newstart, srcstart, - sizeof(odp_packet_hdr_t) - meta_offset); - - if (odp_packet_copy_from_pkt(newpkt, 0, pkt, 0, - pktlen) != 0) { + if (_odp_packet_copy_md_to_packet(pkt, newpkt) || + odp_packet_copy_from_pkt(newpkt, 0, pkt, 0, pktlen)) { odp_packet_free(newpkt); newpkt = ODP_PACKET_INVALID; } @@ -966,7 +955,7 @@ int odp_packet_is_valid(odp_packet_t pkt) * */ -void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt) +int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt) { odp_packet_hdr_t *srchdr = odp_packet_hdr(srcpkt); odp_packet_hdr_t *dsthdr = odp_packet_hdr(dstpkt); @@ -986,6 +975,7 @@ void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt) odp_atomic_load_u32( &srchdr->buf_hdr.ref_count)); copy_packet_parser_metadata(srchdr, dsthdr); + return dsthdr->buf_hdr.uarea_size < srchdr->buf_hdr.uarea_size; } /**
Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2310 by copying the user area as part of odp_packet_copy(). The copy fails if the user area size of the destination pool is not large enough to hold the source packet user area. Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/include/odp_packet_internal.h | 2 +- platform/linux-generic/odp_packet.c | 18 ++++-------------- 2 files changed, 5 insertions(+), 15 deletions(-)