[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH v1] arch/x86: Livepatch: fix overflow check when computing ELF relocations


  • To: Roger Pau Monne <roger.pau@xxxxxxxxxx>, Bjoern Doebel <doebel@xxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>
  • From: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
  • Date: Tue, 8 Mar 2022 16:54:11 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=FC7QdKjT9i5NWBaZkVCaWzT+y7FpGPy85enFCASv+WA=; b=YlJs377b5JwXiAGTqsABv+Wtq2ixhrbjafFftmT0IZbFf1267JPMZtfqRds0gR6P0bFDbOzlKgwp2ntMusmgHT9IgnBRyZAX5s5Sj+1XAfJ70j8Y5vbneuM17QAYDIR/os1kK5DR1j/PB9nMergqhmtuklO5ERAiIC3mCXtmm+RjN+G5W5TVG6/Bkcf5wLP1vk4TZrlBSfha3iutaR2sPfvOaJ7CP2PkqUmqEOjMRVSQSFbg2ymeHAd0XdoLXcT2lPsyCtu9aDwhQqSBT4Uk4pwEIt9cmea3HQ8ViWYaTjqEUOPsJGpxr0NXkleINVeemTvmolcLmRhLZyI9BIMs1w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=CQRGOmRNat2PcHaKC2hCl1G3SdGYh50yCzAv7Ynu+p0nVrkX4Yo4+PzeO7EewafBnIKhsfYL559ucgWXVIsjySPU2wJYI0hBL2Cf6DgIwyFHGs3CdbesCtI9VWBU4GFZU5RmK4QvrQcuFr/jNMfaHPZJmFOnJMVsRHpLuZ3oNsLYfUhi0PbHk+DR9eEhKH4Ti5zXPHKRPBqDMMqshufwAinKAh3KWth/hSmZ6FAkRk8ZQpNuRsBYz3u2Avulf2zBcgr4qMIOJwYxf6Z0CPqL+SWGiNvIiJbmj4LPrsmbUj4NkJ48mXqV1N8a93MJGP7xuWDeTsNTb6mlhxfYQe+HQw==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Michael Kurth <mku@xxxxxxxxx>, Martin Pohlack <mpohlack@xxxxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 08 Mar 2022 16:54:33 +0000
  • Ironport-data: A9a23:EuEP5aJuOuNt+i0QFE+RDpUlxSXFcZb7ZxGr2PjKsXjdYENS1mYHy TQfXWmGbvaCMGfwLY0nbdjk/R4AuZHSyNFiT1FlqX01Q3x08seUXt7xwmUcns+xwm8vaGo9s q3yv/GZdJhcokf0/0vrav67xZVF/fngqoDUUYYoAQgsA148IMsdoUg7wbRh2dY42YLR7z6l4 rseneWOYDdJ5BYsWo4kw/rrRMRH5amaVJsw5zTSVNgT1LPsvyB94KE3fMldG0DQUIhMdtNWc s6YpF2PEsE1yD92Yj+tuu6TnkTn2dc+NyDW4pZdc/DKbhSvOkXee0v0XRYRQR4/ttmHozx+4 IkRss3hcFkKBerzqupDDCJVCx16GIQTrdcrIVDn2SCS50jPcn+qyPRyFkAme4Yf/46bA0kXq 6ZecmpUKEne2aTmm9pXScE17ignBNPsJ5hZvnhvwCvGBPIiaZvCX7/L9ZlT2zJYasVmQ6iHP ZdAMWIHgBLoTh8QYlE1Ms8EnPqaiHjGaj0ImRWfuv9ii4TU5FMoi+W8WDbPQfSPXcJVmk+Tp UrP+m3rBRdcONH34TmC9GiliqnQnCf4cIUIHba8+7hhh1j77nweDlgaWEW2pdG9i1WiQJRPJ koM4C0soKMuskuxQbHVUxq1qnOAlhcZUshXFas25WmlyKDZ/gKYDWgsVSNaZZots8pebTkpy 3eAmtr7AjopvLD9dJ6G3u7K93XoY3FTdDJcI39fJecY3zX9iN1viUOed4hAK+mKvNOrRgr92 wyOiQFr0t3/kvU3/6m8+FnGhRelqZ7IUhM5623rY4610u9qTNX7PtL1sDA3+d4Fdd/EFQfZ4 BDojuDDtLhmMH2bqMCarAzh9pmN7u3NDjDTiEUH83IJp2X0oC7LkWy9DVhDyKZV3iQsJGeBj Kz741o5CHpv0J2CN/cfj2WZUZhC8EQYPY65Ps04l/IXCnSLSCeJ/Tt1eWmb1H33nU4nnMkXY MnHL5jzXStBUPQ6l1JaotvxN5dxnUjSIkuJGfjGI+mPi+LCNBZ5t59fWLdxUgzJxPzd+1iEm zquH8CL1w9eQIXDjtr/quYuwaQxBSFjX/je8pUPHsbae1YOMDxxWpf5nOJ6E6Q4zvs9qws91 izkMqOu4AGk3iOvxMTjQi0LVY4Dqr4j9SJrZ3J9Zw30s5XhCK72hJoim1IMVeBP3MRozOJuT ulDfMOFA/9VTS/A9ShbZp74xLGOvjzx32pi4wLNjOADQqNd
  • Ironport-hdrordr: A9a23:AqIaa6ogzJ8ssvT9tIjiXFYaV5t8LNV00zEX/kB9WHVpm5Oj+f xGzc516farslossSkb6Ky90KnpewK5yXcH2/hvAV7EZniphILIFvAs0WKG+Vzd8kLFh5ZgPM tbAspD4ZjLfCVHZKXBkUiF+rQbsaK6GcmT7I+0pRoMPGJXguNbnn1E426gYxZLrWJ9dP0E/e +nl7N6Tk2bCBIqh6qAdxw4dtmGg+eOuIPtYBYACRJiwhKJlymU5LnzFAXd9gsCUhtUqI1SsV Ttokjc3OGOovu7whjT2yv49JJNgubszdNFGYilltUVEDPxkQylDb4RGIFq/QpF4t1H2mxa1O UkkC1QePibLEmhOF1dlCGdnjUIFgxeskMKh2Xo2UcL6vaJOg7SQ/Ax9L6xNCGpsXbI9esMo5 5jxX6WuZZMEB/Mqizh+tDDVhVnkVeDu3Y5i+4UiEpeXOIlGc9shJ1a80VPHJgaGiXmrIghDe l1FcnZoO1baFWAchnizyNSKfGXLzwO9y29MwM/Uw2uok9rtWE8y1FdyN0Un38G+p54Q55Y5/ 7cOqAtkL1VVMcZYa90Ge9EGKKMeyDwaAOJNHjXLUXsFakBNX6Io5nr4K8t7OXvfJAT1pM9lJ nITVsdv28vfEDlD9GIwfRwg13waXT4WS6oxtBV5pB/tLG5TL33MTebQFRriMekq+V3OLyTZx 9yAuMhPxbOFxqaJW8S5XyNZ3B7EwhrbPEo
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Suggested_attachment_session_id: 66eece14-4597-27b6-5bde-ba02e6652242
  • Thread-index: AQHYMwJZT6OrTo6ebU64Rq0PJcoA5qy1oRQAgAAIYICAAAMNAIAABoB6
  • Thread-topic: [RFC PATCH v1] arch/x86: Livepatch: fix overflow check when computing ELF relocations

> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Sent: Tuesday, March 8, 2022 4:26 PM
> To: Bjoern Doebel <doebel@xxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Michael Kurth <mku@xxxxxxxxx>; Martin Pohlack <mpohlack@xxxxxxxxx>; 
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Ross Lagerwall 
> <ross.lagerwall@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx 
> <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [RFC PATCH v1] arch/x86: Livepatch: fix overflow check when 
> computing ELF relocations 
>  
> On Tue, Mar 08, 2022 at 05:15:33PM +0100, Roger Pau Monné wrote:
> > On Tue, Mar 08, 2022 at 04:45:34PM +0100, Jan Beulich wrote:
> > > On 08.03.2022 16:36, Bjoern Doebel wrote:
> > > > --- a/xen/arch/x86/livepatch.c
> > > > +++ b/xen/arch/x86/livepatch.c
> > > > @@ -339,7 +339,7 @@ int arch_livepatch_perform_rela(struct 
> > > > livepatch_elf *elf,
> > > >  
> > > >              val -= (uint64_t)dest;
> > > >              *(int32_t *)dest = val;
> > > 
> > > Afaict after this assignment ...
> > > 
> > > > -            if ( (int64_t)val != *(int32_t *)dest )
> > > > +            if ( (int32_t)val != *(int32_t *)dest )
> > > 
> > > ... this condition can never be false. The cast really wants to be
> > > to int64_t, and the overflow you saw being reported is quite likely
> > > for a different reason. But from the sole message you did quote
> > > it's not really possible to figure what else is wrong.
> > 
> > It seems Linux has that check ifdef'ed [0], but there's no reference
> > as to why it's that way (I've tracked it back to the x86-64 import on
> > the historical tree, f3081f5b66a06175ff2dabfe885a53fb04e71076).
> > 
> > It's a 64bit relocation using a 32bit value, but it's unclear to me
> > that modifying the top 32bits is not allowed/intended.
> 
> Urg, I've worded this very badly. It's a 64bit relocation using a
> 32bit value, but it's unclear to me that modifying the top 32bits is
> not allowed/intended and fine to be dropped.
> 
> Thanks, Roger.

I'm not sure what you mean by that. The value is computed based on the
load address and the address of the target symbol - i.e. it is a
PC-relative relocation, and the code is checking that the computed
relative value hasn't overflowed the 32-bit destination in memory
e.g. in the unlikely case that the live patch is loaded far away in
memory from the hypervisor.

The code looks correct to me. It needs investigation to find out why this
particular patch is causing an issue since the code is unchanged since v7
of the original xSplice patch series.

Ross


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.