[PATCH xserver] modesetting: fix conn_id termination and potential overrun by 1 byte

[Ilia Mirkin]

On Tue, Dec 11, 2018 at 3:23 AM Pekka Paalanen <ppaalanen at gmail.com> wrote:
>
> On Mon, 10 Dec 2018 23:34:11 -0500
> Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>
> > Noticed when porting this logic to xf86-video-nouveau, and valgrind
> > complained about conditional jump based on uninitialized data.
> >
> > Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> > ---
> >
> > memcpy sets conn_id[0..len-1], so conn_id[len] is the one that should
> > get the 0.
>
> Hi,
>
> you're certainly right about memcpy vs. len. I didn't check the type of
> conn_id, but if it's an array of bytes, then
>
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.com>

Thanks! Here are the relevant bits of the function:

    char conn_id[5];
...
    len = conn - (blob_data + 4);
    if (len + 1> 5)
        return -1;
    memcpy(conn_id, blob_data + 4, len);
    conn_id[len] = '\0';
    id = strtoul(conn_id, NULL, 10);

(previously that was conn_id[len+1])

>
>
> Thanks,
> pq
>
> >
> >  hw/xfree86/drivers/modesetting/drmmode_display.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
> > index 939f07f8f..5c1b0ea96 100644
> > --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> > +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> > @@ -2834,7 +2834,7 @@ static int parse_path_blob(drmModePropertyBlobPtr path_blob, int *conn_base_id,
> >      if (len + 1> 5)
> >          return -1;
> >      memcpy(conn_id, blob_data + 4, len);
> > -    conn_id[len + 1] = '\0';
> > +    conn_id[len] = '\0';
> >      id = strtoul(conn_id, NULL, 10);
> >
> >      *conn_base_id = id;
>