Bug 278546 - fdevname_r: pass partially uninitialized memory to kernel
Summary: fdevname_r: pass partially uninitialized memory to kernel
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 14.0-STABLE
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-bugs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-04-23 10:24 UTC by Ivan Rozhuk
Modified: 2024-05-01 07:42 UTC (History)
3 users (show)

See Also:


Attachments
patch (762 bytes, patch)
2024-04-23 10:24 UTC, Ivan Rozhuk
no flags Details | Diff
patch (687 bytes, patch)
2024-04-24 16:24 UTC, Ivan Rozhuk
rozhuk.im: maintainer-approval?
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ivan Rozhuk 2024-04-23 10:24:08 UTC
Created attachment 250173 [details]
patch

valgrind:

==55093== Syscall param ioctl(generic) points to uninitialised byte(s)
==55093==    at 0x860342A: ioctl (in /lib/libc.so.7)
==55093==    by 0x855D1B6: fdevname_r (in /lib/libc.so.7)
==55093==    by 0x863842C: ptsname (in /lib/libc.so.7)
==55093==    by 0x76DCBD2: openpty (in /lib/libutil.so.9)
==55093==    by 0x76DCC93: forkpty (in /lib/libutil.so.9)
==55093==    by 0x7E9F8B4: UnixProcessImpl::Execute(wxEvtHandler*, wxArrayString const&, unsigned long, wxString const&, IProcessCallback*) (CodeLite/unixprocess_impl.cpp:312)
==55093==    by 0x7D1F4CA: CreateAsyncProcess(wxEvtHandler*, wxArrayString const&, unsigned long, wxString const&, std::__1::vector<std::__1::pair<wxString, wxString>, std::__1::allocator<std::__1::pair<wxString, wxString> > > const*, wxString const&) (CodeLite/asyncprocess.cpp:274)
==55093==    by 0x7D20BA0: CreateAsyncProcess(wxEvtHandler*, wxString const&, unsigned long, wxString const&, std::__1::vector<std::__1::pair<wxString, wxString>, std::__1::allocator<std::__1::pair<wxString, wxString> > > const*, wxString const&) (CodeLite/asyncprocess.cpp:282)
==55093==    by 0x1495BE1E: GitPlugin::AsyncRunGit(wxEvtHandler*, wxString const&, unsigned long, wxString const&, bool) (git/git.cpp:2961)
==55093==    by 0x149587CF: GitPlugin::ProcessGitActionQueue() (git/git.cpp:1274)
==55093==    by 0x149569E2: GitPlugin::DoSetRepoPath(wxString const&) (git/git.cpp:496)
==55093==    by 0x1494509D: GitPlugin::OnWorkspaceLoaded(clWorkspaceEvent&) (git/git.cpp:1013)
==55093==  Address 0x1ffbffcfc4 is on thread 1's stack
==55093==  in frame #1, created by fdevname_r (???:)
==55093==  Uninitialised value was created by a stack allocation
==55093==    at 0x855D180: fdevname_r (in /lib/libc.so.7)
==55093== 


struct fiodgname_arg {
	int	len;
	void	*buf;
};
#define	FIODGNAME	_IOW('f', 120, struct fiodgname_arg) /* get dev. name */

char *
fdevname_r(int fd, char *buf, int len)
{
	struct fiodgname_arg fgn;

	fgn.buf = buf;
	fgn.len = len;

	if (_ioctl(fd, FIODGNAME, &fgn) == -1)
		return (NULL);
	return (buf);
}

memory pad between len and buf is uninitialized.
Comment 1 Ivan Rozhuk 2024-04-23 10:27:31 UTC
This code may be guarded by condition:

#if (sizeof(int) + sizeof(void*)) != sizeof(struct fiodgname_arg)
Comment 2 Mark Johnston freebsd_committer freebsd_triage 2024-04-24 13:40:48 UTC
Is it possible for valgrind to suppress this warning, perhaps based on a builtin model of FIODGNAME?  The uninitialized bytes are harmless.  If needed we can zero them but it's a bit silly.
Comment 3 Paul Floyd 2024-04-24 13:55:46 UTC
ioctls are one of the things that Valgrind on FreeBSD needs to improve. Currently there is minimal checking beyond the basic read/write encoding. It needs to be extended ideally for each kind of descriptor and for each corresponding data structur and field that gets read from or written to.

Linux does a much better job in this case.
Comment 4 Ivan Rozhuk 2024-04-24 16:08:16 UTC
struct fiodgname_arg fgn = { 0 }; - also work as memset() and used in kernel and userspace.
struct fiodgname_arg fgn = { .buf = buf, .len = len };- does NOT work: some memory leave uninitialized
Comment 5 Ivan Rozhuk 2024-04-24 16:24:40 UTC
Created attachment 250210 [details]
patch

This is smallest impact to code and performance that I can do :)
Comment 6 Paul Floyd 2024-04-24 18:32:18 UTC
If you can use a recent Valgrind (valgrind-devel or build from source) then this should have been resolved.

Kyle Evans pointed out the same issue to me and I fixed it back in January 2024.

Here is the commit

https://sourceware.org/git/?p=valgrind.git;a=commit;h=d0c17b5431e5448c73f99322afb5aac442c2ee70

Other than painstakingly reading through the kernel source, is there any place where information about ioctls is gathered together (something like syscalls.master)?
Comment 7 Mark Johnston freebsd_committer freebsd_triage 2024-04-24 18:48:55 UTC
(In reply to Paul Floyd from comment #6)
Sadly, there is no central registry of ioctls.  Having one would be very useful, but there is no good mechanism to enumerate them (the best you can do is grep for _IO(R|W)? in the system headers) and no way in general to tell what the kernel will do with the parameter.
Comment 8 Paul Floyd 2024-05-01 07:42:47 UTC
find . -name "*h" -exec egrep "_IO(R|W|WR)\(" {} /dev/null \; | wc
    1716   10762  139454

1036 of those use a struct

And using different structs

find . -name "*h" -exec egrep "_IO(R|W|WR)\(" {} /dev/null \; | grep struct | sed 's/.*struct//' | sed 's/).*//' | sort -u | wc    
     538     539    8235