* [bug report] fs/ntfs3: Add initialization of super block
@ 2021-08-24 7:58 Dan Carpenter
2021-08-24 10:33 ` Kari Argillander
2026-02-09 15:24 ` [PATCH] fs/ntfs3: avoid calling run_get_entry() when run == NULL in ntfs_read_run_nb_ra() Konstantin Komarov
0 siblings, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-08-24 7:58 UTC (permalink / raw)
To: almaz.alexandrovich; +Cc: ntfs3
Hello Konstantin Komarov,
The patch 82cae269cfa9: "fs/ntfs3: Add initialization of super block"
from Aug 13, 2021, leads to the following
Smatch static checker warning:
fs/ntfs3/index.c:238 bmp_buf_get()
warn: 'bh' could be an error pointer
fs/ntfs3/index.c
229 data_size = le64_to_cpu(b->nres.data_size);
230 if (WARN_ON(off >= data_size)) {
231 /* looks like filesystem error */
232 return -EINVAL;
233 }
234
235 valid_size = le64_to_cpu(b->nres.valid_size);
236
237 bh = ntfs_bread_run(sbi, &indx->bitmap_run, off);
--> 238 if (!bh)
239 return -EIO;
240
241 if (IS_ERR(bh))
This is not a bug, but it is wrong style. When a function returns both
error pointers and NULL then the NULL return is means the feature is
disabled. It's not an error. Just that the feature is turned off
deliberately in the Kconfig or whatever. Don't print an error message,
just continue with the feature disabled as the admin has requested.
But here NULL is just an error. The ntfs_bread_run() should do:
bh = ntfs_bread();
if (!bh)
return ERR_PTR(-EIO);
return bh;
242 return PTR_ERR(bh);
243
244 bbuf->bh = bh;
245
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] fs/ntfs3: Add initialization of super block
2021-08-24 7:58 [bug report] fs/ntfs3: Add initialization of super block Dan Carpenter
@ 2021-08-24 10:33 ` Kari Argillander
2026-02-09 15:24 ` [PATCH] fs/ntfs3: avoid calling run_get_entry() when run == NULL in ntfs_read_run_nb_ra() Konstantin Komarov
1 sibling, 0 replies; 5+ messages in thread
From: Kari Argillander @ 2021-08-24 10:33 UTC (permalink / raw)
To: Dan Carpenter; +Cc: almaz.alexandrovich, ntfs3
On Tue, Aug 24, 2021 at 10:58:19AM +0300, Dan Carpenter wrote:
> Hello Konstantin Komarov,
>
> The patch 82cae269cfa9: "fs/ntfs3: Add initialization of super block"
> from Aug 13, 2021, leads to the following
> Smatch static checker warning:
>
> fs/ntfs3/index.c:238 bmp_buf_get()
> warn: 'bh' could be an error pointer
>
> fs/ntfs3/index.c
> 229 data_size = le64_to_cpu(b->nres.data_size);
> 230 if (WARN_ON(off >= data_size)) {
> 231 /* looks like filesystem error */
> 232 return -EINVAL;
> 233 }
> 234
> 235 valid_size = le64_to_cpu(b->nres.valid_size);
> 236
> 237 bh = ntfs_bread_run(sbi, &indx->bitmap_run, off);
> --> 238 if (!bh)
> 239 return -EIO;
> 240
> 241 if (IS_ERR(bh))
>
> This is not a bug, but it is wrong style. When a function returns both
> error pointers and NULL then the NULL return is means the feature is
> disabled. It's not an error. Just that the feature is turned off
> deliberately in the Kconfig or whatever. Don't print an error message,
> just continue with the feature disabled as the admin has requested.
>
> But here NULL is just an error. The ntfs_bread_run() should do:
Agreed.
>
> bh = ntfs_bread();
> if (!bh)
> return ERR_PTR(-EIO);
> return bh;
Can also be like below but probably your version is more readable.
return ntfs_bread(sb, lbo >> sb->s_blocksize_bits) ? : ERR_PTR(-EIO);
>
> 242 return PTR_ERR(bh);
> 243
> 244 bbuf->bh = bh;
> 245
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] fs/ntfs3: avoid calling run_get_entry() when run == NULL in ntfs_read_run_nb_ra()
2021-08-24 7:58 [bug report] fs/ntfs3: Add initialization of super block Dan Carpenter
2021-08-24 10:33 ` Kari Argillander
@ 2026-02-09 15:24 ` Konstantin Komarov
1 sibling, 0 replies; 5+ messages in thread
From: Konstantin Komarov @ 2026-02-09 15:24 UTC (permalink / raw)
To: ntfs3
Cc: linux-kernel, linux-fsdevel, Konstantin Komarov,
kernel test robot, Dan Carpenter
When ntfs_read_run_nb_ra() is invoked with run == NULL the code later
assumes run is valid and may call run_get_entry(NULL, ...), and also
uses clen/idx without initializing them. Smatch reported uninitialized
variable warnings and this can lead to undefined behaviour. This patch
fixes it.
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202512230646.v5hrYXL0-lkp@intel.com/
Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
---
fs/ntfs3/fsntfs.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c
index e9c39c62aea4..2ef500f1a9fa 100644
--- a/fs/ntfs3/fsntfs.c
+++ b/fs/ntfs3/fsntfs.c
@@ -1256,6 +1256,12 @@ int ntfs_read_run_nb_ra(struct ntfs_sb_info *sbi, const struct runs_tree *run,
} while (len32);
+ if (!run) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ /* Get next fragment to read. */
vcn_next = vcn + clen;
if (!run_get_entry(run, ++idx, &vcn, &lcn, &clen) ||
vcn != vcn_next) {
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: Support needed to continue Smatch work
@ 2026-02-06 13:38 Dan Carpenter
2026-02-06 13:41 ` [bug report] fs/ntfs3: Add initialization of super block Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2026-02-06 13:38 UTC (permalink / raw)
To: linux-kernel, ksummit, Bill Fletcher, Randy Linnell,
Brad Spengler
Cc: vincent.guittot, lina.iyer
I need to post an update on the current situation with Smatch.
First of all, I want to start by thanking Brad Spengler from grsecurity
who reached out to me on this, offered some funding, and has been
trying to push the Smatch work forward. It really means a lot to me.
Unfortunately, we haven't been able to raise enough support to continue
my Smatch work. I have still been filtering zero day bot warnings and
I am a bit worried that people have the impression that I'm reviewing
static checker warnings when I am not.
The situation isn't great. The zero day bot can't do cross function
analsysis and it only looks at checks with a low false positive rate.
We're missing out on a bunch of bugs. I'm going to add some of the
those missed warnings to this thread so people have a better picture of
what we're missing. There are some buffer overflows in there. A bunch
of off by one bugs. A missing error code in fork(). And random other
minor things as well.
https://lore.kernel.org/all/caa37f28-a2e8-4e0a-a9ce-a365ce805e4b@stanley.mountain/
I am still trying to figure out a way to restart Smatch checking. The
funding model would be that several companies would support this project
by paying a proportion of my salary. Part of that goes to reporting
bugs like the ones above and part of that goes to developing Smatch and
writing new checks. Please, contact
Bill Fletcher <bill.fletcher@linaro.org> if you would like to support
this work.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* [bug report] fs/ntfs3: Add initialization of super block
2026-02-06 13:38 Support needed to continue Smatch work Dan Carpenter
@ 2026-02-06 13:41 ` Dan Carpenter
2026-02-09 10:20 ` Konstantin Komarov
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2026-02-06 13:41 UTC (permalink / raw)
To: Konstantin Komarov; +Cc: ntfs3, linux-kernel
[ Smatch checking is paused while we raise funding. #SadFace
https://lore.kernel.org/all/aTaiGSbWZ9DJaGo7@stanley.mountain/ -dan ]
Hello Konstantin Komarov,
Commit 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
from Aug 13, 2021 (linux-next), leads to the following Smatch static
checker warning:
fs/ntfs3/fsntfs.c:1260 ntfs_read_run_nb_ra() error: we previously assumed 'run' could be null (see line 1178)
fs/ntfs3/fsntfs.c:1259 ntfs_read_run_nb_ra() error: uninitialized symbol 'clen'.
fs/ntfs3/fsntfs.c:1260 ntfs_read_run_nb_ra() error: uninitialized symbol 'idx'.
fs/ntfs3/fsntfs.c
1161 int ntfs_read_run_nb_ra(struct ntfs_sb_info *sbi, const struct runs_tree *run,
1162 u64 vbo, void *buf, u32 bytes, struct ntfs_buffers *nb,
1163 struct file_ra_state *ra)
1164 {
1165 int err;
1166 struct super_block *sb = sbi->sb;
1167 struct address_space *mapping = sb->s_bdev->bd_mapping;
1168 u32 blocksize = sb->s_blocksize;
1169 u8 cluster_bits = sbi->cluster_bits;
1170 u32 off = vbo & sbi->cluster_mask;
1171 u32 nbh = 0;
1172 CLST vcn_next, vcn = vbo >> cluster_bits;
1173 CLST lcn, clen;
1174 u64 lbo, len;
1175 size_t idx;
1176 struct buffer_head *bh;
1177
1178 if (!run) {
1179 /* First reading of $Volume + $MFTMirr + $LogFile goes here. */
1180 if (vbo > MFT_REC_VOL * sbi->record_size) {
1181 err = -ENOENT;
1182 goto out;
1183 }
1184
1185 /* Use absolute boot's 'MFTCluster' to read record. */
1186 lbo = vbo + sbi->mft.lbo;
1187 len = sbi->record_size;
If run is NULL then "clen" is uninitialized.
1188 } else if (!run_lookup_entry(run, vcn, &lcn, &clen, &idx)) {
1189 err = -ENOENT;
1190 goto out;
1191 } else {
1192 if (lcn == SPARSE_LCN) {
1193 err = -EINVAL;
1194 goto out;
1195 }
1196
1197 lbo = ((u64)lcn << cluster_bits) + off;
1198 len = ((u64)clen << cluster_bits) - off;
1199 }
1200
1201 off = lbo & (blocksize - 1);
1202 if (nb) {
1203 nb->off = off;
1204 nb->bytes = bytes;
1205 }
1206
1207 if (ra && !ra->ra_pages)
1208 file_ra_state_init(ra, mapping);
1209
1210 for (;;) {
1211 u32 len32 = len >= bytes ? bytes : len;
1212 sector_t block = lbo >> sb->s_blocksize_bits;
1213
1214 if (ra) {
1215 pgoff_t index = lbo >> PAGE_SHIFT;
1216 if (!ra_has_index(ra, index)) {
1217 page_cache_sync_readahead(mapping, ra, NULL,
1218 index, 1);
1219 ra->prev_pos = (loff_t)index << PAGE_SHIFT;
1220 }
1221 }
1222
1223 do {
1224 u32 op = blocksize - off;
1225
1226 if (op > len32)
1227 op = len32;
1228
1229 bh = ntfs_bread(sb, block);
1230 if (!bh) {
1231 err = -EIO;
1232 goto out;
1233 }
1234
1235 if (buf) {
1236 memcpy(buf, bh->b_data + off, op);
1237 buf = Add2Ptr(buf, op);
1238 }
1239
1240 if (!nb) {
1241 put_bh(bh);
1242 } else if (nbh >= ARRAY_SIZE(nb->bh)) {
1243 err = -EINVAL;
1244 goto out;
1245 } else {
1246 nb->bh[nbh++] = bh;
1247 nb->nbufs = nbh;
1248 }
1249
1250 bytes -= op;
1251 if (!bytes)
1252 return 0;
1253 len32 -= op;
1254 block += 1;
1255 off = 0;
1256
1257 } while (len32);
1258
--> 1259 vcn_next = vcn + clen;
^^^^
Used uninitalized here.
1260 if (!run_get_entry(run, ++idx, &vcn, &lcn, &clen) ||
But also if we pass a NULL run to run_get_entry() it will crash. I'm
a bit confused by this code.
1261 vcn != vcn_next) {
1262 err = -ENOENT;
1263 goto out;
1264 }
1265
1266 if (lcn == SPARSE_LCN) {
1267 err = -EINVAL;
1268 goto out;
1269 }
1270
1271 lbo = ((u64)lcn << cluster_bits);
1272 len = ((u64)clen << cluster_bits);
1273 }
1274
1275 out:
1276 if (!nbh)
1277 return err;
1278
1279 while (nbh) {
1280 put_bh(nb->bh[--nbh]);
1281 nb->bh[nbh] = NULL;
1282 }
1283
1284 nb->nbufs = 0;
1285 return err;
1286 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] fs/ntfs3: Add initialization of super block
2026-02-06 13:41 ` [bug report] fs/ntfs3: Add initialization of super block Dan Carpenter
@ 2026-02-09 10:20 ` Konstantin Komarov
0 siblings, 0 replies; 5+ messages in thread
From: Konstantin Komarov @ 2026-02-09 10:20 UTC (permalink / raw)
To: Dan Carpenter; +Cc: ntfs3, linux-kernel
On 2/6/26 14:41, Dan Carpenter wrote:
> [ Smatch checking is paused while we raise funding. #SadFace
> https://lore.kernel.org/all/aTaiGSbWZ9DJaGo7@stanley.mountain/ -dan ]
>
> Hello Konstantin Komarov,
>
> Commit 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
> from Aug 13, 2021 (linux-next), leads to the following Smatch static
> checker warning:
>
> fs/ntfs3/fsntfs.c:1260 ntfs_read_run_nb_ra() error: we previously assumed 'run' could be null (see line 1178)
> fs/ntfs3/fsntfs.c:1259 ntfs_read_run_nb_ra() error: uninitialized symbol 'clen'.
> fs/ntfs3/fsntfs.c:1260 ntfs_read_run_nb_ra() error: uninitialized symbol 'idx'.
>
> fs/ntfs3/fsntfs.c
> 1161 int ntfs_read_run_nb_ra(struct ntfs_sb_info *sbi, const struct runs_tree *run,
> 1162 u64 vbo, void *buf, u32 bytes, struct ntfs_buffers *nb,
> 1163 struct file_ra_state *ra)
> 1164 {
> 1165 int err;
> 1166 struct super_block *sb = sbi->sb;
> 1167 struct address_space *mapping = sb->s_bdev->bd_mapping;
> 1168 u32 blocksize = sb->s_blocksize;
> 1169 u8 cluster_bits = sbi->cluster_bits;
> 1170 u32 off = vbo & sbi->cluster_mask;
> 1171 u32 nbh = 0;
> 1172 CLST vcn_next, vcn = vbo >> cluster_bits;
> 1173 CLST lcn, clen;
> 1174 u64 lbo, len;
> 1175 size_t idx;
> 1176 struct buffer_head *bh;
> 1177
> 1178 if (!run) {
> 1179 /* First reading of $Volume + $MFTMirr + $LogFile goes here. */
> 1180 if (vbo > MFT_REC_VOL * sbi->record_size) {
> 1181 err = -ENOENT;
> 1182 goto out;
> 1183 }
> 1184
> 1185 /* Use absolute boot's 'MFTCluster' to read record. */
> 1186 lbo = vbo + sbi->mft.lbo;
> 1187 len = sbi->record_size;
>
> If run is NULL then "clen" is uninitialized.
>
> 1188 } else if (!run_lookup_entry(run, vcn, &lcn, &clen, &idx)) {
> 1189 err = -ENOENT;
> 1190 goto out;
> 1191 } else {
> 1192 if (lcn == SPARSE_LCN) {
> 1193 err = -EINVAL;
> 1194 goto out;
> 1195 }
> 1196
> 1197 lbo = ((u64)lcn << cluster_bits) + off;
> 1198 len = ((u64)clen << cluster_bits) - off;
> 1199 }
> 1200
> 1201 off = lbo & (blocksize - 1);
> 1202 if (nb) {
> 1203 nb->off = off;
> 1204 nb->bytes = bytes;
> 1205 }
> 1206
> 1207 if (ra && !ra->ra_pages)
> 1208 file_ra_state_init(ra, mapping);
> 1209
> 1210 for (;;) {
> 1211 u32 len32 = len >= bytes ? bytes : len;
> 1212 sector_t block = lbo >> sb->s_blocksize_bits;
> 1213
> 1214 if (ra) {
> 1215 pgoff_t index = lbo >> PAGE_SHIFT;
> 1216 if (!ra_has_index(ra, index)) {
> 1217 page_cache_sync_readahead(mapping, ra, NULL,
> 1218 index, 1);
> 1219 ra->prev_pos = (loff_t)index << PAGE_SHIFT;
> 1220 }
> 1221 }
> 1222
> 1223 do {
> 1224 u32 op = blocksize - off;
> 1225
> 1226 if (op > len32)
> 1227 op = len32;
> 1228
> 1229 bh = ntfs_bread(sb, block);
> 1230 if (!bh) {
> 1231 err = -EIO;
> 1232 goto out;
> 1233 }
> 1234
> 1235 if (buf) {
> 1236 memcpy(buf, bh->b_data + off, op);
> 1237 buf = Add2Ptr(buf, op);
> 1238 }
> 1239
> 1240 if (!nb) {
> 1241 put_bh(bh);
> 1242 } else if (nbh >= ARRAY_SIZE(nb->bh)) {
> 1243 err = -EINVAL;
> 1244 goto out;
> 1245 } else {
> 1246 nb->bh[nbh++] = bh;
> 1247 nb->nbufs = nbh;
> 1248 }
> 1249
> 1250 bytes -= op;
> 1251 if (!bytes)
> 1252 return 0;
> 1253 len32 -= op;
> 1254 block += 1;
> 1255 off = 0;
> 1256
> 1257 } while (len32);
> 1258
> --> 1259 vcn_next = vcn + clen;
> ^^^^
> Used uninitalized here.
>
> 1260 if (!run_get_entry(run, ++idx, &vcn, &lcn, &clen) ||
>
> But also if we pass a NULL run to run_get_entry() it will crash. I'm
> a bit confused by this code.
>
> 1261 vcn != vcn_next) {
> 1262 err = -ENOENT;
> 1263 goto out;
> 1264 }
> 1265
> 1266 if (lcn == SPARSE_LCN) {
> 1267 err = -EINVAL;
> 1268 goto out;
> 1269 }
> 1270
> 1271 lbo = ((u64)lcn << cluster_bits);
> 1272 len = ((u64)clen << cluster_bits);
> 1273 }
> 1274
> 1275 out:
> 1276 if (!nbh)
> 1277 return err;
> 1278
> 1279 while (nbh) {
> 1280 put_bh(nb->bh[--nbh]);
> 1281 nb->bh[nbh] = NULL;
> 1282 }
> 1283
> 1284 nb->nbufs = 0;
> 1285 return err;
> 1286 }
>
> regards,
> dan carpenter
Hello,
Thanks for the Smatch report. I’ll examine the warnings, prepare a fix,
and post a patch.
Regards,
Konstantin
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-02-09 15:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-24 7:58 [bug report] fs/ntfs3: Add initialization of super block Dan Carpenter
2021-08-24 10:33 ` Kari Argillander
2026-02-09 15:24 ` [PATCH] fs/ntfs3: avoid calling run_get_entry() when run == NULL in ntfs_read_run_nb_ra() Konstantin Komarov
-- strict thread matches above, loose matches on Subject: below --
2026-02-06 13:38 Support needed to continue Smatch work Dan Carpenter
2026-02-06 13:41 ` [bug report] fs/ntfs3: Add initialization of super block Dan Carpenter
2026-02-09 10:20 ` Konstantin Komarov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.