Does sys/dev/fxp/if_fxp.c using cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16) make any sense?

[Brooks Davis]

On Fri, Sep 21, 2018 at 01:21:59PM -0600, Warner Losh wrote:
> On Fri, Sep 21, 2018 at 12:35 PM Mark Millard via freebsd-hackers <
> freebsd-hackers at freebsd.org> wrote:
> 
> > [I decided that freebsd-hackers might be better for this,
> > under a different wording.]
> >
> > sys/dev/fxp/if_fxp.c uses the statement:
> >
> > cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16);
> >
> > But sys/dev/fxp/if_fxpreg.h has the types involved as:
> >
> > struct fxp_cb_tx {
> >       uint16_t cb_status;
> >       uint16_t cb_command;
> >       uint32_t link_addr;
> >       uint32_t tbd_array_addr;
> >       uint16_t byte_count;
> >       uint8_t tx_threshold;
> >       uint8_t tbd_number;
> >
> >       /*
> >        * The following structure isn't actually part of the TxCB,
> >        * unless the extended TxCB feature is being used.  In this
> >        * case, the first two elements of the structure below are
> >        * fetched along with the TxCB.
> >        */
> >       union {
> >               struct fxp_ipcb ipcb;
> >               struct fxp_tbd tbd[FXP_NTXSEG + 1];
> >       } tx_cb_u;
> > };
> >
> > So cbp->tbd is not pointing into the middle of an array.
> > Thus the cbp->tbd[-1].tb_size = . . . assignment trashes memory
> > from what I can tell.
> >
> > /usr/src/sys/dev/fxp/if_fxp.c has the [-1] assignment in:
> >
> >       /* Configure TSO. */
> >       if (m->m_pkthdr.csum_flags & CSUM_TSO) {
> >               cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16);
> >               cbp->tbd[1].tb_size |= htole32(tcp_payload << 16);
> >               cbp->ipcb_ip_schedule |= FXP_IPCB_LARGESEND_ENABLE |
> >                   FXP_IPCB_IP_CHECKSUM_ENABLE |
> >                   FXP_IPCB_TCP_PACKET |
> >                   FXP_IPCB_TCPUDP_CHECKSUM_ENABLE;
> >       }
> >
> > This cbp->tbd[-1].tb_size use goes back to -r185330 in 2008-Nov-26.
> >
> > This is also when the "+ 1" was added to the:
> >
> > struct fxp_tbd tbd[FXP_NTXSEG + 1]
> >
> > above.
> >
> > clang 7 via xtoolchain-llvm70 reported:
> >
> > --- if_fxp.o ---
> > /usr/src/sys/dev/fxp/if_fxp.c:1630:3: error: array index -1 is before the
> > beginning of the array [-Werror,-Warray-bounds]
> >               cbp->tbd[-1].tb_size = htole32(m->m_pkthdr.tso_segsz << 16);
> >               ^        ~~
> > /usr/src/sys/dev/fxp/if_fxpreg.h:297:3: note: array 'tbd' declared here
> >               struct fxp_tbd tbd[FXP_NTXSEG + 1];
> >               ^
> > 1 error generated.
> > *** [if_fxp.o] Error code 1
> >
> > It does look odd to me.
> >
> 
> So I did some digging into this a few months ago.
> 
> It turns out the code is right, kinda.
> 
> So, what's it's doing with the [-1] is as follows:
> 
> the sizeof tbd is 8 bytes long. So, we're dereferencing 8 before the array,
> which points to tbd_array_addr. However tb_size is the second element of
> that, so that points us at count + tx_threshold + tbd_number. So, this code
> is setting count = 0 and tx_threshold to the low order bits of the segment
> size and tbd_number to the high order bits. We set the count = 0 later
> anyway, so that part of it isn't so bad.
> 
> Since this is in hardware, it may be special meanings for these bits, and
> this 'trick' is used to just do one write rather than two. I've not looked
> for the hardware manual to see if this is kosher, but that's what we're
> doing. It's not as crazy stupid as it sounds at first blush.

WG14 is discussing making this definitly UB in the next C standard (many
members think it already is, but this would make it more explict).  If
this is required we need to find a better way to express it.

-- Brooks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20180921/02399b7b/attachment.sig>