[prev in list] [next in list] [prev in thread] [next in thread]
List: illumos-developer
Subject: Re: [zfs] [developer] Review: 6314 buffer overflow in dsl_dataset_name
From: "Matthew Ahrens" <mahrens () delphix ! com>
Date: 2016-02-25 18:01:16
Message-ID: CAJjvXiF0FWvmBVp_NAVbfiHUa9OthHAjbePz2sjdpoiuVrg=rQ () mail ! gmail ! com
[Download RAW message or body]
Yes. The fix is available here:
https://github.com/delphix/delphix-os/commit/3d87788554d4c0591595e222b5b0ea25ab0c897f
I haven't gotten to upstreaming it yet. I imagine there will be conflicts
since the change is so pervasive. I would welcome others to rebase it on
illumos and submit a pull request! (see https://github.com/openzfs/openzfs/)
But I'll get to it eventually if nobody else does.
--matt
On Thu, Feb 25, 2016 at 1:06 AM, Simon Klinkert <simon.klinkert@gmail.com>
wrote:
> Matt, are you still working on this? I'd be glad to see a fix upstream :)
>
> Simon
>
> > On 16 Oct 2015, at 22:17, Matthew Ahrens <mahrens@delphix.com> wrote:
> >
> > It looks like this is an off-by-one error due to the difference between
> ZFS_MAXNAMELEN as defined in zfs_znode.h (as MAXNAMELEN - 1, i.e. 255), and
> MAXNAMELEN (256). The fact that ZFS_MAXNAMELEN is defined to be MAXNAMELEN
> (256) in libzfs.h compounds the confusion.
> >
> > The problem is that dsl_dataset_snapshot_check checks (strlen(name) >=
> MAXNAMELEN), when it should be using ZFS_MAXNAMELEN. We should examine all
> uses of ZFS_MAXNAMELEN and NAMELEN in ZFS code and make sure they are doing
> the right thing.
> >
> > --matt
> >
> > On Fri, Oct 16, 2015 at 10:26 AM, Matthew Ahrens <mahrens@delphix.com>
> wrote:
> > The bug is that we allowed a name > ZFS_MAXNAMELEN to be created in the
> first place. There is code in place to prevent this, in
> dsl_dataset_snapshot_check, dsl_dataset_rename_snapshot_check_impl, and
> dsl_valid_rename. Therefore we need to know how you got into this
> situation to begin with, because apparently these checks are insufficient.
> >
> > Adding the VERIFY seems good for safety, but is not a fix for the actual
> bug.
> >
> > --matt
> >
> > On Fri, Oct 16, 2015 at 7:23 AM, Dan McDonald <danmcd@omniti.com> wrote:
> >
> > > On Oct 16, 2015, at 9:02 AM, Simon Klinkert <simon.klinkert@gmail.com>
> wrote:
> > >
> > >>
> > >> Oddly enough, the OpenZFS hackathon is on Tuesday. I wonder if a
> better fix (one that propagates an error up) could be done? Someone should
> add it to the wiki.
> > >
> > > +1
> >
> > I'll volunteer for this right now unless someone else in ZFS-land wants
> to tackle it. :)
> >
> > Dan
> >
> >
> >
> > -------------------------------------------
> > illumos-zfs
> > Archives: https://www.listbox.com/member/archive/182191/=now
> > RSS Feed:
> https://www.listbox.com/member/archive/rss/182191/27179292-3e7f6e2f
> > Modify Your Subscription:
> https://www.listbox.com/member/?&
> > Powered by Listbox: http://www.listbox.com
> >
> >
>
>
-------------------------------------------
illumos-developer
Archives: https://www.listbox.com/member/archive/182179/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182179/25758058-4e9228dc
Modify Your Subscription: https://www.listbox.com/member/?member_id=25758058&id_secret=25758058-c19b436a
Powered by Listbox: http://www.listbox.com
[Attachment #3 (text/html)]
<html><html><div dir="ltr">Yes. The fix is available here: <a \
href="https://github.com/delphix/delphix-os/commit/3d87788554d4c0591595e222b5b0ea25ab0 \
c897f">https://github.com/delphix/delphix-os/commit/3d87788554d4c0591595e222b5b0ea25ab0c897f</a><div><br \
/></div><div>I haven't gotten to upstreaming it yet. I imagine there will \
be conflicts since the change is so pervasive. I would welcome others to rebase \
it on illumos and submit a pull request! (see <a \
href="https://github.com/openzfs/openzfs/">https://github.com/openzfs/openzfs/</a>) \
But I'll get to it eventually if nobody else does.</div><div><br \
/></div><div>--matt</div></div><div class="gmail_extra"><br /><div \
class="gmail_quote">On Thu, Feb 25, 2016 at 1:06 AM, Simon Klinkert <span \
dir="ltr"><<a href="mailto:simon.klinkert@gmail.com" \
target="_blank">simon.klinkert@gmail.com</a>></span> wrote:<br /><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">Matt, are you still working on this? I’d be glad to see \
a fix upstream :)<br /><span class="HOEnZb"><font color="#888888"><br /> Simon<br \
/></font></span><div class="HOEnZb"><div class="h5"><br /> > On 16 Oct 2015, at \
22:17, Matthew Ahrens <<a \
href="mailto:mahrens@delphix.com">mahrens@delphix.com</a>> wrote:<br /> ><br />
> It looks like this is an off-by-one error due to the difference between \
ZFS_MAXNAMELEN as defined in zfs_znode.h (as MAXNAMELEN - 1, i.e. 255), and \
MAXNAMELEN (256). The fact that ZFS_MAXNAMELEN is defined to be MAXNAMELEN \
(256) in libzfs.h compounds the confusion.<br /> ><br />
> The problem is that dsl_dataset_snapshot_check checks (strlen(name) >= \
MAXNAMELEN), when it should be using ZFS_MAXNAMELEN. We should examine all uses \
of ZFS_MAXNAMELEN and NAMELEN in ZFS code and make sure they are doing the right \
thing.<br /> ><br />
> --matt<br />
><br />
> On Fri, Oct 16, 2015 at 10:26 AM, Matthew Ahrens <<a \
href="mailto:mahrens@delphix.com">mahrens@delphix.com</a>> wrote:<br /> > The \
bug is that we allowed a name > ZFS_MAXNAMELEN to be created in the first \
place. There is code in place to prevent this, in dsl_dataset_snapshot_check, \
dsl_dataset_rename_snapshot_check_impl, and dsl_valid_rename. Therefore we need \
to know how you got into this situation to begin with, because apparently these \
checks are insufficient.<br /> ><br />
> Adding the VERIFY seems good for safety, but is not a fix for the actual bug.<br \
/> ><br />
> --matt<br />
><br />
> On Fri, Oct 16, 2015 at 7:23 AM, Dan McDonald <<a \
href="mailto:danmcd@omniti.com">danmcd@omniti.com</a>> wrote:<br /> ><br />
> > On Oct 16, 2015, at 9:02 AM, Simon Klinkert <<a \
href="mailto:simon.klinkert@gmail.com">simon.klinkert@gmail.com</a>> wrote:<br /> \
> ><br /> > >><br />
> >> Oddly enough, the OpenZFS hackathon is on Tuesday. I wonder if a \
better fix (one that propagates an error up) could be done? Someone should add \
it to the wiki.<br /> > ><br />
> > +1<br />
><br />
> I'll volunteer for this right now unless someone else in ZFS-land wants to \
tackle it. :)<br /> ><br />
> Dan<br />
><br />
><br />
><br /><a href="http://www.listbox.com" rel="noreferrer" \
target="_blank">http://www.listbox.com</a><br /> ><br />
><br /><br /></div></div></blockquote></div><br /></div></html><div \
bgcolor="#ffffff" id="listbox-footer" \
style="width:auto;margin:0;padding:5px;background-color:#fff;clear:both;border-top: \
1px solid #ccc;"><table bgcolor="#ffffff" border="0" cellpadding="0" cellspacing="0" \
style="background-color:#fff" width="100%"><tr><td padding="4px"><font \
color="#333333" size="1" style="font-family:helvetica, sans-serif;"> \
<strong>illumos-developer</strong> | <a \
href="https://www.listbox.com/member/archive/182179/=now" \
style="text-decoration:none;color:#669933;border-bottom: 1px solid #444444" title="Go \
to archives for illumos-developer">Archives</a> <a border="0" \
href="https://www.listbox.com/member/archive/rss/182179/25758058-4e9228dc" \
style="text-decoration:none;color:#669933" title="RSS feed for \
illumos-developer"><img border="0" \
src="http://postlink.www.listbox.com/2067950/833487e62783d55fe81f119fb93ef644/25758058 \
/bb3fe179.jpg?uri=aHR0cHM6Ly93d3cubGlzdGJveC5jb20vaW1hZ2VzL2ZlZWQtaWNvbi0xMHgxMC5qcGc" \
/></a> | <a href="https://www.listbox.com/member/?member_id=25758058&id_secret=25758058-c19b436a" \
style="text-decoration:none;color:#669933;border-bottom: 1px solid #444444" \
title="">Modify</a> Your Subscription<td align="right" valign="top"><a \
href="http://www.listbox.com" style="border-bottom:none;"> <img border="0" \
src="http://postlink.www.listbox.com/2067951/3379085af0f1cf7fc3708f04b4471ae2/25758058 \
/bb3fe179.png?uri=aHR0cHM6Ly93d3cubGlzdGJveC5jb20vaW1hZ2VzL2xpc3Rib3gtbG9nby1zbWFsbC5wbmc" \
title="Powered by Listbox" /></a></td></font></td></tr></table></div></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic