Re: [Qemu-devel] [PATCH] 9pfs: check the size of transport buffer before marshaling

[Jan Dakinevich]

Greg,

> I couldn't find a way to break transport
> with 9p2000.L. Do you have a reproducer for the latter ?

You're right! The problem is related only to 9p2000.u protocol. In this 
protocol to list a directory folowing chain is used: TOPEN->TREAD. 
9p2000.L uses for the same goal TLOPEN->TREADDIR.

As I just figured out TREADDIR never fails in marshaling stage because 
it calls v9fs_do_readdir which checks for availble buffer size 
(max_count) before marshaling.

I am preparing new patch being inspired by v9fs_do_readdir()'s behavior.

On 09/15/2017 05:56 PM, Greg Kurz wrote:
> On Thu, 14 Sep 2017 19:31:36 +0300
> Jan Dakinevich <address@hidden> wrote:
>
> > v9fs_do_readdir_with_stat() and v9fs_do_readdir() stores as much data in
> > the buffer as can fit unless marshaling erorr occurs. However, after
> > commit 23a006d the behavior pdu_marshal was changed, and on error the
> > routine assumes that buffers are misconfigured and breaks communication.
>
> I agree and I could easily reproduce with a linux guest using 9p2000.u, ie, the
> v9fs_do_readdir_with_stat() case, but I couldn't find a way to break transport
> with 9p2000.L. Do you have a reproducer for the latter ?
>
> I ask because I would appreciate to see some more details in the changelog,
> for the records.
>
> > Signed-off-by: Jan Dakinevich <address@hidden>
> > ---
>
> Anyway the patch looks fine, but I'll wait for your answer before pushing
> to 9p-next.
>
> Cheers,
>
> --
> Greg
>
> >   hw/9pfs/9p.c | 44 ++++++++++++++++++++++++++++++++------------
> >   1 file changed, 32 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 1e38109..8e0b87e 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1679,6 +1679,16 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, 
> > V9fsPDU *pdu,
> >       qemu_iovec_concat(qiov, &elem, skip, size);
> >   }
> >   
> > +static size_t v9fs_marshal_size(V9fsPDU *pdu)
> > +{
> > +    struct iovec *iov;
> > +    unsigned int niov;
> > +
> > +    pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, 0);
> > +
> > +    return iov_size(iov, niov);
> > +}
> > +
> >   static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
> >                              uint64_t off, uint32_t max_count)
> >   {
> > @@ -1725,6 +1735,10 @@ static int coroutine_fn 
> > v9fs_do_readdir_with_stat(V9fsPDU *pdu,
> >       off_t saved_dir_pos;
> >       struct dirent *dent;
> >   
> > +    /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> > +    size_t offset = 11;
> > +    size_t marshal_size = v9fs_marshal_size(pdu);
> > +
> >       /* save the directory position */
> >       saved_dir_pos = v9fs_co_telldir(pdu, fidp);
> >       if (saved_dir_pos < 0) {
> > @@ -1752,18 +1766,23 @@ static int coroutine_fn 
> > v9fs_do_readdir_with_stat(V9fsPDU *pdu,
> >           if (err < 0) {
> >               break;
> >           }
> > -        /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> > -        len = pdu_marshal(pdu, 11 + count, "S", &v9stat);
> >   
> > -        v9fs_readdir_unlock(&fidp->fs.dir);
> > +        if (v9stat.size + 2 > MIN(marshal_size - offset, max_count - count)) {
> > +            v9fs_readdir_unlock(&fidp->fs.dir);
> >   
> > -        if ((len != (v9stat.size + 2)) || ((count + len) > max_count)) {
> >               /* Ran out of buffer. Set dir back to old position and return */
> >               v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
> >               v9fs_stat_free(&v9stat);
> >               v9fs_path_free(&path);
> >               return count;
> >           }
> > +
> > +        len = pdu_marshal(pdu, offset, "S", &v9stat);
> > +        BUG_ON(len != v9stat.size + 2);
> > +
> > +        v9fs_readdir_unlock(&fidp->fs.dir);
> > +
> > +        offset += len;
> >           count += len;
> >           v9fs_stat_free(&v9stat);
> >           v9fs_path_free(&path);
> > @@ -1884,6 +1903,10 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
> > V9fsFidState *fidp,
> >       off_t saved_dir_pos;
> >       struct dirent *dent;
> >   
> > +    /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> > +    size_t offset = 11;
> > +    size_t marshal_size = v9fs_marshal_size(pdu);
> > +
> >       /* save the directory position */
> >       saved_dir_pos = v9fs_co_telldir(pdu, fidp);
> >       if (saved_dir_pos < 0) {
> > @@ -1899,7 +1922,8 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
> > V9fsFidState *fidp,
> >           }
> >           v9fs_string_init(&name);
> >           v9fs_string_sprintf(&name, "%s", dent->d_name);
> > -        if ((count + v9fs_readdir_data_size(&name)) > max_count) {
> > +        if (v9fs_readdir_data_size(&name) > MIN(marshal_size - offset,
> > +                                                max_count - count)) {
> >               v9fs_readdir_unlock(&fidp->fs.dir);
> >   
> >               /* Ran out of buffer. Set dir back to old position and return */
> > @@ -1918,18 +1942,14 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, 
> > V9fsFidState *fidp,
> >           qid.type = 0;
> >           qid.version = 0;
> >   
> > -        /* 11 = 7 + 4 (7 = start offset, 4 = space for storing count) */
> > -        len = pdu_marshal(pdu, 11 + count, "Qqbs",
> > +        len = pdu_marshal(pdu, offset, "Qqbs",
> >                             &qid, dent->d_off,
> >                             dent->d_type, &name);
> > +        BUG_ON(len != v9fs_readdir_data_size(&name));
> >   
> >           v9fs_readdir_unlock(&fidp->fs.dir);
> >   
> > -        if (len < 0) {
> > -            v9fs_co_seekdir(pdu, fidp, saved_dir_pos);
> > -            v9fs_string_free(&name);
> > -            return len;
> > -        }
> > +        offset += len;
> >           count += len;
> >           v9fs_string_free(&name);
> >           saved_dir_pos = dent->d_off;

--
Best regards,
Jan Dakinevich