* [PATCH] ext4: Don't leak old mountpoint samples @ 2020-12-01 15:13 Richard Weinberger 2020-12-09 23:24 ` harshad shirwadkar 2020-12-17 18:27 ` Theodore Y. Ts'o 0 siblings, 2 replies; 5+ messages in thread From: Richard Weinberger @ 2020-12-01 15:13 UTC (permalink / raw) To: tytso; +Cc: adilger.kernel, linux-ext4, linux-kernel, Richard Weinberger As soon the first file is opened, ext4 samples the mountpoint of the filesystem in 64 bytes of the super block. It does so using strlcpy(), this means that the remaining bytes in the super block string buffer are untouched. If the mount point before had a longer path than the current one, it can be reconstructed. Consider the case where the fs was mounted to "/media/johnjdeveloper" and later to "/". The the super block buffer then contains "/\x00edia/johnjdeveloper". This case was seen in the wild and caused confusion how the name of a developer ands up on the super block of a filesystem used in production... Fix this by clearing the string buffer before writing to it, Signed-off-by: Richard Weinberger <richard@nod.at> --- fs/ext4/file.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/ext4/file.c b/fs/ext4/file.c index 3ed8c048fb12..dba521250d01 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -809,6 +809,7 @@ static int ext4_sample_last_mounted(struct super_block *sb, err = ext4_journal_get_write_access(handle, sbi->s_sbh); if (err) goto out_journal; + memset(sbi->s_es->s_last_mounted, 0x00, sizeof(sbi->s_es->s_last_mounted)); strlcpy(sbi->s_es->s_last_mounted, cp, sizeof(sbi->s_es->s_last_mounted)); ext4_handle_dirty_super(handle, sb); -- 2.26.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: Don't leak old mountpoint samples 2020-12-01 15:13 [PATCH] ext4: Don't leak old mountpoint samples Richard Weinberger @ 2020-12-09 23:24 ` harshad shirwadkar 2020-12-17 18:27 ` Theodore Y. Ts'o 1 sibling, 0 replies; 5+ messages in thread From: harshad shirwadkar @ 2020-12-09 23:24 UTC (permalink / raw) To: Richard Weinberger Cc: Theodore Y. Ts'o, Andreas Dilger, Ext4 Developers List, linux-kernel Thanks for the patch Richard, it looks good to me. Reviewed-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com> On Tue, Dec 1, 2020 at 7:29 AM Richard Weinberger <richard@nod.at> wrote: > > As soon the first file is opened, ext4 samples the mountpoint > of the filesystem in 64 bytes of the super block. > It does so using strlcpy(), this means that the remaining bytes > in the super block string buffer are untouched. > If the mount point before had a longer path than the current one, > it can be reconstructed. > > Consider the case where the fs was mounted to "/media/johnjdeveloper" > and later to "/". > The the super block buffer then contains "/\x00edia/johnjdeveloper". > > This case was seen in the wild and caused confusion how the name > of a developer ands up on the super block of a filesystem used > in production... > > Fix this by clearing the string buffer before writing to it, > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > fs/ext4/file.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > index 3ed8c048fb12..dba521250d01 100644 > --- a/fs/ext4/file.c > +++ b/fs/ext4/file.c > @@ -809,6 +809,7 @@ static int ext4_sample_last_mounted(struct super_block *sb, > err = ext4_journal_get_write_access(handle, sbi->s_sbh); > if (err) > goto out_journal; > + memset(sbi->s_es->s_last_mounted, 0x00, sizeof(sbi->s_es->s_last_mounted)); > strlcpy(sbi->s_es->s_last_mounted, cp, > sizeof(sbi->s_es->s_last_mounted)); > ext4_handle_dirty_super(handle, sb); > -- > 2.26.2 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: Don't leak old mountpoint samples 2020-12-01 15:13 [PATCH] ext4: Don't leak old mountpoint samples Richard Weinberger 2020-12-09 23:24 ` harshad shirwadkar @ 2020-12-17 18:27 ` Theodore Y. Ts'o 2020-12-17 21:09 ` Andreas Dilger 1 sibling, 1 reply; 5+ messages in thread From: Theodore Y. Ts'o @ 2020-12-17 18:27 UTC (permalink / raw) To: Richard Weinberger; +Cc: adilger.kernel, linux-ext4, linux-kernel On Tue, Dec 01, 2020 at 04:13:01PM +0100, Richard Weinberger wrote: > As soon the first file is opened, ext4 samples the mountpoint > of the filesystem in 64 bytes of the super block. > It does so using strlcpy(), this means that the remaining bytes > in the super block string buffer are untouched. > If the mount point before had a longer path than the current one, > it can be reconstructed. > > Consider the case where the fs was mounted to "/media/johnjdeveloper" > and later to "/". > The the super block buffer then contains "/\x00edia/johnjdeveloper". > > This case was seen in the wild and caused confusion how the name > of a developer ands up on the super block of a filesystem used > in production... > > Fix this by clearing the string buffer before writing to it, > > Signed-off-by: Richard Weinberger <richard@nod.at> Thank for reporting this issue. In fact, the better fix is to use strncpy(). See my revised patch for an explanation of why.... commit cdc9ad7d3f201a77749432878fb4caa490862de6 Author: Theodore Ts'o <tytso@mit.edu> Date: Thu Dec 17 13:24:15 2020 -0500 ext4: don't leak old mountpoint samples When the first file is opened, ext4 samples the mountpoint of the filesystem in 64 bytes of the super block. It does so using strlcpy(), this means that the remaining bytes in the super block string buffer are untouched. If the mount point before had a longer path than the current one, it can be reconstructed. Consider the case where the fs was mounted to "/media/johnjdeveloper" and later to "/". The super block buffer then contains "/\x00edia/johnjdeveloper". This case was seen in the wild and caused confusion how the name of a developer ands up on the super block of a filesystem used in production... Fix this by using strncpy() instead of strlcpy(). The superblock field is defined to be a fixed-size char array, and it is already marked using __nonstring in fs/ext4/ext4.h. The consumer of the field in e2fsprogs already assumes that in the case of a 64+ byte mount path, that s_last_mounted will not be NUL terminated. Reported-by: Richard Weinberger <richard@nod.at> Signed-off-by: Theodore Ts'o <tytso@mit.edu> diff --git a/fs/ext4/file.c b/fs/ext4/file.c index 1cd3d26e3217..349b27f0dda0 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -810,7 +810,7 @@ static int ext4_sample_last_mounted(struct super_block *sb, if (err) goto out_journal; lock_buffer(sbi->s_sbh); - strlcpy(sbi->s_es->s_last_mounted, cp, + strncpy(sbi->s_es->s_last_mounted, cp, sizeof(sbi->s_es->s_last_mounted)); ext4_superblock_csum_set(sb); unlock_buffer(sbi->s_sbh); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: Don't leak old mountpoint samples 2020-12-17 18:27 ` Theodore Y. Ts'o @ 2020-12-17 21:09 ` Andreas Dilger 2020-12-17 21:15 ` Richard Weinberger 0 siblings, 1 reply; 5+ messages in thread From: Andreas Dilger @ 2020-12-17 21:09 UTC (permalink / raw) To: Theodore Y. Ts'o Cc: Richard Weinberger, Ext4 Developers List, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 3327 bytes --] On Dec 17, 2020, at 11:27 AM, Theodore Y. Ts'o <tytso@mit.edu> wrote: > > On Tue, Dec 01, 2020 at 04:13:01PM +0100, Richard Weinberger wrote: >> As soon the first file is opened, ext4 samples the mountpoint >> of the filesystem in 64 bytes of the super block. >> It does so using strlcpy(), this means that the remaining bytes >> in the super block string buffer are untouched. >> If the mount point before had a longer path than the current one, >> it can be reconstructed. >> >> Consider the case where the fs was mounted to "/media/johnjdeveloper" >> and later to "/". >> The the super block buffer then contains "/\x00edia/johnjdeveloper". >> >> This case was seen in the wild and caused confusion how the name >> of a developer ands up on the super block of a filesystem used >> in production... >> >> Fix this by clearing the string buffer before writing to it, >> >> Signed-off-by: Richard Weinberger <richard@nod.at> > > Thank for reporting this issue. In fact, the better fix is to use > strncpy(). See my revised patch for an explanation of why.... > > commit cdc9ad7d3f201a77749432878fb4caa490862de6 > Author: Theodore Ts'o <tytso@mit.edu> > Date: Thu Dec 17 13:24:15 2020 -0500 > > ext4: don't leak old mountpoint samples > > When the first file is opened, ext4 samples the mountpoint of the > filesystem in 64 bytes of the super block. It does so using > strlcpy(), this means that the remaining bytes in the super block > string buffer are untouched. If the mount point before had a longer > path than the current one, it can be reconstructed. > > Consider the case where the fs was mounted to "/media/johnjdeveloper" > and later to "/". The super block buffer then contains > "/\x00edia/johnjdeveloper". > > This case was seen in the wild and caused confusion how the name > of a developer ands up on the super block of a filesystem used > in production... > > Fix this by using strncpy() instead of strlcpy(). The superblock > field is defined to be a fixed-size char array, and it is already > marked using __nonstring in fs/ext4/ext4.h. The consumer of the field > in e2fsprogs already assumes that in the case of a 64+ byte mount > path, that s_last_mounted will not be NUL terminated. > > Reported-by: Richard Weinberger <richard@nod.at> > Signed-off-by: Theodore Ts'o <tytso@mit.edu> Color me confused, but I don't see how this change makes any difference? If "cp" is only "/" then it will *still* not overwrite "edia/johnjdeveloper" at the end of the s_last_mounted array. To my mind, the only difference between using strlcpy() and strncpy() would be whether the last byte in the array can be used or not, but doesn't affect the remaining bytes. > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > index 1cd3d26e3217..349b27f0dda0 100644 > --- a/fs/ext4/file.c > +++ b/fs/ext4/file.c > @@ -810,7 +810,7 @@ static int ext4_sample_last_mounted(struct super_block *sb, > if (err) > goto out_journal; > lock_buffer(sbi->s_sbh); > - strlcpy(sbi->s_es->s_last_mounted, cp, > + strncpy(sbi->s_es->s_last_mounted, cp, > sizeof(sbi->s_es->s_last_mounted)); > ext4_superblock_csum_set(sb); > unlock_buffer(sbi->s_sbh); Cheers, Andreas [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 873 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: Don't leak old mountpoint samples 2020-12-17 21:09 ` Andreas Dilger @ 2020-12-17 21:15 ` Richard Weinberger 0 siblings, 0 replies; 5+ messages in thread From: Richard Weinberger @ 2020-12-17 21:15 UTC (permalink / raw) To: Andreas Dilger; +Cc: tytso, linux-ext4, linux-kernel ----- Ursprüngliche Mail ----- >> Fix this by using strncpy() instead of strlcpy(). The superblock >> field is defined to be a fixed-size char array, and it is already >> marked using __nonstring in fs/ext4/ext4.h. The consumer of the field >> in e2fsprogs already assumes that in the case of a 64+ byte mount >> path, that s_last_mounted will not be NUL terminated. >> >> Reported-by: Richard Weinberger <richard@nod.at> >> Signed-off-by: Theodore Ts'o <tytso@mit.edu> > > Color me confused, but I don't see how this change makes any difference? > If "cp" is only "/" then it will *still* not overwrite "edia/johnjdeveloper" > at the end of the s_last_mounted array. To my mind, the only difference > between using strlcpy() and strncpy() would be whether the last byte in > the array can be used or not, but doesn't affect the remaining bytes. strncpy() zeros all remaining bytes. Thanks, //richard ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-17 21:26 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-12-01 15:13 [PATCH] ext4: Don't leak old mountpoint samples Richard Weinberger 2020-12-09 23:24 ` harshad shirwadkar 2020-12-17 18:27 ` Theodore Y. Ts'o 2020-12-17 21:09 ` Andreas Dilger 2020-12-17 21:15 ` Richard Weinberger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox