Re: [PATCH] fs: affs: fix a NULL pointer dereference
On Fri, Mar 15, 2019 at 02:42:44AM -0500, Kangjie Lu wrote: > If affs_bread fails, do not use ext_bh to avoid NULL pointer > dereference > > Signed-off-by: Kangjie Lu Tanks for the patch. I'll need some more time to get familiar with the AFFS code to review your patch.
Re: [PATCH] fs: affs: fix a NULL pointer dereference
Hi Kangjie,
On Thu, Mar 14, 2019 at 8:47 AM Kangjie Lu wrote:
> If affs_bread fails, do not use ext_bh to avoid NULL pointer
> dereference
>
> Signed-off-by: Kangjie Lu
Thanks for your patch!
> --- a/fs/affs/file.c
> +++ b/fs/affs/file.c
> @@ -835,7 +835,7 @@ void
> affs_truncate(struct inode *inode)
> {
> struct super_block *sb = inode->i_sb;
> - u32 ext, ext_key;
> + u32 ext, ext_key, ext_bk;
Why adding an intermediate variable (without __be32 tag)?
> u32 last_blk, blkcnt, blk;
> u32 size;
> struct buffer_head *ext_bh;
> @@ -941,8 +941,12 @@ affs_truncate(struct inode *inode)
> size = AFFS_SB(sb)->s_hashsize;
> if (size > blkcnt - blk)
> size = blkcnt - blk;
> - for (i = 0; i < size; i++, blk++)
> - affs_free_block(sb, be32_to_cpu(AFFS_BLOCK(sb,
> ext_bh, i)));
> + if (ext_bh) {
> + for (i = 0; i < size; i++, blk++) {
> + ext_bk = AFFS_BLOCK(sb, ext_bh, i);
> + affs_free_block(sb, be32_to_cpu(ext_bk));
> + }
Now this ignores all errors, silently.
What about handling actual errors, and propagating them up?
> + }
> affs_free_block(sb, ext_key);
> ext_key = be32_to_cpu(AFFS_TAIL(sb, ext_bh)->extension);
> affs_brelse(ext_bh);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] fs: affs: fix a NULL pointer dereference
Hi Kangjie,
Thank you for the patch! Perhaps something to improve:
url:
https://github.com/0day-ci/linux/commits/Kangjie-Lu/fs-affs-fix-a-NULL-pointer-dereference/20190314-170334
New smatch warnings:
fs/affs/file.c:951 affs_truncate() error: we previously assumed 'ext_bh' could
be null (see line 944)
Old smatch warnings:
fs/affs/file.c:806 affs_write_end_ofs() warn: passing zero to 'PTR_ERR'
#
https://github.com/0day-ci/linux/commit/2ee20c56bd586ddaf3ebdb1c3cad26439edc9eb6
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 2ee20c56bd586ddaf3ebdb1c3cad26439edc9eb6
vim +/ext_bh +951 fs/affs/file.c
^1da177e4 Linus Torvalds 2005-04-16 833
^1da177e4 Linus Torvalds 2005-04-16 834 void
^1da177e4 Linus Torvalds 2005-04-16 835 affs_truncate(struct inode *inode)
^1da177e4 Linus Torvalds 2005-04-16 836 {
^1da177e4 Linus Torvalds 2005-04-16 837struct super_block *sb =
inode->i_sb;
2ee20c56b Kangjie Lu 2019-03-14 838u32 ext, ext_key, ext_bk;
^1da177e4 Linus Torvalds 2005-04-16 839u32 last_blk, blkcnt, blk;
^1da177e4 Linus Torvalds 2005-04-16 840u32 size;
^1da177e4 Linus Torvalds 2005-04-16 841struct buffer_head *ext_bh;
^1da177e4 Linus Torvalds 2005-04-16 842int i;
^1da177e4 Linus Torvalds 2005-04-16 843
08fe100d9 Geert Uytterhoeven 2015-02-17 844pr_debug("truncate(inode=%lu,
oldsize=%llu, newsize=%llu)\n",
08fe100d9 Geert Uytterhoeven 2015-02-17 845 inode->i_ino,
AFFS_I(inode)->mmu_private, inode->i_size);
^1da177e4 Linus Torvalds 2005-04-16 846
^1da177e4 Linus Torvalds 2005-04-16 847last_blk = 0;
^1da177e4 Linus Torvalds 2005-04-16 848ext = 0;
^1da177e4 Linus Torvalds 2005-04-16 849if (inode->i_size) {
^1da177e4 Linus Torvalds 2005-04-16 850last_blk =
((u32)inode->i_size - 1) / AFFS_SB(sb)->s_data_blksize;
^1da177e4 Linus Torvalds 2005-04-16 851ext = last_blk /
AFFS_SB(sb)->s_hashsize;
^1da177e4 Linus Torvalds 2005-04-16 852}
^1da177e4 Linus Torvalds 2005-04-16 853
^1da177e4 Linus Torvalds 2005-04-16 854if (inode->i_size >
AFFS_I(inode)->mmu_private) {
^1da177e4 Linus Torvalds 2005-04-16 855struct address_space
*mapping = inode->i_mapping;
^1da177e4 Linus Torvalds 2005-04-16 856struct page *page;
f2b6a16eb Nick Piggin2007-10-16 857void *fsdata;
73516ace9 Fabian Frederick 2014-10-13 858loff_t isize =
inode->i_size;
^1da177e4 Linus Torvalds 2005-04-16 859int res;
^1da177e4 Linus Torvalds 2005-04-16 860
73516ace9 Fabian Frederick 2014-10-13 861res =
mapping->a_ops->write_begin(NULL, mapping, isize, 0, 0, , );
^1da177e4 Linus Torvalds 2005-04-16 862if (!res)
73516ace9 Fabian Frederick 2014-10-13 863res =
mapping->a_ops->write_end(NULL, mapping, isize, 0, 0, page, fsdata);
dca3c3365 Roman Zippel 2008-04-29 864else
dca3c3365 Roman Zippel 2008-04-29 865inode->i_size =
AFFS_I(inode)->mmu_private;
^1da177e4 Linus Torvalds 2005-04-16 866mark_inode_dirty(inode);
^1da177e4 Linus Torvalds 2005-04-16 867return;
^1da177e4 Linus Torvalds 2005-04-16 868} else if (inode->i_size ==
AFFS_I(inode)->mmu_private)
^1da177e4 Linus Torvalds 2005-04-16 869return;
^1da177e4 Linus Torvalds 2005-04-16 870
^1da177e4 Linus Torvalds 2005-04-16 871// lock cache
^1da177e4 Linus Torvalds 2005-04-16 872ext_bh =
affs_get_extblock(inode, ext);
^1da177e4 Linus Torvalds 2005-04-16 873if (IS_ERR(ext_bh)) {
1ee54b099 Fabian Frederick 2014-12-12 874affs_warning(sb,
"truncate",
1ee54b099 Fabian Frederick 2014-12-12 875
"unexpected read error for ext block %u (%ld)",
08fe100d9 Geert Uytterhoeven 2015-02-17 876 ext,
PTR_ERR(ext_bh));
^1da177e4 Linus Torvalds 2005-04-16 877return;
^1da177e4 Linus Torvalds 2005-04-16 878}
^1da177e4 Linus Torvalds 2005-04-16 879if (AFFS_I(inode)->i_lc) {
^1da177e4 Linus Torvalds 2005-04-16 880/* clear linear cache */
^1da177e4 Linus Torvalds 2005-04-16 881i = (ext + 1) >>
AFFS_I(inode)->i_lc_shift;
^1da177e4 Linus Torvalds 2005-04-16 882if
(AFFS_I(inode)->i_lc_size > i) {
^1da177e4 Linus Torvalds 2005-04-16 883
AFFS_I(inode)->i_lc_size = i;
^1da177e4 Linus Torvalds 2005-04-16 884for (; i <
AFFS_LC_SIZE; i++)
^1da177e4 Linus Torvalds 2005-04-16 885
AFFS_I(inode)->i_lc[i] = 0;
^1da177e4 Linus Torvalds 2005-04-16 886}
^1da177e4 Linus Torvalds 2005-04-16 887/* clear associative
cache */
^1da177e4 Linus
[PATCH] fs: affs: fix a NULL pointer dereference
If affs_bread fails, do not use ext_bh to avoid NULL pointer
dereference
Signed-off-by: Kangjie Lu
---
fs/affs/file.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/affs/file.c b/fs/affs/file.c
index a85817f54483..29cbc8eda085 100644
--- a/fs/affs/file.c
+++ b/fs/affs/file.c
@@ -941,8 +941,10 @@ affs_truncate(struct inode *inode)
size = AFFS_SB(sb)->s_hashsize;
if (size > blkcnt - blk)
size = blkcnt - blk;
- for (i = 0; i < size; i++, blk++)
- affs_free_block(sb, be32_to_cpu(AFFS_BLOCK(sb, ext_bh,
i)));
+ if (ext_bh) {
+ for (i = 0; i < size; i++, blk++)
+ affs_free_block(sb, be32_to_cpu(AFFS_BLOCK(sb,
ext_bh, i)));
+ }
affs_free_block(sb, ext_key);
ext_key = be32_to_cpu(AFFS_TAIL(sb, ext_bh)->extension);
affs_brelse(ext_bh);
--
2.17.1
Re: [PATCH] fs: affs: fix a NULL pointer dereference
Hi Kangjie,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.0 next-20190306]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Kangjie-Lu/fs-affs-fix-a-NULL-pointer-dereference/20190314-170334
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
sparse warnings: (new ones prefixed by >>)
fs/affs/file.c:525:23: sparse: expression using sizeof(void)
fs/affs/file.c:525:23: sparse: expression using sizeof(void)
fs/affs/file.c:558:23: sparse: expression using sizeof(void)
fs/affs/file.c:558:23: sparse: expression using sizeof(void)
fs/affs/file.c:577:23: sparse: expression using sizeof(void)
fs/affs/file.c:577:23: sparse: expression using sizeof(void)
fs/affs/file.c:706:23: sparse: expression using sizeof(void)
fs/affs/file.c:706:23: sparse: expression using sizeof(void)
fs/affs/file.c:759:23: sparse: expression using sizeof(void)
fs/affs/file.c:759:23: sparse: expression using sizeof(void)
>> fs/affs/file.c:946:40: sparse: incorrect type in assignment (different base
>> types) @@expected unsigned int [unsigned] [usertype] ext_bk @@got
>> igned] [usertype] ext_bk @@
fs/affs/file.c:946:40:expected unsigned int [unsigned] [usertype] ext_bk
fs/affs/file.c:946:40:got restricted __be32
>> fs/affs/file.c:947:53: sparse: cast to restricted __be32
>> fs/affs/file.c:947:53: sparse: cast to restricted __be32
>> fs/affs/file.c:947:53: sparse: cast to restricted __be32
>> fs/affs/file.c:947:53: sparse: cast to restricted __be32
>> fs/affs/file.c:947:53: sparse: cast to restricted __be32
>> fs/affs/file.c:947:53: sparse: cast to restricted __be32
vim +946 fs/affs/file.c
833
834 void
835 affs_truncate(struct inode *inode)
836 {
837 struct super_block *sb = inode->i_sb;
838 u32 ext, ext_key, ext_bk;
839 u32 last_blk, blkcnt, blk;
840 u32 size;
841 struct buffer_head *ext_bh;
842 int i;
843
844 pr_debug("truncate(inode=%lu, oldsize=%llu, newsize=%llu)\n",
845 inode->i_ino, AFFS_I(inode)->mmu_private,
inode->i_size);
846
847 last_blk = 0;
848 ext = 0;
849 if (inode->i_size) {
850 last_blk = ((u32)inode->i_size - 1) /
AFFS_SB(sb)->s_data_blksize;
851 ext = last_blk / AFFS_SB(sb)->s_hashsize;
852 }
853
854 if (inode->i_size > AFFS_I(inode)->mmu_private) {
855 struct address_space *mapping = inode->i_mapping;
856 struct page *page;
857 void *fsdata;
858 loff_t isize = inode->i_size;
859 int res;
860
861 res = mapping->a_ops->write_begin(NULL, mapping, isize,
0, 0, , );
862 if (!res)
863 res = mapping->a_ops->write_end(NULL, mapping,
isize, 0, 0, page, fsdata);
864 else
865 inode->i_size = AFFS_I(inode)->mmu_private;
866 mark_inode_dirty(inode);
867 return;
868 } else if (inode->i_size == AFFS_I(inode)->mmu_private)
869 return;
870
871 // lock cache
872 ext_bh = affs_get_extblock(inode, ext);
873 if (IS_ERR(ext_bh)) {
874 affs_warning(sb, "truncate",
875 "unexpected read error for ext block %u
(%ld)",
876 ext, PTR_ERR(ext_bh));
877 return;
878 }
879 if (AFFS_I(inode)->i_lc) {
880 /* clear linear cache */
881 i = (ext + 1) >> AFFS_I(inode)->i_lc_shift;
882 if (AFFS_I(inode)->i_lc_size > i) {
883 AFFS_I(inode)->i_lc_size = i;
884 for (; i < AFFS_LC_SIZE; i++)
885 AFFS_I(inode)->i_lc[i] = 0;
886 }
887 /* clear associative cache */
888 for (i = 0; i < AFFS_AC_SIZE; i++)
889 if (AFFS_I(inode)->i_ac[i].ext >= ext)
890 AFFS_I(inode)->i_ac[i].ext = 0;
891 }
892 ext_key = be32_to_cpu(AFFS_TAIL(sb, ext_bh)->extension);
893
894 blkcnt = AFFS_I(inode)->i_blkcnt;
895 i = 0;
896 blk = last_blk;
897 if (inode->i_size) {
898 i = last_blk % AFFS_SB(sb)->s_hashsize + 1;
899 blk++;
900 } else
901
[PATCH] fs: affs: fix a NULL pointer dereference
If affs_bread fails, do not use ext_bh to avoid NULL pointer
dereference
Signed-off-by: Kangjie Lu
---
fs/affs/file.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/fs/affs/file.c b/fs/affs/file.c
index a85817f54483..45b96faa40f1 100644
--- a/fs/affs/file.c
+++ b/fs/affs/file.c
@@ -835,7 +835,7 @@ void
affs_truncate(struct inode *inode)
{
struct super_block *sb = inode->i_sb;
- u32 ext, ext_key;
+ u32 ext, ext_key, ext_bk;
u32 last_blk, blkcnt, blk;
u32 size;
struct buffer_head *ext_bh;
@@ -941,8 +941,12 @@ affs_truncate(struct inode *inode)
size = AFFS_SB(sb)->s_hashsize;
if (size > blkcnt - blk)
size = blkcnt - blk;
- for (i = 0; i < size; i++, blk++)
- affs_free_block(sb, be32_to_cpu(AFFS_BLOCK(sb, ext_bh,
i)));
+ if (ext_bh) {
+ for (i = 0; i < size; i++, blk++) {
+ ext_bk = AFFS_BLOCK(sb, ext_bh, i);
+ affs_free_block(sb, be32_to_cpu(ext_bk));
+ }
+ }
affs_free_block(sb, ext_key);
ext_key = be32_to_cpu(AFFS_TAIL(sb, ext_bh)->extension);
affs_brelse(ext_bh);
--
2.17.1
