drm/drm_edid: read within initialized memory bounds (fix off by one)
Hi Jonathan,
I sent this upstream, but something is not working in the submission
process as I haven't gotten any feedback. If this looks OK, should we
add the fix to our tree? This resolves the sporadic crashes on wake up on
my machine as reported to bugs@...
Thanks
Greg
>From 674554534b9f046f2a4cdab5c2dbc3d019c7daa7 Mon Sep 17 00:00:00 2001
From: Greg Steuck
Date: Wed, 21 Apr 2021 19:33:35 -0700
Subject: [PATCH] drm/drm_edid: Read within initialized memory bounds (fix off
by one)
The problem manifests as a memory out of bounds kernel panic in
OpenBSD which uses this code. The buggy error reporting code path
likely never runs with nominal hardware.
drm_do_get_edid at carp used to invoke connector_bad_edid was num_exts
of 1 even though edid at that point is only allocated a memory block
of size EDID_LENGTH. This in turn led to drm_edid_block_checksum
trying to read memory in the range [edid + EDID_LENGTH, edid +
2*EDID_LENGTH) i.e. outside the allocated boundaries. A similar if a
bit more complicated analysis applies to the other call to
connector_bad_edid. Switching to using valid_extensions limits
connector_bad_edid memory reading to the memory previously written by
drm_do_get_edid.
OpenBSD bug report https://marc.info/?l=openbsd-bugs=161794843427437
Sent upstream:
https://patchwork.kernel.org/project/dri-devel/patch/87wnszm0tw@lenny.nest.cx/
---
sys/dev/pci/drm/drm_edid.c | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/sys/dev/pci/drm/drm_edid.c b/sys/dev/pci/drm/drm_edid.c
index 2e2267410c5..b387864de54 100644
--- a/sys/dev/pci/drm/drm_edid.c
+++ b/sys/dev/pci/drm/drm_edid.c
@@ -1802,25 +1802,21 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int
block, size_t len)
}
static void connector_bad_edid(struct drm_connector *connector,
- u8 *edid, int num_blocks)
+ u8 *edid, int valid_extensions)
{
int i;
- u8 num_of_ext = edid[0x7e];
-
- if (num_of_ext > num_blocks)
- num_of_ext = num_blocks;
/* Calculate real checksum for the last edid extension block data */
connector->real_edid_checksum =
- drm_edid_block_checksum(edid + num_of_ext * EDID_LENGTH);
+ drm_edid_block_checksum(edid + valid_extensions * EDID_LENGTH);
if (connector->bad_edid_counter++ && !drm_debug_enabled(DRM_UT_KMS))
return;
dev_warn(connector->dev->dev,
-"%s: EDID is invalid:\n",
-connector->name);
- for (i = 0; i < num_blocks; i++) {
+"%s: EDID is invalid, edid_corrupt %d, null_edid_counter %d,
valid_extensions %d:\n",
+ connector->name, connector->edid_corrupt,
connector->null_edid_counter, valid_extensions);
+ for (i = 0; i <= valid_extensions; i++) {
u8 *block = edid + i * EDID_LENGTH;
char prefix[20];
@@ -1967,7 +1963,7 @@ struct edid *drm_do_get_edid(struct drm_connector
*connector,
if (valid_extensions != edid[0x7e]) {
u8 *base;
- connector_bad_edid(connector, edid, edid[0x7e] + 1);
+ connector_bad_edid(connector, edid, valid_extensions);
edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
edid[0x7e] = valid_extensions;
@@ -1995,7 +1991,7 @@ struct edid *drm_do_get_edid(struct drm_connector
*connector,
return (struct edid *)edid;
carp:
- connector_bad_edid(connector, edid, 1);
+ connector_bad_edid(connector, edid, 0);
out:
kfree(edid);
return NULL;
--
2.31.1
Re: Improve vi(1) recovery
trondd wrote:
> trondd wrote:
>
> > trondd wrote:
> >
> > >
> > > While investigating an occasional crash when recovering a file with 'vi
> > > -r'
> > > after a power failure, I noticed that the recovery files are actually
> > > never
> > > updated during an editing session. The recovery files are created upon
> > > initial modification of the file which saves the state of the file at the
> > > time of the edit. You can work on a file for as long as you want and even
> > > write it to disk but the recovery file is never updated. If the session
> > > is
> > > then lost due to power failure or a SIGKILL and you attempt to recover
> > > with
> > > -r, you'll be presented with the contents of the file from that first
> > > edit.
> > > It won't contain unsaved changes nor even any changes manually written to
> > > disk to the original file. Accepting the recovered version would lose all
> > > of your work.
> > >
> > > Reading the vi docs, man page, and source comments in the OpenBSD tree,
> > > they
> > > all mention the use of SIGALRM to periodically save changes to the
> > > recovery
> > > file. However, the code never sets up a handler or captures SIGALRM. It
> > > only ever updates the recovery file on a SIGTERM but then also exists, I
> > > guess to cover the case of an inadvertent clean system shutdown.
> > >
> > > I dug through an nvi source repository[0] that seemed to cover it's entire
> > > history and it seems this functionality was lost somewhere around 1994
> > > and I
> > > don't see it having been replaced by anything else. Our version seems to
> > > be
> > > from 1996 and editors/nvi in ports still lacks code to update the recovery
> > > file, as well.
> > >
> > > What I've done is re-implement periodic updates to the recovery file using
> > > SIGALRM and a timer like the original implementation but rewritten a bit
> > > to
> > > fit the newer source file layout and event handling. That keeps the
> > > recovery
> > > updated every 2 minutes. Then it seemed silly to be able to write
> > > changes to
> > > the original file and if a crash happens before the next SIGALRM, recovery
> > > would try to roll you back to before those saved changes. So I've also
> > > added
> > > a call to sync the recovery file if you explicitly write changes to disk.
> > > I
> > > don't think the recovery system should try to punish you for actively
> > > saving
> > > your work even if it is only at most 2 minutes worth.
> > >
> > > Comments or feedback? I'm unsure I've covered all caveats with this code
> > > or
> > > if there are vi/ex usecases where it won't work correctly. For testing,
> > > I've
> > > covered my usage and several scenarios I could contrive but I don't
> > > regularly
> > > use ex, for example, or change many options from the default. I've been
> > > running with this code for a week. And I suppose there must be a reason
> > > no
> > > one has noticed or cared about this for over 20 years. Everyone else uses
> > > vim, I guess?
> > >
> > > Tim.
> > >
> > > [0] https://repo.or.cz/nvi.git
> > >
> >
> > Got positive testing from one user. Anyone else interested? Or any other
> > feedback?
> >
> > Tim.
> >
>
> Throwing this hat back into the ring post-release. Still only gotten user
> feedback so far.
>
> Tim.
>
Gotten more positive user feedback and tests. Any developers interested in
this is some form or another?
Diff regenerated after expandtab change, which didn't really move anything.
Tim.
Index: cl/cl.h
===
RCS file: /cvs/src/usr.bin/vi/cl/cl.h,v
retrieving revision 1.10
diff -u -p -r1.10 cl.h
--- cl/cl.h 27 May 2016 09:18:11 - 1.10
+++ cl/cl.h 9 May 2021 20:03:21 -
@@ -30,8 +30,9 @@ typedef struct _cl_private {
#defineINDX_HUP0
#defineINDX_INT1
#defineINDX_TERM 2
-#defineINDX_WINCH 3
-#defineINDX_MAX4 /* Original signal information. */
+#defineINDX_ALRM 3
+#defineINDX_WINCH 4
+#defineINDX_MAX5 /* Original signal information. */
struct sigaction oact[INDX_MAX];
enum { /* Tty group write mode. */
@@ -48,8 +49,9 @@ typedef struct _cl_private {
#defineCL_SIGHUP 0x0020 /* SIGHUP arrived. */
#defineCL_SIGINT 0x0040 /* SIGINT arrived. */
#defineCL_SIGTERM 0x0080 /* SIGTERM arrived. */
-#defineCL_SIGWINCH 0x0100 /* SIGWINCH arrived. */
-#defineCL_STDIN_TTY0x0200 /* Talking to a terminal. */
+#defineCL_SIGALRM 0x0100 /* SIGALRM arrived. */
+#defineCL_SIGWINCH 0x0200 /* SIGWINCH arrived. */
+#defineCL_STDIN_TTY0x0400 /* Talking to a terminal. */
u_int32_t flags;
} CL_PRIVATE;
Index: cl/cl_main.c
===
RCS
Removal of old users and groups in the upgrade notes
Hello,
This is both a general question and specific example of removal of
old users and groups.
With the release of 6.7, rebound(8) got tedued[0] ;^)
However, there were no specific instructions regarding removal of
_rebound user and group, or the /etc/rebound.conf file, in the
upgrade notes - I had the latter added to my /etc/sysclean.ignore
file and didn't notice it until today.
Grepping for '{user,group}del' under 'faq/upgrade*', shows a handful
of examples - some are straight after a user or a group has been
retired[1], others when the UID/GID got recycled[2].
Probably too little too late but, should the 6.7 upgrade page get:
# rm /etc/rebound.conf
# userdel _rebound
# groupdel _rebound
instructions added, i.e. need I bother you with a diff, or will you
simply add it once the UID/GID gets recycled?
[0] https://www.openbsd.org/faq/upgrade67.html
[1] https://www.openbsd.org/faq/upgrade61.html
[2] https://www.openbsd.org/faq/upgrade64.html
Cheers,
Raf
Re: emutls and dlopen(3) problem - Re: patch: make ld.so aware of pthread_key_create destructor - Re: multimedia/mpv debug vo=gpu crash on exit
So, is there a way to get a work-around ? or should we push for proper tls sooner than later ? It always made me queasy having all those programs segfaulting on exit. Not sure it's exploitable, but giving people the impression it's okay to segfault. I thought it might be bad code in the programs, turns out it's our infrastructure that has issues. Could we just postpone unloading dlopen'd stuff during exit somehow, until after running all the __fini code ?
Re: ld.so: NULL dereference on corrupt library
On Wed, May 05, 2021 at 02:20:45PM -0900, Philip Guenther wrote: > IMHO, the benefit of adding this check is almost zero: it gives a slightly > better experience for a small set of possible data corruption cases, when > similar corruptions that affect other pages aren't helped at all as it'll > crash when it executes zeroed text, or accesses zeroed data, or fails to > find a required symbol because the symbol table was zeroed out. I agree that it only covers a small subset of all the countless corruptions and erroneous ld.so might take. > If we want to protect against that sort of hardware lossage, then a > filesystem which does so is the way to go, not an alarm on one window of a > glass house. And yes, I'd like preventing corruption in the first place as well. But then again, having such mechanisms around seems very unlikely and a clean error message telling users *where* corruption took place still seems worth it; binutils having a similar check seems to support that. I sit on the fence here with no strong opinion (also in part because I barely scratched the surfce of ELF and ld.so so far), so unless someone else wants such a check/error message in ld.so I'll just drop the diff.
