|
From: <sv...@va...> - 2017-03-15 19:35:37
|
Author: philippe
Date: Wed Mar 15 19:35:29 2017
New Revision: 16274
Log:
Fix 376956 syswrap of SNDDRV and DRM_IOCTL_VERSION causing some addresses
to be wrongly marked as addressable
Patch from Daniel Glöckner, slightly modified.
Modified:
trunk/NEWS
trunk/coregrind/m_syswrap/syswrap-linux.c
trunk/include/pub_tool_basics.h
Modified: trunk/NEWS
==============================================================================
--- trunk/NEWS (original)
+++ trunk/NEWS Wed Mar 15 19:35:29 2017
@@ -142,6 +142,8 @@
376611 ppc64 and arm64 don't know about prlimit64 syscall
376729 PPC64, remove R2 from the clobber list
== 371668
+376956 syswrap of SNDDRV and DRM_IOCTL_VERSION causing some addresses
+ to be wrongly marked as addressable
377427 PPC64, lxv instruction failing on odd destination register
377478 PPC64: ISA 3.0 setup fixes
Modified: trunk/coregrind/m_syswrap/syswrap-linux.c
==============================================================================
--- trunk/coregrind/m_syswrap/syswrap-linux.c (original)
+++ trunk/coregrind/m_syswrap/syswrap-linux.c Wed Mar 15 19:35:29 2017
@@ -6069,6 +6069,11 @@
ioctl wrappers
------------------------------------------------------------------ */
+struct vg_drm_version_info {
+ struct vki_drm_version data;
+ struct vki_drm_version *orig; // Original ARG3 pointer value at syscall entry.
+};
+
PRE(sys_ioctl)
{
*flags |= SfMayBlock;
@@ -7686,7 +7691,8 @@
case VKI_DRM_IOCTL_VERSION:
if (ARG3) {
- struct vki_drm_version *data = (struct vki_drm_version *)ARG3;
+ struct vki_drm_version* data = (struct vki_drm_version *)ARG3;
+ struct vg_drm_version_info* info;
PRE_MEM_WRITE("ioctl(DRM_VERSION).version_major", (Addr)&data->version_major, sizeof(data->version_major));
PRE_MEM_WRITE("ioctl(DRM_VERSION).version_minor", (Addr)&data->version_minor, sizeof(data->version_minor));
PRE_MEM_WRITE("ioctl(DRM_VERSION).version_patchlevel", (Addr)&data->version_patchlevel, sizeof(data->version_patchlevel));
@@ -7699,6 +7705,10 @@
PRE_MEM_READ("ioctl(DRM_VERSION).desc_len", (Addr)&data->desc_len, sizeof(data->desc_len));
PRE_MEM_READ("ioctl(DRM_VERSION).desc", (Addr)&data->desc, sizeof(data->desc));
PRE_MEM_WRITE("ioctl(DRM_VERSION).desc", (Addr)data->desc, data->desc_len);
+ info = VG_(malloc)("syswrap.ioctl.1", sizeof(*info));
+ info->data = *data;
+ info->orig = data;
+ ARG3 = (Addr)&info->data;
}
break;
case VKI_DRM_IOCTL_GET_UNIQUE:
@@ -10174,16 +10184,24 @@
case VKI_DRM_IOCTL_VERSION:
if (ARG3) {
- struct vki_drm_version *data = (struct vki_drm_version *)ARG3;
+ struct vki_drm_version* data = (struct vki_drm_version *)ARG3;
+ struct vg_drm_version_info* info = container_of(data, struct vg_drm_version_info, data);
+ const vki_size_t orig_name_len = info->orig->name_len;
+ const vki_size_t orig_date_len = info->orig->date_len;
+ const vki_size_t orig_desc_len = info->orig->desc_len;
+ *info->orig = info->data;
+ ARG3 = (Addr)info->orig;
+ data = info->orig;
+ VG_(free)(info);
POST_MEM_WRITE((Addr)&data->version_major, sizeof(data->version_major));
POST_MEM_WRITE((Addr)&data->version_minor, sizeof(data->version_minor));
POST_MEM_WRITE((Addr)&data->version_patchlevel, sizeof(data->version_patchlevel));
POST_MEM_WRITE((Addr)&data->name_len, sizeof(data->name_len));
- POST_MEM_WRITE((Addr)data->name, data->name_len);
+ POST_MEM_WRITE((Addr)data->name, VG_MIN(data->name_len, orig_name_len));
POST_MEM_WRITE((Addr)&data->date_len, sizeof(data->date_len));
- POST_MEM_WRITE((Addr)data->date, data->date_len);
+ POST_MEM_WRITE((Addr)data->date, VG_MIN(data->date_len, orig_date_len));
POST_MEM_WRITE((Addr)&data->desc_len, sizeof(data->desc_len));
- POST_MEM_WRITE((Addr)data->desc, data->desc_len);
+ POST_MEM_WRITE((Addr)data->desc, VG_MIN(data->desc_len, orig_desc_len));
}
break;
case VKI_DRM_IOCTL_GET_UNIQUE:
Modified: trunk/include/pub_tool_basics.h
==============================================================================
--- trunk/include/pub_tool_basics.h (original)
+++ trunk/include/pub_tool_basics.h Wed Mar 15 19:35:29 2017
@@ -396,6 +396,10 @@
# define offsetof(type,memb) ((SizeT)(HWord)&((type*)0)->memb)
#endif
+#if !defined(container_of)
+# define container_of(ptr, type, member) ((type *)((char *)(ptr) - offsetof(type, member)))
+#endif
+
/* Alignment */
/* We use a prefix vg_ for vg_alignof as its behaviour slightly
differs from the standard alignof/gcc defined __alignof__
|
|
From: Ivo R. <iv...@iv...> - 2017-03-15 20:36:31
|
2017-03-15 20:35 GMT+01:00 <sv...@va...>:
> Author: philippe
> Date: Wed Mar 15 19:35:29 2017
> New Revision: 16274
>
> Log:
> Fix 376956 syswrap of SNDDRV and DRM_IOCTL_VERSION causing some addresses
> to be wrongly marked as addressable
>
> Patch from Daniel Glöckner, slightly modified.
>
>
> Modified:
> trunk/NEWS
> trunk/coregrind/m_syswrap/syswrap-linux.c
> trunk/include/pub_tool_basics.h
>
> Modified: trunk/NEWS
> ==============================================================================
> --- trunk/NEWS (original)
> +++ trunk/NEWS Wed Mar 15 19:35:29 2017
> @@ -142,6 +142,8 @@
> 376611 ppc64 and arm64 don't know about prlimit64 syscall
> 376729 PPC64, remove R2 from the clobber list
> == 371668
> +376956 syswrap of SNDDRV and DRM_IOCTL_VERSION causing some addresses
> + to be wrongly marked as addressable
> 377427 PPC64, lxv instruction failing on odd destination register
> 377478 PPC64: ISA 3.0 setup fixes
>
>
> Modified: trunk/coregrind/m_syswrap/syswrap-linux.c
> ==============================================================================
> --- trunk/coregrind/m_syswrap/syswrap-linux.c (original)
> +++ trunk/coregrind/m_syswrap/syswrap-linux.c Wed Mar 15 19:35:29 2017
> @@ -6069,6 +6069,11 @@
> ioctl wrappers
> ------------------------------------------------------------------ */
>
> +struct vg_drm_version_info {
> + struct vki_drm_version data;
> + struct vki_drm_version *orig; // Original ARG3 pointer value at syscall entry.
> +};
> +
> PRE(sys_ioctl)
> {
> *flags |= SfMayBlock;
> @@ -7686,7 +7691,8 @@
>
> case VKI_DRM_IOCTL_VERSION:
> if (ARG3) {
> - struct vki_drm_version *data = (struct vki_drm_version *)ARG3;
> + struct vki_drm_version* data = (struct vki_drm_version *)ARG3;
> + struct vg_drm_version_info* info;
> PRE_MEM_WRITE("ioctl(DRM_VERSION).version_major", (Addr)&data->version_major, sizeof(data->version_major));
> PRE_MEM_WRITE("ioctl(DRM_VERSION).version_minor", (Addr)&data->version_minor, sizeof(data->version_minor));
> PRE_MEM_WRITE("ioctl(DRM_VERSION).version_patchlevel", (Addr)&data->version_patchlevel, sizeof(data->version_patchlevel));
> @@ -7699,6 +7705,10 @@
> PRE_MEM_READ("ioctl(DRM_VERSION).desc_len", (Addr)&data->desc_len, sizeof(data->desc_len));
> PRE_MEM_READ("ioctl(DRM_VERSION).desc", (Addr)&data->desc, sizeof(data->desc));
> PRE_MEM_WRITE("ioctl(DRM_VERSION).desc", (Addr)data->desc, data->desc_len);
> + info = VG_(malloc)("syswrap.ioctl.1", sizeof(*info));
> + info->data = *data;
> + info->orig = data;
> + ARG3 = (Addr)&info->data;
> }
> break;
Does this create a memory leak if the ioctl fails?
I think it does because POST(sys_ioctl) is called only on success.
I can think of several approaches here:
- have POST(sys_ioctl) called also on failure
- convey the required information in some other way
- leave it as is and document somewhere this could leak some memory
I.
|
|
From: Philippe W. <phi...@sk...> - 2017-03-15 21:55:10
|
On Wed, 2017-03-15 at 21:28 +0100, Ivo Raisr wrote:
> 2017-03-15 20:35 GMT+01:00 <sv...@va...>:
> > + info = VG_(malloc)("syswrap.ioctl.1", sizeof(*info));
> > + info->data = *data;
> > + info->orig = data;
> > + ARG3 = (Addr)&info->data;
> > }
> > break;
>
> Does this create a memory leak if the ioctl fails?
> I think it does because POST(sys_ioctl) is called only on success.
>
> I can think of several approaches here:
> - have POST(sys_ioctl) called also on failure
> - convey the required information in some other way
> - leave it as is and document somewhere this could leak some memory
Good catch, yes, I think it would leak.
I guess we might have to put the flag SfPostOnFail, like
for ppoll and pselect6 ?
And then, in the POST, just execute the POST_MEM_WRITE operations
if success ?
(and always release the memory)
Philippe
|
|
From: Ivo R. <iv...@iv...> - 2017-03-15 22:12:04
|
2017-03-15 22:56 GMT+01:00 Philippe Waroquiers <phi...@sk...>:
> On Wed, 2017-03-15 at 21:28 +0100, Ivo Raisr wrote:
>> 2017-03-15 20:35 GMT+01:00 <sv...@va...>:
>
>> > + info = VG_(malloc)("syswrap.ioctl.1", sizeof(*info));
>> > + info->data = *data;
>> > + info->orig = data;
>> > + ARG3 = (Addr)&info->data;
>> > }
>> > break;
>>
>> Does this create a memory leak if the ioctl fails?
>> I think it does because POST(sys_ioctl) is called only on success.
>>
>> I can think of several approaches here:
>> - have POST(sys_ioctl) called also on failure
>> - convey the required information in some other way
>> - leave it as is and document somewhere this could leak some memory
>
> Good catch, yes, I think it would leak.
>
> I guess we might have to put the flag SfPostOnFail, like
> for ppoll and pselect6?
>
> And then, in the POST, just execute the POST_MEM_WRITE operations
> if success?
> (and always release the memory)
Yes, that's one of the options possible.
Thank you for looking at this.
I.
|