* [PATCH v2] udf: reduce leakage of blocks related to named streams
@ 2019-08-14 12:50 Steven J. Magnani
2019-08-15 12:42 ` Jan Kara
0 siblings, 1 reply; 4+ messages in thread
From: Steven J. Magnani @ 2019-08-14 12:50 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-kernel, linux-fsdevel, Steven J . Magnani
Windows is capable of creating UDF files having named streams.
One example is the "Zone.Identifier" stream attached automatically
to files downloaded from a network. See:
https://msdn.microsoft.com/en-us/library/dn392609.aspx
Modification of a file having one or more named streams in Linux causes
the stream directory to become detached from the file, essentially leaking
all blocks pertaining to the file's streams.
Fix by saving off information about an inode's streams when reading it,
for later use when its on-disk data is updated.
Changes from v1:
Remove modifications that would limit leakage of all inode blocks
on deletion.
This restricts the patch to preservation of stream data during inode
modification.
Signed-off-by: Steven J. Magnani <steve@digidescorp.com>
--- a/fs/udf/udf_i.h 2019-07-26 11:35:28.257563879 -0500
+++ b/fs/udf/udf_i.h 2019-08-06 14:35:55.579654263 -0500
@@ -42,12 +42,15 @@ struct udf_inode_info {
unsigned i_efe : 1; /* extendedFileEntry */
unsigned i_use : 1; /* unallocSpaceEntry */
unsigned i_strat4096 : 1;
- unsigned reserved : 26;
+ unsigned i_streamdir : 1;
+ unsigned reserved : 25;
union {
struct short_ad *i_sad;
struct long_ad *i_lad;
__u8 *i_data;
} i_ext;
+ struct kernel_lb_addr i_locStreamdir;
+ __u64 i_lenStreams;
struct rw_semaphore i_data_sem;
struct udf_ext_cache cached_extent;
/* Spinlock for protecting extent cache */
--- a/fs/udf/super.c 2019-07-26 11:35:28.253563792 -0500
+++ b/fs/udf/super.c 2019-08-06 15:04:30.851086957 -0500
@@ -151,9 +151,13 @@ static struct inode *udf_alloc_inode(str
ei->i_unique = 0;
ei->i_lenExtents = 0;
+ ei->i_lenStreams = 0;
ei->i_next_alloc_block = 0;
ei->i_next_alloc_goal = 0;
ei->i_strat4096 = 0;
+ ei->i_streamdir = 0;
+ ei->i_locStreamdir.logicalBlockNum = 0xFFFFFFFF;
+ ei->i_locStreamdir.partitionReferenceNum = 0xFFFF;
init_rwsem(&ei->i_data_sem);
ei->cached_extent.lstart = -1;
spin_lock_init(&ei->i_extent_cache_lock);
--- a/fs/udf/inode.c 2019-07-26 11:35:28.253563792 -0500
+++ b/fs/udf/inode.c 2019-08-06 15:04:30.851086957 -0500
@@ -1485,6 +1485,10 @@ reread:
iinfo->i_lenEAttr = le32_to_cpu(fe->lengthExtendedAttr);
iinfo->i_lenAlloc = le32_to_cpu(fe->lengthAllocDescs);
iinfo->i_checkpoint = le32_to_cpu(fe->checkpoint);
+ iinfo->i_streamdir = 0;
+ iinfo->i_lenStreams = 0;
+ iinfo->i_locStreamdir.logicalBlockNum = 0xFFFFFFFF;
+ iinfo->i_locStreamdir.partitionReferenceNum = 0xFFFF;
} else {
inode->i_blocks = le64_to_cpu(efe->logicalBlocksRecorded) <<
(inode->i_sb->s_blocksize_bits - 9);
@@ -1498,6 +1502,16 @@ reread:
iinfo->i_lenEAttr = le32_to_cpu(efe->lengthExtendedAttr);
iinfo->i_lenAlloc = le32_to_cpu(efe->lengthAllocDescs);
iinfo->i_checkpoint = le32_to_cpu(efe->checkpoint);
+
+ /* Named streams */
+ iinfo->i_streamdir = (efe->streamDirectoryICB.extLength != 0);
+ iinfo->i_locStreamdir =
+ lelb_to_cpu(efe->streamDirectoryICB.extLocation);
+ iinfo->i_lenStreams = le64_to_cpu(efe->objectSize);
+ if (iinfo->i_lenStreams >= inode->i_size)
+ iinfo->i_lenStreams -= inode->i_size;
+ else
+ iinfo->i_lenStreams = 0;
}
inode->i_generation = iinfo->i_unique;
@@ -1760,9 +1774,19 @@ static int udf_update_inode(struct inode
iinfo->i_ext.i_data,
inode->i_sb->s_blocksize -
sizeof(struct extendedFileEntry));
- efe->objectSize = cpu_to_le64(inode->i_size);
+ efe->objectSize =
+ cpu_to_le64(inode->i_size + iinfo->i_lenStreams);
efe->logicalBlocksRecorded = cpu_to_le64(lb_recorded);
+ if (iinfo->i_streamdir) {
+ struct long_ad *icb_lad = &efe->streamDirectoryICB;
+
+ icb_lad->extLocation =
+ cpu_to_lelb(iinfo->i_locStreamdir);
+ icb_lad->extLength =
+ cpu_to_le32(inode->i_sb->s_blocksize);
+ }
+
udf_adjust_time(iinfo, inode->i_atime);
udf_adjust_time(iinfo, inode->i_mtime);
udf_adjust_time(iinfo, inode->i_ctime);
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] udf: reduce leakage of blocks related to named streams
2019-08-14 12:50 [PATCH v2] udf: reduce leakage of blocks related to named streams Steven J. Magnani
@ 2019-08-15 12:42 ` Jan Kara
2019-08-19 12:10 ` Steve Magnani
0 siblings, 1 reply; 4+ messages in thread
From: Jan Kara @ 2019-08-15 12:42 UTC (permalink / raw)
To: Steven J. Magnani
Cc: Jan Kara, Steven J . Magnani, linux-fsdevel, linux-kernel
On Wed 14-08-19 07:50:02, Steven J. Magnani wrote:
> Windows is capable of creating UDF files having named streams.
> One example is the "Zone.Identifier" stream attached automatically
> to files downloaded from a network. See:
> https://msdn.microsoft.com/en-us/library/dn392609.aspx
>
> Modification of a file having one or more named streams in Linux causes
> the stream directory to become detached from the file, essentially leaking
> all blocks pertaining to the file's streams.
>
> Fix by saving off information about an inode's streams when reading it,
> for later use when its on-disk data is updated.
Thanks for the patch! I agree with the idea of this patch. Just some
small comments below.
> Changes from v1:
> Remove modifications that would limit leakage of all inode blocks
> on deletion.
> This restricts the patch to preservation of stream data during inode
> modification.
Please put patch changelog below Signed-off-by and --- delimiter so that it
does not get included in the final commit message when I do git-am.
> Signed-off-by: Steven J. Magnani <steve@digidescorp.com>
>
> --- a/fs/udf/udf_i.h 2019-07-26 11:35:28.257563879 -0500
> +++ b/fs/udf/udf_i.h 2019-08-06 14:35:55.579654263 -0500
> @@ -42,12 +42,15 @@ struct udf_inode_info {
> unsigned i_efe : 1; /* extendedFileEntry */
> unsigned i_use : 1; /* unallocSpaceEntry */
> unsigned i_strat4096 : 1;
> - unsigned reserved : 26;
> + unsigned i_streamdir : 1;
> + unsigned reserved : 25;
> union {
> struct short_ad *i_sad;
> struct long_ad *i_lad;
> __u8 *i_data;
> } i_ext;
> + struct kernel_lb_addr i_locStreamdir;
> + __u64 i_lenStreams;
> struct rw_semaphore i_data_sem;
> struct udf_ext_cache cached_extent;
> /* Spinlock for protecting extent cache */
> --- a/fs/udf/super.c 2019-07-26 11:35:28.253563792 -0500
> +++ b/fs/udf/super.c 2019-08-06 15:04:30.851086957 -0500
> @@ -151,9 +151,13 @@ static struct inode *udf_alloc_inode(str
>
> ei->i_unique = 0;
> ei->i_lenExtents = 0;
> + ei->i_lenStreams = 0;
> ei->i_next_alloc_block = 0;
> ei->i_next_alloc_goal = 0;
> ei->i_strat4096 = 0;
> + ei->i_streamdir = 0;
> + ei->i_locStreamdir.logicalBlockNum = 0xFFFFFFFF;
> + ei->i_locStreamdir.partitionReferenceNum = 0xFFFF;
I don't think you need to initialize i_locStreamdir when i_streamdir is
already set to 0...
> init_rwsem(&ei->i_data_sem);
> ei->cached_extent.lstart = -1;
> spin_lock_init(&ei->i_extent_cache_lock);
> --- a/fs/udf/inode.c 2019-07-26 11:35:28.253563792 -0500
> +++ b/fs/udf/inode.c 2019-08-06 15:04:30.851086957 -0500
> @@ -1485,6 +1485,10 @@ reread:
> iinfo->i_lenEAttr = le32_to_cpu(fe->lengthExtendedAttr);
> iinfo->i_lenAlloc = le32_to_cpu(fe->lengthAllocDescs);
> iinfo->i_checkpoint = le32_to_cpu(fe->checkpoint);
> + iinfo->i_streamdir = 0;
> + iinfo->i_lenStreams = 0;
> + iinfo->i_locStreamdir.logicalBlockNum = 0xFFFFFFFF;
> + iinfo->i_locStreamdir.partitionReferenceNum = 0xFFFF;
Ditto here...
> } else {
> inode->i_blocks = le64_to_cpu(efe->logicalBlocksRecorded) <<
> (inode->i_sb->s_blocksize_bits - 9);
> @@ -1498,6 +1502,16 @@ reread:
> iinfo->i_lenEAttr = le32_to_cpu(efe->lengthExtendedAttr);
> iinfo->i_lenAlloc = le32_to_cpu(efe->lengthAllocDescs);
> iinfo->i_checkpoint = le32_to_cpu(efe->checkpoint);
> +
> + /* Named streams */
> + iinfo->i_streamdir = (efe->streamDirectoryICB.extLength != 0);
> + iinfo->i_locStreamdir =
> + lelb_to_cpu(efe->streamDirectoryICB.extLocation);
> + iinfo->i_lenStreams = le64_to_cpu(efe->objectSize);
> + if (iinfo->i_lenStreams >= inode->i_size)
> + iinfo->i_lenStreams -= inode->i_size;
> + else
> + iinfo->i_lenStreams = 0;
Hum, maybe you could just have i_objectSize instead of i_lenStreams? You
use the field just to preserve objectSize anyway so there's no point in
complicating it.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] udf: reduce leakage of blocks related to named streams
2019-08-15 12:42 ` Jan Kara
@ 2019-08-19 12:10 ` Steve Magnani
2019-08-26 9:14 ` Jan Kara
0 siblings, 1 reply; 4+ messages in thread
From: Steve Magnani @ 2019-08-19 12:10 UTC (permalink / raw)
To: Jan Kara; +Cc: Jan Kara, Steven J . Magnani, linux-fsdevel, linux-kernel
Jan -
On 8/15/19 7:42 AM, Jan Kara wrote:
> On Wed 14-08-19 07:50:02, Steven J. Magnani wrote:
>> Windows is capable of creating UDF files having named streams.
>> One example is the "Zone.Identifier" stream attached automatically
>> to files downloaded from a network. See:
>> https://msdn.microsoft.com/en-us/library/dn392609.aspx
>>
>> Modification of a file having one or more named streams in Linux causes
>> the stream directory to become detached from the file, essentially leaking
>> all blocks pertaining to the file's streams.
>>
>> Fix by saving off information about an inode's streams when reading it,
>> for later use when its on-disk data is updated.
>> <snip>
>> } else {
>> inode->i_blocks = le64_to_cpu(efe->logicalBlocksRecorded) <<
>> (inode->i_sb->s_blocksize_bits - 9);
>> @@ -1498,6 +1502,16 @@ reread:
>> iinfo->i_lenEAttr = le32_to_cpu(efe->lengthExtendedAttr);
>> iinfo->i_lenAlloc = le32_to_cpu(efe->lengthAllocDescs);
>> iinfo->i_checkpoint = le32_to_cpu(efe->checkpoint);
>> +
>> + /* Named streams */
>> + iinfo->i_streamdir = (efe->streamDirectoryICB.extLength != 0);
>> + iinfo->i_locStreamdir =
>> + lelb_to_cpu(efe->streamDirectoryICB.extLocation);
>> + iinfo->i_lenStreams = le64_to_cpu(efe->objectSize);
>> + if (iinfo->i_lenStreams >= inode->i_size)
>> + iinfo->i_lenStreams -= inode->i_size;
>> + else
>> + iinfo->i_lenStreams = 0;
> Hum, maybe you could just have i_objectSize instead of i_lenStreams? You
> use the field just to preserve objectSize anyway so there's no point in
> complicating it.
>
I started making this change and found that it actually complicates things more,
by forcing the driver to update i_objectSize everywhere that i_size is changed.
Are you sure this is what you want?
------------------------------------------------------------------------
Steven J. Magnani "I claim this network for MARS!
www.digidescorp.com Earthling, return my space modulator!"
#include <standard.disclaimer>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] udf: reduce leakage of blocks related to named streams
2019-08-19 12:10 ` Steve Magnani
@ 2019-08-26 9:14 ` Jan Kara
0 siblings, 0 replies; 4+ messages in thread
From: Jan Kara @ 2019-08-26 9:14 UTC (permalink / raw)
To: Steve Magnani
Cc: Jan Kara, Jan Kara, Steven J . Magnani, linux-fsdevel, linux-kernel
On Mon 19-08-19 07:10:24, Steve Magnani wrote:
> Jan -
>
>
> On 8/15/19 7:42 AM, Jan Kara wrote:
> > On Wed 14-08-19 07:50:02, Steven J. Magnani wrote:
> > > Windows is capable of creating UDF files having named streams.
> > > One example is the "Zone.Identifier" stream attached automatically
> > > to files downloaded from a network. See:
> > > https://msdn.microsoft.com/en-us/library/dn392609.aspx
> > >
> > > Modification of a file having one or more named streams in Linux causes
> > > the stream directory to become detached from the file, essentially leaking
> > > all blocks pertaining to the file's streams.
> > >
> > > Fix by saving off information about an inode's streams when reading it,
> > > for later use when its on-disk data is updated.
> > > <snip>
> > > } else {
> > > inode->i_blocks = le64_to_cpu(efe->logicalBlocksRecorded) <<
> > > (inode->i_sb->s_blocksize_bits - 9);
> > > @@ -1498,6 +1502,16 @@ reread:
> > > iinfo->i_lenEAttr = le32_to_cpu(efe->lengthExtendedAttr);
> > > iinfo->i_lenAlloc = le32_to_cpu(efe->lengthAllocDescs);
> > > iinfo->i_checkpoint = le32_to_cpu(efe->checkpoint);
> > > +
> > > + /* Named streams */
> > > + iinfo->i_streamdir = (efe->streamDirectoryICB.extLength != 0);
> > > + iinfo->i_locStreamdir =
> > > + lelb_to_cpu(efe->streamDirectoryICB.extLocation);
> > > + iinfo->i_lenStreams = le64_to_cpu(efe->objectSize);
> > > + if (iinfo->i_lenStreams >= inode->i_size)
> > > + iinfo->i_lenStreams -= inode->i_size;
> > > + else
> > > + iinfo->i_lenStreams = 0;
> > Hum, maybe you could just have i_objectSize instead of i_lenStreams? You
> > use the field just to preserve objectSize anyway so there's no point in
> > complicating it.
> >
>
> I started making this change and found that it actually complicates things more,
> by forcing the driver to update i_objectSize everywhere that i_size is changed.
> Are you sure this is what you want?
Aha, that's a good point! No, in that case what you did was better. I'll
just take your v2 patch then, I can make the other minor adjustments I was
suggesting when applying the patch. Thanks for looking into this!
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-08-26 9:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14 12:50 [PATCH v2] udf: reduce leakage of blocks related to named streams Steven J. Magnani
2019-08-15 12:42 ` Jan Kara
2019-08-19 12:10 ` Steve Magnani
2019-08-26 9:14 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).