* re: Btrfs: add support for multiple csum algorithms
@ 2012-06-15 19:49 Dan Carpenter
2012-12-11 20:31 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2012-06-15 19:49 UTC (permalink / raw)
To: jbacik; +Cc: linux-btrfs
Hello Josef Bacik,
The patch 607d432da054: "Btrfs: add support for multiple csum
algorithms" from Dec 2, 2008, leads to the following warning:
fs/btrfs/disk-io.c:298 csum_tree_block()
error: memcpy() '&found' too small (4 vs 9)
fs/btrfs/disk-io.c
284 if (csum_size > sizeof(inline_result)) {
285 result = kzalloc(csum_size * sizeof(char), GFP_NOFS);
286 if (!result)
287 return 1;
288 } else {
289 result = (char *)&inline_result;
290 }
291
292 btrfs_csum_final(crc, result);
293
294 if (verify) {
295 if (memcmp_extent_buffer(buf, result, 0, csum_size)) {
296 u32 val;
297 u32 found = 0;
298 memcpy(&found, result, csum_size);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Before that commit we used to memcpy() 4 bytes and it was fine, but now
csum_size can be larger than 4 bytes and there is a potential for
memory corruption.
299
300 read_extent_buffer(buf, &val, 0, csum_size);
^^^^
Smatch complains that "val" is too small as well.
301 printk_ratelimited(KERN_INFO "btrfs: %s checksum verify "
302 "failed on %llu wanted %X found %X "
303 "level %d\n",
304 root->fs_info->sb->s_id,
305 (unsigned long long)buf->start, val, found,
306 btrfs_header_level(buf));
307 if (result != (char *)&inline_result)
308 kfree(result);
309 return 1;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Btrfs: add support for multiple csum algorithms
2012-06-15 19:49 Btrfs: add support for multiple csum algorithms Dan Carpenter
@ 2012-12-11 20:31 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2012-12-11 20:31 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs
This is a memory corruption bug, could someone take a look at it?
regards,
dan carpetner
On Fri, Jun 15, 2012 at 10:49:22PM +0300, Dan Carpenter wrote:
> Hello Josef Bacik,
>
> The patch 607d432da054: "Btrfs: add support for multiple csum
> algorithms" from Dec 2, 2008, leads to the following warning:
> fs/btrfs/disk-io.c:298 csum_tree_block()
> error: memcpy() '&found' too small (4 vs 9)
>
> fs/btrfs/disk-io.c
> 284 if (csum_size > sizeof(inline_result)) {
> 285 result = kzalloc(csum_size * sizeof(char), GFP_NOFS);
> 286 if (!result)
> 287 return 1;
> 288 } else {
> 289 result = (char *)&inline_result;
> 290 }
> 291
> 292 btrfs_csum_final(crc, result);
> 293
> 294 if (verify) {
> 295 if (memcmp_extent_buffer(buf, result, 0, csum_size)) {
> 296 u32 val;
> 297 u32 found = 0;
> 298 memcpy(&found, result, csum_size);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Before that commit we used to memcpy() 4 bytes and it was fine, but now
> csum_size can be larger than 4 bytes and there is a potential for
> memory corruption.
>
> 299
> 300 read_extent_buffer(buf, &val, 0, csum_size);
> ^^^^
> Smatch complains that "val" is too small as well.
>
> 301 printk_ratelimited(KERN_INFO "btrfs: %s checksum verify "
> 302 "failed on %llu wanted %X found %X "
> 303 "level %d\n",
> 304 root->fs_info->sb->s_id,
> 305 (unsigned long long)buf->start, val, found,
> 306 btrfs_header_level(buf));
> 307 if (result != (char *)&inline_result)
> 308 kfree(result);
> 309 return 1;
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* re: Btrfs: add support for multiple csum algorithms
@ 2016-01-25 10:31 Dan Carpenter
2016-01-26 21:22 ` Liu Bo
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2016-01-25 10:31 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs
Hello Josef Bacik,
The patch 607d432da054: "Btrfs: add support for multiple csum
algorithms" from Dec 2, 2008, leads to the following static checker
warning:
fs/btrfs/disk-io.c:320 csum_tree_block()
error: __memcpy() '&found' too small (4 vs 9)
fs/btrfs/disk-io.c
278 static int csum_tree_block(struct btrfs_fs_info *fs_info,
279 struct extent_buffer *buf,
280 int verify)
281 {
282 u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
The problem here is that we wrote some incomplete stub code 8 years ago
an never used it. Can we delete this stuff?
btrfs_super_csum_type() always returns 0 and thus
btrfs_super_csum_size() always returns 4. There is a
btrfs_set_super_csum_type() function but it is never called otherwise
we would hit several bugs.
283 char *result = NULL;
284 unsigned long len;
285 unsigned long cur_len;
286 unsigned long offset = BTRFS_CSUM_SIZE;
287 char *kaddr;
288 unsigned long map_start;
289 unsigned long map_len;
290 int err;
291 u32 crc = ~(u32)0;
292 unsigned long inline_result;
Let's assume this is 64 bit system and inline_result is 8 bytes.
293
294 len = buf->len - offset;
295 while (len > 0) {
296 err = map_private_extent_buffer(buf, offset, 32,
297 &kaddr, &map_start, &map_len);
298 if (err)
299 return 1;
300 cur_len = min(len, map_len - (offset - map_start));
301 crc = btrfs_csum_data(kaddr + offset - map_start,
302 crc, cur_len);
303 len -= cur_len;
304 offset += cur_len;
305 }
306 if (csum_size > sizeof(inline_result)) {
307 result = kzalloc(csum_size, GFP_NOFS);
In the future code we would allocate 9+ bytes.
308 if (!result)
309 return 1;
310 } else {
311 result = (char *)&inline_result;
312 }
313
314 btrfs_csum_final(crc, result);
We only ever use the first 4 bytes. We need to add a size option to
this function to support future code.
315
316 if (verify) {
317 if (memcmp_extent_buffer(buf, result, 0, csum_size)) {
318 u32 val;
319 u32 found = 0;
320 memcpy(&found, result, csum_size);
If csum_size were more than 4 this would corrupt memory.
321
322 read_extent_buffer(buf, &val, 0, csum_size);
323 btrfs_warn_rl(fs_info,
324 "%s checksum verify failed on %llu wanted %X found %X "
325 "level %d",
326 fs_info->sb->s_id, buf->start,
327 val, found, btrfs_header_level(buf));
328 if (result != (char *)&inline_result)
329 kfree(result);
330 return 1;
331 }
332 } else {
333 write_extent_buffer(buf, result, 0, csum_size);
334 }
335 if (result != (char *)&inline_result)
336 kfree(result);
337 return 0;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Btrfs: add support for multiple csum algorithms
2016-01-25 10:31 Dan Carpenter
@ 2016-01-26 21:22 ` Liu Bo
0 siblings, 0 replies; 4+ messages in thread
From: Liu Bo @ 2016-01-26 21:22 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Josef Bacik, linux-btrfs
On Mon, Jan 25, 2016 at 01:31:34PM +0300, Dan Carpenter wrote:
>
> Hello Josef Bacik,
>
> The patch 607d432da054: "Btrfs: add support for multiple csum
> algorithms" from Dec 2, 2008, leads to the following static checker
> warning:
>
> fs/btrfs/disk-io.c:320 csum_tree_block()
> error: __memcpy() '&found' too small (4 vs 9)
>
> fs/btrfs/disk-io.c
> 278 static int csum_tree_block(struct btrfs_fs_info *fs_info,
> 279 struct extent_buffer *buf,
> 280 int verify)
> 281 {
> 282 u16 csum_size = btrfs_super_csum_size(fs_info->super_copy);
>
> The problem here is that we wrote some incomplete stub code 8 years ago
> an never used it. Can we delete this stuff?
>
> btrfs_super_csum_type() always returns 0 and thus
> btrfs_super_csum_size() always returns 4. There is a
> btrfs_set_super_csum_type() function but it is never called otherwise
> we would hit several bugs.
Hmm..since we can only choose one type from btrfs_super_csum_type, this btrfs_set_super_csum_type() is called only when we do mkfs.btrfs.
Thanks,
-liubo
>
> 283 char *result = NULL;
> 284 unsigned long len;
> 285 unsigned long cur_len;
> 286 unsigned long offset = BTRFS_CSUM_SIZE;
> 287 char *kaddr;
> 288 unsigned long map_start;
> 289 unsigned long map_len;
> 290 int err;
> 291 u32 crc = ~(u32)0;
> 292 unsigned long inline_result;
>
> Let's assume this is 64 bit system and inline_result is 8 bytes.
>
> 293
> 294 len = buf->len - offset;
> 295 while (len > 0) {
> 296 err = map_private_extent_buffer(buf, offset, 32,
> 297 &kaddr, &map_start, &map_len);
> 298 if (err)
> 299 return 1;
> 300 cur_len = min(len, map_len - (offset - map_start));
> 301 crc = btrfs_csum_data(kaddr + offset - map_start,
> 302 crc, cur_len);
> 303 len -= cur_len;
> 304 offset += cur_len;
> 305 }
> 306 if (csum_size > sizeof(inline_result)) {
> 307 result = kzalloc(csum_size, GFP_NOFS);
>
> In the future code we would allocate 9+ bytes.
>
> 308 if (!result)
> 309 return 1;
> 310 } else {
> 311 result = (char *)&inline_result;
> 312 }
> 313
> 314 btrfs_csum_final(crc, result);
>
> We only ever use the first 4 bytes. We need to add a size option to
> this function to support future code.
>
> 315
> 316 if (verify) {
> 317 if (memcmp_extent_buffer(buf, result, 0, csum_size)) {
> 318 u32 val;
> 319 u32 found = 0;
> 320 memcpy(&found, result, csum_size);
>
> If csum_size were more than 4 this would corrupt memory.
>
> 321
> 322 read_extent_buffer(buf, &val, 0, csum_size);
> 323 btrfs_warn_rl(fs_info,
> 324 "%s checksum verify failed on %llu wanted %X found %X "
> 325 "level %d",
> 326 fs_info->sb->s_id, buf->start,
> 327 val, found, btrfs_header_level(buf));
> 328 if (result != (char *)&inline_result)
> 329 kfree(result);
> 330 return 1;
> 331 }
> 332 } else {
> 333 write_extent_buffer(buf, result, 0, csum_size);
> 334 }
> 335 if (result != (char *)&inline_result)
> 336 kfree(result);
> 337 return 0;
>
> regards,
> dan carpenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-01-26 21:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-15 19:49 Btrfs: add support for multiple csum algorithms Dan Carpenter
2012-12-11 20:31 ` Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2016-01-25 10:31 Dan Carpenter
2016-01-26 21:22 ` Liu Bo
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).