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

[Pekka Paalanen]

On Tue, 11 Dec 2018 08:28:45 -0500
Ilia Mirkin <imirkin at alum.mit.edu> wrote:

> 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])

Hi,

yeah, that looks fine. Took several read throughs to parse that
correctly. O_o


Thanks,
pq

> 
> >
> >
> > 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;  
> >  

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20181211/d54ab6f8/attachment.sig>