| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240923142208.qjqi2hix4g7v2dmv@quack3>
Date: Mon, 23 Sep 2024 16:22:08 +0200
From: Jan Kara <jack@...e.cz>
To: Gianfranco Trad <s323713@...denti.polito.it>
Cc: kernel test robot <lkp@...el.com>, oe-kbuild@...ts.linux.dev,
Dan Carpenter <error27@...il.com>, jack@...e.com,
linux-kernel@...r.kernel.org,
syzbot+8901c4560b7ab5c2f9df@...kaller.appspotmail.com
Subject: Re: [PATCH] udf: fix uninit-value use in udf_get_fileshortad
On Mon 23-09-24 11:23:48, Gianfranco Trad wrote:
> Hello,
> On 23/09/24 02:26, kernel test robot wrote:
> > BCC: lkp@...el.com
> > CC: oe-kbuild-all@...ts.linux.dev
> > In-Reply-To: <20240919195227.412583-1-gianf.trad@...il.com>
> > References: <20240919195227.412583-1-gianf.trad@...il.com>
> > TO: Gianfranco Trad <gianf.trad@...il.com>
> > TO: jack@...e.com
> > CC: linux-kernel@...r.kernel.org
> > CC: skhan@...uxfoundation.org
> > CC: Gianfranco Trad <gianf.trad@...il.com>
> > CC: syzbot+8901c4560b7ab5c2f9df@...kaller.appspotmail.com
> >
> > Hi Gianfranco,
> >
> > kernel test robot noticed the following build warnings:
> >
> > [auto build test WARNING on brauner-vfs/vfs.all]
> > [also build test WARNING on linus/master v6.11 next-20240920]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Gianfranco-Trad/udf-fix-uninit-value-use-in-udf_get_fileshortad/20240920-035519
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
> > patch link: https://lore.kernel.org/r/20240919195227.412583-1-gianf.trad%40gmail.com
> > patch subject: [PATCH] udf: fix uninit-value use in udf_get_fileshortad
> > :::::: branch date: 3 days ago
> > :::::: commit date: 3 days ago
> > config: mips-randconfig-r073-20240922 (https://download.01.org/0day-ci/archive/20240923/202409230809.5ozgjLm9-lkp@intel.com/config)
> > compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@...el.com>
> > | Reported-by: Dan Carpenter <error27@...il.com>
> > | Closes: https://lore.kernel.org/r/202409230809.5ozgjLm9-lkp@intel.com/
> >
> > smatch warnings:
> > fs/udf/inode.c:2223 udf_current_aext() error: we previously assumed 'epos->bh' could be null (see line 2204)
> Given this error my initial guess is to update the patch to check the
> assumption of epos->bh not null.
Yes, epos->bh can definitely be NULL here. Generally the check looks really
suspicious. I was trying to find your original patch but for some reason
neither LKML archive nor my mailbox have it. What is the motivation of the
added check? Are you checking for overflow on alen? In that case it would
be best to just use check_add_overflow() helper when computing alen few
lines above...
Honza
> > vim +2223 fs/udf/inode.c
> >
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2193
> > ff116fc8d1d439 Jan Kara 2007-05-08 2194 int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
> > 5ca4e4be841e38 Pekka Enberg 2008-10-15 2195 struct kernel_lb_addr *eloc, uint32_t *elen, int inc)
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2196 {
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2197 int alen;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2198 int8_t etype;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2199 uint8_t *ptr;
> > 5ca4e4be841e38 Pekka Enberg 2008-10-15 2200 struct short_ad *sad;
> > 5ca4e4be841e38 Pekka Enberg 2008-10-15 2201 struct long_ad *lad;
> > 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2202 struct udf_inode_info *iinfo = UDF_I(inode);
> > 28de7948a89676 Cyrill Gorcunov 2007-07-21 2203
> > cb00ea3528eb3c Cyrill Gorcunov 2007-07-19 @2204 if (!epos->bh) {
> > ff116fc8d1d439 Jan Kara 2007-05-08 2205 if (!epos->offset)
> > ff116fc8d1d439 Jan Kara 2007-05-08 2206 epos->offset = udf_file_entry_alloc_offset(inode);
> > 382a2287bf9cd2 Jan Kara 2020-09-25 2207 ptr = iinfo->i_data + epos->offset -
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2208 udf_file_entry_alloc_offset(inode) +
> > 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2209 iinfo->i_lenEAttr;
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2210 alen = udf_file_entry_alloc_offset(inode) +
> > 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2211 iinfo->i_lenAlloc;
> > cb00ea3528eb3c Cyrill Gorcunov 2007-07-19 2212 } else {
> > ff116fc8d1d439 Jan Kara 2007-05-08 2213 if (!epos->offset)
> > ff116fc8d1d439 Jan Kara 2007-05-08 2214 epos->offset = sizeof(struct allocExtDesc);
> > ff116fc8d1d439 Jan Kara 2007-05-08 2215 ptr = epos->bh->b_data + epos->offset;
> > 28de7948a89676 Cyrill Gorcunov 2007-07-21 2216 alen = sizeof(struct allocExtDesc) +
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2217 le32_to_cpu(((struct allocExtDesc *)epos->bh->b_data)->
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2218 lengthAllocDescs);
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2219 }
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2220
> > 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2221 switch (iinfo->i_alloc_type) {
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2222 case ICBTAG_FLAG_AD_SHORT:
> > 2aa242080b8dda Gianfranco Trad 2024-09-19 @2223 if (unlikely(alen < 0 && epos->offset == epos->bh->b_size))
> > 2aa242080b8dda Gianfranco Trad 2024-09-19 2224 return -1;
> The check would be inserted up here in the if clause. But if the patch
> doesn't look good, I can redesign it in a better way. If so, I'm more than
> glad to follow your advice.
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2225 sad = udf_get_fileshortad(ptr, alen, &epos->offset, inc);
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2226 if (!sad)
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2227 return -1;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2228 etype = le32_to_cpu(sad->extLength) >> 30;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2229 eloc->logicalBlockNum = le32_to_cpu(sad->extPosition);
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2230 eloc->partitionReferenceNum =
> > 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2231 iinfo->i_location.partitionReferenceNum;
> > 28de7948a89676 Cyrill Gorcunov 2007-07-21 2232 *elen = le32_to_cpu(sad->extLength) & UDF_EXTENT_LENGTH_MASK;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2233 break;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2234 case ICBTAG_FLAG_AD_LONG:
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2235 lad = udf_get_filelongad(ptr, alen, &epos->offset, inc);
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2236 if (!lad)
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2237 return -1;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2238 etype = le32_to_cpu(lad->extLength) >> 30;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2239 *eloc = lelb_to_cpu(lad->extLocation);
> > 28de7948a89676 Cyrill Gorcunov 2007-07-21 2240 *elen = le32_to_cpu(lad->extLength) & UDF_EXTENT_LENGTH_MASK;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2241 break;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2242 default:
> > fcbf7637e6647e Steve Magnani 2017-10-12 2243 udf_debug("alloc_type = %u unsupported\n", iinfo->i_alloc_type);
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2244 return -1;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2245 }
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2246
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2247 return etype;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2248 }
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2249
> >
> Thanks for your time,
>
> --Gian
>
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists